Eagerly normalize in generalize

[jdonszelmann]

View all comments

r? @lcnr

cc: rust-lang/trait-system-refactor-initiative#262

[lcnr]

@bors try @rust-timer queue

[rust-bors]

💔 Test for 92bb985 failed: CI. Failed job:

• try - dist-x86_64-linux (web logs, enhanced plaintext logs)

[lcnr]

collector error: Failed to profile 'serde-1.0.219-new-solver' with Eprintln, recorded: expected success, got exit status: 101

fun, we've got some jank in serde which we don't have in our ui test suite

[rustbot]

changes to the core type system

cc @lcnr

[jdonszelmann]

@lcnr I think this fixes all issues we had. Only one test really regressed, some got a bunch better. Fixed the span on the one test we discussed.

[jdonszelmann]

@GuillaumeGomez is this another flaky test?

[lcnr]

reflowing the comments :<

r=me after this nit

View changes since this review

[lcnr]

Suggested change

	/// Treat aliases as potentially normalizable.
	///
	/// This is the default state that generalization starts in, unless we're treating aliases as rigid.
	/// It also means we're not currently inside an alias, since then we change the state to
	/// `IncompletelyRelateAliasArgs`
	Default,
	/// We enter this state when we're generalizing the arguments of a potentially normalizeable alias.
	///
	/// The behavior here is different between the old and the new solver:
	///
	/// In the old solver, the difference between this and `Default` is needed to
	/// correctly handle `<T as Bar<<?0 as Foo>::Assoc>::Assoc == ?0`.
	/// That equality can hold by either normalizing the outer or the inner associated type.
	/// In the old solver, we always structurally relate aliases,
	/// however, when we encounter an occurs check failure, we propagate the failure to the outermost alias,
	/// for which we then emit a Projection goal.
	///
	/// In the new solver, we rarely get into this state.
	/// When we encounter aliases we instead attempt to normalize them, and treat them as rigid using `ShallowStructurallyRelate`.
	/// Only when an alias is higher-ranked, we continue with similar logic to the old solver,
	/// except now we also explicitly relate the type and consts in the arguments of aliases while in this mode.
	///
	/// FIXME: Because we relate the type and consts in the arguments of aliases while in this mode,
	/// this is "incomplete".
	IncompletelyRelateAliasArgs,
	/// During generalization, when we encounter aliases,
	/// we will first attempt to normalize them when we're using the next trait solver.
	/// We can now treat the normalized alias as rigid, but only for "one layer", hence shallow.
	/// New aliases encountered inside the arguments of the outer alias should once again be related as normal.
	ShallowStructurallyRelateAliases,
	/// Treat aliases as rigid when relating them.
	///
	/// This corresponds to `relation.structurally_relate_aliases()`.
	StructurallyRelateAliases,
	/// Treat aliases as potentially normalizable.
	///
	/// This is the default state that generalization starts in, unless we're
	/// treating aliases as rigid. It also means we're not currently inside an
	/// alias, since then we change the state to `IncompletelyRelateAliasArgs`.
	Default,
	/// We enter this state when we're generalizing the arguments of a
	/// potentially normalizeable alias.
	///
	/// The behavior here is different between the old and the new solver:
	///
	/// In the old solver, the difference between this and `Default` is needed to
	/// correctly handle `<T as Bar<<?0 as Foo>::Assoc>::Assoc == ?0`. That
	/// equality can hold by either normalizing the outer or the inner
	/// associated type. In the old solver, we always structurally relate
	/// aliases. If we we encounter an occurs check failure, we propagate the
	/// failure to the outermost alias, for which we then emit a `Projection`
	/// goal instead.
	///
	/// In the new solver, we rarely get into this state.
	/// When we encounter aliases we instead attempt to normalize them, and treat
	/// them as rigid using `ShallowStructurallyRelate`. Only when an alias has
	/// escaping bound variables do we continue with similar logic to the old
	/// solver, except now we also explicitly relate the type and consts in the
	/// arguments of aliases while in this mode.
	///
	/// FIXME: Because we relate the type and consts in the arguments of aliases
	/// while in this mode, this is incomplete.
	IncompletelyRelateAliasArgs,
	/// During generalization, when we encounter aliases, we will first attempt
	/// to normalize them when we're using the next trait solver. We can now
	/// treat the normalized alias as rigid, but only for "one layer", hence
	/// shallow. New aliases encountered inside the arguments of the outer alias
	/// should once again be related as normal.
	ShallowStructurallyRelateAliases,
	/// Treat aliases as rigid when relating them.
	///
	/// This corresponds to `relation.structurally_relate_aliases()`.
	StructurallyRelateAliases,

[jdonszelmann]

@bors r=lcnr

[rust-bors]

📌 Commit d665c0b has been approved by lcnr

It is now in the queue for this repository.

[rust-bors]

☀️ Test successful - CI
Approved by: lcnr
Duration: 3h 8m 32s
Pushing ac7f9ec to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing bfc05d6 (parent) -> ac7f9ec (this PR)

Test differences

Show 8 test diffs

Stage 1

• [ui] tests/ui/traits/next-solver/generalize/eagely-normalizing-aliases.rs#next: [missing] -> pass (J1)
• [ui] tests/ui/traits/next-solver/generalize/eagely-normalizing-aliases.rs#old: [missing] -> pass (J1)

Stage 2

• [ui] tests/ui/traits/next-solver/generalize/eagely-normalizing-aliases.rs#next: [missing] -> pass (J0)
• [ui] tests/ui/traits/next-solver/generalize/eagely-normalizing-aliases.rs#old: [missing] -> pass (J0)

Additionally, 4 doctest diffs were found. These are ignored, as they are noisy.

Job group index

• J0: aarch64-apple, aarch64-gnu, aarch64-gnu-llvm-21-1, aarch64-msvc-1, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, optional-x86_64-gnu-parallel-frontend, test-various, x86_64-gnu, x86_64-gnu-debug, x86_64-gnu-gcc, x86_64-gnu-llvm-21, x86_64-gnu-llvm-21-2, x86_64-gnu-llvm-22-2, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1
• J1: x86_64-gnu-llvm-21-3, x86_64-gnu-llvm-22-3

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard ac7f9ec7da74d37fd28667c86bf117a39ba5b02a --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-aarch64-llvm-mingw: 1h 51m -> 1h 28m (-20.3%)
2. x86_64-gnu: 2h 10m -> 2h 25m (+11.9%)
3. test-various: 1h 55m -> 2h 8m (+11.8%)
4. x86_64-gnu-llvm-22-3: 1h 45m -> 1h 55m (+9.0%)
5. optional-x86_64-gnu-parallel-frontend: 2h 29m -> 2h 42m (+8.8%)
6. x86_64-gnu-llvm-21-3: 1h 57m -> 2h 7m (+8.4%)
7. dist-x86_64-musl: 2h 19m -> 2h 7m (-8.2%)
8. arm-android: 1h 47m -> 1h 39m (-8.2%)
9. dist-powerpc64-linux-musl: 1h 40m -> 1h 32m (-7.8%)
10. dist-x86_64-llvm-mingw: 1h 56m -> 1h 47m (-7.8%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (ac7f9ec): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.2%	[0.2%, 0.3%]	7
Regressions ❌ (secondary)	0.3%	[0.1%, 0.4%]	10
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	1
All ❌✅ (primary)	0.2%	[0.2%, 0.3%]	7

Max RSS (memory usage)

Results (primary 0.7%, secondary -0.6%)

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

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	1.1%	[1.1%, 1.1%]	1
Improvements ✅ (primary)	-0.9%	[-0.9%, -0.9%]	1
Improvements ✅ (secondary)	-1.5%	[-1.7%, -1.2%]	2
All ❌✅ (primary)	0.7%	[-0.9%, 2.3%]	2

Cycles

Results (secondary -1.2%)

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)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-3.6%, -2.1%]	2
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 485.797s -> 485.09s (-0.15%)
Artifact size: 396.97 MiB -> 394.98 MiB (-0.50%)

[matthiaskrgr]

this unwrap-errs here: #154173

[panstromek]

perf triage:

Reverted in #154257 for other reasons.

@rustbot label: +perf-regression-triaged

[lqd]

A note if we want to re-land this: while investigating the recent bevy/wgpu regression in the impossible predicates pass, I've noticed this PR made the binius_field (the crate which didn't work under the new solver and was fixed by #149904, also causing the bevy regression) check builds under the new solver go from ~2m to 50m/an hour. And just to have a few numbers at the same place, the old solver takes 4s.