clean up `Sized` checking #122493

lukas-code · 2024-03-14T13:08:04Z

This PR cleans up sized_constraint and related functions to make them simpler and faster. This should not make more or less code compile, but it can change error output in some rare cases.

enums and unions are `Sized`, even if they are not WF

The previous code has some special handling for enums, which made them sized if and only if the last field of each variant is sized. For example given this definition (which is not WF)

enum E<T1: ?Sized, T2: ?Sized, U1: ?Sized, U2: ?Sized> {
    A(T1, T2),
    B(U1, U2),
}

the enum was sized if and only if T2 and U2 are sized, while T1 and T2 were ignored for Sized checking. After this PR this enum will always be sized.

Unsized enums are not a thing in Rust and removing this special case allows us to return an Option<Ty> from sized_constraint, rather than a List<Ty>.

Similarly, the old code made an union defined like this

union Union<T: ?Sized, U: ?Sized> {
    head: T,
    tail: U,
}

sized if and only if U is sized, completely ignoring T. This just makes no sense at all and now this union is always sized.

apply the "perf hack" to all (non-error) types, instead of just type parameters

This "perf hack" skips evaluating sized_constraint(adt): Sized if sized_constraint(adt): Sized exactly matches a predicate defined on adt, for example:

// `Foo<T>: Sized` iff `T: Sized`, but we know `T: Sized` from a predicate of `Foo`
struct Foo<T /*: Sized */>(T);

Previously this was only applied to type parameters and now it is applied to every type. This means that for example this type is now always sized:

// Note that this definition is WF, but the type `S<T>` not WF in the global/empty ParamEnv
struct S<T>([T]) where [T]: Sized;

I don't anticipate this to affect compile time of any real-world program, but it makes the code a bit nicer and it also makes error messages a bit more consistent if someone does write such a cursed type.

tuples are sized if the last type is sized

The old solver already has this behavior and this PR also implements it for the new solver and is_trivially_sized. This makes it so that tuples work more like a struct defined like this:

struct TupleN<T1, T2, /* ... */ Tn: ?Sized>(T1, T2, /* ... */ Tn);

This might improve the compile time of programs with large tuples a little, but is mostly also a consistency fix.

`is_trivially_sized` for more types

This function is used post-typeck code (borrowck, const eval, codegen) to skip evaluating T: Sized in some cases. It will now return true in more cases, most notably UnsafeCell<T> and ManuallyDrop<T> where T.is_trivially_sized.

I'm anticipating that this change will improve compile time for some real world programs.

rustbot · 2024-03-14T13:08:12Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-03-14T13:08:14Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the core trait solver

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

oli-obk · 2024-03-14T13:12:46Z

@bors try @rust-timer queue

bors · 2024-03-14T13:13:56Z

⌛ Trying commit b5ef7bd with merge c9b796a...

clean up `Sized` checking This PR cleans up `sized_constraint` and related functions to make them simpler and faster. This should not make more or less code compile, but it can change error output in some rare cases. ## enums and unions are `Sized`, even if they are not WF The previous code has some special handling for enums, which made them sized if and only if the last field of each variant is sized. For example given this definition (which is not WF) ```rust enum E<T1: ?Sized, T2: ?Sized, U1: ?Sized, U2: ?Sized> { A(T1, T2), B(U1, U2), } ``` the enum was sized if and only if `T2` and `U2` are sized, while `T1` and `T2` were ignored for `Sized` checking. After this PR this enum will always be sized. Unsized enums are not a thing in Rust and removing this special case allows us to return an `Option<Ty>` from `sized_constraint`, rather than a `List<Ty>`. Similarly, the old code made an union defined like this ```rust union Union<T: ?Sized, U: ?Sized> { head: T, tail: U, } ``` sized if and only if `U` is sized, completely ignoring `T`. This just makes no sense at all and now this union is always sized. ## apply the "perf hack" to all (non-error) types, instead of just type parameters This "perf hack" skips evaluating `sized_constraint(adt): Sized` if `sized_constraint(adt): Sized` exactly matches a predicate defined on `adt`, for example: ```rust // `Foo<T>: Sized` iff `T: Sized`, but we know `T: Sized` from a predicate of `Foo` struct Foo<T /*: Sized */>(T); ``` Previously this was only applied to type parameters and now it is applied to every type. This means that for example this type is now always sized: ```rust // Note that this definition is WF, but the type `S<T>` not WF in the global/empty ParamEnv struct S<T>([T]) where [T]: Sized; ``` I don't anticipate this to affect compile time of any real-world program, but it makes the code a bit nicer and it also makes error messages a bit more consistent if someone does write such a cursed type. ## tuples are sized if the last type is sized The old solver already has this behavior and this PR also implements it for the new solver and `is_trivially_sized`. This makes it so that tuples work more like a struct defined like this: ```rust struct TupleN<T1, T2, /* ... */ Tn: ?Sized>(T1, T2, /* ... */ Tn); ``` This might improve the compile time of programs with large tuples a little, but is mostly also a consistency fix. ## `is_trivially_sized` for more types This function is used post-typeck code (borrowck, const eval, codegen) to skip evaluating `T: Sized` in some cases. It will now return `true` in more cases, most notably `UnsafeCell<T>` and `ManuallyDrop<T>` where `T.is_trivially_sized`. I'm anticipating that this change will improve compile time for some real world programs.

lcnr · 2024-03-14T14:15:52Z

r? @lcnr

lcnr · 2024-03-14T14:26:54Z

compiler/rustc_ty_utils/src/ty.rs

+
+    let result = sized_constraint_for_ty(tcx, tail_ty);
+
+    let result = result.filter(|&ty| {


instead of filter and map, use if ... { return None(..) }.

question: why not return None if the Sized condition references error?

you should be able to assume that the sized_trait() exists, using tcx.require_lang_item

question: why not return None if the Sized condition references error?

Good question. I thought I tried this and it changed some diagnostics output in some unexpected way, but after trying it again that seems to not be the case. The same also applies to the representability check above: If the type is not representable we could just return None.

👍 on throwing out filter, we can just use let result = sized_constraint_for_ty(tcx, tail_ty)?; (try operator) here.

RalfJung · 2024-03-14T14:36:47Z

compiler/rustc_const_eval/src/interpret/eval_context.rs


-                ty::Tuple(tys) => tys.last().iter().all(|ty| is_very_trivially_sized(**ty)),
+                ty::Tuple(tys) => tys.last().map_or(true, |&ty| is_very_trivially_sized(ty)),


I find map_or pretty hard to understand, I have to basically inline it to make sense of it. That's why I used all().

is_none_or would be the best way to express this, of course (Cc rust-lang/libs-team#212).

Interesting. For me it's the exact opposite: I'm confused every time Option is used as an iterator and it's not immediately obvious to me that this .iter().all(...) checks for at most one element. On the other hand .map_or(true already reads as is_none_or to me, but that's probably just because I got used to it.

Will revert this change if you prefer.

Yeah I think I'd prefer that.

bors · 2024-03-14T14:43:20Z

☀️ Try build successful - checks-actions
Build commit: c9b796a (c9b796a312ca9ef83ec28d1b5d71238d4c1533c6)

rust-timer · 2024-03-14T16:57:31Z

Finished benchmarking commit (c9b796a): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.4%]	9
All ❌✅ (primary)	-0.6%	[-0.6%, -0.6%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.6%	[2.4%, 4.5%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 669.429s -> 671.833s (0.36%)
Artifact size: 310.79 MiB -> 310.82 MiB (0.01%)

bors · 2024-03-14T19:50:41Z

☔ The latest upstream changes (presumably #122497) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr

r=me after some final nits

lcnr · 2024-03-18T17:42:31Z

compiler/rustc_middle/src/query/mod.rs

-    query adt_sized_constraint(key: DefId) -> ty::EarlyBinder<&'tcx ty::List<Ty<'tcx>>> {
-        desc { |tcx| "computing `Sized` constraints for `{}`", tcx.def_path_str(key) }
+    query adt_sized_constraint(key: DefId) -> Option<ty::EarlyBinder<Ty<'tcx>>> {
+        desc { |tcx| "computing `Sized` constraint for `{}`", tcx.def_path_str(key) }


Suggested change

desc { |tcx| "computing `Sized` constraint for `{}`", tcx.def_path_str(key) }

desc { |tcx| "computing the `Sized` constraint for `{}`", tcx.def_path_str(key) }

🤔 🤷

lcnr · 2024-03-18T17:43:33Z

compiler/rustc_trait_selection/src/solve/assembly/mod.rs

@@ -128,7 +128,7 @@ pub(super) trait GoalKind<'tcx>:
        goal: Goal<'tcx, Self>,
    ) -> QueryResult<'tcx>;

-    /// A type is `Copy` or `Clone` if its components are `Sized`.


lcnr · 2024-03-18T17:44:29Z

compiler/rustc_trait_selection/src/solve/assembly/structural_traits.rs

+        //   if the ADTs definition implies that it is sized by for all possible args.
+        //   In this case, the builtin impl will have no nested subgoals. This is a
+        //   "best effort" optimization and `sized_constraint` may return `Some`, even
+        //   if the ADT is sized for all possible args.
        ty::Adt(def, args) => {
            let sized_crit = def.sized_constraint(ecx.tcx());


Suggested change

let sized_crit = def.sized_constraint(ecx.tcx());

if let Some(ty) = def.sized_constraint(ecx.tcx()) {

lcnr · 2024-03-18T17:45:03Z

compiler/rustc_trait_selection/src/traits/select/mod.rs

@@ -2120,11 +2120,9 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
            ty::Adt(def, args) => {
                let sized_crit = def.sized_constraint(self.tcx());


also use if let here

lcnr · 2024-03-18T17:45:55Z

compiler/rustc_ty_utils/src/ty.rs

+            let intermediate = adt.sized_constraint(tcx);
+            intermediate.and_then(|intermediate| {


Suggested change

let intermediate = adt.sized_constraint(tcx);

intermediate.and_then(|intermediate| {

adt.sized_constraint(tcx).and_then(|intermediate| {

very much a nit 😅

lcnr · 2024-03-18T17:47:08Z

@bors delegate+

bors · 2024-03-18T17:47:11Z

✌️ @lukas-code, you can now approve this pull request!

If @lcnr told you to "r=me" after making some further change, please make that change, then do @bors r=@lcnr

lukas-code · 2024-03-18T21:34:27Z

@bors r=lcnr

bors · 2024-03-18T21:34:29Z

📌 Commit 99efae3 has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-03-19T04:21:18Z

⌛ Testing commit 99efae3 with merge 196ff44...

bors · 2024-03-19T06:22:32Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 196ff44 to master...

rust-timer · 2024-03-19T07:37:53Z

Finished benchmarking commit (196ff44): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.6%, -0.3%]	2
Improvements ✅ (secondary)	-0.7%	[-3.2%, -0.3%]	11
All ❌✅ (primary)	-0.4%	[-0.6%, -0.3%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.4%	[1.7%, 3.1%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.1%	[-8.0%, -2.2%]	7
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 667.92s -> 668.321s (0.06%)
Artifact size: 312.84 MiB -> 312.76 MiB (-0.03%)

…errors Restore `pred_known_to_hold_modulo_regions` As requested by `@lcnr` in rust-lang#123275 (comment) this PR restores `pred_known_to_hold_modulo_regions` to fix that "unexpected unsized tail" beta regression. This also adds the reduced repro from rust-lang#123275 (comment) as a sub-optimal test is better than no test at all, and it'll also cover rust-lang#108721. It still ICEs on master, even though https://github.com/phlip9/rustc-warp-ice doesn't on nightly anymore, since rust-lang#122493. Fixes rust-lang#123275. r? `@compiler-errors` but feel free to close if you'd rather have a better test instead cc `@wesleywiser` who had signed up to do the revert Will need a backport if we go with this PR: `@rustbot` label +beta-nominated

Rollup merge of rust-lang#123578 - lqd:regression-123275, r=compiler-errors Restore `pred_known_to_hold_modulo_regions` As requested by `@lcnr` in rust-lang#123275 (comment) this PR restores `pred_known_to_hold_modulo_regions` to fix that "unexpected unsized tail" beta regression. This also adds the reduced repro from rust-lang#123275 (comment) as a sub-optimal test is better than no test at all, and it'll also cover rust-lang#108721. It still ICEs on master, even though https://github.com/phlip9/rustc-warp-ice doesn't on nightly anymore, since rust-lang#122493. Fixes rust-lang#123275. r? `@compiler-errors` but feel free to close if you'd rather have a better test instead cc `@wesleywiser` who had signed up to do the revert Will need a backport if we go with this PR: `@rustbot` label +beta-nominated

rustbot assigned petrochenkov Mar 14, 2024

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 labels Mar 14, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 14, 2024

rustbot assigned lcnr and unassigned petrochenkov Mar 14, 2024

lcnr reviewed Mar 14, 2024

View reviewed changes

RalfJung reviewed Mar 14, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

lukas-code force-pushed the sized-constraint branch from b5ef7bd to f0d157f Compare March 14, 2024 15:08

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 14, 2024

Lukas Markeffsky added 4 commits March 14, 2024 20:52

make Representability::Infinite carry ErrorGuaranteed

0e7e1bf

clean up ADT sized constraint computation

8ad9411

remove unnecessary sized checks

8fe99f5

use a let chain

ee66acb

lukas-code force-pushed the sized-constraint branch from f0d157f to ee66acb Compare March 14, 2024 20:29

lukas-code mentioned this pull request Mar 16, 2024

ICE: should be sized but is not? #122581

Closed

lcnr reviewed Mar 18, 2024

View reviewed changes

address nits

99efae3

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 Mar 18, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 19, 2024

bors merged commit 196ff44 into rust-lang:master Mar 19, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Mar 19, 2024

bors mentioned this pull request Mar 19, 2024

make Thin a supertrait of Sized #122553

Closed

lukas-code deleted the sized-constraint branch March 19, 2024 08:47

matthiaskrgr mentioned this pull request Mar 26, 2024

ICE: !base.layout().is_sized() #123078

Closed

This was referenced Apr 4, 2024

regression: unexpected unsized tail ICE #123275

Closed

Restore pred_known_to_hold_modulo_regions #123578

Merged

zachs18 mentioned this pull request Aug 25, 2024

(Anti-)regression between Rust 1.78.0 and Rust 1.79.0 with struct containing Cow<[Self]> #129541

Closed

fee1-dead mentioned this pull request Aug 25, 2024

Add regression test for #129541 #129542

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean up `Sized` checking #122493

clean up `Sized` checking #122493

lukas-code commented Mar 14, 2024

rustbot commented Mar 14, 2024

rustbot commented Mar 14, 2024

oli-obk commented Mar 14, 2024

This comment has been minimized.

bors commented Mar 14, 2024

lcnr commented Mar 14, 2024

lcnr Mar 14, 2024 •

edited

Loading

lukas-code Mar 14, 2024

RalfJung Mar 14, 2024

lukas-code Mar 14, 2024

RalfJung Mar 14, 2024

bors commented Mar 14, 2024

This comment has been minimized.

rust-timer commented Mar 14, 2024

bors commented Mar 14, 2024

lcnr left a comment

lcnr Mar 18, 2024

lcnr Mar 18, 2024

lcnr Mar 18, 2024

lcnr Mar 18, 2024

lcnr Mar 18, 2024

lcnr commented Mar 18, 2024

bors commented Mar 18, 2024

lukas-code commented Mar 18, 2024

bors commented Mar 18, 2024

bors commented Mar 19, 2024

bors commented Mar 19, 2024

rust-timer commented Mar 19, 2024


		let result = sized_constraint_for_ty(tcx, tail_ty);

		let result = result.filter(\|&ty\| {


		ty::Tuple(tys) => tys.last().iter().all(\|ty\| is_very_trivially_sized(**ty)),
		ty::Tuple(tys) => tys.last().map_or(true, \|&ty\| is_very_trivially_sized(ty)),

	desc { \|tcx\| "computing `Sized` constraint for `{}`", tcx.def_path_str(key) }
	desc { \|tcx\| "computing the `Sized` constraint for `{}`", tcx.def_path_str(key) }

	let sized_crit = def.sized_constraint(ecx.tcx());
	if let Some(ty) = def.sized_constraint(ecx.tcx()) {

		@@ -2120,11 +2120,9 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
		ty::Adt(def, args) => {
		let sized_crit = def.sized_constraint(self.tcx());

		let intermediate = adt.sized_constraint(tcx);
		intermediate.and_then(\|intermediate\| {

	let intermediate = adt.sized_constraint(tcx);
	intermediate.and_then(\|intermediate\| {
	adt.sized_constraint(tcx).and_then(\|intermediate\| {

clean up Sized checking #122493

clean up Sized checking #122493

Conversation

lukas-code commented Mar 14, 2024

enums and unions are Sized, even if they are not WF

apply the "perf hack" to all (non-error) types, instead of just type parameters

tuples are sized if the last type is sized

is_trivially_sized for more types

rustbot commented Mar 14, 2024

rustbot commented Mar 14, 2024

oli-obk commented Mar 14, 2024

This comment has been minimized.

bors commented Mar 14, 2024

lcnr commented Mar 14, 2024

lcnr Mar 14, 2024 • edited Loading

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

bors commented Mar 14, 2024

This comment has been minimized.

rust-timer commented Mar 14, 2024

Overall result: ✅ improvements - no action needed

bors commented Mar 14, 2024

lcnr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr commented Mar 18, 2024

bors commented Mar 18, 2024

lukas-code commented Mar 18, 2024

bors commented Mar 18, 2024

bors commented Mar 19, 2024

bors commented Mar 19, 2024

rust-timer commented Mar 19, 2024

Overall result: ✅ improvements - no action needed

clean up `Sized` checking #122493

clean up `Sized` checking #122493

enums and unions are `Sized`, even if they are not WF

`is_trivially_sized` for more types

lcnr Mar 14, 2024 •

edited

Loading