Move `lint_store` #117649

nnethercote · 2023-11-07T00:40:23Z

Some nice cleanups enabled by the removal of compiler plugins.

r? @cjgillot

compiler/rustc_interface/src/interface.rs

nnethercote · 2023-11-07T04:04:40Z

The job x86_64-gnu-tools failed!

Two weird clippy failures.

In src/tools/clippy/tests/ui/macro_use_imports.rs, the four errors are being emitted in a different order.
In src/tools/clippy/tests/ui/dbg_macro.rs we're getting a slightly different suggestion in one place, due to somehow hitting the else branch instead of the if branch in this code.

I bisected locally and found it's third commit that's causing these changes, but I don't know why.

Alexendoo · 2023-11-07T13:41:35Z

It's likely because spans are changing, clippy loads clippy.toml into the source map in the register_lints callback

dbg_macro handled them improperly and will be fixed by rust-lang/rust-clippy#11743

macro_use_imports keys on a span among other things in an FxHashMap, haven't opened a PR for this one yet

Blessing them would be fine, we can fix the other one on the clippy side

nnethercote · 2023-11-07T20:05:01Z

It's likely because spans are changing, clippy loads clippy.toml into the source map in the register_lints callback

dbg_macro handled them improperly and will be fixed by rust-lang/rust-clippy#11743

macro_use_imports keys on a span among other things in an FxHashMap, haven't opened a PR for this one yet

Blessing them would be fine, we can fix the other one on the clippy side

Thanks for the info, that's a huge help!

I had suspected the macro_use_imports one was an FxHashMap ordering thing. Good to have that confirmed.

The dbg_macro one was indeed a span issue, I found that the offset for a particular span had changed by about 70 bytes. I didn't think about the offset operation being bogus.

If I remove src/tools/clippy/clippy.toml and src/tools/clippy/tests/clippy.toml then all the clippy tests pass.

But I don't understand how my change affected clippy's use of register_lints, which concerns me. This change wasn't supposed to have any visible effects.

Alexendoo · 2023-11-07T22:49:25Z

Haven't finished it yet but a local clippy change I have to read the config file in Config::hash_untracked_state broke these same two tests, I believe the cause would be the same here

Because register_lints is being called earlier it's loading clippy.toml into the source map before e.g. dbg_macro.rs, meaning BytePos 0 no longer points to the start of the test file

This was made possible by the removal of plugin support, which simplified lint store creation. This simplifies the places in rustc and rustdoc that call `describe_lints`, which are early on. The lint store is now built before those places, so they don't have to create their own lint store for temporary use, they can just use the main one. Note: there are two clippy test changes. clippy loads the `clippy.toml` file into the source map during lint registration, and this now happens earlier than it used to, before the Rust source code is loaded. This exposes some latent bugs in clippy where it assumes invalid things about spans and source map offsets. These are in the process of being fixed elsewhere, as described in rust-lang#117649.

rustbot · 2023-11-08T03:28:30Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

nnethercote · 2023-11-08T03:29:07Z

Thanks, @Alexendoo. I have updated the expected test outputs. This should now be ready to review.

blyxyas · 2023-11-08T09:40:37Z

This PR would totally block and mean a total re-write of the current efforts for not processing allowed lints (both in Clippy and Rustc) as it removed practically everything that the current version of that patch uses.

I advise against this cleanup, at least until that patch is open and merged.

src/tools/clippy/tests/ui/dbg_macro.stderr

nnethercote · 2023-11-08T09:43:11Z

This PR would totally block and mean a total re-write of the current efforts for not processing allowed lints (both in Clippy and Rustc) as it removed practically everything that the current version of that patch uses.

Do you have a link to that patch?

nnethercote · 2023-11-08T21:17:57Z

This PR would totally block and mean a total re-write of the current efforts for not processing allowed lints (both in Clippy and Rustc) as it removed practically everything that the current version of that patch uses.

Do you have a link to that patch?

@flip1995 pointed me at #106983, which has an attempted fix in #116271. I looked through the code for #116271 and I don't see any real overlap with the changes in this PR. So I think "totally block and mean a total re-write" is untrue.

blyxyas · 2023-11-08T21:46:15Z

I wasn't talking about #116271, I was talking about a more modern form of this patch (which I lost and will have to rewrite from memory).

And blocking that patch would block the Clippy efforts to ignore allowed lints (apart from Alexendoo's patch, which I don't know the progress of, or usage of this objects)

