Skip to content

Make TypeId const comparable #142789

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Make TypeId const comparable #142789

wants to merge 7 commits into from
+337 −769

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 20, 2025 •
edited
Loading

This should unblock stabilizing const TypeId::of and allow us to progress into any possible future we want to take TypeId to.

To achieve that TypeId now contains 16 / size_of<usize>() pointers which each are actually just size_of<usize>() bytes of the stable hash. At compile-time these pointers cannot be dereferenced or otherwise inspected (at present doing so might ICE the compiler). Preventing inspection of this data allows us to refactor TypeId to any other scheme in the future without breaking anyone who was tempted to transmute TypeId to obtain the hash at compile-time.

cc @eddyb for their previous work on #95845 (which we still can do in the future if we want to get rid of the hash as the final thing that declares two TypeIds as equal).

r? @RalfJung

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 20, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 20, 2025

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

# to view
Dante-Broggi
Dante-Broggi reviewed Jun 21, 2025
View reviewed changes
Noratrieb
Noratrieb reviewed Jun 21, 2025
View reviewed changes
@oli-obk oli-obk force-pushed the constable-type-id branch from 3cddd21 to 1fd7b66 Compare June 21, 2025 10:20
@rust-log-analyzer

This comment has been minimized.

# to view
@RalfJung
Copy link
Member

It will be a while until I have the capacity to review a PR of this scale.

Meanwhile, could you say a bit more about the architecture of the change? It seems you want for the "new kind of allocation" approach, but it's not clear from the PR description how exactly that shows up in TypeId.

Also, I am definitely not comfortable landing this by myself, I can only review the const-eval parts. Changing the representation of TypeId has ramifications well beyond that that I do not feel qualified to evaluate -- I think an MCP would be justified.

@rustbot
Copy link
Collaborator

rustbot commented Jun 21, 2025

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 21, 2025

Well, I got private feedback yesterday that instead of encoding a 16 byte value as an 8 byte pointer to the 16 byte value and an 8 byte hash, I should just do the thing where we split up type id internally into pointer sized chunks and codegen will make a hash out of it again.

TLDR: no changes to runtime type id anymore in the latest revision of this PR. Only compile-time type id is now a bit funny

@oli-obk oli-obk mentioned this pull request Jun 21, 2025
Open
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 21, 2025

It will be a while until I have the capacity to review a PR of this scale.

I'm splitting unrelated parts out, so the high level feedback is already useful and I'll look for libs and codegen ppl to review the appropriate parts

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
Checking tidy rustdoc_json...
Running eslint on rustdoc JS files
`rustdoc-json-types` was not modified.
tidy: Skipping binary file check, read-only filesystem
##[error]tidy error: /checkout/library/core/src/any.rs:777: undocumented unsafe
##[error]tidy error: /checkout/library/core/src/any.rs:797: undocumented unsafe
removing old virtual environment
creating virtual environment at '/checkout/obj/build/venv' using 'python3.10' and 'venv'
creating virtual environment at '/checkout/obj/build/venv' using 'python3.10' and 'virtualenv'
Requirement already satisfied: pip in ./build/venv/lib/python3.10/site-packages (25.1.1)
linting python files
All checks passed!
checking python file formatting
28 files already formatted
checking C++ file formatting
some tidy checks failed
Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:01:14
  local time: Sat Jun 21 11:22:56 UTC 2025
  network time: Sat, 21 Jun 2025 11:22:56 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

antoyo
antoyo reviewed Jun 21, 2025
View reviewed changes
compiler/rustc_codegen_gcc/src/common.rs
@@ -303,6 +303,8 @@ impl<'gcc, 'tcx> ConstCodegenMethods for CodegenCx<'gcc, 'tcx> {
let init = self.const_data_from_alloc(alloc);
self.static_addr_of(init, alloc.inner().align, None)
}
// TODO: generate segment of type id hash
GlobalAlloc::Type { .. } => todo!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be mostly the same as the LLVM implementation, except the call to LLVMConstIntToPtr will be a call to Builder::inttoptr and the call to const_alloc_to_llvm should be a call to CodegenCx::const_data_from_alloc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I should really move this to codegen_as and expose those primitives

bjorn3
bjorn3 reviewed Jun 21, 2025
View reviewed changes
compiler/rustc_codegen_cranelift/src/constant.rs
@@ -471,6 +474,8 @@ fn define_all_allocs(tcx: TyCtxt<'_>, module: &mut dyn Module, cx: &mut Constant
.principal()
.map(|principal| tcx.instantiate_bound_regions_with_erased(principal)),
),
// TODO
GlobalAlloc::Type { .. } => todo!(),
Copy link
Member

Choose a reason for hiding this comment

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

This probably needs something like data_id_for_type similar to data_id_for_vtable, which can then also be used in codegen_const_value.

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@antoyo antoyo antoyo left review comments

@bjorn3 bjorn3 bjorn3 left review comments

@Dante-Broggi Dante-Broggi Dante-Broggi left review comments

@Noratrieb Noratrieb Noratrieb left review comments

Assignees

@RalfJung RalfJung

Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@oli-obk @rustbot @rust-log-analyzer @RalfJung @antoyo @bjorn3 @Dante-Broggi @Noratrieb