Move even more early buffered lints to dyn lint diagnostics

[fmease]

Follow-up to #145881 and #145747.

I originally wanted to migrate most if not the entire rest of the early buffered lints. However, when trying to migrate the buffered lints used by check-cfg I encountered a roadblock. Namely, it depends on TyCtxt (well, Option<TyCtxt>) which makes it quite hard to migrate (see also #147634 (comment), #147634 (comment) & #149215).

So for now, I won't migrate it (maybe #149215 will find a solution), nor will I migrate the rest since it's quite tedious to migrate these. I'll leave them for future me.

[Urgau]

• Complete check-cfg migration if feasible or drop it for now (it's nasty due to it requiring a TyCtxt)

Yeah, and it's even more complex as check-cfg lints are emitted from both rustc & rustdoc, which do not operate at the same time, rustc is at expansion time, while rustdoc is after expansion (and therefore has direct access to a TyCtxt).

[fmease]

The thing is, while currently only the post-expansion lint pass gets passed a TyCtxt (since your PR), in theory the pre-expansion lint pass has access to the TyCtxt, too, because both run inside create_and_enter_global_ctxt and TyCtxt::crate_name is already fed for the local crate (and I'm pretty decoding from metadata for foreign DefIds should also work inside the pre-expansion pass).

We can easily pass the TyCtxt to the pre-expansion pass that gets run on the root source file (crate root + inline modules) but the problem I was struggling with was passing the TyCtxt to the pre-expansion passes that get run for out-of-line/file modules because this happens in rustc_expand when expanding ModKind::Unloadeds where we only have access to an ExtCtxt¹ and a dyn LintStoreExpand but I'm not sure if it's sound (wrt. thread-local storage and query caching and maybe some other hidden assumptions²) to (make rustc_expand depend on rustc_middle and) store a TyCtxt inside ExtCtxt esp. once we parallelize things (CC #145354). That's where I gave up but I should probably just ping the relevant people.

Not being able to name TyCtxt inside rustc_attr_parsing shouldn't that huge of a problem, I'll just create a new proxy trait EarlyTyCtxt that defines crate_name similar to the LintEmitter and HirTyCtxt shenanigans we already have to do.

Footnotes

1. I still don't know what the Ext stands for. If it really does stand for extended then I must say that's a really non-descriptive name ^^' ↩

2. I really don't know what I'm talking about here btw. I know that both TyCtxt and ExtCtxt are !Send and !Sync so maybe everything is sufficiently locked down already? ↩

[fmease]

We can easily pass the TyCtxt to the pre-expansion pass

And I know we don't do that right now (we use Option<TyCtxt> as you know) but since I'm already trying to clean things up I was attempting to make TyCtxt unconditionally available to check-cfg. I don't remember if dropping that requirement I've imposed on myself makes the migration easier or not because we probably need to parametrize things over some proxy TyCtxt trait regardless.

[fmease]

Fully dropped the check-cfg migration due to the reasons above. Maybe #149215 will fix the TyCtxt issue for me in some way. Moreover, I won't migrate any other early lints in this PR since this process is quite tedious & I need to be in the right mood to do it. I'll leave that for more follow-up PRs.

[fmease]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: cb75625 (cb756251a1b5963834143557fd6b220741a300f7, parent: f40a70d2bcd830a4f1f8c7ca1a7f93f1d9d703d6)

[rust-timer]

Finished benchmarking commit (cb75625): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-0.5%	[-0.8%, -0.0%]	14
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	1

Max RSS (memory usage)

Results (primary 3.3%, secondary -4.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.3%	[3.3%, 3.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.1%	[-8.1%, -1.4%]	11
All ❌✅ (primary)	3.3%	[3.3%, 3.3%]	1

Cycles

Results (secondary -3.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.7%, -3.1%]	5
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 469.915s -> 468.944s (-0.21%)
Artifact size: 386.98 MiB -> 386.93 MiB (-0.01%)

[jdonszelmann]

cc: @JonathanBrouwer you were also running into the cfg lint issue recently weren't you? take a look at this pr

[jdonszelmann]

cool that we get error codes too now

[jdonszelmann]

hm though the error code index only mentions other ambiguities that are actually errors, not this warning. Might be fine? Idk if we generally put error codes on lints.

[fmease]

I've grepped tests/ for warning\[ (I know it doesn't account for denied lints) and most results come from a lint I wrote, lol. Right, lint warnings are already associated with a lint name, so strictly speaking an error code doesn't make sense for them (~ well).

I'll change it.

[fmease]

Actually ambiguous_glob_imports is deny-by-default due to future_incompatible being deny-by-default. In the tests where it's a warning there's actually a #![warn(ambiguous_glob_imports)].

Anyways, once it's a hard error it'll get the error code and people might update the error code entry to better fit this case (but frankly that's unlikely ^^; and the error code index might even go away in the future ^^). Moreover, this PR is meant to be a refactoring PR :D

[jdonszelmann]

I hate all the proxy traits but I guess it will be the way to do it... what's here looks good. The lints that are migrated look good. @fmease if you know more about error codes on lints, lmk, otherwise r=me

[JonathanBrouwer]

@jdonszelmann Yeah the linked PR #149215 is my PR :p

The problem blocking that PR is that attribute lints have their own enum (AttributeLindKind), and the emitting code of that also doesn't have access to TyCtxt. One fix I attempted (but not fully succeeded in yet) for that PR is to actually move all of the AttributeLindKind variants from there into BuiltinLintDiag. But that won't help you with the problem in this PR I'm afraid.

[jdonszelmann]

@rustbot author

[fmease]

I didn't even link to the dropped commit even though most of the comments in this PR are concerned with it. So for reference / "posterity" (will probably get pruned at some point): fmease@c97e79f.

[fmease]

@bors r=jdonszelmann

[bors]

📌 Commit 109e5e5 has been approved by jdonszelmann

It is now in the queue for this repository.

[bors]

⌛ Testing commit 109e5e5 with merge 47cd712...