Allow constraining opaque types during various unsizing casts

[oli-obk]

allows unsizing of tuples, arrays and Adts to constraint opaque types in their generic parameters to concrete types on either side of the unsizing cast.

Also allows constraining opaque types during trait object casts that only differ in auto traits or lifetimes.

cc #116652

[rustbot]

r? @estebank

rustbot has assigned @estebank.
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

[oli-obk]

all the tests and insta-stable behaviour changes are of the form

struct Foo<T: ?Sized>(T);

fn hello() -> Foo<[impl Sized; 2]> {
    if false {
        let x = hello();
        let _: &Foo<[i32]> = &x; // used to error, now constrains the opaque type
    }
    todo!()
}

just for different types, but always for unsizing casts (trait object to trait object unsizing casts that only differ in auto traits also belong in this category).

@rfcbot merge

[rfcbot]

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @aliemjay
• @compiler-errors
• @jackh726
• @lcnr
• @nikomatsakis
• @oli-obk
• @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[jackh726]

I'm really conflicted on whether I expect this to work or not. On other hand, I like that it just works™️, but on the other hand, it just feels weird. Like, constraining the opaque type while doing an implicit cast feels like a recipe for disaster and confusion.

[oli-obk]

in #124297 we changed the implicit conversion from function items to functions pointers to register hidden types

We already allow the exact same thing (just more convoluted) in

struct Foo<T: ?Sized>(T);


fn k() -> Foo<[impl Sized; 2]> {
    if false {
        let k: &Foo<[_]> = &k();
        let x = match true {
            true => {
                k
            }
            false => &Foo([42, 43]) as &Foo<[i32]>,
        };
    }
    todo!()
}

fn main() {}

which we added in #123794

Similar to all my other PRs, these are all required for the new solver unless we add significant complexity to avoid a few cases, but not others.

[compiler-errors]

Like, constraining the opaque type while doing an implicit cast feels like a recipe for disaster and confusion.

@jackh726: I think "disaster" is a bit hyperbolic, and calling this change "a recipe for disaster and confusion" isn't very productive for the purpose of this FCP without explaining why you feel that way from a technical perspective.

I think I actually hold the opposite view, in favor of this and all the other changes that oli is doing -- any place in a defining scope that the user could define an opaque type, but where we currently don't allow it due to DefineOpaqueTypes::No has a very high burden of proof to justify why it needs to be that way. All of these call sites either Just Work(TM), or they lead to unnecessary, obtuse type error messages à la "expected impl Sized, found i32" -- cases where the user is bound to just go "but I just set impl Sized to i32 right above?!".

Do you have an example of a situation in which an unsizing cast would not want to rely on defining a TAIT?

[BoxyUwU]

Do we have a test for what Box<impl Trait> assigned to some Box<dyn Trait> does? i.e. does it create a trait object out of impl Trait or does it constrain the hidden type to dyn Trait.

Other scenarios seem relatively straightforward to me though, for example I don't think there's any way that &[impl Trait; N] -> &[u32] could be confusing as to what is happening. There's really only one thing that could be happening if the code compiles

[compiler-errors]

,Unsizing Box<impl Trait> into Box<dyn Trait> works on nightly right now:

trait Trait {}

impl Trait for u32 {}

fn hello() -> Box<impl Trait> {
    // FIXME(-Znext-solver): Works if you constrain
    // `impl Trait := u32` before the recursive call.
    if true {
        let x = hello();
        let y: Box<dyn Trait> = x;
    }
    Box::new(1u32)
}

Though it curiously doesn't work on the new trait solver -- I feel like it should work, because the alias-relate impl Trait = dyn Trait should fail since the opaque has an (implicit) Sized bound that isn't satisfied by dyn Trait, and therefore we should instead try to unsize during coercion 🤔

[BoxyUwU]

