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 use placement for suggestions near main. #85427

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented May 18, 2021

This fixes an edge case for the suggestion to add a use. When running with --test, the main function will be annotated with an #[allow(dead_code)] attribute. The UsePlacementFinder would end up using the dummy span of that synthetic attribute. If there are top-level inner attributes, this would place the use in the wrong position. The solution here is to ignore attributes with dummy spans.

In the process of working on this, I discovered that the use_suggestion_placement test was broken. UsePlacementFinder is unaware of active attributes. Attributes like #[derive] don't exist in the AST since they are removed. Fixing that is difficult, since the AST does not retain enough information. I considered trying to place the use towards the top of the module after any extern crate items, but I couldn't find a way to get a span for the start of a module block (the mod span starts at the mod keyword, and it seems tricky to find the spot just after the opening bracket and past inner attributes). For now, I just put some comments about the issue. This appears to have been a known issue in #44215 where the test for it was introduced, and the fix seemed to be deferred to later.

@rust-highfive
Copy link
Collaborator

r? @jackh726

(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 May 18, 2021
@rust-log-analyzer

This comment has been minimized.

@ehuss ehuss force-pushed the fix-use-placement branch from 9dfc24d to 1400cb0 Compare May 18, 2021 14:37
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

One question but this LGTM

// FIXME: UsePlacementFinder is broken because active attributes are
// removed, and thus the `derive` attribute here is not in the AST.
// An inert attribute should work, though.
// #[derive(Debug)]
Copy link
Member

Choose a reason for hiding this comment

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

What does this look like uncommented in the fixed output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like this (after formatting):

    #[derive(Debug)]
    use std::path::Path;
    #[allow(warnings)]
    pub struct Foo;

@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 24, 2021

📌 Commit 1400cb0 has been approved by jackh726

@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 Jun 24, 2021
@bors
Copy link
Contributor

bors commented Jun 24, 2021

⌛ Testing commit 1400cb0 with merge d95745e...

@bors
Copy link
Contributor

bors commented Jun 24, 2021

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing d95745e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 24, 2021
@bors bors merged commit d95745e into rust-lang:master Jun 24, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants