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

rustc_lint: Some early linting refactorings #91526

Merged
merged 5 commits into from
Jan 24, 2022
Merged

rustc_lint: Some early linting refactorings #91526

merged 5 commits into from
Jan 24, 2022

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Dec 4, 2021 •
edited
Loading

The first one removes and renames some fields and methods from EarlyContext.

The second one uses the set of registered tools (for tool attributes and tool lints) in a more centralized way.

The third one removes creation of a fake ast::Crate from fn pre_expansion_lint.
Pre-expansion linting is done with per-module granularity on freshly loaded modules, and it previously synthesized an ast::Crate to visit non-root modules, now they are visited as modules.
The node ID used for pre-expansion linting is also made more precise (the loaded module ID is used).

@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive
Copy link
Collaborator

r? @estebank

(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 Dec 4, 2021
@petrochenkov
Copy link
Contributor Author

The removal of the fake ast::Crate will be done in a different way, I need to tweak the first commit.

@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 Dec 6, 2021
@petrochenkov petrochenkov changed the title rustc_lint: Two early linting refactorings rustc_lint: Three early linting refactorings Dec 7, 2021
@petrochenkov
Copy link
Contributor Author

Updated.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 7, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 9, 2021
@petrochenkov petrochenkov changed the title rustc_lint: Three early linting refactorings rustc_lint: Some early linting refactorings Dec 11, 2021
@bors

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor Author

Ping @estebank, it's been 3 weeks.

@bors

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor Author

Ping @estebank, it's been 6 weeks.

cjgillot
cjgillot approved these changes Jan 22, 2022
View reviewed changes
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

The pre-expansion lint infra is only used for the KeywordIdents lint. I'm wondering if such infra is really worth the complexity, or if we should just hardcode this lint and drop pre-expansion lints.

Either way, r=me after a rebase.

compiler/rustc_middle/src/ty/mod.rs
@@ -118,6 +118,8 @@ mod sty;

// Data types

pub type RegisteredTools = FxHashSet<Ident>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Pre-existing: do we use the span or the hygiene in the Ident, or can it simply be a FxHashSet<Symbol>?

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's used during name resolution, but in ResolverOutputs it could already be FxHashSet<Symbol>, I just didn't bother converting and used the same type everywhere.

@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 Jan 23, 2022
@petrochenkov
Copy link
Contributor Author

The pre-expansion lint infra is only used for the KeywordIdents lint. I'm wondering if such infra is really worth the complexity, or if we should just hardcode this lint and drop pre-expansion lints.

I'm also not sure how useful all of this is, but there are also a couple of lints in clippy using the same pre-expansion lint infra.

@petrochenkov
rustc_lint: Remove some redundant fields from EarlyContext
452aa81 
Use consistent function parameter order for early context construction and early linting
Rename some functions to make it clear that they do not necessarily work on the whole crate
@petrochenkov
rustc_lint: Reuse the set of registered tools from resolver
51b2338
@petrochenkov
Update clippy
9c70b6d
@petrochenkov
rustc_lint: Stop creating a fake ast::Crate for running early lints
05cd755 
Add a trait generalizing over the crate root and freshly loaded modules instead
This also makes node IDs used for pre-expansion linting more precise
@petrochenkov
expand: Pass everything by reference to pre-expansion lint callback
67cccaf
@petrochenkov
Copy link
Contributor Author

r? @cjgillot @bors r=cjgillot

@rust-highfive rust-highfive assigned cjgillot and unassigned estebank Jan 23, 2022
@bors
Copy link
Contributor

bors commented Jan 23, 2022

📌 Commit 67cccaf has been approved by cjgillot

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 23, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 23, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 23, 2022
@bors
Auto merge of rust-lang#93245 - matthiaskrgr:rollup-djsi6jr, r=matthi…
42313dd 
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#91526 (rustc_lint: Some early linting refactorings)
 - rust-lang#92555 (Implement RFC 3151: Scoped threads.)
 - rust-lang#93213 (Fix `let_chains` and `if_let_guard` feature flags)
 - rust-lang#93219 (Add preliminary support for inline assembly for msp430.)
 - rust-lang#93226 (Normalize field access types during borrowck)
 - rust-lang#93227 (Liberate late bound regions when collecting GAT substs in wfcheck)
 - rust-lang#93229 (Remove DiagnosticBuilder.quiet)
 - rust-lang#93234 (rustc_mir_itertools: Avoid needless `collect` with itertools)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 89baf0f into rust-lang:master Jan 24, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot approved these changes

Assignees

@cjgillot cjgillot

Labels
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
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
8 participants
@petrochenkov @rust-highfive @rust-log-analyzer @bors @cjgillot @estebank @apiraino @rustbot