-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rustup to rustc 1.36.0-nightly (acc7e652f 2019-05-10) #4080
Conversation
af9e59d
to
3e598cf
Compare
Builds now. |
|
@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 |
As for if-let-chains (allowing |
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 |
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. |
It works! r? @oli-obk |
span.ctxt().outer().expn_info().is_some() | ||
if let Some(info) = span.ctxt().outer().expn_info() { | ||
if let ExpnFormat::CompilerDesugaring(..) = info.format { | ||
false |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
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 <[email protected]> wrote:Fixes breakages from rust-lang/rust#59288
Not finished yet, help appreciated.
You can view, comment on, or merge this pull request online at:
#4080
Commit 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+
Yea I was on mobile and didn't see the further messages ;) |
📌 Commit 19cfb84 has been approved by |
Rustup to rustc 1.36.0-nightly (acc7e65 2019-05-10) Fixes breakages from rust-lang/rust#59288 Not finished yet, help appreciated. Todo: - [x] Needs to build - [x] Util should handle DropTemps, #4080 (comment) - [x] Author lint should spit out higher::if_block checks - [x] Unsure what to do in consts.rs - [x] Needs to pass tests
☀️ Test successful - checks-travis, status-appveyor |
Reintroduce hir::ExprKind::If Basically copied and paste rust-lang#59288/rust-lang/rust-clippy#4080 with some modifications. The vast majority of tests were fixed and now there are only a few remaining. Since I am still unable to figure out the missing pieces, any help with the following list is welcome. - [ ] **Unnecessary `typeck` exception**: [Cheated on this one to make CI green.](https://github.com/rust-lang/rust/pull/79328/files#diff-3faee9ba23fc54a12b7c43364ba81f8c5660045c7e1d7989a02a0cee1c5b2051) - [x] **Incorrect span**: [Span should reference `then` and `else` separately.](https://github.com/rust-lang/rust/pull/79328/files#diff-cf2c46e82222ee4b1037a68fff8a1af3c4f1de7a6b3fd798aacbf3c0475abe3d) - [x] **New note regarding `assert!`**: [Modified but not "wrong". Maybe can be a good thing?](https://github.com/rust-lang/rust/pull/79328/files#diff-9e0d7c89ed0224e2b62060c957177c27db43c30dfe3c2974cb6b5091cda9cfb5) - [x] **Inverted report location**: [Modified but not "wrong". Locations were inverted.](https://github.com/rust-lang/rust/pull/79328/files#diff-f637ce7c1f68d523a165aa9651765df05e36c4d7d279194b1a6b28b48a323691) - [x] **`src/test/ui/point-to-type-err-cause-on-impl-trait-return.rs` has weird errors**: [Not sure why this is happening.](https://github.com/rust-lang/rust/pull/79328/files#diff-c823c09660f5b112f95e97e8ff71f1797b6c7f37dbb3d16f8e98bbaea8072e95) - [x] **Missing diagnostic**: [???](https://github.com/rust-lang/rust/pull/79328/files#diff-6b8ab09360d725ba4513933827f9796b42ff9522b0690f80b76de067143af2fc)
Reintroduce hir::ExprKind::If Basically copied and paste #59288/rust-lang#4080 with some modifications. The vast majority of tests were fixed and now there are only a few remaining. Since I am still unable to figure out the missing pieces, any help with the following list is welcome. - [ ] **Unnecessary `typeck` exception**: [Cheated on this one to make CI green.](https://github.com/rust-lang/rust/pull/79328/files#diff-3faee9ba23fc54a12b7c43364ba81f8c5660045c7e1d7989a02a0cee1c5b2051) - [x] **Incorrect span**: [Span should reference `then` and `else` separately.](https://github.com/rust-lang/rust/pull/79328/files#diff-cf2c46e82222ee4b1037a68fff8a1af3c4f1de7a6b3fd798aacbf3c0475abe3d) - [x] **New note regarding `assert!`**: [Modified but not "wrong". Maybe can be a good thing?](https://github.com/rust-lang/rust/pull/79328/files#diff-9e0d7c89ed0224e2b62060c957177c27db43c30dfe3c2974cb6b5091cda9cfb5) - [x] **Inverted report location**: [Modified but not "wrong". Locations were inverted.](https://github.com/rust-lang/rust/pull/79328/files#diff-f637ce7c1f68d523a165aa9651765df05e36c4d7d279194b1a6b28b48a323691) - [x] **`src/test/ui/point-to-type-err-cause-on-impl-trait-return.rs` has weird errors**: [Not sure why this is happening.](https://github.com/rust-lang/rust/pull/79328/files#diff-c823c09660f5b112f95e97e8ff71f1797b6c7f37dbb3d16f8e98bbaea8072e95) - [x] **Missing diagnostic**: [???](https://github.com/rust-lang/rust/pull/79328/files#diff-6b8ab09360d725ba4513933827f9796b42ff9522b0690f80b76de067143af2fc)
Fixes breakages from rust-lang/rust#59288
Not finished yet, help appreciated.
Todo: