Rustup to rustc 1.36.0-nightly (acc7e652f 2019-05-10) by Manishearth · Pull Request #4080 · rust-lang/rust-clippy (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation24 Commits11 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
if_same_then_else is overzealous now, and the question_mark lint isn't triggering. Will investigate later. Feel free to push fixes.
@Centril are chained ifs now just nested matches? It seems like if_same_then_else is hitting this.
@Manishearth Yeah; the HIR lowering for this part just operates recursively on one expression at a time so you'll get matches within matches. Also note that I changed the desugaring of if let to use _ => {}, for the else part rather than _ => (),
As for if-let-chains (allowing &&), those are not implemented yet. Just the refactoring has been done for now.
Found the bug, if blocks without else desugar to empty match arms (not missing match arms)
Nope, still have a bunch of failures :/
Never mind, I didn't clear test results properly and ran update-all-references 😄
We only have two test failures now:
diff --git a/tests/ui/block_in_if_condition.stderr b/tests/ui/block_in_if_condition.stderr
index 0876d5db..105a71f6 100644
--- a/tests/ui/block_in_if_condition.stderr
+++ b/tests/ui/block_in_if_condition.stderr
@@ -50,13 +50,5 @@ LL | | x == target
LL | | },
| |_________^
-error: this boolean expression can be simplified
- --> $DIR/block_in_if_condition.rs:77:8
- |
-LL | if true && x == 3 {
- | ^^^^^^^^^^^^^^ help: try: x == 3
- |
- = note: -D clippy::nonminimal-bool implied by -D warnings
-error: aborting due to 5 previous errors
+error: aborting due to 4 previous errors
diff --git a/tests/ui/booleans.stderr b/tests/ui/booleans.stderr
index eebab8c3..23ebfe90 100644
--- a/tests/ui/booleans.stderr
+++ b/tests/ui/booleans.stderr
@@ -175,29 +175,5 @@ error: this boolean expression can be simplified
LL | let _ = !c ^ c || !a.is_some();
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: !c ^ c || a.is_none()
-error: this boolean expression can be simplified
- --> $DIR/booleans.rs:128:8
- |
-LL | if !res.is_ok() {}
- | ^^^^^^^^^^^^ help: try: res.is_err()
-error: this boolean expression can be simplified
- --> $DIR/booleans.rs:129:8
- |
-LL | if !res.is_err() {}
- | ^^^^^^^^^^^^^ help: try: res.is_ok()
-error: this boolean expression can be simplified
- --> $DIR/booleans.rs:132:8
- |
-LL | if !res.is_some() {}
- | ^^^^^^^^^^^^^^ help: try: res.is_none()
-error: this boolean expression can be simplified
- --> $DIR/booleans.rs:133:8
- |
-LL | if !res.is_none() {}
- | ^^^^^^^^^^^^^^ help: try: res.is_some()
-error: aborting due to 25 previous errors +error: aborting due to 21 previous errors
Looks like if foo && bar is being treated differently?
But that part isn't implemented yet.
But also, nonminimal_bool doesn't deal with if blocks at all. Something's fishy
(it's possible that hir::intravisit broke)
Ah, the macro check is catching it.
| span.ctxt().outer().expn_info().is_some() |
|---|
| if let Some(info) = span.ctxt().outer().expn_info() { |
| if let ExpnFormat::CompilerDesugaring(..) = info.format { |
| false |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually want this in the macro check? As you can see it inadvertently fixed a bug in into_iter_on_ref.rs, but there may be cases where we don't care about desugarings. Perhaps we should just have both kinds of checks.
The nonminimal bool test fails without this because the visitor bails when it sees the if desugaring.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may be able to do this more fine-grained in the future by passing in more than a span (e.g. a CompilerDesugaring variant that we allow). For now it seems ok, since this can only happen in HIR anyway, and our HIR lints already know about the compiler expansions they care about
| let right_pat = self.next("right"); |
|---|
| // handle if desugarings |
| // TODO add more desugarings here |
| if let Some((cond, then, opt_else)) = higher::if_block(&expr) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff is basically just me adding this if let and sticking the rest of the function in the else.
WIP rustup to rustc 1.36.0-nightly (acc7e652f 2019-05-10) Rustup to rustc 1.36.0-nightly (acc7e652f 2019-05-10)
Oh! Thanks for doing this. I planned to do it today. Sorry about not communicating this breakage better. I'll fix the rest of this PR in a few hours On 11 May 2019 04:19, Manish Goregaokar notifications@github.com wrote:Fixes breakages from rust-lang/rust#59288Not finished yet, help appreciated. You can view, comment on, or merge this pull request online at:#4080Commit Summary Add util function for desugaring if blockFix unwrap.rsFix shadow.rsFix question_mark.rs File Changes M clippy_lints/src/question_mark.rs (4) M clippy_lints/src/shadow.rs (7) M clippy_lints/src/unwrap.rs (4) M clippy_lints/src/utils/higher.rs (12) Patch Links:https://github.com/rust-lang/rust-clippy/pull/4080.patchhttps://github.com/rust-lang/rust-clippy/pull/4080.diff—You are receiving this because you are subscribed to this thread.Reply to this email directly, view it on GitHub, or mute the thread.
No problem, but yeah in the future advance warning would be good so we can start prepping the fix while it goes through review, and catch any issues early in the process.
Also to be clear this should pass CI, I don't think there's more work to be done.
@bors r+
Also to be clear this should pass CI, I don't think there's more work to be done.
Yea I was on mobile and didn't see the further messages ;)
📌 Commit 19cfb84 has been approved by oli-obk
bors added a commit that referenced this pull request
Rustup to rustc 1.36.0-nightly (acc7e65 2019-05-10)
Fixes breakages from rust-lang/rust#59288
Not finished yet, help appreciated.
Todo:
- Needs to build
- Util should handle DropTemps, #4080 (comment)
- Author lint should spit out higher::if_block checks
- Unsure what to do in consts.rs
- Needs to pass tests
bors added a commit to rust-lang-ci/rust that referenced this pull request
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request