Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Account for macros when suggesting a new let binding #114178

Merged
merged 1 commit into from
Aug 2, 2023
Merged

Account for macros when suggesting a new let binding #114178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Member

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.

Copy link
Contributor Author

@estebank estebank Jul 28, 2023 •
edited
Loading

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.

Copy link
Member

@compiler-errors compiler-errors Jul 28, 2023 •
edited
Loading

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.

Copy link
Contributor Author

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.

self.found = self.current;
}
walk_expr(self, expr);
}
}
let source_info = self.body.source_info(location);
let proper_span = proper_span.source_callsite();
if let Some(scope) = self.body.source_scopes.get(source_info.scope)
&& let ClearCrossCrate::Set(scope_data) = &scope.local_data
&& let Some(node) = self.infcx.tcx.hir().find(scope_data.lint_root)
Expand Down
6 changes: 5 additions & 1 deletion tests/ui/borrowck/borrowck-borrowed-uniq-rvalue-2.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,12 @@ LL | let x = defer(&vec!["Goodbye", "world!"]);
LL | x.x[0];
| ------ borrow later used here
|
= note: consider using a `let` binding to create a longer lived value
= note: this error originates in the macro `vec` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider using a `let` binding to create a longer lived value
|
LL ~ let binding = vec!["Goodbye", "world!"];
LL ~ let x = defer(&binding);
|

error: aborting due to previous error

Expand Down
6 changes: 5 additions & 1 deletion tests/ui/lifetimes/borrowck-let-suggestion.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@ LL |
LL | x.use_mut();
| - borrow later used here
|
= note: consider using a `let` binding to create a longer lived value
= note: this error originates in the macro `vec` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider using a `let` binding to create a longer lived value
|
LL ~ let binding = vec![1];
LL ~ let mut x = binding.iter();
|

error: aborting due to previous error

Expand Down
6 changes: 5 additions & 1 deletion tests/ui/pin-macro/lifetime_errors_on_promotion_misusage.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@ LL |
LL | stuff(phantom_pinned)
| -------------- borrow later used here
|
= note: consider using a `let` binding to create a longer lived value
= note: this error originates in the macro `pin` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider using a `let` binding to create a longer lived value
|
LL ~ let binding = pin!(PhantomPinned);
LL ~ let phantom_pinned = identity(binding);
|

error[E0716]: temporary value dropped while borrowed
--> $DIR/lifetime_errors_on_promotion_misusage.rs:18:30
Expand Down
Loading