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

Improve closure dummy capture suggestion in macros. #88543

Merged

Conversation

m-ou-se
Copy link
Member

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

@m-ou-se m-ou-se added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. A-edition-2021 Area: The 2021 edition labels Aug 31, 2021
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(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 31, 2021
@michaelwoerister
Copy link
Member

r? @estebank

Comment on lines +2 to +12
--> $DIR/closure-body-macro-fragment.rs:8:17
|
LL | let f = || $body;
| _________________^
LL | |
LL | | f();
LL | | }};
| | - in Rust 2018, `a` is dropped here, but in Rust 2021, only `a.0` will be dropped here as part of the closure
LL | | ($body:block) => {{
LL | | m!(@ $body);
| |__________________^
Copy link
Contributor

Choose a reason for hiding this comment

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

That's... an unfortunate span.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, some weird things are going on when mixing :block and :expr fragments. And closures just use their first and last token to create their full span, which isn't always great. Something to improve another time. ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh aboslutely, mixed spans involving macros have long standing issues :)

= note: this warning originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info)
help: add a dummy let to cause `a` to be fully captured
|
LL ~ m!({
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm intrigued at why this line is shown to have a change. The correct fix might be changing the suggestion printing machinery to verify that before and after are actually different.

Copy link
Member Author

Choose a reason for hiding this comment

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

It got a \n appended to it. I'm using the span at the end of this line to add a "\n let _ = .." suggestion. I guess the \n is counted as part of this line.

@estebank
Copy link
Contributor

estebank commented Sep 1, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Sep 1, 2021

📌 Commit 7d18052 has been approved by estebank

@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 Sep 1, 2021

if let Ok(mut s) = self.tcx.sess.source_map().span_to_snippet(closure_body_span) {
if s.starts_with('$') {
// Looks like a macro fragment. Try to find the real block.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can/should we walk up the backtrace in the expansion instead of this? I guess this works, I'm thinking to think if there is some kind of "catch".

Copy link
Member Author

Choose a reason for hiding this comment

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

We track things like "1 is expanded from foo!()" (which was and is already used here), but we don't track "1 was substituted from $a", which is sort-of the opposite, and the problem here.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I see.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 1, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 1, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 2, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 2, 2021
Rollup of 12 pull requests

Successful merges:

 - rust-lang#88177 (Stabilize std::os::unix::fs::chroot)
 - rust-lang#88505 (Use `unwrap_unchecked` where possible)
 - rust-lang#88512 (Upgrade array_into_iter lint to include Deref-to-array types.)
 - rust-lang#88532 (Remove single use variables)
 - rust-lang#88543 (Improve closure dummy capture suggestion in macros.)
 - rust-lang#88560 (`fmt::Formatter::pad`: don't call chars().count() more than one time)
 - rust-lang#88565 (Add regression test for issue 83190)
 - rust-lang#88567 (Remove redundant `Span` in `QueryJobInfo`)
 - rust-lang#88573 (rustdoc: Don't panic on ambiguous inherent associated types)
 - rust-lang#88582 (Implement rust-lang#88581)
 - rust-lang#88589 (Correct doc comments inside `use_expr_visitor.rs`)
 - rust-lang#88592 (Fix ICE in const check)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ffbce26 into rust-lang:master Sep 2, 2021
@rustbot rustbot added this to the 1.56.0 milestone Sep 2, 2021
@m-ou-se m-ou-se deleted the closure-migration-macro-block-fragment branch September 2, 2021 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2021 Area: The 2021 edition A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. 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.

7 participants