Skip to content

Remove indices from fresh ty/const vars. #144581

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nnethercote
Copy link
Contributor

Each of InferTy::Fresh{Ty,IntTy,FloatTy} and InferConst::Fresh have a u32 index. It turns out the index is only used for some weak sanity checking during freshening and can be removed, making these values a bit like ReErased.

This change simplifies things quite a bit, e.g. TypeFreshener no longers needs maps, and we can eliminate a bunch of pre-interned fresh types.

r? @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 28, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 28, 2025

changes to the core type system

cc @compiler-errors, @lcnr

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

@nnethercote
Copy link
Contributor Author

I'm not sure if this is legitimate. All the tests pass locally, but I'm getting an ICE when compiling nalgebra-0.33.0:

error: internal compiler error: compiler/rustc_trait_selection/src/traits/select/confirmation.rs:243:17: Where clause `Binder { value: <base::default_allocator::DefaultAllocator as base::allocator::Allocator<R, C>>, bound_vars: [] }` was applicable to `Obligation(predicate=Binder { value: TraitPredicate(<base::default_allocator::DefaultAllocator as base::allocator::Allocator<_, _>>, polarity:Positive), bound_vars: [] }, depth=1)` but now is not

@nnethercote
Copy link
Contributor Author

@bors try

rust-bors bot added a commit that referenced this pull request Jul 28, 2025
Remove indices from fresh ty/const vars.
@rust-bors
Copy link

rust-bors bot commented Jul 28, 2025

⌛ Trying commit db9c00f with merge 12882b8

To cancel the try build, run the command @bors try cancel.

@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Jul 28, 2025

it's not legitimate sadly

we use their index to make sure different infer vars result in different fresh variables. We use freshened infer vars for caching and need a differetn cache entry for T: Trait<?0, ?1> and T: Trait<?0,?1>

@rust-bors
Copy link

rust-bors bot commented Jul 28, 2025

☀️ Try build successful (CI)
Build commit: 12882b8 (12882b84109d06697f8278cd473a9906315add23, parent: 65b6cdb6a6d33987b9d642a4882283c71fbe3957)

@nnethercote
Copy link
Contributor Author

need a different cache entry for T: Trait<?0, ?1> and T: Trait<?0,?1>

Would that end up being T: Trait<Fresh(0), Fresh(1)> and T: Trait<Fresh(2), Fresh(3)>?

It's curious how close this is to working. When I first tried it I was expecting failures all over the place. When the ui tests and other tests passed I thought it was legitimate.

Each of `InferTy::Fresh{Ty,IntTy,FloatTy}` and `InferConst::Fresh` have
a `u32` index. It turns out the index is only used for some weak
sanity checking during freshening and can be removed, making these
values a bit like `ReErased`.

This change simplifies things quite a bit, e.g. `TypeFreshener` no
longers needs maps, and we can eliminate a bunch of pre-interned fresh
types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants