Sized Hierarchy: Part I #137944

davidtwco · 2025-03-03T14:51:33Z

This patch implements the non-const parts of rust-lang/rfcs#3729. It introduces two new traits to the standard library, MetaSized and PointeeSized. See the RFC for the rationale behind these traits and to discuss whether this change makes sense in the abstract.

These traits are unstable (as is their constness), so users cannot refer to them without opting-in to feature(sized_hierarchy). These traits are not behind cfgs as this would make implementation unfeasible, there would simply be too many cfgs required to add the necessary bounds everywhere. So, like Sized, these traits are automatically implemented by the compiler.

RFC 3729 describes changes which are necessary to preserve backwards compatibility given the introduction of these traits, which are implemented and as follows:

?Sized is rewritten as MetaSized
MetaSized is added as a default supertrait for all traits w/out an explicit sizedness supertrait already.

There are no edition migrations implemented in this, as these are primarily required for the constness parts of the RFC and prior to stabilisation of this (and so will come in follow-up PRs alongside the const parts). All diagnostic output should remain the same (showing ?Sized even if the compiler sees MetaSized) unless the sized_hierarchy feature is enabled.

Due to the use of unstable extern types in the standard library and rustc, some bounds in both projects have had to be relaxed already - this is unfortunate but unavoidable so that these extern types can continue to be used where they were before. Performing these relaxations in the standard library and rustc are desirable longer-term anyway, but some bounds are not as relaxed as they ideally would be due to the inability to relax Deref::Target (this will be investigated separately).

It is hoped that this is implemented such that it could be merged and these traits could exist "under the hood" without that being observable to the user (other than in any performance impact this has on the compiler, etc). Some details might leak through due to the standard library relaxations, but this has not been observed in test output.

Notes:

Any commits starting with "upstream:" can be ignored, as these correspond to other upstream PRs that this is based on which have yet to be merged.
This best reviewed commit-by-commit. I've attempted to make the implementation easy to follow and keep similar changes and test output updates together.
- Each commit has a short description describing its purpose.
- This patch is large but it's primarily in the test suite.
I've worked on the performance of this patch and a few optimisations are implemented so that the performance impact is neutral-to-minor.
PointeeSized is a different name from the RFC just to make it more obvious that it is different from std::ptr::Pointee but all the names are yet to be bikeshed anyway.
@nikomatsakis has confirmed that this can proceed as an experiment from the t-lang side

Fixes #79409.

r? @ghost (I'll discuss this with relevant teams to find a reviewer)

davidtwco · 2025-03-03T15:21:57Z

~~I can reproduce this locally but I have no idea why it would be related to this patch.~~ Clippy needed adjusting.

compiler/rustc_type_ir/src/flags.rs

compiler/rustc_passes/src/check_attr.rs

tests/ui/target-feature/tied-features-no-implication-1.rs

traviscross · 2025-03-04T01:46:50Z

cc @rust-lang/lang

tmiasko · 2025-03-04T10:12:47Z

Does this perhaps fix #127336 by rejecting it?

davidtwco · 2025-03-06T10:22:24Z

Does this perhaps fix #127336 by rejecting it?

It doesn't currently.

davidtwco · 2025-03-06T15:59:23Z

Undrafting now that CI passes

davidtwco · 2025-04-22T08:25:18Z

Had an initial glance yesterday (not posting the review comment yet), might have more time to do a full review next week, but I might reroll depending on my IRL availability.

No worries, feel free to post what you have. @oli-obk said he'd try and review this week so don't worry about having to get through it all yourself. Thanks for reviewing so far.

compiler/rustc_hir_analysis/src/collect/item_bounds.rs

oli-obk

Still need to review bounds.rs

Is there any new stuff that people can rely on and get wrong in unsafe code that we should be checking in miri?

compiler/rustc_trait_selection/src/traits/coherence.rs

compiler/rustc_trait_selection/src/traits/util.rs

compiler/rustc_ty_utils/src/ty.rs

compiler/rustc_type_ir/src/elaborate.rs

library/core/src/ops/unsize.rs

tests/incremental/hashes/trait_defs.rs

oli-obk · 2025-04-22T09:27:36Z

tests/ui/layout/unconstrained-param-ice-137308.rs

-impl<C: ?Sized> A for u8 { //~ ERROR: the type parameter `C` is not constrained
+impl<C: PointeeSized> A for u8 { //~ ERROR: the type parameter `C` is not constrained


did you search and replace the test suite for ?Sized? I'm not sure what's best, but I would have assumed keeping most of the test suite on ?Sized and only changing what needs changing would be best.

I didn't, I only changed tests that failed and needed changing. In this test, for example, ?Sized would no longer be unconstrained, it would be MetaSized-constrained, and so the test would be doing something different (and failing as a consequence).

I think something is wrong then. C is unconstrained because it is not used in the impl, what bounds are on it are irrelevant.

I can check if this test still fails because it might just have been the state of the implementation at the time.

I've revisited these tests and they fail with some additional "type annotations needed" errors, which I expect is what I was trying to avoid, but these are likely just downstream of the unconstrained parameter error as there's now an unconstrained C: MetaSized instead of bare C. I've just blessed these instead in the commit which was changing them.

davidtwco · 2025-04-22T11:33:00Z

Rebased and addressed feedback.

Is there any new stuff that people can rely on and get wrong in unsafe code that we should be checking in miri?

I don't think so, but I'm not as familiar with what type of stuff that would be as I ought to be.

fee1-dead

Already got some comments stashed up, posting them now

compiler/rustc_middle/src/ty/adt.rs

fee1-dead · 2025-04-22T04:34:58Z

compiler/rustc_ty_utils/src/ty.rs

-/// Calculates the `Sized` constraint.
+/// Calculates the `Sized`, `MetaSized` or `PointeeSized` constraint.
 ///
-/// In fact, there are only a few options for the types in the constraint:
-///     - an obviously-unsized type
+/// For `Sized`, there are only a few options for the types in the constraint:
+///     - an metasized type (str, slices, trait objects, etc)
+///     - an pointee-sized type (extern types)
+///     - a type parameter or projection whose sizedness can't be known
+///
+/// For `MetaSized`, there are only a few options for the types in the constraint:
+///     - an pointee-sized type (extern types)
+///     - a type parameter or projection whose sizedness can't be known
+///
+/// For `PointeeSized`, there is only one option for the types in the constraint:
 ///     - a type parameter or projection whose sizedness can't be known
 #[instrument(level = "debug", skip(tcx), ret)]
-fn adt_sized_constraint<'tcx>(
+fn adt_sizedness_constraint<'tcx>(


Yeah, as above, this needs some revamp with how it is presented and named.

See previous comment

fee1-dead · 2025-04-22T04:47:17Z

tests/ui/sized-hierarchy/impls.rs

+    struct StructAllFieldsMetaSized { x: [u8], y: [u8] }
+    //~^ ERROR the size for values of type `[u8]` cannot be known at compilation time
+    needs_sized::<StructAllFieldsMetaSized>();
+    //~^ ERROR the size for values of type `[u8]` cannot be known at compilation time
+    needs_metasized::<StructAllFieldsMetaSized>();


I'd like to see a more thorough exploration of how allowing this would affect our semantic model. But that is not a blocker for this PR.

I feel like if T: MetaSized then &T should be well-formed. But that's not what's shown here. But StructAllFieldsMetaSized already isn't wf since only the trailing field is allowed to be !Sized right now, but still I think this is a point worth thinking about.

fee1-dead · 2025-04-22T05:07:35Z

library/core/src/ops/deref.rs

@@ -267,7 +269,8 @@ impl<T: ?Sized> const Deref for &mut T {
 #[stable(feature = "rust1", since = "1.0.0")]
 #[const_trait]
 #[rustc_const_unstable(feature = "const_deref", issue = "88955")]
-pub trait DerefMut: ~const Deref {
+#[allow(multiple_supertrait_upcastable)] // FIXME(sized_hierarchy): remove this


These now needs to be removed in the "lint: don't consider sizedness in upcastable lint" commit.

It's still necessary for bootstrap, but I'll add a cfg_attr(bootstrap, ..) in that commit

compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs

compiler/rustc_hir_analysis/src/collect/item_bounds.rs

compiler/rustc_hir_analysis/src/hir_ty_lowering/bounds.rs

fee1-dead · 2025-04-22T13:24:22Z

r? compiler

sorry about the reroll, but I currently have exams coming up and I expect to be back to full activity on May 22nd

davidtwco · 2025-04-22T13:27:38Z

I'll assign this to @oli-obk having already done a review rather than add a new reviewer entirely.

oli-obk · 2025-04-24T14:09:00Z

@bors try

cratertime

Sized Hierarchy: Part I This patch implements the non-const parts of rust-lang/rfcs#3729. It introduces two new traits to the standard library, `MetaSized` and `PointeeSized`. See the RFC for the rationale behind these traits and to discuss whether this change makes sense in the abstract. These traits are unstable (as is their constness), so users cannot refer to them without opting-in to `feature(sized_hierarchy)`. These traits are not behind `cfg`s as this would make implementation unfeasible, there would simply be too many `cfg`s required to add the necessary bounds everywhere. So, like `Sized`, these traits are automatically implemented by the compiler. RFC 3729 describes changes which are necessary to preserve backwards compatibility given the introduction of these traits, which are implemented and as follows: - `?Sized` is rewritten as `MetaSized` - `MetaSized` is added as a default supertrait for all traits w/out an explicit sizedness supertrait already. There are no edition migrations implemented in this, as these are primarily required for the constness parts of the RFC and prior to stabilisation of this (and so will come in follow-up PRs alongside the const parts). All diagnostic output should remain the same (showing `?Sized` even if the compiler sees `MetaSized`) unless the `sized_hierarchy` feature is enabled. Due to the use of unstable extern types in the standard library and rustc, some bounds in both projects have had to be relaxed already - this is unfortunate but unavoidable so that these extern types can continue to be used where they were before. Performing these relaxations in the standard library and rustc are desirable longer-term anyway, but some bounds are not as relaxed as they ideally would be due to the inability to relax `Deref::Target` (this will be investigated separately). It is hoped that this is implemented such that it could be merged and these traits could exist "under the hood" without that being observable to the user (other than in any performance impact this has on the compiler, etc). Some details might leak through due to the standard library relaxations, but this has not been observed in test output. **Notes:** - Any commits starting with "upstream:" can be ignored, as these correspond to other upstream PRs that this is based on which have yet to be merged. - This best reviewed commit-by-commit. I've attempted to make the implementation easy to follow and keep similar changes and test output updates together. - Each commit has a short description describing its purpose. - This patch is large but it's primarily in the test suite. - I've worked on the performance of this patch and a few optimisations are implemented so that the performance impact is neutral-to-minor. - `PointeeSized` is a different name from the RFC just to make it more obvious that it is different from `std::ptr::Pointee` but all the names are yet to be bikeshed anyway. - `@nikomatsakis` has confirmed [that this can proceed as an experiment from the t-lang side](https://rust-lang.zulipchat.com/#narrow/channel/435869-project-goals/topic/SVE.20and.20SME.20on.20AArch64.20.28goals.23270.29/near/506196491) Fixes rust-lang#79409. r? `@ghost` (I'll discuss this with relevant teams to find a reviewer)

bors · 2025-04-24T14:10:13Z

⌛ Trying commit 60dec26 with merge 0b080f0...

bors · 2025-04-24T16:06:39Z

☀️ Try build successful - checks-actions
Build commit: 0b080f0 (0b080f04a8020716beb803629c1dfc0f9dc25d66)

oli-obk · 2025-04-24T16:09:51Z

@craterbot check

craterbot · 2025-04-24T16:10:06Z

👌 Experiment pr-137944 created and queued.
🤖 Automatically detected try build 0b080f0
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

lcnr

only reviewed the type system changes and have some general thoughts:

I mentioned this in a review comment. Please move all the unrelated refactorings and style changes out of this PR and land them separately
a lot of type system tests currently use ?Sized to mean "no implicit bounds". For most tests, this is just to simplify debugging, but there are at least some tests, e.g. for cycle/overflow handling, where the issue only reproduces without proving Sized. I expect these to no longer cover the original issue if we now add a MetaSized bound. Please add a #![rustc_no_implicit_bounds] (or sth like it) crate attribute in a separate PR which can be used to avoid relying on ?Sized.
these sort of type system changes generally make me uncomfortable as it's really easy to miss subtle interactions and insufficiencies. Changing the approach later on tends to be a lot more difficult as it needs to be pretty much more powerful than the current impl to avoid introducing regressions. I am somewhat sorry that I am insisting on overwhelmingly theoretical issues. I deeply care about avoiding any user-facing hacks

lcnr · 2025-04-24T19:03:12Z

compiler/rustc_lint/src/multiple_supertrait_upcastable.rs

-                .filter_map(|(pred, _)| pred.as_trait_clause());
+                .filter_map(|(pred, _)| pred.as_trait_clause())
+                .filter(|pred| {
+                    !cx.tcx.is_lang_item(pred.def_id(), hir::LangItem::Sized)


i'd expect this lint to never trigger for Sized 🤔 if Sized is a super trait the trait cannot be used as a trait object, can it?

lcnr · 2025-04-24T19:05:09Z

compiler/rustc_next_trait_solver/src/solve/effect_goals.rs

        }
+
+        Err(NoSolution)


please move these spurious cleanups/changes into a separate PR

lcnr · 2025-04-24T19:11:41Z

compiler/rustc_next_trait_solver/src/solve/trait_goals.rs

+                    let expected_self_ty = ecx.resolve_vars_if_possible(goal.predicate.self_ty());
+                    let found_self_ty =
+                        ecx.resolve_vars_if_possible(trait_clause.self_ty().skip_binder());
+                    goal.predicate.polarity == trait_clause.polarity()
+                        && expected_self_ty == found_self_ty


structurally equatingn (using ==) types does not handle alias types. Create a <sized_self_type>: MetaSized assumption instead and then recur.

With your impl, a <T as Id>::This: Sized where-clause does not imply T: MetaSized. Please add this as a test.

lcnr · 2025-04-24T19:13:23Z

compiler/rustc_trait_selection/src/traits/coherence.rs

+        )
+    {
+        return false;
+    }


why is that necessary. I would expect the change in the trait solver itself to be sufficient.

lcnr · 2025-04-24T19:18:21Z

compiler/rustc_trait_selection/src/traits/util.rs

+        } else if tcx.is_lang_item(trait_ref.def_id(), LangItem::PointeeSized) {
+            Some(SizedTraitKind::PointeeSized)
+        } else {
+            None


style: instead of using an Option here, early return if the trait isn't a Sized trait

lcnr · 2025-04-24T19:22:32Z

compiler/rustc_trait_selection/src/traits/util.rs

+            let has_sized_in_param_env =
+                param_env.caller_bounds().iter().filter_map(|c| c.as_trait_clause()).any(|c| {
+                    trait_ref.self_ty() == c.skip_binder().self_ty()
+                        && tcx.is_lang_item(c.def_id(), LangItem::Sized)
+                });
+            if has_sized_in_param_env {
+                debug!("fast path -- metasized paramenv");
+                return true;
+            }


this is observable if you've got the following environment:

where MyType<'a, T>: Sized, MyType<'static, T>: Sized,

Actually proving MyType<'some_region, T>: [Meta]Sized would be ambiguous. This fast path does structural equality instead so it may succeed if 'some_region ended up as literally the same region var as 'a or 'static.

I am confident that this can cause MIR borrowck ICE similar to the issue in rust-lang/trait-system-refactor-initiative#27

We're able to prove MyType<'a, T>: Sized during HIR typeck. We then erase the region during writeback and replace it with a fresh nll var at the start of borrowck. At this point the fastpath no longer applies, we get ambiguity and an ICE.

What's the perf impact of this fast path?

lcnr · 2025-04-24T19:24:19Z

compiler/rustc_trait_selection/src/traits/util.rs

+
+        let expected_self_ty = infcx.resolve_vars_if_possible(predicate.self_ty());
+        let found_self_ty = infcx.resolve_vars_if_possible(c.self_ty());
+        predicate.polarity() == c.polarity() && expected_self_ty == found_self_ty


again, structurally comparing types is insufficient if there are aliases/regions

This fast path means we fail to prove MyType<'_, T>: MetaSized if there's MyType<'a, T>: Sized in the env

lcnr · 2025-04-24T19:25:14Z

compiler/rustc_trait_selection/src/traits/wf.rs

@@ -568,6 +568,14 @@ impl<'a, 'tcx> WfPredicates<'a, 'tcx> {
        def_id: DefId,
        args: GenericArgsRef<'tcx>,
    ) -> PredicateObligations<'tcx> {
+        // PERF: `Sized`'s predicates include `MetaSized`, but both are compiler implemented marker
+        // traits, so `MetaSized` will never be WF if `Sized` is WF and vice-versa. Determining


Suggested change

// traits, so `MetaSized` will never be WF if `Sized` is WF and vice-versa. Determining

// traits, so `MetaSized` will always be WF if `Sized` is WF and vice-versa. Determining

lcnr · 2025-04-24T19:27:26Z

compiler/rustc_type_ir/src/elaborate.rs

@@ -132,12 +168,14 @@ impl<I: Interner, O: Elaboratable<I>> Elaborator<I, O> {
                // Get predicates implied by the trait, or only super predicates if we only care about self predicates.
                match self.mode {
                    Filter::All => self.extend_deduped(
+                        Some(clause),
                        cx.explicit_implied_predicates_of(data.def_id())


why not instead do this filter in implied_predicates_of and super_predicates_of? 🤔

rustbot added A-attributes A-run-make A-rustdoc-json A-rustdoc-search PG-exploit-mitigations S-waiting-on-review T-compiler T-libs T-rustdoc T-rustdoc-frontend WG-trait-system-refactor labels Mar 3, 2025

This was referenced Mar 3, 2025

Hierarchy of Sized traits rust-lang/rfcs#3729

Open

SVE and SME on AArch64 rust-lang/rust-project-goals#270

Open

This comment has been minimized.

Sign in to view

rustbot added the T-bootstrap label Mar 3, 2025

This comment has been minimized.

Sign in to view

davidtwco force-pushed the sized-hierarchy branch from 73f1b4f to 7f509ab Compare March 3, 2025 16:29

This comment has been minimized.

Sign in to view

davidtwco force-pushed the sized-hierarchy branch from 2537bfb to b31fd85 Compare March 3, 2025 18:04

workingjubilee reviewed Mar 3, 2025

View reviewed changes

compiler/rustc_type_ir/src/flags.rs Outdated Show resolved Hide resolved

compiler/rustc_passes/src/check_attr.rs Outdated Show resolved Hide resolved

tests/ui/target-feature/tied-features-no-implication-1.rs Outdated Show resolved Hide resolved

traviscross added the T-lang label Mar 4, 2025

fee1-dead self-assigned this Mar 4, 2025

This comment was marked as resolved.

Sign in to view

davidtwco force-pushed the sized-hierarchy branch from b31fd85 to 2beed43 Compare March 6, 2025 10:21

davidtwco force-pushed the sized-hierarchy branch from 2beed43 to 839b844 Compare March 6, 2025 14:22

fixup! tests: bless remaining tests

Loading
Loading status checks…

60dec26

oli-obk reviewed Apr 22, 2025

View reviewed changes

compiler/rustc_hir_analysis/src/collect/item_bounds.rs Show resolved Hide resolved

oli-obk requested changes Apr 22, 2025

View reviewed changes

davidtwco force-pushed the sized-hierarchy branch from 9540d13 to 7e8918d Compare April 22, 2025 11:32

fee1-dead requested changes Apr 22, 2025

View reviewed changes

rustbot added S-waiting-on-author and removed S-waiting-on-review labels Apr 22, 2025

oli-obk reviewed Apr 22, 2025

View reviewed changes

compiler/rustc_hir_analysis/src/collect/item_bounds.rs Outdated Show resolved Hide resolved

fmease reviewed Apr 22, 2025

View reviewed changes

compiler/rustc_hir_analysis/src/hir_ty_lowering/bounds.rs Outdated Show resolved Hide resolved

davidtwco force-pushed the sized-hierarchy branch from 7e8918d to 60dec26 Compare April 22, 2025 13:00

rustbot assigned SparrowLii and unassigned fee1-dead Apr 22, 2025

davidtwco assigned oli-obk and unassigned SparrowLii Apr 22, 2025

davidtwco added S-waiting-on-review and removed S-waiting-on-author labels Apr 23, 2025

craterbot added S-waiting-on-crater and removed S-waiting-on-review labels Apr 24, 2025

lcnr reviewed Apr 24, 2025

View reviewed changes

lcnr self-assigned this Apr 24, 2025

		impl<C: ?Sized> A for u8 { //~ ERROR: the type parameter `C` is not constrained
		impl<C: PointeeSized> A for u8 { //~ ERROR: the type parameter `C` is not constrained

	// traits, so `MetaSized` will never be WF if `Sized` is WF and vice-versa. Determining
	// traits, so `MetaSized` will always be WF if `Sized` is WF and vice-versa. Determining

Sized Hierarchy: Part I #137944

Are you sure you want to change the base?

Sized Hierarchy: Part I #137944

Conversation

davidtwco commented Mar 3, 2025 • edited Loading

This comment has been minimized.

davidtwco commented Mar 3, 2025 • edited Loading

This comment has been minimized.

This comment has been minimized.

traviscross commented Mar 4, 2025

tmiasko commented Mar 4, 2025

This comment was marked as resolved.

davidtwco commented Mar 6, 2025

davidtwco commented Mar 6, 2025

davidtwco commented Apr 22, 2025

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidtwco Apr 22, 2025 • 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

davidtwco commented Apr 22, 2025

fee1-dead 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

fee1-dead commented Apr 22, 2025

davidtwco commented Apr 22, 2025

oli-obk commented Apr 24, 2025

bors commented Apr 24, 2025

bors commented Apr 24, 2025

oli-obk commented Apr 24, 2025

craterbot commented Apr 24, 2025

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

lcnr Apr 24, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidtwco commented Mar 3, 2025 •

edited

Loading

davidtwco commented Mar 3, 2025 •

edited

Loading

davidtwco Apr 22, 2025 •

edited

Loading

lcnr Apr 24, 2025 •

edited

Loading