Skip to content

Get rid of StructurallyRelateAliases#158731

Merged
rust-bors[bot] merged 3 commits into
rust-lang:mainfrom
adwinwhite:rid-structurally-relate-aliases
Jul 20, 2026
Merged

Get rid of StructurallyRelateAliases#158731
rust-bors[bot] merged 3 commits into
rust-lang:mainfrom
adwinwhite:rid-structurally-relate-aliases

Conversation

@adwinwhite

@adwinwhite adwinwhite commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

View all comments

Part of #155345

Finally we can get rid of the last use of StructurallyRelateAliases::Yes.

r? lcnr

@rustbot

rustbot commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

changes to the core type system

cc @lcnr

@rustbot 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Jul 3, 2026
@rust-log-analyzer

This comment has been minimized.

Comment thread compiler/rustc_infer/src/infer/context.rs Outdated
Comment thread compiler/rustc_next_trait_solver/src/canonical/mod.rs
}

(ty::Infer(ty::TyVar(a_vid)), _) => {
infcx.equate_ty_var_raw(a_vid, b);

@lcnr lcnr Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a debug_assert to instantiate_ty_var_raw tht the type we instantiate it with does not refer to anything from a higher universe?

View changes since the review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for regions and consts :>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The debug_assert in instantiate_ty_var_eq_raw would make tests/ui/async-await/async-closures/post-mono-higher-ranked-hang.rs hang again, probably due to max_universe being slow on deeply nested types.
And these asserts are likely unnecessary as we need to replace the universes anyway? We do meet higher universes in response. Generalization handles this by creating infer vars in current universe and I copied that.

We can't assert for regions either as we encounter higher universes in response var_values.
In proof tree visitor we have newly created original_values in lower universe.
Related #154329 (comment)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The asserts I added: 09388a3

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, having this assert right after we have a type folder whicih should do it correctly feels 🤷 to me

I worry about type_variables().instantiate more generally and would love to have that assert in there.

The debug_assert in instantiate_ty_var_eq_raw would make tests/ui/async-await/async-closures/post-mono-higher-ranked-hang.rs hang again, probably due to max_universe being slow on deeply nested types.

does the max_universe visitor use a cache?

Comment on lines +340 to +341
let a = infcx.shallow_resolve_const(a);
let b = infcx.shallow_resolve_const(b);

@lcnr lcnr Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be able to not do that but debug_assert that this is a noop?

View changes since the review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did that work out?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partially, see the fixme.

Comment on lines +380 to +381
if let Some(a_inner) = a.no_bound_vars()
&& let Some(b_inner) = b.no_bound_vars()

@lcnr lcnr Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a and b have exactly the same bound vars here. we don't have to bother with the rebinding 🤔 actually, we can just recurse without instantiating bound vars at all

we can just assert that the bound vars are equal, and then recurse :>

View changes since the review

@lcnr lcnr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beautiful ❤️ i love rigid aliases

View changes since this review

@rust-bors

This comment has been minimized.

@adwinwhite
adwinwhite force-pushed the rid-structurally-relate-aliases branch from 4a27c1d to 5920ca5 Compare July 8, 2026 07:38
@rustbot

This comment has been minimized.

@adwinwhite

Copy link
Copy Markdown
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 8, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Jul 8, 2026
…=<try>

Get rid of `StructurallyRelateAliases`
@rust-bors

rust-bors Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: a8102e7 (a8102e767108a4da38b2788dcc91946783fc743e)
Base parent: 99576cf (99576cf2ebc870c305225e9856b9423228872c56)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (a8102e7): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing 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
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.8% [-16.8%, -0.2%] 25
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary 2.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% [0.8%, 3.7%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results (secondary -11.1%)

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)
-11.1% [-17.7%, -1.6%] 5
All ❌✅ (primary) - - 0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 488.508s -> 490.377s (0.38%)
Artifact size: 388.95 MiB -> 388.58 MiB (-0.10%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 8, 2026
self.inner.borrow_mut().const_unification_table().union(a, b);
}

fn instantiate_ty_var_eq_raw(&self, vid: ty::TyVid, ty: Ty<'tcx>) {

@lcnr lcnr Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename instantiate_ty_var_raw to generalize_and_instantiate_ty_var_raw maybe? Not a big fan of the eq in instantiate_ty_var_eq_raw

actually, just rename it to instantiate_ty_var. I don't think the _raw is useful and its impl is called instantiate_ty_var already

View changes since the review

return answer;
}

let folded = if let ty::Infer(ty::TyVar(vid)) = t.kind() {

@lcnr lcnr Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you change this to a match and also add an assert that the universe of placeholders is "correct"

View changes since the review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or I guess don't handle placeholders but do keep the debug_assert 🤔

Comment on lines +441 to +444
// We always set the universes in generalized term to the infer var's
// in generalization, if those universes are higher than the infer var's.
// We replicate the behavior here.
struct UniverseFolder<'a, 'tcx> {

@lcnr lcnr Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// We always set the universes in generalized term to the infer var's
// in generalization, if those universes are higher than the infer var's.
// We replicate the behavior here.
struct UniverseFolder<'a, 'tcx> {
/// Canonicalizing inputs puts all inference variables and placeholders
/// into the root universe.
///
/// This means when instantiating the query response we need to pull
/// down the universe of returned `var_values` to the universe of
/// the inference variable in `orig_values`.
///
/// This folder is similar to the `Generalizer`, except that it simply
/// structurally folds non-rigid aliases as these should have already
/// been generalized in the query so we shouldn't try to do it again.
struct LowerUniverseFolder<'a, 'tcx> {

View changes since the review

}
}

impl<Infcx, I> PredicateEmittingRelation<Infcx> for ResponseRelating<'_, Infcx, I>

@lcnr lcnr Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that we don't use super_combine, do we even need to implement this?

View changes since the review

return answer;
}

let folded = if let ty::Infer(ty::TyVar(vid)) = t.kind() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or I guess don't handle placeholders but do keep the debug_assert 🤔

@adwinwhite
adwinwhite force-pushed the rid-structurally-relate-aliases branch from 5920ca5 to 33d6455 Compare July 17, 2026 06:14
@rustbot

rustbot commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rust-log-analyzer

This comment has been minimized.

Comment on lines +491 to +496
debug_assert!(
self.for_universe.can_name(p.universe),
"variable in universe {:?} can't name type in universe {:?}",
self.for_universe,
p.universe
);

@lcnr lcnr Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you instead do a separate max_universe check after this folder has returned. Given that we have a fast path for

        if !(t.has_free_regions() || t.has_infer()) {
            return t;
        }

this is unreachable

View changes since the review

Comment thread compiler/rustc_type_ir/src/universe.rs Outdated
@@ -1,3 +1,4 @@
use rustc_data_structures::fx::FxHashSet;

@lcnr lcnr Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should lint on using rustc_data_structures in rustc_type_ir as it breaks rustanalyzer.

there's rustc_type_ir::data_structures::HashMap instead

View changes since the review

// FIXME: make this a debug_assert.
// Currently proof tree evaluation can unify infer vars in original
// vars while not resolving them.
// See `tests/ui/traits/next-solver/transmute-from-async-closure.rs`

@lcnr lcnr Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like proof tree evaluation really shouldn't use this at all 😁

proof trees are just generally deeply annoying 😅 :x I guess this is fine for now

View changes since the review

@adwinwhite
adwinwhite force-pushed the rid-structurally-relate-aliases branch from 33d6455 to 03d5f39 Compare July 20, 2026 06:42
Comment on lines +557 to +558
// FIXME: unfortunately we lose the relating span here unless we take another
// argument.

@lcnr lcnr Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, surprised this doesn't cause diagnostics issues 🤔

View changes since the review

@lcnr

lcnr commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

@bors r+ rollup=never

@rust-bors

rust-bors Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 03d5f39 has been approved by lcnr

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 Jul 20, 2026
@rust-bors

This comment has been minimized.

@rust-bors rust-bors Bot added merged-by-bors This PR was explicitly merged by bors. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 20, 2026
@rust-bors

rust-bors Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

☀️ Test successful - CI
Approved by: lcnr
Duration: 3h 30m 26s
Pushing 9e71b3b to main...

@rust-bors
rust-bors Bot merged commit 9e71b3b into rust-lang:main Jul 20, 2026
14 checks passed
@rustbot rustbot added this to the 1.99.0 milestone Jul 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor
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 14ead7d (parent) -> 9e71b3b (this PR)

Test differences

Show 2 test diffs

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

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 9e71b3bc704eea68d39bd0f6a46703c7d22f5d3b --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. x86_64-gnu-gcc-core-tests: 8m 32s -> 13m 48s (+61.7%)
  2. pr-check-1: 20m 24s -> 31m 34s (+54.8%)
  3. dist-x86_64-illumos: 1h 14m -> 1h 48m (+45.1%)
  4. optional-x86_64-gnu-parallel-frontend: 1h 31m -> 2h 4m (+36.1%)
  5. dist-x86_64-linux-alt: 1h 52m -> 2h 24m (+28.2%)
  6. x86_64-gnu-nopt: 1h 51m -> 2h 21m (+27.4%)
  7. dist-powerpc64le-linux-musl: 1h 15m -> 1h 35m (+26.6%)
  8. dist-i686-linux: 1h 50m -> 1h 22m (-25.3%)
  9. dist-armv7-linux: 1h 32m -> 1h 9m (-25.0%)
  10. dist-ohos-aarch64: 1h 1m -> 1h 16m (+24.2%)
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

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (9e71b3b): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.4% [-4.7%, -0.2%] 24
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary 4.8%)

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)
4.8% [4.5%, 5.1%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results (primary -2.7%, secondary -3.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)
- - 0
Improvements ✅
(primary)
-2.7% [-3.6%, -1.8%] 3
Improvements ✅
(secondary)
-3.2% [-6.6%, -1.7%] 10
All ❌✅ (primary) -2.7% [-3.6%, -1.8%] 3

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 485.673s -> 486.168s (0.10%)
Artifact size: 391.74 MiB -> 392.47 MiB (0.19%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants