Skip to content

Fix if_same_then_else false positive #3590

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Dec 31, 2018

Conversation

jorpic
Copy link
Contributor

@jorpic jorpic commented Dec 28, 2018

This fixes false positive in #3559.
The problem was that SpanlessEq does not check patterns in declarations. So this two blocks considered equal.

if true {
    let (x, y) = foo();
} else {
   let (y, x) = foo();
}

Not sure if the proposed change is safe as SpanlessEq is used extensively in other lints, but I tried hard to come up with counterexample and failed.

@jorpic
Copy link
Contributor Author

jorpic commented Dec 28, 2018

Don't understand how my change causes build failure.
Seems that hir::ExprKind::Err was introduced in rust-lang/rust@a5c52c7 11 days ago.

Should I address these errors in this PR?

error[E0004]: non-exhaustive patterns: `Err` not covered
   --> clippy_lints\src\utils\inspector.rs:172:11
    |
172 |     match expr.node {
    |           ^^^^^^^^^ pattern `Err` not covered
error[E0004]: non-exhaustive patterns: `Err` not covered
   --> clippy_lints\src\loops.rs:691:11
    |
691 |     match expr.node {
    |           ^^^^^^^^^ pattern `Err` not covered
error[E0004]: non-exhaustive patterns: `Err` not covered
   --> clippy_lints\src\utils\author.rs:205:15
    |
205 |         match expr.node {
    |               ^^^^^^^^^ pattern `Err` not covered
error[E0004]: non-exhaustive patterns: `Err` not covered
   --> clippy_lints\src\utils\hir_utils.rs:410:15
    |
410 |         match e.node {
    |               ^^^^^^ pattern `Err` not covered
error[E0004]: non-exhaustive patterns: `Err` not covered
  --> clippy_lints\src\utils\sugg.rs:58:19
   |
58 |             match expr.node {
   |                   ^^^^^^^^^ pattern `Err` not covered
error[E0004]: non-exhaustive patterns: `Err` not covered
   --> clippy_lints\src\utils\sugg.rs:128:15
    |
128 |         match expr.node {
    |               ^^^^^^^^^ pattern `Err` not covered
error: aborting due to 6 previous errors

@oli-obk
Copy link
Contributor

oli-obk commented Dec 28, 2018

You probably just have an outdated master branch. If you rebase over the master branch of this repository, everything should work again

@mati865
Copy link
Member

mati865 commented Dec 28, 2018

It's upstream breakage, rust-lang/rust#56999 was merged last night.
You can wait until somebody fixes it or do it yourself (preferrably in the other PR).

@flip1995
Copy link
Member

Fixed in #3591

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 28, 2018
@phansch phansch closed this Dec 28, 2018
@phansch phansch reopened this Dec 28, 2018
@bors
Copy link
Contributor

bors commented Dec 28, 2018

☔ The latest upstream changes (presumably #3587) made this pull request unmergeable. Please resolve the merge conflicts.

@jorpic jorpic force-pushed the i3559-if_same_then_else branch from bc93633 to aebc9d9 Compare December 28, 2018 18:28
@@ -335,6 +335,17 @@ fn if_same_then_else() -> Result<&'static str, ()> {
let foo = "";
return Ok(&foo[0..]);
}

// See #3559
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd like this line to be slightly more descriptive, something like
// false positive if_same_then_else, let(x,y) vs let(y,x), see #3559

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed that. Thanks for feedback!

@jorpic jorpic force-pushed the i3559-if_same_then_else branch from aebc9d9 to 911a752 Compare December 30, 2018 11:02
@phansch
Copy link
Member

phansch commented Dec 31, 2018

@bors r+ thanks!

@bors
Copy link
Contributor

bors commented Dec 31, 2018

📌 Commit 911a752 has been approved by phansch

@phansch phansch removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 31, 2018
@bors
Copy link
Contributor

bors commented Dec 31, 2018

⌛ Testing commit 911a752 with merge 6f39128...

bors added a commit that referenced this pull request Dec 31, 2018
Fix if_same_then_else false positive

This fixes false positive in #3559.
The problem was that `SpanlessEq` does not check patterns in declarations. So this two blocks considered equal.
```rust
if true {
    let (x, y) = foo();
} else {
   let (y, x) = foo();
}
```
Not sure if the proposed change is safe as `SpanlessEq` is used extensively in other lints, but I tried hard to come up with counterexample and failed.
@bors
Copy link
Contributor

bors commented Dec 31, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: phansch
Pushing 6f39128 to master...

@bors bors merged commit 911a752 into rust-lang:master Dec 31, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants