make `Thin` a supertrait of `Sized` #122553

lukas-code · 2024-03-15T15:09:47Z

This is a follow-up to #120354 to address #120354 (comment) and remove the "fallback impl" hack from <T as Pointee>::Metadata normalization in both trait solvers.

We make Thin and therefore Pointee<Metadata = ()> a supertrait of Sized, such that every (implicit or explicit) T: Sized bound now also requires T: Thin and <T as Pointee>::Metadata == (). These new bounds will be used when normalizing <T as Pointee>::Metadata instead of a hacky builtin impl that only applies for type parameters and aliases and overlaps with other builtin impls.

Note that RFC 2580, which introduced Pointee and Thin, lists these unresolved questions:

Should Thin be added as a supertrait of Sized? Or could it ever make sense to have fat pointers to statically-sized types?

For now, they are answered with yes and no respectively. If we end up deciding that we do want types that are Sized + !Thin, then a possible alternative is to add make Thin a defaulted trait bound that can be relaxed with T: ?Thin and remove Thin as supertrait from Sized before the stabilization of feature(ptr_metadata).

The removal of the "fallback impl" also allows us to always project to the shallow struct tail in the old solver, making the implementation in the old solver almost identical to the new solver. This is required to make <Wrapper<T> as Pointee>::Metadata work correctly in the presence of trivial bounds, for example if we know [T]: Thin, then we should also know Wrapper<T>: Thin.

Lastly, this PR implements some diagnostics changes to hide errors about type mismatch resolving `<T as Pointee>::Metadata == ()` when the actual error comes from T: Sized. This is a continuation of #121863.

r? @lcnr

rustbot · 2024-03-15T15:09:57Z

Some changes occurred to the core trait solver

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

compiler-errors · 2024-03-15T15:27:58Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

+                    // about `<T as Pointee>::Metadata == ()` that can be fixed by `T: Sized`.
+                    if error.predicate.to_opt_poly_projection_pred().is_some()
+                        && error2.predicate.to_opt_poly_trait_pred().is_some()
+                        && self.error_fixed_by(


I wonder if we can just use error_fixed_by everywhere -- it seems more general than error_implied_by?

Not necessarily in this PR, but maybe tracked as a FIXME.

That's what I tried first, but there are multiple problems with this approach:

error_fixed_by does not work when there are any infer vars in cond (first arg). This adds a lot of duplicate diagnostics.

There are some cases where A fixed-by B + not (B fixed-by A) + B fixed-by C + not (C fixed-by B) + C fixed-by A + not (A fixed-by C). In these cases, replacing error_implied_by with error_fixed_by causes no diagnostics to be emitted. This cannot happen with error_implied_by, because A implied-by B is transitive.

There are some cases where A fixed-by B + not (B fixed-by A) + B fixed-by C + not (C fixed-by B) + C fixed-by A + not (A fixed-by C).

After thinking about this again, I'm actually not sure anymore whether this can actually happen and can't come up with a concrete example. I had a test failing and thought that was due a cycle like that, but after trying it again it turned out to be an unrelated implementation bug.

rustbot · 2024-03-15T17:01:08Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

rustbot · 2024-03-15T18:05:35Z

The Miri subtree was changed

cc @rust-lang/miri

lukas-code · 2024-03-17T18:03:08Z

tests/ui/traits/bad-sized.stderr

@@ -9,33 +9,33 @@ LL |     let x: Vec<dyn Trait + Sized> = Vec::new();
   = help: consider creating a new trait with all of these as supertraits and using that trait here instead: `trait NewTrait: Trait + Sized {}`
   = note: auto-traits like `Send` and `Sync` are traits that have special properties; for more information on them, visit <https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits>

-error[E0277]: the size for values of type `dyn Trait` cannot be known at compilation time
+error[E0277]: the size for values of type `dyn Trait<Metadata = ()>` cannot be known at compilation time


This happens, because we do lower multiple principals in trait objects like dyn Trait + Copy during astconv (HIR->Ty lowering), but most code that consumes Tys does not handle trait objects with multiple principals correctly, including the pretty printer.

I can fix this in a follow-up, either by making the pretty print work with multiple principals or by just stopping to lower more than one principal.

bors · 2024-03-19T07:10:53Z

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

lcnr · 2024-03-19T11:32:29Z

the changes look good. Want a perf + crater run here though

lcnr · 2024-03-19T12:35:37Z

@bors try @rust-timer queue

bors · 2024-03-19T12:36:46Z

⌛ Trying commit 81a8fae with merge 32beed4...

make `Thin` a supertrait of `Sized` This is a follow-up to rust-lang#120354 to address rust-lang#120354 (comment) and remove the "fallback impl" hack from `<T as Pointee>::Metadata` normalization in both trait solvers. We make `Thin` and therefore `Pointee<Metadata = ()>` a supertrait of `Sized`, such that every (implicit or explicit) `T: Sized` bound now also requires `T: Thin` and `<T as Pointee>::Metadata == ()`. These new bounds will be used when normalizing `<T as Pointee>::Metadata` instead of a hacky builtin impl that only applies for type parameters and aliases and overlaps with other builtin impls. Note that [RFC 2580](https://rust-lang.github.io/rfcs/2580-ptr-meta.html), which introduced `Pointee` and `Thin`, lists these unresolved questions: > Should `Thin` be added as a supertrait of `Sized`? Or could it ever make sense to have fat pointers to statically-sized types? For now, they are answered with yes and no respectively. If we end up deciding that we do want types that are `Sized + !Thin`, then a possible alternative is to add make `Thin` a defaulted trait bound that can be relaxed with `T: ?Thin` and remove `Thin` as supertrait from `Sized` before the stabilization of `feature(ptr_metadata)`. --- The removal of the "fallback impl" also allows us to always project to the shallow struct tail in the old solver, making the implementation in the old solver almost identical to the new solver. This is required to make `<Wrapper<T> as Pointee>::Metadata` work correctly in the presence of trivial bounds, for example if we know `[T]: Thin`, then we should also know `Wrapper<T>: Thin`. Lastly, this PR implements some diagnostics changes to hide errors about `` type mismatch resolving `<T as Pointee>::Metadata == ()` `` when the actual error comes from `T: Sized`. This is a continuation of rust-lang#121863. r? `@lcnr`

bors · 2024-03-19T14:09:05Z

☀️ Try build successful - checks-actions
Build commit: 32beed4 (32beed45d7e6464e619266a350792881aba447f5)

rust-timer · 2024-03-19T15:57:09Z

Finished benchmarking commit (32beed4): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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)	2.5%	[0.1%, 22.4%]	98
Regressions ❌ (secondary)	1.4%	[0.2%, 3.1%]	50
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	2
All ❌✅ (primary)	2.5%	[0.1%, 22.4%]	98

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)	5.5%	[1.4%, 16.8%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.7%	[-4.7%, -4.7%]	1
All ❌✅ (primary)	5.5%	[1.4%, 16.8%]	5

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)	6.0%	[1.0%, 15.8%]	23
Regressions ❌ (secondary)	2.0%	[1.8%, 2.4%]	10
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	6.0%	[1.0%, 15.8%]	23

Binary size

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

Bootstrap: 669.396s -> 672.715s (0.50%)
Artifact size: 312.81 MiB -> 312.51 MiB (-0.10%)

lcnr · 2024-03-21T08:30:35Z

could the perf impact be reduced by also checking is_trivially_sized for Thin obligations?

lukas-code · 2024-03-21T08:43:13Z

could the perf impact be reduced by also checking is_trivially_sized for Thin obligations?

That would be wrong if there are any T::Metadata == SomethingNotUnit clauses in the param env. And even if there are only T::Metadata == () clauses, it could still return holds, even though the result should be ambig. This is actually currenlty a bug in ty.is_sized() that it can return true, even though the result should be false due to being ambiguous.

I've already found out that 100% of the extra time is spent in select_where_possible, but only less than half of it in solving projection obligations, so I suspect that Thin and Pointee obligations also need to be optimized in some way. Will do more investigation today.

[perf] cache type info for ParamEnv This is an attempt to mitigate some of the perf regressions in rust-lang#122553 (comment), but seems worth to test and land separately. r? `@ghost`

lukas-code · 2024-03-26T13:12:02Z

Ok so, update on the perf situation and why fixing perf is taking so long: After some extensive testing, it looks like the majority of the slowdown is actually caused by the larger ParamEnvs and only indirectly related to having to prove more predicates. However due to proving linearly more predicates in a linearly larger ParamEnv, we're kind of seeing a a quadratic slowdown here.

A hacky workaround would be to reduce the clauses in the param env, by "downgrading" Sized: Thin to Sized: Pointee<Metadata = ()> and also deleting all T: Pointee bounds from the param env. That, however, does not entirely fix the perf regression, which makes sense since the param env is still up to twice as big. ¹

The real fix is to make the trait solver not slow in the presence of large param envs, which I'll try to work towards to. To start this endeavor I've opened #123058, which already seems to help significantly.

@rustbot author

Note: If we end up taking this way, then we will need a form of https://github.com/rust-lang/rust/pull/84559 for projections. ↩

[perf] cache type info for ParamEnv This is an attempt to mitigate some of the perf regressions in rust-lang#122553 (comment), but seems worth to test and land separately, since it is mostly unrelated to that PR.

