-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Account for macros when suggesting a new let binding #114178
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
@@ -2133,13 +2133,14 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { | |||
self.current -= 1; | |||
} | |||
fn visit_expr(&mut self, expr: &hir::Expr<'tcx>) { | |||
if self.span == expr.span { | |||
if self.span == expr.span.source_callsite() { |
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.
Can you explain what this is actually doing? It's not immediately clear what the failure mode was previously before this PR.
Side-note, I wonder if this should be using Span::find_ancestor_inside
instead of source_callsite
, perhaps to deal with multiply nested macro exprs.
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.
source_callsite
is recursive as well, from what I could tell.
When you have let x = vec![1, 2, 3].iter();
, the span in the obligation corresponds to the expression inside the macro, which points into stdlib. By calling source_callsite
we get the span to vec![1, 2, 3]
. We then also account for it in the visitor. The reason it wasn't found before was mainly because we also checked that the span we saw was within the statement, if it wasn't we just bailed in line 2157.
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 may be purely academic, but the usage of Span::*_callsite
functions (which are absolute, in a sense) instead of Span::find_ancestor_*
(which are relative) means that we somewhat arbitrarily provide poorer suggestions for code like:
macro_rules! foo {
() => {
fn main() {
let mut x = vec![1].iter();
x.next();
}
};
}
foo!();
But whatever I guess. Just wanted to note that using source_callsite
/etc is a tiny bit sketchy here.
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.
Ah I see. The fact that it recursively climbs is the problem you wanted to highlight. That's fair. I was thinking of the opposite case, where lets say vec
is actually nested a few macros deep before actually expanding to an expression, where we wouldn't suggest the binding. I guess either one or the other works, until we start properly tracking the expression instead of trying to reconstruct things from a stray span.
Not sure what is the status here, given the discussion above. |
@petrochenkov I would lean towards landing with the current approach, at the cost of not providing a structured suggestion within macros. |
@bors r+ |
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#114178 (Account for macros when suggesting a new let binding) - rust-lang#114199 (Don't unsize coerce infer vars in select in new solver) - rust-lang#114301 (Don't check unnecessarily that impl trait is RPIT) - rust-lang#114314 (Tweaks to `adt_sized_constraint`) - rust-lang#114322 (Fix invalid slice coercion suggestion reported in turbofish) - rust-lang#114340 ([rustc_attr][nit] Replace `filter` + `is_some` with `map_or`.) r? `@ghost` `@rustbot` modify labels: rollup
Provide a structured suggestion when the expression comes from a macro expansion: