Introduce a wrapper for "typed valtrees" and properly check the type before extracting the value #136180

lukas-code · 2025-01-28T12:10:38Z

This PR adds a new wrapper type ty::Value to replace the tuple (Ty, ty::ValTree) and become the new canonical representation of type-level constant values.

The value extraction methods try_to_bits/try_to_bool/try_to_target_usize are moved to this new type. For try_to_bits in particular, this avoids some redundant matches on ty::ConstKind::Value. Furthermore, these methods and will now properly check the type before extracting the value, which fixes some ICEs.

The name ty::Value was chosen to be consistent with ty::Expr.

Commit 1 should be non-functional and commit 2 adds the type check.

fixes #131102
supercedes #136130

r? @oli-obk
cc @FedericoBruzzone @BoxyUwU

rustbot · 2025-01-28T12:10:48Z

Some changes occurred in rustc_ty_utils::consts.rs

cc @BoxyUwU

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in compiler/rustc_sanitizers

cc @rust-lang/project-exploit-mitigations, @rcvalle

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in match checking

cc @Nadrieril

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

BoxyUwU · 2025-01-28T12:48:18Z

compiler/rustc_middle/src/ty/consts.rs

-    pub fn try_to_bool(self) -> Option<bool> {
-        self.try_to_valtree()?.0.try_to_scalar_int()?.try_to_bool().ok()
-    }
-
    #[inline]
    pub fn try_to_target_usize(self, tcx: TyCtxt<'tcx>) -> Option<u64> {


Keeping this on ty::Const because there's a large amount of code that inspects array lengths that would be made worse by having to do try_to_value().and_then(|val| val.try_to_target_usize(tcx))?

Seems reasonable to me 🤔 can a comment be added to state that since it doesn't seem unlikely that someone might come across this and be confused why it's the way it is

Keeping this on ty::Const because there's a large amount of code that inspects array lengths that would be made worse by having to do try_to_value().and_then(|val| val.try_to_target_usize(tcx))?

Yeah, this is used all over the place, including codegen, mir opts and clippy lints.

Seems reasonable to me 🤔 can a comment be added to state that since it doesn't seem unlikely that someone might come across this and be confused why it's the way it is

👍

BoxyUwU · 2025-01-28T12:51:43Z

compiler/rustc_middle/src/ty/structural_impls.rs

                    bug!("we checked that this is a valtree")
                };
                let mut cx = FmtPrinter::new(tcx, Namespace::ValueNS);
-                cx.pretty_print_const_valtree(valtree, ty, /*print_ty*/ true)?;
+                cx.pretty_print_const_valtree(cv.valtree, cv.ty, /*print_ty*/ true)?;


would it make sense to make pretty_print_const_valtree just be pretty_print_const_value and accept ty::Value?

There are also valtree_to_const_value (the fn that backs the query) and valtree_to_pat that still accept ty and valtree separately. I left these out for now to keep the diff smaller, but I can also change them here if you prefer.

Now that you mention valtree_to_const_value I'm realising that there's an unfortunate naming overlap between mir::ConstValue and ty::Const::Value lol. We should probably just leave the name as const_valtree even if it's updated to take a ty::Value as otherwise you'd wind up with weird functions like const_value_to_const_value...

for valtree_to_pat I'd just leave that up to oli as he's more familiar with the way mir pattern logic interacts with ty::Const and I don't want to make a decision there since I don't really work on that logic 🤔

Seems reasonable to me to use this in valtree_to_pat.

Leave a FIXME for pretty_print_const_valtree to be changed to using this type

FedericoBruzzone · 2025-01-28T16:56:28Z

Probably, this closure

rust/compiler/rustc_middle/src/mir/pretty.rs

Lines 1432 to 1436 in aa6f5ab

    
           // FIXME: call pretty_print_const_valtree? 
        
           let fmt_valtree = |valtree: &ty::ValTree<'tcx>| match valtree { 
        
               ty::ValTree::Leaf(leaf) => format!("Leaf({leaf:?})"), 
        
               ty::ValTree::Branch(_) => "Branch(..)".to_string(), 
        
           };

should become something like

let fmt_valtree = |cv: &ty::Value<'tcx>| {
    let mut cx = FmtPrinter::new(self.tcx, Namespace::ValueNS);
    cx.pretty_print_const_valtree(cv.valtree, cv.ty, /*print_ty*/ true).unwrap();
    cx.into_buffer()
};

IMO, it should accepts the new ty::Value.

Note that it implies that mod pretty; in compiler/rustc_middle/src/ty/print/mod.rs should be public.

This may or may not be a follow-up of this PR.

oli-obk

I don't think everything needs to happen in this PR, but leave some FIXMEs and add the doc comment on the ty::Const method

oli-obk · 2025-01-30T06:50:27Z

compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs

-            ty::ConstKind::Value(_, val) => convert.valtree_to_pat(val, ty),
+            ty::ConstKind::Value(cv) => convert.valtree_to_pat(cv.valtree, cv.ty),


Leave a FIXME to change valtree_to_pat to taking a Value directly

oli-obk · 2025-01-30T06:50:54Z

compiler/rustc_middle/src/ty/structural_impls.rs

                    bug!("we checked that this is a valtree")
                };
                let mut cx = FmtPrinter::new(tcx, Namespace::ValueNS);
-                cx.pretty_print_const_valtree(valtree, ty, /*print_ty*/ true)?;
+                cx.pretty_print_const_valtree(cv.valtree, cv.ty, /*print_ty*/ true)?;


Leave a FIXME for pretty_print_const_valtree to be changed to using this type

oli-obk · 2025-01-30T06:51:49Z

compiler/rustc_const_eval/src/lib.rs

+            cv.ty,
+            cv.valtree,


Leave a FIXME to change this to take the cv instead of its components

oli-obk · 2025-01-30T16:31:12Z

@bors r+

bors · 2025-01-30T16:31:15Z

📌 Commit ab2eb92 has been approved by oli-obk

It is now in the queue for this repository.

oli-obk · 2025-01-30T16:47:13Z

@bors r-

Co-authored-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>

and remove `ty::Const::try_to_scalar` because it becomes redundant

oli-obk · 2025-01-30T18:20:35Z

@bors r+

bors · 2025-01-30T18:20:38Z

📌 Commit ca3ff83 has been approved by oli-obk

It is now in the queue for this repository.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#135026 (Cast global variables to default address space) - rust-lang#135475 (uefi: Implement path) - rust-lang#135852 (Add `AsyncFn*` to `core` prelude) - rust-lang#136004 (tests: Skip const OOM tests on aarch64-unknown-linux-gnu) - rust-lang#136157 (override build profile for bootstrap tests) - rust-lang#136180 (Introduce a wrapper for "typed valtrees" and properly check the type before extracting the value) - rust-lang#136256 (Add release notes for 1.84.1) - rust-lang#136271 (Remove minor future footgun in `impl Debug for MaybeUninit`) - rust-lang#136288 (Improve documentation for file locking) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#136180 - lukas-code:typed-valtree, r=oli-obk Introduce a wrapper for "typed valtrees" and properly check the type before extracting the value This PR adds a new wrapper type `ty::Value` to replace the tuple `(Ty, ty::ValTree)` and become the new canonical representation of type-level constant values. The value extraction methods `try_to_bits`/`try_to_bool`/`try_to_target_usize` are moved to this new type. For `try_to_bits` in particular, this avoids some redundant matches on `ty::ConstKind::Value`. Furthermore, these methods and will now properly check the type before extracting the value, which fixes some ICEs. The name `ty::Value` was chosen to be consistent with `ty::Expr`. Commit 1 should be non-functional and commit 2 adds the type check. --- fixes rust-lang#131102 supercedes rust-lang#136130 r? `@oli-obk` cc `@FedericoBruzzone` `@BoxyUwU`

… r=oli-obk Use the type-level constant value `ty::Value` where needed **Follow-up to rust-lang#136180** ### Summary This PR refactors functions to accept a single type-level constant value `ty::Value` instead of separate `ty::ValTree` and `ty::Ty` parameters: - `valtree_to_const_value`: now takes `ty::Value` - `pretty_print_const_valtree`: now takes `ty::Value` - Uses `pretty_print_const_valtree` for formatting valtrees when `visit_const_operand` - Moves `try_to_raw_bytes` from `ty::Valtree` to `ty::Value` --- r? `@lukas-code` `@oli-obk`

… r=oli-obk Use the type-level constant value `ty::Value` where needed **Follow-up to rust-lang#136180** ### Summary This PR refactors functions to accept a single type-level constant value `ty::Value` instead of separate `ty::ValTree` and `ty::Ty` parameters: - `valtree_to_const_value`: now takes `ty::Value` - `pretty_print_const_valtree`: now takes `ty::Value` - Uses `pretty_print_const_valtree` for formatting valtrees when `visit_const_operand` - Moves `try_to_raw_bytes` from `ty::Valtree` to `ty::Value` --- r? ``@lukas-code`` ``@oli-obk``

Rollup merge of rust-lang#136430 - FedericoBruzzone:follow-up-136180, r=oli-obk Use the type-level constant value `ty::Value` where needed **Follow-up to rust-lang#136180** ### Summary This PR refactors functions to accept a single type-level constant value `ty::Value` instead of separate `ty::ValTree` and `ty::Ty` parameters: - `valtree_to_const_value`: now takes `ty::Value` - `pretty_print_const_valtree`: now takes `ty::Value` - Uses `pretty_print_const_valtree` for formatting valtrees when `visit_const_operand` - Moves `try_to_raw_bytes` from `ty::Valtree` to `ty::Value` --- r? ``@lukas-code`` ``@oli-obk``

valtree performance tuning Summary: This PR makes type checking of code with many type-level constants faster. After rust-lang#136180 was merged, we observed a small perf regression (rust-lang#136318 (comment)). This happened because that PR introduced additional copies in the fast reject code path for consts, which is very hot for certain crates: https://github.com/rust-lang/rust/blob/6c1d960d88dd3755548b3818630acb63fa98187e/compiler/rustc_type_ir/src/fast_reject.rs#L486-L487 This PR improves the performance again by properly interning the valtrees so that copying and comparing them becomes faster. This will become especially useful with `feature(adt_const_params)`, so the fast reject code doesn't have to do a deep compare of the valtrees. A less invasive alternative that I considered is simply avoiding copies introduced by rust-lang#136180 and comparing the valtrees it in-place (see commit: rust-lang@9e91e50 / perf results: rust-lang#136593 (comment)), however that was still measurably slower than interning.

valtree performance tuning Summary: This PR makes type checking of code with many type-level constants faster. After rust-lang#136180 was merged, we observed a small perf regression (rust-lang#136318 (comment)). This happened because that PR introduced additional copies in the fast reject code path for consts, which is very hot for certain crates: https://github.com/rust-lang/rust/blob/6c1d960d88dd3755548b3818630acb63fa98187e/compiler/rustc_type_ir/src/fast_reject.rs#L486-L487 This PR improves the performance again by properly interning the valtrees so that copying and comparing them becomes faster. This will become especially useful with `feature(adt_const_params)`, so the fast reject code doesn't have to do a deep compare of the valtrees. Note that we can't just compare the interned consts themselves in the fast reject, because sometimes `'static` lifetimes in the type are be replaced with inference variables (due to canonicalization) on one side but not the other. A less invasive alternative that I considered is simply avoiding copies introduced by rust-lang#136180 and comparing the valtrees it in-place (see commit: rust-lang@9e91e50 / perf results: rust-lang#136593 (comment)), however that was still measurably slower than interning. There are some minor regressions in secondary benchmarks: These happen due to changes in memory allocations and seem acceptable to me. The crates that make heavy use of valtrees show no significant changes in memory usage.

valtree performance tuning Summary: This PR makes type checking of code with many type-level constants faster. After rust-lang/rust#136180 was merged, we observed a small perf regression (rust-lang/rust#136318 (comment)). This happened because that PR introduced additional copies in the fast reject code path for consts, which is very hot for certain crates: https://github.com/rust-lang/rust/blob/6c1d960d88dd3755548b3818630acb63fa98187e/compiler/rustc_type_ir/src/fast_reject.rs#L486-L487 This PR improves the performance again by properly interning the valtrees so that copying and comparing them becomes faster. This will become especially useful with `feature(adt_const_params)`, so the fast reject code doesn't have to do a deep compare of the valtrees. Note that we can't just compare the interned consts themselves in the fast reject, because sometimes `'static` lifetimes in the type are be replaced with inference variables (due to canonicalization) on one side but not the other. A less invasive alternative that I considered is simply avoiding copies introduced by rust-lang/rust#136180 and comparing the valtrees it in-place (see commit: rust-lang/rust@9e91e50 / perf results: rust-lang/rust#136593 (comment)), however that was still measurably slower than interning. There are some minor regressions in secondary benchmarks: These happen due to changes in memory allocations and seem acceptable to me. The crates that make heavy use of valtrees show no significant changes in memory usage.

rustbot assigned oli-obk Jan 28, 2025

oli-obk changed the title ~~Intrutuce a wrapper for "typed valtrees" and properly check the type before extrating the value~~ Introduce a wrapper for "typed valtrees" and properly check the type before extrating the value Jan 28, 2025

oli-obk changed the title ~~Introduce a wrapper for "typed valtrees" and properly check the type before extrating the value~~ Introduce a wrapper for "typed valtrees" and properly check the type before extracting the value Jan 28, 2025

BoxyUwU reviewed Jan 28, 2025

View reviewed changes

FedericoBruzzone approved these changes Jan 29, 2025

View reviewed changes

oli-obk approved these changes Jan 30, 2025

View reviewed changes

FedericoBruzzone mentioned this pull request Jan 30, 2025

Move try_to_bits from Const to Valtree #136130

Closed

bors 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 Jan 30, 2025

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 30, 2025

Lukas Markeffsky and others added 3 commits January 30, 2025 17:47

introduce ty::Value

10fc0b1

Co-authored-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>

check the types in ty::Value to value conversion

0055fb9

and remove `ty::Const::try_to_scalar` because it becomes redundant

add comments

ca3ff83

lukas-code force-pushed the typed-valtree branch from ab2eb92 to ca3ff83 Compare January 30, 2025 17:14

bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 30, 2025

matthiaskrgr mentioned this pull request Jan 30, 2025

Rollup of 9 pull requests #136318

Merged

bors merged commit 6a66a27 into rust-lang:master Jan 31, 2025
6 checks passed

rustbot added this to the 1.86.0 milestone Jan 31, 2025

lukas-code deleted the typed-valtree branch January 31, 2025 02:33

FedericoBruzzone mentioned this pull request Feb 2, 2025

Use the type-level constant value ty::Value where needed #136430

Merged

lukas-code mentioned this pull request Feb 9, 2025

valtree performance tuning #136593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a wrapper for "typed valtrees" and properly check the type before extracting the value #136180

Introduce a wrapper for "typed valtrees" and properly check the type before extracting the value #136180

lukas-code commented Jan 28, 2025 •

edited

Loading

rustbot commented Jan 28, 2025

BoxyUwU Jan 28, 2025

lukas-code Jan 28, 2025

BoxyUwU Jan 28, 2025

lukas-code Jan 28, 2025

BoxyUwU Jan 28, 2025

BoxyUwU Jan 28, 2025

RalfJung Jan 28, 2025

oli-obk Jan 30, 2025

FedericoBruzzone commented Jan 28, 2025 •

edited

Loading

oli-obk left a comment

oli-obk Jan 30, 2025

oli-obk Jan 30, 2025

oli-obk Jan 30, 2025

oli-obk commented Jan 30, 2025

bors commented Jan 30, 2025

This comment has been minimized.

oli-obk commented Jan 30, 2025

oli-obk commented Jan 30, 2025

bors commented Jan 30, 2025

		ty::ConstKind::Value(_, val) => convert.valtree_to_pat(val, ty),
		ty::ConstKind::Value(cv) => convert.valtree_to_pat(cv.valtree, cv.ty),

Introduce a wrapper for "typed valtrees" and properly check the type before extracting the value #136180

Introduce a wrapper for "typed valtrees" and properly check the type before extracting the value #136180

Conversation

lukas-code commented Jan 28, 2025 • edited Loading

rustbot commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FedericoBruzzone commented Jan 28, 2025 • edited Loading

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jan 30, 2025

bors commented Jan 30, 2025

This comment has been minimized.

oli-obk commented Jan 30, 2025

oli-obk commented Jan 30, 2025

bors commented Jan 30, 2025

lukas-code commented Jan 28, 2025 •

edited

Loading

FedericoBruzzone commented Jan 28, 2025 •

edited

Loading