Normalize the RHS of an `Unsize` goal in the new solver #113393

compiler-errors · 2023-07-06T02:55:59Z

Unsize goals are... tricky. Not only do they structurally match on their self type, but they're also structural on their other type parameter. I'm pretty certain that it is both incomplete and also just plain undesirable to not consider normalizing the RHS of an unsize goal. More practically, I'd like for this code to work:

trait A {}
trait B: A {}

impl A for usize {}
impl B for usize {}

trait Mirror {
    type Assoc: ?Sized;
}

impl<T: ?Sized> Mirror for T {
    type Assoc = T;
}

fn main() {
    // usize: Unsize<dyn B>
    let x = Box::new(1usize) as Box<<dyn B as Mirror>::Assoc>;
    // dyn A: Unsize<dyn B>
    let y = x as Box<<dyn A as Mirror>::Assoc>;
}

In order to achieve this, we add EvalCtxt::normalize_non_self_ty (naming modulo bikeshedding), which must be used for all non-self type arguments that are structurally matched in candidate assembly. Currently this is only necessary for Unsize's argument, but I could see future traits requiring this (hopefully rarely) in the future. It uses repeat_while_none to limit infinite looping, and normalizes the self type until it is no longer an alias.

Also, we need to fix feature gate detection for trait_upcasting and unsized_tuple_coercion when HIR typeck has unnormalized types. We can do that by checking the ImplSource returned by selection, which necessitates adding a new impl source for tuple upcasting.

bors · 2023-07-07T09:31:26Z

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

