-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Clean up "pattern doesn't bind x" messages #39713
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
3d144f0
to
738b0f3
Compare
This is still inconsistent I think. I don't understand why the first error message is any different than the second one. If first one reports that a is missing from the second and third patterns, second shoyuld return that b is missing from first, third, and fourth pattern in the exact same way. I don't see why would you say the same thing in a different way second time. |
963ee74
to
90aa9b2
Compare
@estebank - any thoughts on the proposal at #39698 (comment) ? |
90aa9b2
to
fa3e19f
Compare
@jonathandturner the current output looks this way, which I believe is what you were proposing in that comment: |
It's a minor thing, but do you think it's worth sorting the pattern numbers? Instead of: We could have: |
In the example in the screenshot, can we avoid printing a message for variable Likewise for |
@estebank Looking good! There are some errors in the output now, though. The first error's text talks about variable |
@estebank - is that latest screenshot the real thing? If so, the second error looks wrong. Shouldn't it be 'c'? |
They're actually all wrong. :) There's a mismatch between the text and spans, but at least the spans in isolation look good now (i.e. blue points to variables and corresponding red points to patterns missing those variables). |
@estebank Looks great! My only remaining question is whether we should have |
Yeah, looks good. Once we get the travis stuff sorted will review. Is that specific to this PR or a general travis issue? |
I'd lean towards keeping the label.
It's a regression:
I just fixed it, and now the diagnostic actually checks for mismatched bindings in patterns other than the first one: instead of |
Great! Is this ready for review now? |
Yes, it is 😃 |
@estebank - seems like the failure is potentially legit, as if the error still isn't quite deterministic https://ci.appveyor.com/project/rust-lang/rust/build/1.0.2047/job/bys2pl5ekyaomw69 |
fc22cb8
to
b0d0272
Compare
@jonathandturner sorry for the slow turn around, was offline for the past two weeks. The code is now sorting all spans, to avoid differences caused by different orderings when traversing HashMaps across platforms. I believe this should be enough. While I was at it, I also rebased and squashed against last master. |
@estebank You can always use |
@jonathandturner sure! @eddyb I changed the code to use Current colored output for reference: |
📌 Commit 780bea5 has been approved by |
…rner Clean up "pattern doesn't bind x" messages Group "missing variable bind" spans in `or` matches and clarify wording for the two possible cases: when a variable from the first pattern is not in any of the subsequent patterns, and when a variable in any of the other patterns is not in the first one. Before: ```rust error[E0408]: variable `a` from pattern #1 is not bound in pattern rust-lang#2 --> file.rs:10:23 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^^^^ pattern doesn't bind `a` error[E0408]: variable `b` from pattern rust-lang#2 is not bound in pattern #1 --> file.rs:10:32 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^ pattern doesn't bind `b` error[E0408]: variable `a` from pattern #1 is not bound in pattern rust-lang#3 --> file.rs:10:37 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^ pattern doesn't bind `a` error[E0408]: variable `d` from pattern #1 is not bound in pattern rust-lang#3 --> file.rs:10:37 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^ pattern doesn't bind `d` error[E0408]: variable `c` from pattern rust-lang#3 is not bound in pattern #1 --> file.rs:10:43 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^ pattern doesn't bind `c` error[E0408]: variable `d` from pattern #1 is not bound in pattern rust-lang#4 --> file.rs:10:48 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^ pattern doesn't bind `d` error: aborting due to 6 previous errors ``` After: ```rust error[E0408]: variable `d` is not bound in all patterns --> $DIR/issue-39698.rs:20:37 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | - - ^^^^^^^^ ^^^^^^^^ pattern doesn't bind `d` | | | | | | | pattern doesn't bind `d` | | variable not in all patterns | variable not in all patterns error[E0408]: variable `c` is not bound in all patterns --> $DIR/issue-39698.rs:20:48 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^^^^ ^^^^^^^^^^^ - ^^^^^^^^ pattern doesn't bind `c` | | | | | | | variable not in all patterns | | pattern doesn't bind `c` | pattern doesn't bind `c` error[E0408]: variable `a` is not bound in all patterns --> $DIR/issue-39698.rs:20:37 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | - ^^^^^^^^^^^ ^^^^^^^^ - variable not in all patterns | | | | | | | pattern doesn't bind `a` | | pattern doesn't bind `a` | variable not in all patterns error[E0408]: variable `b` is not bound in all patterns --> $DIR/issue-39698.rs:20:37 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^^^^ - ^^^^^^^^ ^^^^^^^^ pattern doesn't bind `b` | | | | | | | pattern doesn't bind `b` | | variable not in all patterns | pattern doesn't bind `b` error: aborting due to 4 previous errors ``` Fixes rust-lang#39698.
🔒 Merge conflict |
Group "missing variable bind" spans in `or` matches and clarify wording for the two possible cases: when a variable from the first pattern is not in any of the subsequent patterns, and when a variable in any of the other patterns is not in the first one. Before: ``` error[E0408]: variable `a` from pattern rust-lang#1 is not bound in pattern rust-lang#2 --> file.rs:10:23 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^^^^ pattern doesn't bind `a` error[E0408]: variable `b` from pattern rust-lang#2 is not bound in pattern rust-lang#1 --> file.rs:10:32 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^ pattern doesn't bind `b` error[E0408]: variable `a` from pattern rust-lang#1 is not bound in pattern rust-lang#3 --> file.rs:10:37 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^ pattern doesn't bind `a` error[E0408]: variable `d` from pattern rust-lang#1 is not bound in pattern rust-lang#3 --> file.rs:10:37 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^ pattern doesn't bind `d` error[E0408]: variable `c` from pattern rust-lang#3 is not bound in pattern rust-lang#1 --> file.rs:10:43 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^ pattern doesn't bind `c` error[E0408]: variable `d` from pattern rust-lang#1 is not bound in pattern rust-lang#4 --> file.rs:10:48 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^ pattern doesn't bind `d` error: aborting due to 6 previous errors ``` After: ``` error[E0408]: variable `a` is not bound in all patterns --> file.rs:20:37 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { intln!("{:?}", a); } | - ^^^^^^^^^^^ ^^^^^^^^ - variable t in all patterns | | | | | | | pattern doesn't bind `a` | | pattern doesn't bind `a` | variable not in all patterns error[E0408]: variable `d` is not bound in all patterns --> file.rs:20:37 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { intln!("{:?}", a); } | - - ^^^^^^^^ ^^^^^^^^ pattern esn't bind `d` | | | | | | | pattern doesn't bind `d` | | variable not in all patterns | variable not in all patterns error[E0408]: variable `b` is not bound in all patterns --> file.rs:20:37 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { intln!("{:?}", a); } | ^^^^^^^^^^^ - ^^^^^^^^ ^^^^^^^^ pattern esn't bind `b` | | | | | | | pattern doesn't bind `b` | | variable not in all patterns | pattern doesn't bind `b` error[E0408]: variable `c` is not bound in all patterns --> file.rs:20:48 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { intln!("{:?}", a); } | ^^^^^^^^^^^ ^^^^^^^^^^^ - ^^^^^^^^ pattern esn't bind `c` | | | | | | | variable not in all tterns | | pattern doesn't bind `c` | pattern doesn't bind `c` error: aborting due to 4 previous errors ``` * Have only one presentation for binding consistency errors * Point to same binding in multiple patterns when possible * Check inconsistent bindings in all arms * Simplify wording of diagnostic message * Sort emition and spans of binding errors for deterministic output
Rebased against master HEAD because of the bors merge conflict message, but wasn't necessary. |
@bors: r=jonathandturner |
📌 Commit 3ffa4b5 has been approved by |
⌛ Testing commit 3ffa4b5 with merge 2e6413c... |
💔 Test failed - status-travis |
… On Tue, Mar 7, 2017 at 12:08 PM, bors ***@***.***> wrote:
💔 Test failed - status-travis
<https://travis-ci.org/rust-lang/rust/builds/208653831>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#39713 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAD95AeIdXd99eLx7aikJGD-6E7M7ar6ks5rjZ0KgaJpZM4L86IO>
.
|
Clean up "pattern doesn't bind x" messages Group "missing variable bind" spans in `or` matches and clarify wording for the two possible cases: when a variable from the first pattern is not in any of the subsequent patterns, and when a variable in any of the other patterns is not in the first one. Before: ```rust error[E0408]: variable `a` from pattern #1 is not bound in pattern #2 --> file.rs:10:23 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^^^^ pattern doesn't bind `a` error[E0408]: variable `b` from pattern #2 is not bound in pattern #1 --> file.rs:10:32 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^ pattern doesn't bind `b` error[E0408]: variable `a` from pattern #1 is not bound in pattern #3 --> file.rs:10:37 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^ pattern doesn't bind `a` error[E0408]: variable `d` from pattern #1 is not bound in pattern #3 --> file.rs:10:37 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^ pattern doesn't bind `d` error[E0408]: variable `c` from pattern #3 is not bound in pattern #1 --> file.rs:10:43 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^ pattern doesn't bind `c` error[E0408]: variable `d` from pattern #1 is not bound in pattern #4 --> file.rs:10:48 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^ pattern doesn't bind `d` error: aborting due to 6 previous errors ``` After: ```rust error[E0408]: variable `d` is not bound in all patterns --> $DIR/issue-39698.rs:20:37 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | - - ^^^^^^^^ ^^^^^^^^ pattern doesn't bind `d` | | | | | | | pattern doesn't bind `d` | | variable not in all patterns | variable not in all patterns error[E0408]: variable `c` is not bound in all patterns --> $DIR/issue-39698.rs:20:48 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^^^^ ^^^^^^^^^^^ - ^^^^^^^^ pattern doesn't bind `c` | | | | | | | variable not in all patterns | | pattern doesn't bind `c` | pattern doesn't bind `c` error[E0408]: variable `a` is not bound in all patterns --> $DIR/issue-39698.rs:20:37 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | - ^^^^^^^^^^^ ^^^^^^^^ - variable not in all patterns | | | | | | | pattern doesn't bind `a` | | pattern doesn't bind `a` | variable not in all patterns error[E0408]: variable `b` is not bound in all patterns --> $DIR/issue-39698.rs:20:37 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^^^^ - ^^^^^^^^ ^^^^^^^^ pattern doesn't bind `b` | | | | | | | pattern doesn't bind `b` | | variable not in all patterns | pattern doesn't bind `b` error: aborting due to 4 previous errors ``` Fixes #39698.
☀️ Test successful - status-appveyor, status-travis |
Group "missing variable bind" spans in
or
matches and clarify wordingfor the two possible cases: when a variable from the first pattern is
not in any of the subsequent patterns, and when a variable in any of the
other patterns is not in the first one.
Before:
After:
Fixes #39698.