Do we have tests that the behaviour with Adt<..., impl Trait> -> Adt<..., dyn Trait> under this PR is the same as with Box then? (and same for all the other unsizing coercions that we're now allowing to define opaque tys)

[jackh726]

I think "disaster" is a bit hyperbolic, and calling this change "a recipe for disaster and confusion" isn't very productive for the purpose of this FCP without explaining why you feel that way from a technical perspective.

Sure - I used "recipe for disaster" more as an idiom and less of a "this will literally be a disaster". I didn't have much time earlier when I commented, but you're right that I should have elaborated more on my thoughts technically.

Do we have a test for what Box<impl Trait> assigned to some Box<dyn Trait> does? i.e. does it create a trait object out of impl Trait or does it constrain the hidden type to dyn Trait.

This is actually pretty close to where my mind was - if we unsize, implicitly coerce, and constrain opaque types together, I very much worry that it's not clear what exactly we're constraining the opaque type to: the sized type, that we then can unsize; or, the unsized type.

Other scenarios seem relatively straightforward to me though, for example I don't think there's any way that &[impl Trait; N] -> &[u32] could be confusing as to what is happening. There's really only one thing that could be happening if the code compiles

Right, so this is true. This unsizing is straightforward, but there is Box<X> to Box<dyn X>, Box<dyn X> to Box<dyn Y>, and generally all the subtle bits around new regions and such.

Ultimately, I'm not saying that this isn't the right change to land. But, at the very least, I think we probably need some more tests to consider the subtleties involved. The other option is just to be stricter on the surface level of what kind of opaque type constraints we allow during unsizing or implicit coercions.

in #124297 we changed the implicit conversion from function items to functions pointers to register hidden types

We already allow the exact same thing (just more convoluted) in

struct Foo<T: ?Sized>(T);


fn k() -> Foo<[impl Sized; 2]> {
    if false {
        let k: &Foo<[_]> = &k();
        let x = match true {
            true => {
                k
            }
            false => &Foo([42, 43]) as &Foo<[i32]>,
        };
    }
    todo!()
}

fn main() {}

which we added in #123794

That's definitely a bit weird to me. The unsizing point didn't come up then, but had it been thought about, I likely would have brought this up then.

Similar to all my other PRs, these are all required for the new solver unless we add significant complexity to avoid a few cases, but not others.

I'm not sure that it's true that we would need to add significant complexity. As I said, we could detect this and disallow at the surface level, before even attempting any coercions, for example.

[lcnr]

,Unsizing Box<impl Trait> into Box<dyn Trait> works on nightly right now:

trait Trait {}

impl Trait for u32 {}

fn hello() -> Box<impl Trait> {
    // FIXME(-Znext-solver): Works if you constrain
    // `impl Trait := u32` before the recursive call.
    if true {
        let x = hello();
        let y: Box<dyn Trait> = x;
    }
    Box::new(1u32)
}

Though it curiously doesn't work on the new trait solver -- I feel like it should work, because the alias-relate impl Trait = dyn Trait should fail since the opaque has an (implicit) Sized bound that isn't satisfied by dyn Trait, and therefore we should instead try to unsize during coercion 🤔

Looked into this: let x = hello(); does subtyping, which ends up emitting ALiasRElate(impl Trait, ?x_nested) we then try to coerce from Box<?x_nexted> to Box<dyn Trait>

this ends up encountering a nested <?7t as std::marker::Unsize<(dyn Trait + '?1)>> resolve step, where ?7t is not known to be Sized rn 🤔 this is the case as ?7t is only a subtype of impl Sized, not equal to it.

[estebank]

The code changes look reasonable to me. I strongly believe we need a crater run at the very least. I am slightly concerned by the insta-stability of this, and would much rather we had a few nightly cycles to bake and give the community a chance to raise issues for more than 12 weeks at best (as little as 6 weeks at worst), but I might be overly cautious lacking context that t-types has.

[traviscross]

Regarding the motivation for this and similar work, it's worth recalling this comment from lcnr:

Is this just for consistency? Or was there some motivating example for this?

consistency with -Znext-solver 😊 the new solver does not have the concept of "non-defining use of opaque" right now and I would like to ideally keep it that way. I consider moving to DefineOpaqueTypes::Yes in more cases to remove subtlety from the type system. RIght now we have to be careful when relating Opaque with another type as the behavior changes depending on whether we later use the Opaque or its hidden type directly (even though they are equal) edit: if that later use is with DefineOpaqueTypes::No

The broader picture here is that we're trying to flip as many of these as possible to DefineOpaqueTypes::Yes, and thereby to make the old trait solver as much like the ideal behavior of the new trait solver as possible, with respect to opaque types, before stabilizing any further impl Trait features. The purpose of this is to facilitate later stabilizing the new trait solver without adding unwanted complexity to it.

There are many of these left to do, and for practical reasons, @oli-obk can only handle doing so many of these in parallel. Thanks to everyone for helping to review and move these along steadily, as they have been, as that helps to keep the pipeline filled.

[nikomatsakis]

@rfcbot reviewed

…

On Mon, May 27, 2024, at 4:08 AM, Rust RFC bot wrote: Team member @oli-obk <https://github.com/oli-obk> has proposed to merge this. The next step is review by the rest of the tagged team members: • @BoxyUwU <https://github.com/BoxyUwU> • @aliemjay <https://github.com/aliemjay> • @compiler-errors <https://github.com/compiler-errors> • @jackh726 <https://github.com/jackh726> • @lcnr <https://github.com/lcnr> • @nikomatsakis <https://github.com/nikomatsakis> • @oli-obk <https://github.com/oli-obk> • @spastorino <https://github.com/spastorino> No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document <https://github.com/rust-lang/rfcbot-rs/blob/master/README.md> for info about what commands tagged team members can give me. — Reply to this email directly, view it on GitHub <#125610 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF4ZRNL5LWHN4BPM266J3ZEMH2LAVCNFSM6AAAAABIK7JFK6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZTGI2DEOBRGU>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[jackh726]

I'm still feeling some angst about checking my box here, thought I think I'm realizing it's maybe not about the PR itself, but that we don't quite seem to know what exactly we're (both us collectively in some cases, but me alone at least in others) stabilizing as we make these changes to DefineOpaqueTypes::No => Yes (as indicated by the examples in #125610 (comment) and #125610 (comment)).

I overall am very on board with the general idea of "we should make the old solver act as close to the new solver as possible", but that is not limitless: first, as much progress has been made on it, the new solver is still WIP and not only has bugs/limitations, but we may need to reintroduce limitations for backwards-compatibility reasons at some point - so (while this doesn't apply so much to this PR) imo we still very much need to be aware of new behavior we're stabilizing; and second, even logically sound interpretations of the type system rules can still be difficult to handle as a language (see leak check), and personally I think we should be carefully to ensure we don't over-generalize and make the language harder to reason about (though some limitations may not be "technical", as in the type system itself, but "higher-level" in a sense).

---

That all being said, we all know the examples given here are pretty obscure and that they will never be used very much in the wild. So, I probably shouldn't be too worried, since if we have to break them again in the future, it's likely not going to be a big problem.

I'm curious if this PR changes anything about it,, but there are few variations of the test above, that look along the lines of:

#![feature(trait_upcasting)]
trait Trait {}

impl Trait for u32 {}
impl<T: Trait + ?Sized> Trait for Box<T> {}

fn hello() -> impl Trait {
    if true {
        let x = hello();
        // Does uncommenting this line change opaque type inference ?
        //let y: Box<dyn Trait> = x;
        // What about these variations?
        //let y: Box<dyn Trait + Send> = x;
        //let y: Box<dyn Send> = x;
    }
    // Is there a difference between these two? What gets registered as the hidden type for each?
    //Box::new(1u32)
    Box::new(1u32) as Box<dyn Trait>
}

I'll tell you now, playing with this test in both the old and new solver on nightly definitely surprises me on first glance. For example, consider:

fn hello() -> impl Trait {
    if true {
        let x = hello();
        let y: Box<dyn Send> = x;
    }
    Box::new(1u32)
}

This is an interesting case that really highlights the concerns I've had: this doesn't compile, though I would argue that it either should compile (since I would expect that the hidden type to be Box<u32>) or that we should be rejecting it a "higher-level" than the underlying type system.

Interestingly, changing to let x = hello() as Box<u32>; compiles (expectedly); but let y: Box<dyn Send> = x as Box<dyn Trait + Send>; doesn't work on the old solver and works on the new solver.

I'm sure there are other interesting combinations there.

---

So, I don't know what to do. I very much want to raise a concern that the opaque type constraints from unsizing & coercions are not very well-defined, but I'm not sure this is the PR for that. However, I don't know a better place for it.

[oli-obk]

I overall am very on board with the general idea of "we should make the old solver act as close to the new solver as possible"

That's the motivation for doing this work now, not for doing this work.

The reason I didn't default to Yes (well, we didn't have the enum then, but a bool and an opt-in method, but it's the same) in the original scheme was just that we didn't have test coverage, so it seemed like the right choice to use No to make sure these cases error, so that we have to add a test for the code path when switching to Yes. It was always the intention to change everything but those two sites deep in the old solver that rely on opaque types being opaque because it also relies on erroring out on inference vars. In the new solver those sites will Just Work ™️ because the new solver eagerly replaces opaque types with infer vars so we actually go down the infer var code paths.

When I did these changes originally, there was no "new solver plan that requires all of these to be Yes".

So, you should look at it from the perspective of the overall plan of "register hidden types everywhere", the new solver is somewhat of a red herring here.

logically sound interpretations of the type system rules can still be difficult to handle as a language (see leak check), and personally I think we should be carefully to ensure we don't over-generalize and make the language harder to reason about (though some limitations may not be "technical", as in the type system itself, but "higher-level" in a sense).

to me any type mismatch error with an opaque type is a bug.

This is an interesting case that really highlights the concerns I've had: this doesn't compile, though I would argue that it either should compile

I guess this is a funny case, because it fails with the following chain of rules:

• try to subtype, fails because dyn Send is !Sized (but no error, because doing so prospectively)
• since the previous failed, do unsizing by registering an impl Trait: Unsize<dyn Send> obligation, which then (to me) expectantly fails, because we can't check if Trait: Send

we should be rejecting it a "higher-level" than the underlying type system.

I don't understand what this could be. Can you explain what you mean?

Interestingly, changing to let x = hello() as Box; compiles (expectedly); but let y: Box = x as Box<dyn Trait + Send>; doesn't work on the old solver and works on the new solver.

I added tests for both of these

The second one is (due to non-opaque-type reasons) failing because the old solver tries to prove <dyn Trait + std::marker::Send as std::marker::Unsize<dyn std::marker::Send>> and fails, I have not debugged this, but it's unrelated. The opaque type related failure is just the same as the case you mentioned above

impl<T: Trait + ?Sized> Trait for Box {}

not sure that actually is relevant in your test, it looks like all it changes is adding another layer to think about, but that doesn't introduce new behaviour (unlike the other tests you proposed, which are actually untested code paths)

[oli-obk]

I very much want to raise a concern that the opaque type constraints from unsizing & coercions are not very well-defined, but I'm not sure this is the PR for that.

it is the right PR, but I have no idea how to resolve your concerns, because I don't understand them concretely. I understand the general notion of "these casts are all weird", but I'm not sure that is specific to opaque types, beyond those being more confusing as you kind of have a way to name (something like) inference vars now. You can get most of these situations if you use funny inference, and then you usually just get a could not infer type error and have to annotate things explicitly.

As you noticed

Interestingly, changing to let x = hello() as Box<u32>; compiles (expectedly)

explicitly annotating things with types makes everything compile.

Basically opaque types have (almost) all the same issues as inference vars, and may need explicit type annotations to resolve errors.

[lcnr]

I explained why this fails with -Znext-solver in #125610 (comment)

I feel like this should compile. We can fix this by partially reverting #119989:

• store sub_relations in the FnCtxt instead of the InferCtxt (so they aren't used by anything inside of the trait solver
• return them as external constraints inside of the solver

This allows us to consider subtype relations in obligations_for_self_ty, e.g. when coercing types, which seems a lot more clearly desirable to me, without having to pass them into canonical queries. The reason I removed them is

we don't track them when canonicalizing or when freshening,
resulting in instable caching in the old solver, and issues when
instantiating query responses in the new one.

Tracking them in canonical inputs has a significant performance and complexity cost and is unnecessary for this case.

This is very similar to the following which has previously never worked:

fn hello() {
    let y = Default::default();
    let x = Box::new(y);
    let z: Box<dyn Send> = x;
    //~^ the size for values of type `dyn Send` cannot be known at compilation time
    let _: i32 = y;
}

I personally am in favor of implementing that change in the new solver but don't think it's a priority until we're close to stabilizing it

[jackh726]

Sorry all, I had some stuff come up and I lost my mental context here.

Essentially, I talked to @oli-obk a bit about this and I believe that they at least understand my underlying concerns here, which essentially come down to the lang concern of "implicit coercions with opaques can lead to surprising behavior". As-is, I don't feel comfortable checking my box here, because that underlying concern still exists. But, I also don't feel so strongly that I should formally raise a concern (and I also think that @oli-obk has much better context than me and I trust their judgement if otherwise the team has consensus to land).

Opening an issue with the relevant question here for T-lang to decide on before TAIT stabilization (hopefully with at least some experimentation) would be enough to resolve my concern for this PR (for me to check my box) - and I honestly would like to see it done regardless. Though, it's worth pointing out that this does have insta-stable effects via RPIT, but I am not terribly concerned because I don't think these cases will come up a bunch in practice.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[compiler-errors]

@bors r+

[bors]

📌 Commit 45da035 has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit 45da035 with merge 164e129...

[bors]

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 164e129 to master...

[rust-timer]

Finished benchmarking commit (164e129): 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)

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

Cycles

Results (secondary 5.4%)

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)	5.4%	[2.8%, 8.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (secondary -0.0%)

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

Bootstrap: 693.565s -> 694.699s (0.16%)
Artifact size: 326.74 MiB -> 326.68 MiB (-0.02%)