…in-selection, r=lcnr Structurally normalize in selection We need to do this because of the fact that we're checking the `Ty::kind` on a type during selection, but goals passed into select are not necessarily normalized. Right now, we're (kinda) unnecessarily normalizing the RHS of a trait upcasting goal, which is broken for different reasons (rust-lang#113393). But I'm waiting for this PR to land before discussing that one. r? `@lcnr`

bors · 2023-07-13T20:13:16Z

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

…in-selection, r=lcnr Structurally normalize in selection We need to do this because of the fact that we're checking the `Ty::kind` on a type during selection, but goals passed into select are not necessarily normalized. Right now, we're (kinda) unnecessarily normalizing the RHS of a trait upcasting goal, which is broken for different reasons (rust-lang#113393). But I'm waiting for this PR to land before discussing that one. r? `@lcnr`

rustbot · 2023-07-19T18:44:35Z

Some changes occurred to the core trait solver

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

compiler-errors · 2023-07-19T18:44:41Z

r? lcnr

lcnr

vibe: instead of adding separate impl sources for "special builtin sources", would it be nicer to change impl source builtin to

Builtin(BuiltinSourceData<'tcx>, Vec<N>)`

apart from that r=me

lcnr · 2023-07-20T08:49:26Z

compiler/rustc_trait_selection/src/solve/trait_goals.rs

-            return vec![];
-        };
+        // Need to wrap in a probe since `normalize_non_self_ty` has side-effects.
+        ecx.probe(|_| CandidateKind::DynUpcastingAssembly).enter(|ecx| {


nit: this probe feels quite complex now 🤔 is there something in here that we can move into a separate function?

also realized that

trait Trait { fn foo(&self); } fn foo(x: Box<dyn Trait + Send>) -> Box<dyn Send> { x }

doesn't compile, this feels like an oversight to me? 🤔 i would except us to accept this 🤔 at least unstably

Yeah, I probably just forgot to handle the "dropping principal" case. I'll have to see how the current solver handles it, and see where I forgot to do this.

Wait, this doesn't work in the old solver either. I need to put this up in a separate PR -- while I do want to support this, it'll need T-lang FCP 🤣

compiler/rustc_trait_selection/src/solve/trait_goals.rs

compiler-errors · 2023-07-20T18:50:14Z

vibe: instead of adding separate impl sources for "special builtin sources", would it be nicer to change impl source builtin to [...]

I would, and I agree that adding a new ImplSource variant is kinda ugly, but that would require maintaining two different BuiltinImplSources (or accepting the fact that some variants will go unused in certain situations). This is because some of the variants of the existing BuiltinImplSource (from the new solver) get mapped to other ImplSource variants (during selection), e.g. BuiltinImplSource::Object would not be represented as ImplSource::Builtin(BuiltinImplSource::Object, ..) but through the ImplSource::Object variant that carries vtable info.

lcnr

a lot of style suggestions and nits, I would love to see all of them implemented, but feel free to leave some of them as future work if you want. Going to not merge any PR which conflicts in the new solver until we land this.

lcnr · 2023-07-25T07:15:07Z

compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs

@@ -172,7 +173,8 @@ impl Qualif for NeedsNonConstDrop {

        if !matches!(
            impl_src,
-            ImplSource::Builtin(_) | ImplSource::Param(_, ty::BoundConstness::ConstIfConst)
+            ImplSource::Builtin(BuiltinImplSource::Misc, _)


all builtin destruct impls are not const 😅 i would like us to still use all ImplSource::Builtin in this match, or even make this exhaustive xx

I'm gonna leave this here for now

all builtin destruct impls are not const

Also, what do you mean? They're definitely const in the old solver?

this is a question of what exactly this fn does... missed the double negation of 1) returning true if it is not const and 2) the ! infront of the match.

This automatically considers all builtin impls const if their source is Misc which feels like a footgun to me 🤔 anyways, const trait is a bigger issue ^^

Oh, sorry, I didn't understand what you meant there. Yeah, shrug... not really changing behavior here

compiler/rustc_hir_typeck/src/coercion.rs

compiler/rustc_middle/src/traits/mod.rs

compiler/rustc_middle/src/traits/structural_impls.rs

compiler/rustc_trait_selection/src/solve/trait_goals.rs

lcnr · 2023-07-25T07:41:14Z

compiler/rustc_trait_selection/src/solve/trait_goals.rs

+        param_env: ty::ParamEnv<'tcx>,
+        a_ty: Ty<'tcx>,
+        b_ty: Ty<'tcx>,


Suggested change

param_env: ty::ParamEnv<'tcx>,

a_ty: Ty<'tcx>,

b_ty: Ty<'tcx>,

goal: Goal<'tcx, (Ty<'tcx, Ty<'tcx>)>,

🤔 and then let (a, b) = goal.predicate;

vibe: maybe this match should live in consider_builtin_unsize_and_upcast_candidates and we probe in the match relevant match arms?

I don't really know what you're asking for in that "vibe" comment lol

instead of calling consider_builtin_unsize_candidate and consider_builtin_upcast_candidates for all a and b and then matching in each function individually, match on a and b in consider_builtin_unsize_and_upcast_candidates... I guess merge unsize and upcast builtin impls back together completely 😅

Oh, I just thought the nesting was getting kinda gnarly when they were in one method. Yeah, I guess I can try that.

I guess maybe move bigger match branches into separate functions, so we'd have consider_trait_upcast_candidates, consider_tuple_unsize_candidate and so on

That refactoring work is literally miserable actually, lol. I'm gonna do this in a follow-up, I don't think the current split we have right now is that different from what we currently have -- you're essentially just asking to break up consider_builtin_unsize_candidate.

compiler/rustc_trait_selection/src/solve/trait_goals.rs

lcnr · 2023-07-25T07:50:17Z

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

@@ -48,12 +48,12 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
        let mut impl_src = match candidate {
            BuiltinCandidate { has_nested } => {


for future PR, we should do the same for old style candidates 😁

have BuiltinCandidate with a CandidateKind. Definitely not in this PR

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

rustbot · 2023-07-25T16:02:35Z

Some changes occurred in abstract_const.rs

cc @BoxyUwU

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

lcnr · 2023-07-25T16:23:32Z

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

+        // FIXME: We only need to do *any* of this if we're considering a trait goal,
+        // since we don't need to look at any supertrait or anything if we are doing
+        // a projection goal.
+        if let Some(principal) = bounds.principal() {


will maybe look at this myself in a separate PR, but this would be in the ExistentialPredicate::Trait match arm instead 🤷

Only realized what you meant here after the PR. Yeah, if you want to follow-up with this and any other structural changes you'd like to see about this code, I'd be glad to review it.

lcnr · 2023-07-25T16:26:09Z

compiler/rustc_trait_selection/src/solve/trait_goals.rs

@@ -404,20 +526,27 @@ impl<'tcx> assembly::GoalKind<'tcx> for TraitPredicate<'tcx> {
                    // Check that the type implements all of the predicates of the def-id.
                    // (i.e. the principal, all of the associated types match, and any auto traits)
                    ecx.add_goals(
-                        data.iter().map(|pred| goal.with(tcx, pred.with_self_ty(tcx, a_ty))),
+                        data.iter()
+                            .map(|pred| Goal::new(tcx, param_env, pred.with_self_ty(tcx, a_ty))),


suggested to use goal as an arg to use goal.with here, will experiment a bit with the code layout myself here after this is merged

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

lcnr · 2023-07-25T16:29:13Z

@bors r+ rollup=never likely to get merge conflicts

bors · 2023-07-25T16:29:15Z

📌 Commit a7ed9c1 has been approved by lcnr

It is now in the queue for this repository.

bors · 2023-07-25T17:10:34Z

⌛ Testing commit a7ed9c1 with merge 8327047...

bors · 2023-07-25T18:58:24Z

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

rust-timer · 2023-07-25T22:11:33Z

Finished benchmarking commit (8327047): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.0%	[-4.0%, -4.0%]	1
All ❌✅ (primary)	-	-	0

Cycles

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

Binary size

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

Bootstrap: 650.083s -> 650.593s (0.08%)

…hs, r=lcnr Normalize the RHS of an `Unsize` goal in the new solver `Unsize` goals are... tricky. Not only do they structurally match on their self type, but they're also structural on their other type parameter. I'm pretty certain that it is both incomplete and also just plain undesirable to not consider normalizing the RHS of an unsize goal. More practically, I'd like for this code to work: ```rust trait A {} trait B: A {} impl A for usize {} impl B for usize {} trait Mirror { type Assoc: ?Sized; } impl<T: ?Sized> Mirror for T { type Assoc = T; } fn main() { // usize: Unsize<dyn B> let x = Box::new(1usize) as Box<<dyn B as Mirror>::Assoc>; // dyn A: Unsize<dyn B> let y = x as Box<<dyn A as Mirror>::Assoc>; } ``` --- In order to achieve this, we add `EvalCtxt::normalize_non_self_ty` (naming modulo bikeshedding), which *must* be used for all non-self type arguments that are structurally matched in candidate assembly. Currently this is only necessary for `Unsize`'s argument, but I could see future traits requiring this (hopefully rarely) in the future. It uses `repeat_while_none` to limit infinite looping, and normalizes the self type until it is no longer an alias. Also, we need to fix feature gate detection for `trait_upcasting` and `unsized_tuple_coercion` when HIR typeck has unnormalized types. We can do that by checking the `ImplSource` returned by selection, which necessitates adding a new impl source for tuple upcasting.

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 Jul 6, 2023

compiler-errors changed the title ~~Normalize the RHS of an Unsize goal~~ Normalize the RHS of an Unsize goal in the new solver Jul 6, 2023

This comment has been minimized.

Sign in to view

compiler-errors mentioned this pull request Jul 12, 2023

Structurally normalize in selection #113625

Merged

rust-cloud-vms bot force-pushed the next-solver-unsize-rhs branch from a04d098 to 7035cb0 Compare July 13, 2023 18:55

rust-cloud-vms bot force-pushed the next-solver-unsize-rhs branch from 7035cb0 to 6a0d852 Compare July 16, 2023 21:33

compiler-errors mentioned this pull request Jul 18, 2023

matching on b for Unsize requires normalization rust-lang/trait-system-refactor-initiative#48

Closed

rust-cloud-vms bot force-pushed the next-solver-unsize-rhs branch 2 times, most recently from 84a2a55 to e054b79 Compare July 19, 2023 18:39

compiler-errors marked this pull request as ready for review July 19, 2023 18:44

rustbot assigned lcnr Jul 19, 2023

lcnr reviewed Jul 20, 2023

View reviewed changes

compiler/rustc_trait_selection/src/solve/trait_goals.rs Outdated Show resolved Hide resolved

rust-cloud-vms bot force-pushed the next-solver-unsize-rhs branch 2 times, most recently from 8b8198b to 71512ff Compare July 20, 2023 18:58

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the next-solver-unsize-rhs branch from 71512ff to 0ffff70 Compare July 20, 2023 19:13

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 24, 2023

lcnr reviewed Jul 25, 2023

View reviewed changes

compiler-errors added 4 commits July 25, 2023 15:15

Normalize the RHS of an unsize goal

7e66c0b

Make sure to detect trait upcasting coercion even after normalization

24eefd0

Restore tuple unsizing feature gate

c02d1a6

Consolidate trait upcasting and unsize into one normalization

de81007

rust-cloud-vms bot force-pushed the next-solver-unsize-rhs branch from 0f97129 to 17ca258 Compare July 25, 2023 16:02

rust-cloud-vms bot force-pushed the next-solver-unsize-rhs branch from 17ca258 to eb92cf8 Compare July 25, 2023 16:08

Make everything builtin!

a7ed9c1

rust-cloud-vms bot force-pushed the next-solver-unsize-rhs branch from eb92cf8 to a7ed9c1 Compare July 25, 2023 16:09

lcnr reviewed Jul 25, 2023

View reviewed changes

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 Jul 25, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 25, 2023

bors merged commit 8327047 into rust-lang:master Jul 25, 2023
11 checks passed

rustbot added this to the 1.73.0 milestone Jul 25, 2023

compiler-errors deleted the next-solver-unsize-rhs branch August 9, 2023 05:55

compiler-errors mentioned this pull request Aug 10, 2023

Allow dropping dyn trait object's principal #114679

Closed

		@@ -48,12 +48,12 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
		let mut impl_src = match candidate {
		BuiltinCandidate { has_nested } => {

Normalize the RHS of an Unsize goal in the new solver #113393

Normalize the RHS of an Unsize goal in the new solver #113393

Conversation

compiler-errors commented Jul 6, 2023 • edited Loading

This comment has been minimized.

bors commented Jul 7, 2023

bors commented Jul 13, 2023

rustbot commented Jul 19, 2023

compiler-errors commented Jul 19, 2023

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

compiler-errors Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

compiler-errors commented Jul 20, 2023

This comment has been minimized.

This comment has been minimized.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Jul 25, 2023 • 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

rustbot commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr commented Jul 25, 2023

bors commented Jul 25, 2023

bors commented Jul 25, 2023

bors commented Jul 25, 2023

rust-timer commented Jul 25, 2023

Overall result: no relevant changes - no action needed

Normalize the RHS of an `Unsize` goal in the new solver #113393

Normalize the RHS of an `Unsize` goal in the new solver #113393

compiler-errors commented Jul 6, 2023 •

edited

Loading

compiler-errors Jul 20, 2023 •

edited

Loading

compiler-errors Jul 25, 2023 •

edited

Loading