Lockless LintStore #65193
Merged
Lockless LintStore #65193
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
We should hold off on landing this until clippy's recentmost rustup lands, fwiw (i.e. only land this if clippy is toolstate green, because if this breaks things in an unfixable way we'd want to be able to deal with things immediately) |
|
Won't have time to do a proper review but what you describe in the PR seems fine from a clippy POV. Tying passes to lints, weakly, may still be useful because it would be nice to selectively run passes based on what lints are allowed (provided those passes are pure, of course) |
|
I'm happy to try and help get a PR going on the clippy side to fix this, FWIW, though I've never contributed a patch to clippy. It also looks like it'll break RLS based on some quick grepping, but not quite sure why there. Would that be helpful? |
I think that might be an interesting optimization in the future -- these days it's not something we do -- but this PR does not prevent such work from happening if/when it becomes desirable. |
|
If you want to make a PR for clippy that would be great, of course! I'm not too worried about the breakage from this: it sounds like a lot of mechanical fixes, but I won't say no to help 😄 (As clippy toolstate is broken right now you'll have to graft in the rustup PR first) |
Yep, this was more of a note than a blocker |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Mark-Simulacrum
added
the
S-waiting-on-review
Zoxc
reviewed
Oct 8, 2019
|
I'm not sure about |
Already landed on Clippy side and should soon land in Rust: #65206 |
Mark-Simulacrum
force-pushed
the
lockless-lintstore
branch
from
221a207 to
0ec0d82
Compare
|
Pushed up new commits moving future compatibility information to lints directly as another step in moving us off of LintStore on Session -- will try to spend some time tomorrow finishing the decoupling entirely. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Mark-Simulacrum
force-pushed
the
lockless-lintstore
branch
from
0ec0d82 to
35585bc
Compare
|
And, with the last set of commits, this PR now entirely drops LintStore as a mutable (via locks) field from global-ish structures (Session, in particular). Plugin registration of lints is also altered; they are now expected to directly call methods of LintStore rather than pushing into vectors and then those vectors being appended onto lint store. This seems both easier and less error prone. |
Zoxc
reviewed
Oct 9, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Zoxc
reviewed
Oct 9, 2019
Zoxc
reviewed
Oct 9, 2019
Zoxc
reviewed
Oct 9, 2019
| @@ -103,32 +89,6 @@ impl<'a> Registry<'a> { | |||
| self.register_syntax_extension(Symbol::intern(name), ext); | |||
| } | |||
|
|
|||
| /// Register a compiler lint pass. | |||
There was a problem hiding this comment.
Why did you remove these functions? This seems to break plugins registering lints?
There was a problem hiding this comment.
We publicly expose the lint store now, and it should be a simple transform to use registry.lint_store.register vs these functions. Plugins needed to change anyway as we've changed the signature here and expect separate registration of lints and lint passes.
Mark-Simulacrum
force-pushed
the
lockless-lintstore
branch
2 times, most recently
from
ba73f32 to
a266474
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Okay I've added one more commit that makes clippy changes easier and filed a PR on clippy -- rust-lang/rust-clippy#4650 -- that makes the relevant changes there as well. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Mark-Simulacrum
force-pushed
the
lockless-lintstore
branch
from
cc2cff4 to
87d9769
Compare
Move to using Box<dyn Fn() -> ...> so that we can let plugins register state. This also adds a callback that'll get called from plugin registration so that Clippy and other tools can register lints without using the plugin API. The plugin API still works, but this new API is more compatible with drivers other than rustc.
Mark-Simulacrum
force-pushed
the
lockless-lintstore
branch
from
b3fcae5 to
6be0a70
Compare
nikomatsakis
approved these changes
Oct 22, 2019
| @@ -125,7 +126,11 @@ declare_lint! { | |||
| declare_lint! { | |||
| pub PRIVATE_IN_PUBLIC, | |||
| Warn, | |||
| "detect private items in public interfaces not caught by the old implementation" | |||
| "detect private items in public interfaces not caught by the old implementation", | |||
There was a problem hiding this comment.
we should probably update the page on this in the forge -- actually, that content should really move to rustc-guide I expect -- or at least some of it
src/librustc/lint/context.rs
Outdated
| @@ -155,20 +155,24 @@ impl LintStore { | |||
| .collect() | |||
| } | |||
|
|
|||
| pub fn register_early_pass(&mut self, pass: fn() -> EarlyLintPassObject) { | |||
| self.early_passes.push(pass); | |||
| pub fn register_early_pass(&mut self, | |||
There was a problem hiding this comment.
Nit: put &mut self on next line, to align with rustfmt? (Similarly, the ) { on the next line)
| @@ -106,6 +106,7 @@ pub fn abort_on_err<T>(result: Result<T, ErrorReported>, sess: &Session) -> T { | |||
| pub trait Callbacks { | |||
| /// Called before creating the compiler instance | |||
| fn config(&mut self, _config: &mut interface::Config) {} | |||
| fn extra_lints(&mut self, _ls: &mut lint::LintStore) {} | |||
| @@ -26,6 +25,8 @@ pub struct Registry<'a> { | |||
| /// from the plugin registrar. | |||
| pub sess: &'a Session, | |||
|
|
|||
| pub lint_store: &'a mut LintStore, | |||
There was a problem hiding this comment.
Nit: doc-comment, I think something simple like this would suffice
The LintStore permits plugins to register new lints.
| @@ -80,6 +81,8 @@ pub struct Config { | |||
|
|
|||
| pub crate_name: Option<String>, | |||
| pub lint_caps: FxHashMap<lint::LintId, lint::Level>, | |||
|
|
|||
| pub register_lints: Option<Box<dyn Fn(&Session, &mut lint::LintStore) + Send + Sync>>, | |||
There was a problem hiding this comment.
It'd be nice to have a comment here. Something like:
Callback that is invoked during lint registration. Use the &mut LintStore value to register new lints with the compiler as needed.
|
r=me with nits addressed |
|
@bors r=nikomatsakis |
|
📌 Commit 4e8d1b2 has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
bors
added
S-waiting-on-bors
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 23, 2019
… r=nikomatsakis
Lockless LintStore
This removes mutability from the lint store after registration. Each commit stands alone, for the most part, though they don't make sense out of sequence.
The intent here is to move LintStore to a more parallel-friendly architecture, although also just a cleaner one from an implementation perspective. Specifically, this has the following changes:
* We no longer implicitly register lints when registering lint passes
* For the most part this means that registration calls now likely want to call something like:
`lint_store.register_lints(&Pass::get_lints())` as well as `register_*_pass`.
* In theory this is a simplification as it's much easier for folks to just register lints and then have passes that implement whichever lint however they want, rather than necessarily tying passes to lints.
* Lint passes still have a list of associated lints, but a followup PR could plausibly change that
* This list must be known for a given pass type, not instance, i.e., `fn get_lints()` is the signature instead of `fn get_lints(&self)` as before.
* We do not store pass objects, instead storing constructor functions. This means we always get new passes when running lints (this happens approximately once though for a given compiler session, so no behavior change is expected).
* Registration API is _much_ simpler: generally all functions are just taking `Fn() -> PassObject` rather than several different `bool`s.
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 23, 2019
… r=nikomatsakis
Lockless LintStore
This removes mutability from the lint store after registration. Each commit stands alone, for the most part, though they don't make sense out of sequence.
The intent here is to move LintStore to a more parallel-friendly architecture, although also just a cleaner one from an implementation perspective. Specifically, this has the following changes:
* We no longer implicitly register lints when registering lint passes
* For the most part this means that registration calls now likely want to call something like:
`lint_store.register_lints(&Pass::get_lints())` as well as `register_*_pass`.
* In theory this is a simplification as it's much easier for folks to just register lints and then have passes that implement whichever lint however they want, rather than necessarily tying passes to lints.
* Lint passes still have a list of associated lints, but a followup PR could plausibly change that
* This list must be known for a given pass type, not instance, i.e., `fn get_lints()` is the signature instead of `fn get_lints(&self)` as before.
* We do not store pass objects, instead storing constructor functions. This means we always get new passes when running lints (this happens approximately once though for a given compiler session, so no behavior change is expected).
* Registration API is _much_ simpler: generally all functions are just taking `Fn() -> PassObject` rather than several different `bool`s.
bors
added a commit
that referenced
this pull request
Oct 24, 2019
Rollup of 12 pull requests Successful merges: - #64178 (More Clippy fixes for alloc, core and std) - #65144 (Add Cow::is_borrowed and Cow::is_owned) - #65193 (Lockless LintStore) - #65479 (Add the `matches!( $expr, $pat ) -> bool` macro) - #65518 (Avoid ICE when checking `Destination` of `break` inside a closure) - #65583 (rustc_metadata: use a table for super_predicates, fn_sig, impl_trait_ref.) - #65641 (Derive `Rustc{En,De}codable` for `TokenStream`.) - #65648 (Eliminate `intersect_opt`.) - #65657 (Remove `InternedString`) - #65691 (Update E0659 error code long explanation to 2018 edition) - #65696 (Fix an issue with const inference variables sticking around under Chalk + NLL) - #65704 (relax ExactSizeIterator bound on write_bytes) Failed merges: r? @ghost
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 26, 2019
…omatsakis Remove lint callback from driver This is leftover from a restructuring of lint registration for drivers; it should now happen via the register_lints field on Config rather than this function. This is not used by anyone to my knowledge (including the compiler itself); it was introduced in an abandoned refactor in rust-lang#65193.
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 26, 2019
…omatsakis Remove lint callback from driver This is leftover from a restructuring of lint registration for drivers; it should now happen via the register_lints field on Config rather than this function. This is not used by anyone to my knowledge (including the compiler itself); it was introduced in an abandoned refactor in rust-lang#65193.
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 27, 2019
…omatsakis Remove lint callback from driver This is leftover from a restructuring of lint registration for drivers; it should now happen via the register_lints field on Config rather than this function. This is not used by anyone to my knowledge (including the compiler itself); it was introduced in an abandoned refactor in rust-lang#65193.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes mutability from the lint store after registration. Each commit stands alone, for the most part, though they don't make sense out of sequence.
The intent here is to move LintStore to a more parallel-friendly architecture, although also just a cleaner one from an implementation perspective. Specifically, this has the following changes:
lint_store.register_lints(&Pass::get_lints())as well asregister_*_pass.fn get_lints()is the signature instead offn get_lints(&self)as before.Fn() -> PassObjectrather than several differentbools.