Some things can indeed be cleaned up, but others like LintContext or get_lints() being derived on each lint cannot.

nnethercote · 2023-11-08T23:36:38Z

I wasn't talking about #116271

Your original comment said this:

This PR would totally block and mean a total re-write of the current efforts for not processing allowed lints (both in Clippy > and Rustc) as it removed practically everything that the current version of that patch uses.

Strong words! But there was no link to the patch and you didn't respond to my request for a link to it. So I contacted @flip1995 on Zulip and they pointed me at #116271.

I was talking about a more modern form of this patch (which I lost and will have to rewrite from memory).

Ok, that's helpful. I just looked at that patch and... again, it doesn't seem to conflict with this PR at all?

And blocking that patch would block the Clippy efforts to ignore allowed lints (apart from Alexendoo's patch, which I don't know the progress of, or usage of this objects)

Some things can indeed be cleaned up, but others like LintContext or get_lints() being derived on each lint cannot.

This PR removes LintContext::PassObject and LintContext::lints. It doesn't touch get_lints.

I don't want to get in people's way and I'm happy to make adjustments if there are specific changes here that cause problems. But I don't see any such problems so far. I can't read minds so I don't know what was in past patches that have been lost, nor what will be in future patches yet to be written. I feel like we can all work together to get most or all of this PR merged.

blyxyas · 2023-11-09T12:28:07Z

On an even closer inspection, the current patch indeed isn't blocked by this PR, let's hope that the next iteration isn't as well.
Sorry for the false alarm.

cjgillot

Just one question. r=me if no issue.

compiler/rustc_interface/src/interface.rs

nnethercote · 2023-11-14T02:20:29Z

I've addressed @cjgillot's comment. I will wait until the next clippy sync (Thursday) before merging this, to avoid the problem with dbg_macro.rs's expected output.

bors · 2023-11-14T05:13:37Z

☔ The latest upstream changes (presumably #117773) made this pull request unmergeable. Please resolve the merge conflicts.

This was made possible by the removal of plugin support, which simplified lint store creation. This simplifies the places in rustc and rustdoc that call `describe_lints`, which are early on. The lint store is now built before those places, so they don't have to create their own lint store for temporary use, they can just use the main one.

Lint registration now happens early enough that we can run it from `Config`, before `Compiler` is created.

nnethercote · 2023-11-16T23:57:35Z

The clippy sync has occurred, and I have updated.

@bors r=cjgillot

bors · 2023-11-16T23:57:37Z

📌 Commit dededd2 has been approved by cjgillot

It is now in the queue for this repository.

Rollup of 5 pull requests Successful merges: - rust-lang#117649 (Move `lint_store`) - rust-lang#117850 (bootstrap: simplify setting unstable-options for tools) - rust-lang#117889 (docs(release): Clarify cargo entries) - rust-lang#117946 (avoid exhaustive i16 test in Miri) - rust-lang#117963 (`rustc_query_system` cleanups) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#117649 - nnethercote:mv-lint_store, r=cjgillot Move `lint_store` Some nice cleanups enabled by the removal of compiler plugins. r? `@cjgillot`

Currently we have an inconsistency between the "input" and "no input" cases: - no input: `rustc --print=sysroot -Whelp` prints the lint help. - input: `rustc --print=sysroot -Whelp a.rs` prints the sysroot. It makes sense to print the lint help in both cases, because that's what happens with `--help`/`-Zhelp`/`-Chelp`. In fact, the `describe_lints` in the "input" case happens amazingly late, after *parsing*. This is because, with plugins, lints used to be registered much later, when the global context was created. But rust-lang#117649 moved lint registration much earlier, during session construction. So this commit moves the `describe_lints` call to a single spot for both for both the "input" and "no input" cases, as early as possible. This is still not as early as `--help`/`-Zhelp`/`-Chelp`, because `-Whelp` must wait until the session is constructed.

…bjorn3 Unify "input" and "no input" paths in `run_compiler` A follow-up to rust-lang#117649. r? `@bjorn3`

Make `macro_use_imports` lint ordering more stable changelog: none Fixes [the `macro_use_imports` ordering dependence](rust-lang/rust#117649 (comment)) on the hash of `Span`s

rustbot assigned cjgillot Nov 7, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 7, 2023

nnethercote mentioned this pull request Nov 7, 2023

Remove support for compiler plugins. #116412

Merged

This comment has been minimized.

Sign in to view

tamird reviewed Nov 7, 2023

View reviewed changes

compiler/rustc_interface/src/interface.rs Outdated Show resolved Hide resolved

nnethercote force-pushed the mv-lint_store branch from bfc1d3d to 0034727 Compare November 8, 2023 03:28

flip1995 requested changes Nov 8, 2023

View reviewed changes

src/tools/clippy/tests/ui/dbg_macro.stderr Outdated Show resolved Hide resolved

Alexendoo mentioned this pull request Nov 9, 2023

Make macro_use_imports lint ordering more stable rust-lang/rust-clippy#11782

Merged

cjgillot reviewed Nov 13, 2023

View reviewed changes

compiler/rustc_interface/src/interface.rs Outdated Show resolved Hide resolved

nnethercote force-pushed the mv-lint_store branch from 0034727 to 31ce67e Compare November 14, 2023 02:18

This comment has been minimized.

Sign in to view

nnethercote force-pushed the mv-lint_store branch from 31ce67e to 1be32dd Compare November 15, 2023 02:21

This comment has been minimized.

Sign in to view

nnethercote added 4 commits November 17, 2023 10:39

Inline and remove create_lint_store.

5dfe47a

Remove dead lint code.

73c1fc5

Remove Compiler::register_lints.

dededd2

Lint registration now happens early enough that we can run it from `Config`, before `Compiler` is created.

nnethercote force-pushed the mv-lint_store branch from 1be32dd to dededd2 Compare November 16, 2023 23:56

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 Nov 16, 2023

TaKO8Ki mentioned this pull request Nov 17, 2023

Rollup of 5 pull requests #118001

Merged

bors merged commit 2b2dd25 into rust-lang:master Nov 17, 2023
11 checks passed

rustbot added this to the 1.76.0 milestone Nov 17, 2023

nnethercote deleted the mv-lint_store branch November 17, 2023 06:11

nnethercote mentioned this pull request Nov 17, 2023

Unify "input" and "no input" paths in run_compiler #118002

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 18, 2023

Auto merge of rust-lang#118002 - nnethercote:unify-input-no-input, r=…

28345f0

…bjorn3 Unify "input" and "no input" paths in `run_compiler` A follow-up to rust-lang#117649. r? `@bjorn3`

xFrednet mentioned this pull request Nov 19, 2023

Chore: Bump nightly -> 2023-12-28 rust-marker/marker#315

Open

ehuss mentioned this pull request Dec 5, 2023

Remove mentions of plugin lints rust-lang/rustc-dev-guide#1833

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `lint_store` #117649

Move `lint_store` #117649

nnethercote commented Nov 7, 2023

This comment has been minimized.

nnethercote commented Nov 7, 2023

Alexendoo commented Nov 7, 2023

nnethercote commented Nov 7, 2023

Alexendoo commented Nov 7, 2023

rustbot commented Nov 8, 2023

nnethercote commented Nov 8, 2023

blyxyas commented Nov 8, 2023

nnethercote commented Nov 8, 2023

nnethercote commented Nov 8, 2023

blyxyas commented Nov 8, 2023

nnethercote commented Nov 8, 2023

blyxyas commented Nov 9, 2023

cjgillot left a comment

nnethercote commented Nov 14, 2023

This comment has been minimized.

bors commented Nov 14, 2023

This comment has been minimized.

nnethercote commented Nov 16, 2023

bors commented Nov 16, 2023

Move lint_store #117649

Move lint_store #117649

Conversation

nnethercote commented Nov 7, 2023

This comment has been minimized.

nnethercote commented Nov 7, 2023

Alexendoo commented Nov 7, 2023

nnethercote commented Nov 7, 2023

Alexendoo commented Nov 7, 2023

rustbot commented Nov 8, 2023

nnethercote commented Nov 8, 2023

blyxyas commented Nov 8, 2023

nnethercote commented Nov 8, 2023

nnethercote commented Nov 8, 2023

blyxyas commented Nov 8, 2023

nnethercote commented Nov 8, 2023

blyxyas commented Nov 9, 2023

cjgillot left a comment

Choose a reason for hiding this comment

nnethercote commented Nov 14, 2023

This comment has been minimized.

bors commented Nov 14, 2023

This comment has been minimized.

nnethercote commented Nov 16, 2023

bors commented Nov 16, 2023

Move `lint_store` #117649

Move `lint_store` #117649