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

Fix Clippy handling of ExpnKind::Desugaring #73468

Closed
wants to merge 4 commits into from
Closed

Fix Clippy handling of ExpnKind::Desugaring #73468

wants to merge 4 commits into from

Conversation

Aaron1011
Copy link
Member

PR #72389 exposed a latent issue in Clippy: only the top-most ExpnKind was checked for ExpnKind::Macro or ExpnKind::AstPass. Under certain circumstances, this could result in Clippy emitting warnings for code produced by a macro expansion, due to the topmost ExpnId having ExpnKind::Desugaring.

This PR tracks the presence of ExpnKind::Macro and ExpnKind::AstPass in the call_site chain through a new ClosestAstOrMacro. This enum is used in Clippy, allowing all of Clippy's UI tests to pass. As a result, we can re-enable Clippy tests in src/bootstrap/test.rs

Aaron1011 added 3 commits June 18, 2020 00:30
@Aaron1011
Add ClosestAstOrMacro to allow detecting macro expansions
b16f199 
Following PR rust-lang#72389, we create many more spans with
`ExpnKind::Desugaring`. This exposed a latent bug in Clippy - only the
top-most `ExpnData` is considered when checking if code is the result of
a macro expansion. If code emitted by a macro expansion gets annotated
with an `ExpnKind::Desugaring` (e.g. an operator or a for loop), Clippy
will incorrectly act as though this code is not the result of a macro
expansion.

This PR introduces the `ClosestAstOrMacro` enum, which allows linting
code to quickly determine if a given `Span` is the result of a macro
expansion. For any `ExpnId`, we keep track of closest `ExpnKind::Macro`
or `ExpnKind::AstPass` in the `call_site` chain. This is determined when
the `ExpnData` is set for an `ExpnId`, which allows us to avoid walking
the entire chain in Clippy.
@Aaron1011
Use in_macro instead of Span::from_expansion
1416904 
Using `Span::from_expansion` results in false positives - it will return
`true` if have an `ExpnKind::Desugaring`, even if is not the result of a
macro expansion.

Using `ClosestAstOrMacro` from rustc, we can now cheaply determine if a
macro or ast expansion occurs anywhere in the call chain.
@Aaron1011
Re-enable Clippy tests
0f8700a
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(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 Jun 18, 2020
@Aaron1011
Copy link
Member Author

cc @flip1995 @nikomatsakis @Manishearth

@Aaron1011 Aaron1011 changed the title Fix Clippy handling of ExpnKind::Desugard Fix Clippy handling of ExpnKind::Desugaring Jun 18, 2020
Manishearth
Manishearth approved these changes Jun 18, 2020
View reviewed changes
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

clippy side looks great, thanks!

@petrochenkov petrochenkov self-assigned this Jun 18, 2020
@flip1995
Copy link
Member

Hm, there is still the empty_line_after_outer_attribute.rs clippy test, that fails because of something related to proc_macros (it seems).

Also it seems, that Clippy is not tested on CI / PR runs, but only on CI / auto runs. I think we should change that in the future, but this is probably something for a different PR.

@nikomatsakis
Copy link
Contributor

r=me with nit, but @petrochenkov self-assigned, so probably good to wait for them to weigh in

r? @petrochenkov

@Aaron1011
Copy link
Member Author

@flip1995: The ui/empty_line_after_outer_attribute.rs test is passing locally for me with this branch.

@petrochenkov
Copy link
Contributor

I haven't seen #72389 but it appears to be a serious misuse of expansions.
I was pinged on that PR, but unfortunately didn't notice that something named "explain move errors" makes changes to entirely unrelated parts of the compiler.

I need to look at #72389 in more detail and see what it does and whether it can be done without hacks.

(That said, in_external_macro and friends are also a mess.)

@petrochenkov
Copy link
Contributor

I would personally prefer to revert #72389 in the meantime and fix clippy.
Otherwise this unexpected task really messes up my work queue and it would be great to delay it until mid-July.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2020
@Manishearth
Copy link
Member

Yeah like I said before diagnostics should not take priority over existing functionality

@Aaron1011
Copy link
Member Author

I haven't seen #72389 but it appears to be a serious misuse of expansions.

@petrochenkov: Can you elaborate on this? While operators are not 'desugared' in the same way that 'if' statements and 'for' loops are desugared (operators are desugared during MIR building, rather than AST lowering), I believe it fits the way that way that DesugaringKind is used. For both 'for' loops and operators, we have non-user-written method calls being inserted, which can be distinguished from explicit calls (iter.into_iter(), foo.add(rhs)) by examining the Span.

@Manishearth
Copy link
Member

MIR desugaring is not the same as the source-to-source desugaring that occurs in if let, MIR is not rust source. DesugaringKind is specifically for stuff that behaves like macros, and that's why it's a part of spans/etc. MIR does all kinds of transforms; closures become structs, etc, at best you should have a different mechanism for tracking that.

When it comes to diagnostics source-to-source transforms are the ones that cause the most pain and that's why they're handled this way.

I hadn't looked closely at this before but now that I look through it I find myself agreeing with @petrochenkov, it's a mistake to lump MIR transforms into DesugaringKind.

@Aaron1011 Aaron1011 mentioned this pull request Jun 21, 2020
Closed
@Aaron1011
Copy link
Member Author

@Manishearth @petrochenkov: PR #73571 is up, which reverts just the operator desugaring changes.

Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Jun 21, 2020
@Aaron1011
Don't use DesugaringKind to track desugared operators
571beb4 
See rust-lang#73468 (comment)

This removes the immediate need for PR rust-lang#73468, since Clippy will no
longer be confused by the precense of a new `ExpnId` 'on top of' a
macro expansion `ExpnId`.

This makes some of the 'self move diagnostics' added by PR rust-lang#72389
slightly worse. I've left the operator-related diagnostics code in
place, so it should be easy for a followup PR to plug in a new way of
detecting desugared operators.
@petrochenkov
Copy link
Contributor

Closing in favor of the revert.

@Aaron1011 Aaron1011 reopened this Jun 23, 2020
@Aaron1011
Copy link
Member Author

Now that the revert has landed in #73594, I think we should consider landing this. Clippy's current method detecting macro expansions is very fragile - any additional ExpnKind::Desugarings that we apply may trigger the underlying issue that necessitated the revert.

@Manishearth
Copy link
Member

So I still don't understand how this would lead to a false positive in clippy: could you give an example?

@Manishearth
Copy link
Member

Like, overall AST desugarings are pretty rare, and it's basically only the initial few that have always existed. Changes to desugarings necessitate changes in Clippy anyway since sometimes we need to un-desugar them.

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 10, 2020
@Elinvynia Elinvynia added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 25, 2020
@petrochenkov
Copy link
Contributor

I'm going to close this for now, with #72389 reverted it's not super urgent, and I'd like to consider eliminating lowering-time expansions entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis requested changes

@flip1995 flip1995 flip1995 approved these changes

@Manishearth Manishearth Manishearth approved these changes

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Aaron1011 @rust-highfive @flip1995 @nikomatsakis @petrochenkov @Manishearth @crlf0710 @Elinvynia