rustc: Rearchitect lints to be emitted more eagerly #43522

alexcrichton · 2017-07-28T14:15:55Z

In preparation for incremental compilation this commit refactors the lint
handling infrastructure in the compiler to be more "eager" and overall more
incremental-friendly. Many passes of the compiler can emit lints at various
points but before this commit all lints were buffered in a table to be emitted
at the very end of compilation. This commit changes these lints to be emitted
immediately during compilation using pre-calculated lint level-related data
structures.

Linting today is split into two phases, one set of "early" lints run on the
syntax::ast and a "late" set of lints run on the HIR. This commit moves the
"early" lints to running as late as possible in compilation, just before HIR
lowering. This notably means that we're catching resolve-related lints just
before HIR lowering. The early linting remains a pass very similar to how it was
before, maintaining context of the current lint level as it walks the tree.

Post-HIR, however, linting is structured as a method on the TyCtxt which
transitively executes a query to calculate lint levels. Each request to lint on
a TyCtxt will query the entire crate's 'lint level data structure' and then go
from there about whether the lint should be emitted or not.

The query depends on the entire HIR crate but should be very quick to calculate
(just a quick walk of the HIR) and the red-green system should notice that the
lint level data structure rarely changes, and should hopefully preserve
incrementality.

Overall this resulted in a pretty big change to the test suite now that lints
are emitted much earlier in compilation (on-demand vs only at the end). This in
turn necessitated the addition of many #![allow(warnings)] directives
throughout the compile-fail test suite and a number of updates to the UI test
suite.

Closes #42511

rust-highfive · 2017-07-28T14:16:07Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2017-07-28T14:16:08Z

r? @michaelwoerister

petrochenkov · 2017-07-28T14:47:40Z

Can compiletest set allow(unused) for all tests by default through command line?
Then tests checking lints from the unused group specifically could override the default in source.

petrochenkov · 2017-07-28T14:57:55Z

src/test/compile-fail/lint-type-overflow2.rs

@@ -9,16 +9,18 @@
 // except according to those terms.
 //

-#![deny(overflowing_literals)]
-#![deny(const_err)]
+#![warn(overflowing_literals)]


I see a few tests changed from deny(lint) to warn(lint), did they start failing with deny?

I think what happened is that errors were emitted earlier in compilation, and then presumably something had abort_if_errors which aborted compilation before the next lint location was reached. Before this PR all those errors were buffered until the end, but now that they're emitted on-demand they presumably cause compilation to abort halfway through (or somethign like that)

petrochenkov · 2017-07-28T15:01:14Z

src/test/ui/lint/outer-forbid.stderr

-19 | #[allow(unused, unused_variables, bad_style)]
-   |                                   ^^^^^^^^^ overruled by previous forbid
-
-error: aborting due to 3 previous errors


This looks like a regression - the "overruled by outer forbid" is reported only for one lint. (But this may be intended?)

This is currently intentioanl and I felt it wasn't really a "big enough" regression to investigate a fix for.

petrochenkov · 2017-07-28T15:05:18Z

src/librustc_driver/driver.rs

@@ -804,6 +799,10 @@ pub fn phase_2_configure_and_expand<F>(sess: &Session,
        })
    })?;

+    time(time_passes,
+         "early lint checks",
+         || lint::check_ast_crate(sess, &krate));


So, with this PR lints can no longer be reported during lowering to HIR, right?
It's too late for "early lints" and too early for "late lints", but previously they could be reported as "late".
What if I have a lint that should work this way?

Correct, yes. Presumably we could just empty the "lint buffer" at the end though if need be instead of at the end of the early lint passes.

alexcrichton · 2017-07-28T15:14:05Z

Can compiletest set allow(unused) for all tests by default through command line?

It could! I ended up just taking the alternate strategy of tagging tests.

eddyb · 2017-07-28T18:00:58Z

It could! I ended up just taking the alternate strategy of tagging tests.

Hah! It'd be nice to have avoided this in the first place (by writing the code to not produce warnings). I think I hit something similar a while back. cc @nikomatsakis

petrochenkov · 2017-07-28T18:12:49Z

It'd be nice to have avoided this in the first place (by writing the code to not produce warnings).

It's hard to not hit any of unused / dead code warnings when writing a compile-fail test.
I'd still prefer allow(unused) to boilerplate code "using" everything defined in the test and distracting from the test's primary purpose.

Not producing other kinds of warnings in tests may be quite reasonable though. This PR for example adds allow(safe_extern_statics) which certainly shouldn't happen.

alexcrichton · 2017-07-28T19:25:20Z

So... start over and add -A unused to compile-fail tests? I don't really want to oscillate on this, it was hard enough to do once.

Not producing other kinds of warnings in tests may be quite reasonable though. This PR for example adds allow(safe_extern_statics) which certainly shouldn't happen.

Can you elaborate on this? The safe_extern_statics lint was needed to get the tests compiling again. Presumably we can delete the tests once it's a hard error.

arielb1 · 2017-07-29T13:11:01Z

src/librustc/ty/context.rs

+    pub fn lint_level_at_node(self, lint: &'static Lint, mut id: NodeId)
+        -> (lint::Level, lint::LintSource)
+    {
+        let sets = self.lint_levels(LOCAL_CRATE);


This creates an edge to the crate-wide lint_levels and will force a recompilation of everything with a lint assigned too it whenever the crate-wide lint_levels changes, even with red-green. If e.g. you have a ton of functions that are allow(everything) and have an allowed lint, you'll have to recompile them every time you add an item.

Until we get red-green this is hard to improve, but if you made LintLevelMap a query that would work. I suppose you should add a // FIXME: red-green or something.

Ah yeah when discussing with @nikomatsakis it sounded like red/green would solve this problem because while the lint levels map would always be calculated it would rarely change (in theory) and should also be quick to reconstruct.

The lint level map by itself would be recalculated every time you add an item (at least how I read it), even with red/green..

Indeed. This is where I have only one suggestion: make the lint computation bottom-up instead of top-down, i.e. the query gets its parent node's lint levels, then appends the ones from its own attributes and returns. That should have really good tracking.

Then again, if the map is only for items, then I actually wanted to have this done during HIR lowering anyway so the cost might be hidden there shrug.

I don't think this is an urgent problem: Yes the map will be recomputed every time but if it has not actually changed, red/green will detect that via it's unchanged ICH (= the maps stable hash). I don't think we should make this more complicated until profiling shows it to be a problem.

michaelwoerister · 2017-07-31T17:28:49Z

Thanks for the huge PR, @alexcrichton :)

Some comments:

I'm a bit skeptical too about all those new unused XXX warnings, especially in the UI tests. In most cases they seem to distract from the actual purpose of the test. I'd be fine with either allowing unused stuff in compile-fail by default or adding the respective attributes to the specific tests.
Although I'm okay with having one global LintLevels map and rebuilding it during every compilation, I also think it is problematic that we have a dependency on DepNode::Krate now. The failing tests on travis seem to indicate that there are cases where this dependency is introduced also in non-error cases and if so then I expect this to happen in every non-trivial project, which would essentially break incr. comp. until red/green arrives. There are at least three ways of fixing this:
- Block this PR on red/green. This is very undesirable because of bit-rot.
- Make the map available only through a function/method (e.g. of the HIR map) that will manually register DepNode::Hir dependencies along the path from the queried node to the HIR root. This is additional implementation work and will be obsolete when red/green arrives, so not ideal either.
- Wrap building the map in dep_graph.with_ignore() until red/green arrives. We don't cache any warnings or error messages yet, so this should be harmless, right @nikomatsakis?

In preparation for incremental compilation this commit refactors the lint handling infrastructure in the compiler to be more "eager" and overall more incremental-friendly. Many passes of the compiler can emit lints at various points but before this commit all lints were buffered in a table to be emitted at the very end of compilation. This commit changes these lints to be emitted immediately during compilation using pre-calculated lint level-related data structures. Linting today is split into two phases, one set of "early" lints run on the `syntax::ast` and a "late" set of lints run on the HIR. This commit moves the "early" lints to running as late as possible in compilation, just before HIR lowering. This notably means that we're catching resolve-related lints just before HIR lowering. The early linting remains a pass very similar to how it was before, maintaining context of the current lint level as it walks the tree. Post-HIR, however, linting is structured as a method on the `TyCtxt` which transitively executes a query to calculate lint levels. Each request to lint on a `TyCtxt` will query the entire crate's 'lint level data structure' and then go from there about whether the lint should be emitted or not. The query depends on the entire HIR crate but should be very quick to calculate (just a quick walk of the HIR) and the red-green system should notice that the lint level data structure rarely changes, and should hopefully preserve incrementality. Overall this resulted in a pretty big change to the test suite now that lints are emitted much earlier in compilation (on-demand vs only at the end). This in turn necessitated the addition of many `#![allow(warnings)]` directives throughout the compile-fail test suite and a number of updates to the UI test suite.

alexcrichton · 2017-08-09T16:14:49Z

Ok I've updated to use with_ignore which solves the incremental test issue as well and hopefully will paper over the problem until red/green comes I think?

I also hardcoded -A unused in compiletest for compile-fail and UI tests.

alexcrichton · 2017-08-09T16:14:57Z

re-r? @michaelwoerister

michaelwoerister · 2017-08-09T16:16:19Z

Cool! I'll take a look at it tomorrow.

michaelwoerister · 2017-08-10T09:10:40Z

@bors r+

Thank you, Alex!

bors · 2017-08-10T09:10:41Z

📌 Commit 0374e6a has been approved by michaelwoerister

rustc: Rearchitect lints to be emitted more eagerly In preparation for incremental compilation this commit refactors the lint handling infrastructure in the compiler to be more "eager" and overall more incremental-friendly. Many passes of the compiler can emit lints at various points but before this commit all lints were buffered in a table to be emitted at the very end of compilation. This commit changes these lints to be emitted immediately during compilation using pre-calculated lint level-related data structures. Linting today is split into two phases, one set of "early" lints run on the `syntax::ast` and a "late" set of lints run on the HIR. This commit moves the "early" lints to running as late as possible in compilation, just before HIR lowering. This notably means that we're catching resolve-related lints just before HIR lowering. The early linting remains a pass very similar to how it was before, maintaining context of the current lint level as it walks the tree. Post-HIR, however, linting is structured as a method on the `TyCtxt` which transitively executes a query to calculate lint levels. Each request to lint on a `TyCtxt` will query the entire crate's 'lint level data structure' and then go from there about whether the lint should be emitted or not. The query depends on the entire HIR crate but should be very quick to calculate (just a quick walk of the HIR) and the red-green system should notice that the lint level data structure rarely changes, and should hopefully preserve incrementality. Overall this resulted in a pretty big change to the test suite now that lints are emitted much earlier in compilation (on-demand vs only at the end). This in turn necessitated the addition of many `#![allow(warnings)]` directives throughout the compile-fail test suite and a number of updates to the UI test suite. Closes #42511

bors · 2017-08-10T11:20:26Z

⌛ Testing commit 0374e6a with merge 2400ebf...

bors · 2017-08-10T13:50:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 2400ebf to master...

oli-obk · 2017-08-11T11:49:44Z

I think this broke #[allow(unknown_lints)] (as in it doesn't work anymore for early lints)

alexcrichton · 2017-08-11T15:12:48Z

@oli-obk do you have a test case or an issue report for that?

The lint refactoring in rust-lang#43522 didn't account for `#[allow(unknown_lints)]` happening at the same node as an unknown lint itself, so this commit updates the handling to ensure that the local set of lint configuration being built is queried before looking at the chain of lint levels. Closes rust-lang#43809

rustc: Fix `unknown_lints` next to an unknown lint The lint refactoring in #43522 didn't account for `#[allow(unknown_lints)]` happening at the same node as an unknown lint itself, so this commit updates the handling to ensure that the local set of lint configuration being built is queried before looking at the chain of lint levels. Closes #43809

bjorn3 · 2017-08-17T14:10:45Z

src/librustc/lint/context.rs

    NotFound,
    Removed,
 }

+pub enum CheckLintNameResult<'a> {
+    Ok(&'a [LintId]),
+    // Lint doesn't exist


Please make it a doc comment

bjorn3 · 2017-08-17T14:10:51Z

src/librustc/lint/context.rs

+    Ok(&'a [LintId]),
+    // Lint doesn't exist
+    NoLint,
+    // The lint is either renamed or removed. This is the warning


bjorn3 · 2017-08-17T14:11:43Z

src/librustc/lint/context.rs

-            None => (Allow, Default),
-        }
-    }
+    // Checks the validity of lint names derived from the command line


rust-highfive assigned pnkfelix Jul 28, 2017

rust-highfive assigned michaelwoerister and unassigned pnkfelix Jul 28, 2017

alexcrichton mentioned this pull request Jul 28, 2017

refactor lint handling to be more "eager" #42511

Closed

oli-obk mentioned this pull request Jul 28, 2017

Breakage incoming: incremental AST lints rust-lang/rust-clippy#1907

Closed

petrochenkov reviewed Jul 28, 2017

View reviewed changes

alexcrichton force-pushed the rewrite-lints branch from 3eab07b to 428b653 Compare July 28, 2017 15:10

shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2017

alexcrichton force-pushed the rewrite-lints branch from 428b653 to e971197 Compare July 28, 2017 20:28

mattico mentioned this pull request Jul 28, 2017

[WIP] Add lint for int to ptr cast #43339

Closed

arielb1 reviewed Jul 29, 2017

View reviewed changes

shepmaster 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 Aug 4, 2017

alexcrichton force-pushed the rewrite-lints branch from e971197 to 0374e6a Compare August 9, 2017 16:14

bors merged commit 0374e6a into rust-lang:master Aug 10, 2017

oli-obk mentioned this pull request Aug 10, 2017

incoming breakage from rustc rust-lang/rust-clippy#1944

Closed

eddyb mentioned this pull request Aug 11, 2017

Check #[thread_local] statics correctly in the compiler. #43746

Merged

atorstling mentioned this pull request Aug 11, 2017

clippy_lints doesn't build on rust 1.21.0-nightly (13d94d5fa 2017-08-10) rust-lang/rust-clippy#1946

Closed

alexcrichton deleted the rewrite-lints branch August 11, 2017 15:12

alexcrichton mentioned this pull request Aug 11, 2017

#![allow(unknown_lints)] does not work if there is a unknow lint at the same level since nightly-2017-08-11-x86_64-unknown-linux-gnu #43809

Closed

alexcrichton mentioned this pull request Aug 13, 2017

rustc: Fix unknown_lints next to an unknown lint #43841

Merged

alexcrichton mentioned this pull request Aug 14, 2017

futures-rs tests ICE on nightly #43853

Closed

bjorn3 reviewed Aug 17, 2017

View reviewed changes

RalfJung mentioned this pull request Apr 9, 2020

forbid overwritten by later allow on the same "scope level" #70819

Open

ehuss mentioned this pull request Feb 28, 2021

Renamed lints not honored in attributes #82615

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: Rearchitect lints to be emitted more eagerly #43522

rustc: Rearchitect lints to be emitted more eagerly #43522

alexcrichton commented Jul 28, 2017 •

edited

Loading

rust-highfive commented Jul 28, 2017

alexcrichton commented Jul 28, 2017

petrochenkov commented Jul 28, 2017

petrochenkov Jul 28, 2017

alexcrichton Jul 28, 2017

petrochenkov Jul 28, 2017

alexcrichton Jul 28, 2017

petrochenkov Jul 28, 2017

alexcrichton Jul 28, 2017

alexcrichton commented Jul 28, 2017

eddyb commented Jul 28, 2017

petrochenkov commented Jul 28, 2017

alexcrichton commented Jul 28, 2017

arielb1 Jul 29, 2017

alexcrichton Jul 30, 2017

arielb1 Jul 31, 2017

eddyb Jul 31, 2017

eddyb Jul 31, 2017

michaelwoerister Jul 31, 2017

michaelwoerister commented Jul 31, 2017 •

edited

Loading

alexcrichton commented Aug 9, 2017

alexcrichton commented Aug 9, 2017

michaelwoerister commented Aug 9, 2017

michaelwoerister commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 10, 2017

oli-obk commented Aug 11, 2017

alexcrichton commented Aug 11, 2017

bjorn3 Aug 17, 2017

bjorn3 Aug 17, 2017

bjorn3 Aug 17, 2017

rustc: Rearchitect lints to be emitted more eagerly #43522

rustc: Rearchitect lints to be emitted more eagerly #43522

Conversation

alexcrichton commented Jul 28, 2017 • edited Loading

rust-highfive commented Jul 28, 2017

alexcrichton commented Jul 28, 2017

petrochenkov commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jul 28, 2017

eddyb commented Jul 28, 2017

petrochenkov commented Jul 28, 2017

alexcrichton commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented Jul 31, 2017 • edited Loading

alexcrichton commented Aug 9, 2017

alexcrichton commented Aug 9, 2017

michaelwoerister commented Aug 9, 2017

michaelwoerister commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 10, 2017

oli-obk commented Aug 11, 2017

alexcrichton commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jul 28, 2017 •

edited

Loading

michaelwoerister commented Jul 31, 2017 •

edited

Loading