[perf] cache type info for ParamEnv This is an attempt to mitigate some of the perf regressions in rust-lang/rust#122553 (comment), but seems worth to test and land separately, since it is mostly unrelated to that PR.

Dylan-DPC · 2024-05-03T06:36:47Z

@lukas-code any updates on this? thanks

lukas-code · 2024-05-03T11:44:30Z

@Dylan-DPC I'm still working on the performance regressions an have some updates on an experimental branch here: https://github.com/lukas-code/rust/tree/sized-thin-wip. But I'm not entirely happy with my implementation yet and there are still some outstanding performance issues, especially for the doc benchmarks. I'm still planning push this PR over the finish line, but things are moving slowly, because I cannot commit as much time as I'd like to to Rust at the moment.

Dylan-DPC · 2024-05-03T14:51:36Z

Thanks, that's fine.

Though alternatively i would recommend closing this pr and working on your branch and once you have found the way through to submit a pr to prevent this pr going stale and bitrotting and then turning out a mess to rebase but i'll keep it open for now

oskgo · 2024-07-18T23:14:52Z

@lukas-code

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

rustbot assigned lcnr Mar 15, 2024

compiler-errors reviewed Mar 15, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

lukas-code commented Mar 17, 2024

View reviewed changes

clean up fulfillment error reporting

06a72d2

Lukas Markeffsky added 6 commits March 19, 2024 12:45

add tests for mismatched super projections with nested obligations

dbdc03c

make Thin a supertrait of Sized

504da11

always use shallow tail for metadata projections

a7f12dc

suppress errors about T: !Thin caused by T: !Sized

c02b3c7

clippy: handle supertraits of Sized

e399970

miri: adjust layout cycle test to avoid overflow error

81a8fae

lukas-code force-pushed the sized-thin branch from 0647d73 to 81a8fae Compare March 19, 2024 12:20

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 19, 2024

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 19, 2024

lukas-code mentioned this pull request Mar 25, 2024

[perf] cache type info for ParamEnv #123058

Merged

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2024

oskgo closed this Jul 18, 2024

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jul 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make `Thin` a supertrait of `Sized` #122553

make `Thin` a supertrait of `Sized` #122553

lukas-code commented Mar 15, 2024 •

edited

Loading

rustbot commented Mar 15, 2024

compiler-errors Mar 15, 2024

compiler-errors Mar 15, 2024

lukas-code Mar 15, 2024

lukas-code Mar 16, 2024

This comment has been minimized.

rustbot commented Mar 15, 2024

This comment has been minimized.

rustbot commented Mar 15, 2024

lukas-code Mar 17, 2024

bors commented Mar 19, 2024

lcnr commented Mar 19, 2024

lcnr commented Mar 19, 2024

This comment has been minimized.

bors commented Mar 19, 2024

bors commented Mar 19, 2024

This comment has been minimized.

rust-timer commented Mar 19, 2024

lcnr commented Mar 21, 2024

lukas-code commented Mar 21, 2024 •

edited

Loading

lukas-code commented Mar 26, 2024 •

edited

Loading

Dylan-DPC commented May 3, 2024

lukas-code commented May 3, 2024

Dylan-DPC commented May 3, 2024

oskgo commented Jul 18, 2024

make Thin a supertrait of Sized #122553

make Thin a supertrait of Sized #122553

Conversation

lukas-code commented Mar 15, 2024 • edited Loading

rustbot commented Mar 15, 2024

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

This comment has been minimized.

rustbot commented Mar 15, 2024

This comment has been minimized.

rustbot commented Mar 15, 2024

Choose a reason for hiding this comment

bors commented Mar 19, 2024

lcnr commented Mar 19, 2024

lcnr commented Mar 19, 2024

This comment has been minimized.

bors commented Mar 19, 2024

bors commented Mar 19, 2024

This comment has been minimized.

rust-timer commented Mar 19, 2024

Overall result: ❌ regressions - ACTION NEEDED

lcnr commented Mar 21, 2024

lukas-code commented Mar 21, 2024 • edited Loading

lukas-code commented Mar 26, 2024 • edited Loading

Footnotes

Dylan-DPC commented May 3, 2024

lukas-code commented May 3, 2024

Dylan-DPC commented May 3, 2024

oskgo commented Jul 18, 2024

make `Thin` a supertrait of `Sized` #122553

make `Thin` a supertrait of `Sized` #122553

lukas-code commented Mar 15, 2024 •

edited

Loading

lukas-code commented Mar 21, 2024 •

edited

Loading

lukas-code commented Mar 26, 2024 •

edited

Loading