Extract `rustc_ast_passes`, move gating, & refactor linting #67806

Centril · 2020-01-02T11:57:39Z

Based on #67770.

This PR extracts a crate rustc_ast_passes:

ast_validation.rs, which is contributed by rustc_passes (now only has HIR based passes) -- the goal here is to improve recompilation of the parser,
feature_gate.rs, which is contributed by syntax and performs post-expansion-gating & final erroring for pre-expansion gating,
show_span, which is contributed by syntax.

To facilitate this, we first have to also:

Move {leveled_}feature_err{_err} from syntax::feature_gate::check into rustc_session::parse.
Move get_features into rustc_parse::config, the only place it is used.
Move some some lint datatypes and traits, e.g. LintBuffer, BufferedEarlyLint, BuiltinLintDiagnostics, LintPass, and LintArray into rustc_session::lint.
Move all the hard-wired lint statics into rustc_session::lint::builtin.

Centril · 2020-01-05T11:03:14Z

r? @petrochenkov

petrochenkov · 2020-01-05T12:50:57Z

src/librustc_session/Cargo.toml

@@ -10,6 +10,7 @@ path = "lib.rs"

 [dependencies]
 log = "0.4"
+rustc_error_codes = { path = "../librustc_error_codes" }


What happened with the idea of rustc_driver being the only crate depending on rustc_error_codes?
Without that the separation of error codes into a single crate only made things worse.

It's on my agenda to fix this but I haven't gotten around to it yet. Basically you only need to remove the let _= $code; thing and you can remove all the dependencies.

petrochenkov · 2020-01-05T13:00:44Z

src/librustc_session/parse.rs

@@ -70,6 +72,20 @@ pub enum GateStrength {
    Soft,


Hmm, looks like we have two mechanisms for soft feature-gating - this one and the soft_unstable lint - of which this one is unused.

I think we should remove GateStrength entirely and simplify the feature gating infra.
"Soft" feature gates are exceptional and having an isolated lint is more than enough.

Also, placing the feature gating stuff into parse.rs looks strange, it's not related to parsing.

I think we should remove GateStrength entirely and simplify the feature gating infra.

Sure, that seems like a good idea, though it seems like an independent change?

Also, placing the feature gating stuff into parse.rs looks strange, it's not related to parsing.

Well... GatedSpans is inside rustc_session::parse. My goal here is to remove any mentions of ParseSess within syntax so that I can invert the current dependency on rustc_session that syntax has and which is bad for parallelism. This parse.rs file felt like the best location to put it besides a new module, but I know you don't like those tiny ones. ;)

Sure, that seems like a good idea, though it seems like an independent change?

Yeah, this is material for a different PR (perhaps I'll do it myself today).

My goal here is to remove any mentions of ParseSess within syntax so that I can invert the current dependency on rustc_session that syntax

I see.

So rustc_session::parse means ParseSess and its methods.
I guess I can live with that, especially given that #68018 reduces the surface of feature gating API exposed by rustc_session::parse.

petrochenkov · 2020-01-05T13:27:02Z

Two questions:

Why is some feature gating moved to rustc_session instead of staying in syntax?
The primary feature gating pass is in rustc_ast_passes, but rustc_ast_passes still depends on syntax (and will depend on rustc_ast in the future), so moving feature gating doesn't break any dependencies.
How is the linting infra refactoring related to the rest of the PR. As I understand, the lint infra is moved earlier so that a single lint infra can be used in the whole compiler instead of having some separate "early buffered linting" for the frontend crates.
However, this move doesn't break any dependencies (with rustc_ast_passes in particular), and I'd prefer someone else (e.g. Mark-Simulacrum) to review it.

Centril · 2020-01-06T10:45:27Z

Why is some feature gating moved to rustc_session instead of staying in syntax?

See #67806 (comment).

How is the linting infra refactoring related to the rest of the PR. As I understand, the lint infra is moved earlier so that a single lint infra can be used in the whole compiler instead of having some separate "early buffered linting" for the frontend crates.
However, this move doesn't break any dependencies (with rustc_ast_passes in particular), and I'd prefer someone else (e.g. Mark-Simulacrum) to review it.

The reason I included the lint infra refactoring in this PR is because it allows us to break rustc_ast_passes's dependency on rustc, which is always nice for parallelism and incr.comp. This is also not the only crate which would benefit from this. For example, rustc_ast_lowering needs to refer to a specific &'static Lint as well as the LintBuffer type, but is otherwise close to losing its dependency on rustc as well (this is my overall strategy: I look for imports to rustc:: and then try to remove them over time in various PRs -- but I try to do it in a way that makes sense semantically).

Bigger picture, my goal with lints is to move out these data+traits parts of the rustc::lint infrastructure into rustc_session::lint. Eventually, this can become its own crate (e.g. rustc_lint_infra and we might be able to nix rustc::lint entirely.

Centril · 2020-01-07T12:17:19Z

r? @Mark-Simulacrum

Centril · 2020-01-11T02:31:29Z

@bors r=petrochenkov

bors · 2020-01-11T02:31:31Z

📌 Commit 682f500 has been approved by petrochenkov

Extract `rustc_ast_passes`, move gating, & refactor linting Based on rust-lang#67770. This PR extracts a crate `rustc_ast_passes`: - `ast_validation.rs`, which is contributed by `rustc_passes` (now only has HIR based passes) -- the goal here is to improve recompilation of the parser, - `feature_gate.rs`, which is contributed by `syntax` and performs post-expansion-gating & final erroring for pre-expansion gating, - `show_span`, which is contributed by `syntax`. To facilitate this, we first have to also: - Move `{leveled_}feature_err{_err}` from `syntax::feature_gate::check` into `rustc_session::parse`. - Move `get_features` into `rustc_parse::config`, the only place it is used. - Move some some lint datatypes and traits, e.g. `LintBuffer`, `BufferedEarlyLint`, `BuiltinLintDiagnostics`, `LintPass`, and `LintArray` into `rustc_session::lint`. - Move all the hard-wired lint `static`s into `rustc_session::lint::builtin`.

@ghost

Rollup of 8 pull requests Successful merges: - #67666 (make use of pointer::is_null) - #67806 (Extract `rustc_ast_passes`, move gating, & refactor linting) - #68043 (Add some missing timers) - #68074 (Add `llvm-skip-rebuild` flag to `x.py`) - #68079 (Clarify suggestion for E0013) - #68084 (Do not ICE on unicode next point) - #68102 (Inline some conversion methods around OsStr) - #68106 (Fix issue with using `self` module via indirection) Failed merges: r? @ghost

@Centril

feature_gate: Remove `GateStrength` The "soft feature gating" from `feature_gate/check.rs` is unused, and even if it were used, hardcoded warning is not a good solution and [deny-by-default lint](rust-lang#64266) would be a better way to do this. cc rust-lang#67806 (comment) r? @Centril

@Centril

feature_gate: Remove `GateStrength` The "soft feature gating" from `feature_gate/check.rs` is unused, and even if it were used, hardcoded warning is not a good solution and [deny-by-default lint](rust-lang#64266) would be a better way to do this. cc rust-lang#67806 (comment) r? @Centril

…lacrum Move more of `rustc::lint` into `rustc_lint` Based on rust-lang#67806. Here we try to consolidate more of the linting infra into `rustc::lint`. Some high-level notes: - We now store an `Lrc<dyn Any + Send + Sync>` as opposed to `Lrc<LintStore>` in the `GlobalCtxt`. This enables us to avoid referring to the type, breaking a cyclic dependency, and so we can move things from `rustc::lint` to `rustc_lint`. - `in_derive_expansion` is, and needs to, be moved as a method on `Span`. - We reduce the number of ways on `tcx` to emit a lint so that the developer UX is more streamlined. - `LintLevelsBuilder` is moved to `rustc_lint::levels`, leaving behind `LintLevelMap/Set` in a purified form due to current constraints (hopefully fixable in the future after rust-lang#68133). - `struct_lint_level` is moved to `rustc::lint` due to current dependency constraints. - `rustc::context` is moved to `rustc_lint::context`. - The visitors in `rustc::lint` are moved to `rustc_lint::passes`.

…lacrum Move more of `rustc::lint` into `rustc_lint` Based on rust-lang#67806. Here we try to consolidate more of the linting infra into `rustc::lint`. Some high-level notes: - We now store an `Lrc<dyn Any + Send + Sync>` as opposed to `Lrc<LintStore>` in the `GlobalCtxt`. This enables us to avoid referring to the type, breaking a cyclic dependency, and so we can move things from `rustc::lint` to `rustc_lint`. - `in_derive_expansion` is, and needs to, be moved as a method on `Span`. - We reduce the number of ways on `tcx` to emit a lint so that the developer UX is more streamlined. - `LintLevelsBuilder` is moved to `rustc_lint::levels`, leaving behind `LintLevelMap/Set` in a purified form due to current constraints (hopefully fixable in the future after rust-lang#68133). - `struct_lint_level` is moved to `rustc::lint` due to current dependency constraints. - `rustc::lint::context` is moved to `rustc_lint::context`. - The visitors in `rustc::lint` are moved to `rustc_lint::passes`.

@Centril

Fix crate paths in comments Tiny follow-up of rust-lang#67806 and others r? @Centril