Lazily normalize inside trait ref during orphan check & consider ty params in rigid alias types to be uncovered

[fmease]

Fixes #99554, fixes rust-lang/types-team#104.
Fixes #114061.

Supersedes #100555.

Tracking issue for the future compatibility lint: #124559.

r? lcnr

[fmease]

While I could update the diagnostic to say something like

Suggested change

	= note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local, and no uncovered type parameters appear before that first local type
	= note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local, and no type parameters not covered by a nominal type appear before that first local type

or

- error[E0210]: type parameter `T` must be covered by another type
+ error[E0210]: type parameter `T` must be covered by a nominal type

we could instead change the definition of "covered" (e.g., in the Rust Reference) to reflect that the covering type has to be nominal for the contained type to be considered covered.

[fmease]

not using FmtPrinter since remapping of ty vars now happens here, not over at the diagnostic code. Furthermore, the diagnostic code in hir analysis does further Ty processing so we can't fmt it here to a String.

Note that doing the remapping here allows us not to pass around an InferCtxt / Box<_> from trait selection to hir analysis which was super gross. You can see the perks of the new structuring by cmp'ing commit 2 and 3. I'm so happy I've managed to get rid of that.

[lcnr]

vibe: the current structure of orphan_check_trait_ref is confusing to me and I didn't fully get how the backcompat is handled (only spent a few minutes on the train on this tbf).

Have you considered to instead toggle the behavior of fn orphan_check_trait_ref depending on its input, e.g. by changing InCrate to the following?

enum InCrate {
   Local { orphan_check_backcompat: bool },
   Remote,
}

This way it should be more clear that the behavior for "consider aliases which may normalize to unconvered ty" does not accidentally change the desired behavior outside of linting[^1]

separate vibe: while I know that there's precendent to name things Issue69420, I really dislike this as it requires looking up the issue to figure out the purpose of such a variant. Actually referring to the underlying issue, e.g. AliasMayNormToUncovered or whatever and then linking to the issue in a doc comment feels a lot more desirable to me. We should ideally rename all the functions and enums which currently use issue numbers.

[bors]

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

[lcnr]

@bors r+ rollup=never

[bors]

📌 Commit c68c273 has been approved by lcnr

It is now in the queue for this repository.

[fmease]

I've added regression tests for #114061.

@bors r=lcnr

[bors]

📌 Commit 951e902 has been approved by lcnr

It is now in the queue for this repository.

[bors]

⌛ Testing commit 951e902 with merge f705de5...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing f705de5 to master...

[rust-timer]

Finished benchmarking commit (f705de5): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.7%	[-2.4%, -0.9%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.7%	[-2.4%, -0.9%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Binary size

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

Bootstrap: 672.826s -> 673.884s (0.16%)
Artifact size: 315.97 MiB -> 316.00 MiB (0.01%)

[apiraino]

we have some crater run regressions, maybe these changes need to be in the changelog

@rustbot label +relnotes

[lcnr]

The changes belong into the changelog, we automatically go through all T-types FCP'd PRs for each release, so the explicit relnotes tag isn't necessary imo (and easy to forget, which is why I want us to go through all FCPs when writing the release notes).

[apiraino]

The changes belong into the changelog, we automatically go through all T-types FCP'd PRs for each release, so the explicit relnotes tag isn't necessary imo (and easy to forget, which is why I want us to go through all FCPs when writing the release notes).

oh interesting I wasn't aware (or just forgot) about this flow, thanks for explaining!

(I thought that the only workflow was T-release getting all relnotes patches and put them into human-redable form in the changelog 🙂 )

[lcnr]

yeah, it's fairly new and also not really formalized anywhere, see https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/1.2E78/near/430856836 😊