Point at const definition when used instead of a binding in a let statement
#132708
Conversation
rustbot
added
S-waiting-on-review
|
Some changes occurred in match lowering cc @Nadrieril Some changes occurred in exhaustiveness checking cc @Nadrieril Some changes occurred in match checking cc @Nadrieril Some changes occurred in cc @BoxyUwU |
There was a problem hiding this comment.
This PR needs a lot more explanation that details why we need to add new THIR, or better yet, it could use some more investigation towards how we can avoid needing to add THIR at all -- since this is for the diagnostic path. Like, why can't we just extract the def span of the const from the UnevaluatedConst that is inside the mir::Const?
Adding new THIR nodes seems like it's ripe for adding confusion to an already confusing area of the compiler, and this PR has no explanation or documentation other than a "before" and "after". I'm not certain if it's worth the overhead to add all this to the compiler to improve this diagnostic if this is the only way to do it.
|
Even if we need to end up plumbing this span separately, why not just add an |
Because by the time the error occurs, it is no longer an
It's providing additional context to the output from what we have today. Today the output can be quite confusing, particularly if the constant is defined far away with a non-standard identifier, and it can be even worse if it happens due to a re-export that renames it. The error as it is today takes me a triple take to understand what it was telling me about.
I'm more than happy to hear about alternative ways to do this. I disagree that the output as is today is reasonable. I'd go as far as saying it is inescrutable for anyone who doesn't yet know about the handling of
I'm ok with that alternative. |
|
All of that information would've been awesome to include in the PR description. I hope you don't take this in the wrong way, but it's a bit annoying from a reviewer's perspective (and kinda leads to additional work on the reviewer) to have to try to deduce the reasoning that went into this approach from basically no information, or to have to leave a bunch of confused questions and go back and forth on a semi-synchronous github convo. This is especially true when it touches something very important like THIR, as opposed to diagnostics PRs that generally stay out of the way of the good path. Divergence in the way we handle
I think you're misinterpreting me. What I mean is that I'm not certain if the approach is worth the overhead. I'm not saying that the output today is reasonable -- what I'm saying is that I'm not certain if we should land this approach, as opposed to deferring fixing this in favor of investigating a better solution for this.
Well, if you plumb in the |
| subpattern: box Pat { kind: PatKind::Constant { opt_def: Some(def_id), .. }, .. }, | ||
| .. | ||
| } = pat.kind | ||
| && let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(pat.span) |
There was a problem hiding this comment.
Ah, you read my mind with the DefId -- but we can take it further and use the tcx.item_name here to get the name of the constant, rather than using snippets. This will regress reexported constants, but is more resilient IMO and is worth the change.
There was a problem hiding this comment.
Specifically, this pat name will not possibly be tied to arbitrary source code, in pat_macro!(/* uwu */).
|
This is a pleasant diagnostic improvement. Am I correct to understand that this only catches constants of primitive type? Is there a way to catch also const PAT: (u32, u32) = ...;
let PAT = ...;? |
|
@Nadrieril for that case, the output is Looking at the error it seems it is because it is checking the sub-pattern for the first tuple item. I can take a look to see if we can make it work for tuples (and arrays, and structs). For enums we already have reasonable output before this PR: |
|
It would be nice to have it point at the definition of the constant in these cases too. Once we get to |
|
@Nadrieril We do have |
|
If we rename |
|
@Nadrieril there's one wrinkle there: |
|
We can store a DefId and add a comment "if |
estebank
force-pushed
the
const-as-binding
branch
from
447b212
to
d02ba09
Compare
| PatKind::ExpandedConstant { subpattern: ref pattern, def_id: _, is_inline: false } => { | ||
| subpairs.push(MatchPairTree::for_pattern(place_builder, pattern, cx)); | ||
| default_irrefutable() | ||
| } | ||
| PatKind::ExpandedConstant { subpattern: ref pattern, def_id, is_inline: true } => { |
| PatKind::ExpandedConstant { def_id, is_inline, .. } => { | ||
| if let Some(def) = def_id.as_local() | ||
| && *is_inline | ||
| { | ||
| self.visit_inner_body(def); | ||
| } |
There was a problem hiding this comment.
...and here are the two places where is_inline has any effect in the constant evaluation. @compiler-errors can you take a look to see if this is a reasonable level of complexity? We can also add comments explaining why is_inline (or rather, why we don't want to evaluate the unsafety of const items). The current approach has the nice property of not making the types any larger than they were.
| // We only want to mark constants when referenced as bare names that could have been | ||
| // new bindings if the `const` didn't exist. |
There was a problem hiding this comment.
Hm, I assumed we'd be indiscriminately recording all constant expansions and would filter later. I'm not sure which is best, but at least please mention this choice in the PatKind variant.
There was a problem hiding this comment.
Instead of only setting them in this case we could add another flag to the variant to track this and not suggest if the flag is set.
There was a problem hiding this comment.
That would be an improvement imo. I would still prefer to avoid carrying a boolean in THIR for a single diagnostic suggestion tbh, depends how hard it is to do without the boolean.
There was a problem hiding this comment.
My preference would be: ExpandedConst { def_id, subpattern } and do the span_to_snippet(subpattern.span).chars().all(is_alphanumeric) dance for diagnostics.
There was a problem hiding this comment.
Let me know what you think with the last commit.
There was a problem hiding this comment.
That's what I had in mind, ty. Could you add the ExpandedConstant layer inside const_to_pat by any chance? To be sure we don't miss one
There was a problem hiding this comment.
I looked at const_to_pat, it wouldn't be super clean to add it there. I'm therefore ok with the current way of doing it.
| LL | | ||
| LL | E_SL => {} | ||
| | ---- this pattern doesn't introduce a new catch-all binding, but rather pattern matches against the value of constant `E_SL` | ||
| | | ||
| = note: the matched value is of type `&[E]` | ||
| note: constant `E_SL` defined here | ||
| --> $DIR/incomplete-slice.rs:6:1 | ||
| | | ||
| LL | const E_SL: &[E] = &[E::A]; | ||
| | ^^^^^^^^^^^^^^^^ | ||
| help: if you meant to introduce a binding, use a different name | ||
| | | ||
| LL | E_SL_var => {} | ||
| | ++++ |
|
In terms of additional complexity, this now seems perfectly fine. The one thing I have mixed feelings about is that this only adds an |
estebank
force-pushed
the
const-as-binding
branch
from
cdbfcfc
to
83bf37e
Compare
estebank
force-pushed
the
const-as-binding
branch
from
d9cb691
to
3f84747
Compare
This comment has been minimized.
This comment has been minimized.
|
I'm ok with this now! It's quite a nice diagnostic improvement. I think you have to fix THIR parsing, and I left one suggestion about a comment. |
Nadrieril
added
S-waiting-on-author
… statement After: ``` error[E0005]: refutable pattern in local binding --> $DIR/bad-pattern.rs:19:13 | LL | const PAT: u32 = 0; | -------------- missing patterns are not covered because `PAT` is interpreted as a constant pattern, not a new variable ... LL | let PAT = v1; | ^^^ | | | pattern `1_u32..=u32::MAX` not covered | help: introduce a variable instead: `PAT_var` | = note: `let` bindings require an "irrefutable pattern", like a `struct` or an `enum` with only one variant = note: for more information, visit https://doc.rust-lang.org/book/ch18-02-refutability.html = note: the matched value is of type `u32` ``` Before: ``` error[E0005]: refutable pattern in local binding --> $DIR/bad-pattern.rs:19:13 | LL | let PAT = v1; | ^^^ | | | pattern `1_u32..=u32::MAX` not covered | missing patterns are not covered because `PAT` is interpreted as a constant pattern, not a new variable | help: introduce a variable instead: `PAT_var` | = note: `let` bindings require an "irrefutable pattern", like a `struct` or an `enum` with only one variant = note: for more information, visit https://doc.rust-lang.org/book/ch18-02-refutability.html = note: the matched value is of type `u32` ```
```
error[E0004]: non-exhaustive patterns: `i32::MIN..=3_i32` and `5_i32..=i32::MAX` not covered
--> $DIR/intended-binding-pattern-is-const.rs:2:11
|
LL | match 1 {
| ^ patterns `i32::MIN..=3_i32` and `5_i32..=i32::MAX` not covered
LL | x => {}
| - this pattern doesn't introduce a new catch-all binding, but rather pattern matches against the value of constant `x`
|
= note: the matched value is of type `i32`
note: constant `x` defined here
--> $DIR/intended-binding-pattern-is-const.rs:7:5
|
LL | const x: i32 = 4;
| ^^^^^^^^^^^^
help: if you meant to introduce a binding, use a different name
|
LL | x_var => {}
| ++++
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern, a match arm with multiple or-patterns as shown, or multiple match arms
|
LL | x => {}, i32::MIN..=3_i32 | 5_i32..=i32::MAX => todo!()
| ++++++++++++++++++++++++++++++++++++++++++++++++
```
- Remove check for "how many path segments is the pattern" - Check before suggesting if the path has multiple path segments
estebank
force-pushed
the
const-as-binding
branch
from
3f84747
to
29acf8b
Compare
estebank
added
S-waiting-on-review
|
Ty! @bors r+ |
bors
added
S-waiting-on-bors
…eril Point at `const` definition when used instead of a binding in a `let` statement Modify `PatKind::InlineConstant` to be `ExpandedConstant` standing in not only for inline `const` blocks but also for `const` items. This allows us to track named `const`s used in patterns when the pattern is a single binding. When we detect that there is a refutable pattern involving a `const` that could have been a binding instead, we point at the `const` item, and suggest renaming. We do this for both `let` bindings and `match` expressions missing a catch-all arm if there's at least one single binding pattern referenced. After: ``` error[E0005]: refutable pattern in local binding --> $DIR/bad-pattern.rs:19:13 | LL | const PAT: u32 = 0; | -------------- missing patterns are not covered because `PAT` is interpreted as a constant pattern, not a new variable ... LL | let PAT = v1; | ^^^ pattern `1_u32..=u32::MAX` not covered | = note: `let` bindings require an "irrefutable pattern", like a `struct` or an `enum` with only one variant = note: for more information, visit https://doc.rust-lang.org/book/ch18-02-refutability.html = note: the matched value is of type `u32` help: introduce a variable instead | LL | let PAT_var = v1; | ~~~~~~~ ``` Before: ``` error[E0005]: refutable pattern in local binding --> $DIR/bad-pattern.rs:19:13 | LL | let PAT = v1; | ^^^ | | | pattern `1_u32..=u32::MAX` not covered | missing patterns are not covered because `PAT` is interpreted as a constant pattern, not a new variable | help: introduce a variable instead: `PAT_var` | = note: `let` bindings require an "irrefutable pattern", like a `struct` or an `enum` with only one variant = note: for more information, visit https://doc.rust-lang.org/book/ch18-02-refutability.html = note: the matched value is of type `u32` ``` CC rust-lang#132582.
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#129838 (uefi: process: Add args support) - rust-lang#130800 (Mark `get_mut` and `set_position` in `std::io::Cursor` as const.) - rust-lang#132708 (Point at `const` definition when used instead of a binding in a `let` statement) - rust-lang#133226 (Make `PointerLike` opt-in instead of built-in) - rust-lang#133244 (Account for `wasm32v1-none` when exporting TLS symbols) - rust-lang#133257 (Add `UnordMap::clear` method) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#132708 - estebank:const-as-binding, r=Nadrieril Point at `const` definition when used instead of a binding in a `let` statement Modify `PatKind::InlineConstant` to be `ExpandedConstant` standing in not only for inline `const` blocks but also for `const` items. This allows us to track named `const`s used in patterns when the pattern is a single binding. When we detect that there is a refutable pattern involving a `const` that could have been a binding instead, we point at the `const` item, and suggest renaming. We do this for both `let` bindings and `match` expressions missing a catch-all arm if there's at least one single binding pattern referenced. After: ``` error[E0005]: refutable pattern in local binding --> $DIR/bad-pattern.rs:19:13 | LL | const PAT: u32 = 0; | -------------- missing patterns are not covered because `PAT` is interpreted as a constant pattern, not a new variable ... LL | let PAT = v1; | ^^^ pattern `1_u32..=u32::MAX` not covered | = note: `let` bindings require an "irrefutable pattern", like a `struct` or an `enum` with only one variant = note: for more information, visit https://doc.rust-lang.org/book/ch18-02-refutability.html = note: the matched value is of type `u32` help: introduce a variable instead | LL | let PAT_var = v1; | ~~~~~~~ ``` Before: ``` error[E0005]: refutable pattern in local binding --> $DIR/bad-pattern.rs:19:13 | LL | let PAT = v1; | ^^^ | | | pattern `1_u32..=u32::MAX` not covered | missing patterns are not covered because `PAT` is interpreted as a constant pattern, not a new variable | help: introduce a variable instead: `PAT_var` | = note: `let` bindings require an "irrefutable pattern", like a `struct` or an `enum` with only one variant = note: for more information, visit https://doc.rust-lang.org/book/ch18-02-refutability.html = note: the matched value is of type `u32` ``` CC rust-lang#132582.
Modify
PatKind::InlineConstantto beExpandedConstantstanding in not only for inlineconstblocks but also forconstitems. This allows us to track namedconsts used in patterns when the pattern is a single binding. When we detect that there is a refutable pattern involving aconstthat could have been a binding instead, we point at theconstitem, and suggest renaming. We do this for bothletbindings andmatchexpressions missing a catch-all arm if there's at least one single binding pattern referenced.After:
Before:
CC #132582.