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.

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

Fix closure migration suggestion when the body is a macro. #87956

Merged
merged 4 commits into from
Aug 13, 2021

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Aug 12, 2021

Fixes #87955

Before:

warning: changes to closure capture in Rust 2021 will affect drop order
 --> src/main.rs:5:13
  |
5 |     let _ = || panic!(a.0);
  |             ^^^^^^^^^^---^
  |                       |
  |                       in Rust 2018, closure captures all of `a`, but in Rust 2021, it only captures `a.0`
6 | }
  | - in Rust 2018, `a` would be dropped here, but in Rust 2021, only `a.0` would be dropped here alongside the closure
  |

help: add a dummy let to cause `a` to be fully captured
  |
20~     ($msg:expr $(,)?) => ({ let _ = &a; 
21+         $crate::rt::begin_panic($msg)
22~     }),
  |

After:

warning: changes to closure capture in Rust 2021 will affect drop order
 --> src/main.rs:5:13
  |
5 |     let _ = || panic!(a.0);
  |             ^^^^^^^^^^---^
  |                       |
  |                       in Rust 2018, closure captures all of `a`, but in Rust 2021, it only captures `a.0`
6 | }
  | - in Rust 2018, `a` would be dropped here, but in Rust 2021, only `a.0` would be dropped here alongside the closure
  |
help: add a dummy let to cause `a` to be fully captured
  |
5 |     let _ = || { let _ = &a; panic!(a.0) };
  |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~

@m-ou-se m-ou-se added A-closures Area: Closures (`|…| { … }`) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. D-edition Diagnostics: An error or lint that should account for edition differences. labels Aug 12, 2021
@rust-highfive
Copy link
Collaborator

r? @LeSeulArtichaut

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 12, 2021
@m-ou-se m-ou-se added the A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` label Aug 12, 2021
@LeSeulArtichaut
Copy link
Contributor

r? @Aaron1011 do you want to take this?

// invocation, as the body is not contained inside the
// closure span. In that case, we walk up the expansion
// until we find the span before the expansion.
while !closure_body_span.is_dummy() && !closure_span.contains(closure_body_span) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we end with a dummy span, then the overall diagnostic will end up worse than it currently is. Can you restore closure_body_span to its original value if the loop ends up setting it to DUMMY_SP?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept the dummy span for the suggestion part. Otherwise .span_to_snippet is going to return the wrong snippet and the suggestion will be wrong and confusing. The fallback span now uses the entire closure, which chas the highest chance of pointing to the place where modifications need to be made.

| in Rust 2018, closure captures all of `a`, but in Rust 2021, it only captures `a.0`
...
LL | }
| - in Rust 2018, `a` would be dropped here, but in Rust 2021, only `a.0` would be dropped here alongside the closure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing: I think saying 'as part as the closure' would make more sense than 'alongside the closure'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aaron1011
Copy link
Member

r=me, unless you want to change the note I mentioned in this PR as well.

@m-ou-se
Copy link
Member Author

m-ou-se commented Aug 12, 2021

@bors r=Aaron1011

@bors
Copy link
Contributor

bors commented Aug 12, 2021

📌 Commit 26c590d has been approved by Aaron1011

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 12, 2021
@bors
Copy link
Contributor

bors commented Aug 13, 2021

⌛ Testing commit 26c590d with merge 2fc3c69...

@bors
Copy link
Contributor

bors commented Aug 13, 2021

☀️ Test successful - checks-actions
Approved by: Aaron1011
Pushing 2fc3c69 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 13, 2021
@bors bors merged commit 2fc3c69 into rust-lang:master Aug 13, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 13, 2021
@m-ou-se m-ou-se deleted the closure-migration-macro-body branch August 13, 2021 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: Closures (`|…| { … }`) A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-edition Diagnostics: An error or lint that should account for edition differences. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rust_2021_incompatible_closure_captures suggestion expands macro
6 participants