Suggest struct pattern when destructuring Range with .. syntax #149952
Conversation
rustbot
added
S-waiting-on-review
|
r? @Alexendoo |
|
I'm not part of the compiler team so I'll pass it back over to r? @lcnr |
| err.span_suggestion_verbose( | ||
| pat.span, | ||
| "if you meant to destructure a `Range`, use the struct pattern", | ||
| format!("std::ops::Range {{ start: {}, end: {} }}", start_name, end_name), |
There was a problem hiding this comment.
It'd be helpful if this suggested std::range::Range/core::range::Range when #![feature(new_range)] is enabled (might not be necessary since new_range is still unstable).
There was a problem hiding this comment.
Regarding new_range: I'm sticking with std::ops::Range for now since it's the stable default, but we can revisit that once the feature stabilizes.
There was a problem hiding this comment.
Could you please add a // FIXME(new_range): Also account for new range types?
| err.span_suggestion_verbose( | ||
| pat.span, | ||
| "if you meant to destructure a `Range`, use a struct pattern", | ||
| format!("std::ops::Range {{ start: {}, end: {} }}", start_name, end_name), |
There was a problem hiding this comment.
Instead of unconditionally suggesting a qualified path, IINM you should be able to take advantage of path trimming (that's a machinery that shortens paths according to what's in scope / imported).
| format!("std::ops::Range {{ start: {}, end: {} }}", start_name, end_name), | |
| format!("{path} {{ start: {}, end: {} }}", start_name, end_name), |
where path is tcx.def_path_str(range_struct) where range_struct comes from tcx.lang_items().range_struct() (for RangeEnd::Excluded; it should he range_inclusive_struct() for RangeEnd::Included(RangeSyntax::DotDotEq)) if it's Some(_) (bail out if it's None).
I hope this works in rustc_resolve 🤞. If these queries hang, dead-lock or crash, then they obv can't be used in the resolver since the info isn't available yet. If it comes to that, there might be alternative Resolver APIs you could use, not sure. Check what other resolver code is doing about lang items.
There was a problem hiding this comment.
I tried implementing path trimming using tcx.lang_items() and def_path_str as suggested, but it caused a cycle detected [E0391] error exactly as you warned (since resolution info isn't ready yet).
Instead, I implemented a safe manual check using self.resolve_path. It checks if the short name (e.g., Range) is visible in the current scope. If it is, I use it; otherwise, I fall back to the fully qualified path (std::ops::Range). Tests confirm this works correctly!
| } | ||
|
|
||
| if let Some(pat) = self.diag_metadata.current_pat | ||
| && let ast::PatKind::Range(Some(start_expr), Some(end_expr), _) = &pat.kind |
There was a problem hiding this comment.
As alluded to in another review comment of mine, you can't ignore the RangeEnd of the PatKind::Range, it's crucial for suggesting the right struct!
There was a problem hiding this comment.
Yes, I'm now matching on end_kind.node to distinguish between RangeEnd::Excluded (suggesting Range) and RangeEnd::Included (suggesting RangeInclusive)
| } | ||
|
|
||
| if let Some(pat) = self.diag_metadata.current_pat | ||
| && let ast::PatKind::Range(Some(start_expr), Some(end_expr), _) = &pat.kind |
There was a problem hiding this comment.
It would be very nice to also handle the open ranges, RangeTo (..b) and RangeFrom (a..) & suggest the appropriate structs.
|
|
||
| err.span_suggestion_verbose( | ||
| pat.span, | ||
| "if you meant to destructure a `Range`, use a struct pattern", |
There was a problem hiding this comment.
| "if you meant to destructure a `Range`, use a struct pattern", | |
| "if you meant to destructure a range, use a struct pattern", |
or
| "if you meant to destructure a `Range`, use a struct pattern", | |
| "if you meant to destructure a `{name}`, use a struct pattern", |
where name is tcx.item_name(range_struct) (see other comments about accounting for RangeInclusive & open ranges). The query item_name might not work in rustc_resolve (it might hang or crash since the data isn't available) but then there should be alternative Resolver APIs for obtaining that.
| && let (ast::ExprKind::Path(None, start_path), ast::ExprKind::Path(None, end_path)) = | ||
| (&start_expr.kind, &end_expr.kind) |
There was a problem hiding this comment.
Note that you're disqualifying cases like if let 1..x = my_range {}. Personally speaking I'm not sure if we want to disqualify this or not 🤔. Note that you're including cases like if let path::to::CONST..x = my_range {} OTOH, so it's a bit inconsistent at the moment.
There was a problem hiding this comment.
I addressed this by switching to span_to_snippet, so the logic now treats literals 1..x and variables exactly the same way.
However, I cannot verify this with a UI test case because writing let 1..x = my_range triggers a hard Type Mismatch error (E0308) the compiler complains that a literal pattern 1 doesn't match the Range struct type which masks the resolution error/suggestion. But the logic to handle it is definitely in place now.
| let start_name = start_path.segments[0].ident; | ||
| let end_name = end_path.segments[0].ident; |
There was a problem hiding this comment.
This is incorrect since you're using an || above. Meaning it's possible that either start_path or end_path has more than one segment but you're still merely retrieving the first segment.
Concretely, it means that you're suggesting std::ops::Range { start: path, end: y } given path::to::x..y (printing path instead of path::to::x).
There was a problem hiding this comment.
I completely removed the segments[0] logic. I replaced it with span_to_snippet
| err.span_suggestion_verbose( | ||
| pat.span, | ||
| "if you meant to destructure a `Range`, use a struct pattern", | ||
| format!("std::ops::Range {{ start: {}, end: {} }}", start_name, end_name), |
There was a problem hiding this comment.
(low priority) in the special case of start..y or x..end (etc.) it would be nice to suggest std::ops::Range { start, end: y } and std::ops::Range { start: x, end } respectively (i.e., leveraging field shorthands).
| && path.len() == 1 | ||
| { | ||
| let ident = path[0].ident; | ||
|
|
||
| if (start_path.segments.len() == 1 && start_path.segments[0].ident == ident) | ||
| || (end_path.segments.len() == 1 && end_path.segments[0].ident == ident) |
There was a problem hiding this comment.
You can use slice pattern [x] to express xs.len() == 1 && let x = xs[0] in one go. It's more robust, too.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
Delta17920
force-pushed
the
fix/149777-range-destructuring
branch
from
37fa028 to
e3f2dc4
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
6 participants
implemented a new diagnostic in rustc_resolve to detect invalid range destructuring attempts (e.g., let start..end = range). The fix identifies when resolution fails for identifiers acting as range bounds specifically handling cases where bounds are parsed as expressions and suggests the correct struct pattern syntax (std::ops::Range { start, end }). This replaces confusing "cannot find value" errors with actionable help, verified by a new UI test covering various identifier names.
Fixes #149777