consider subtyping when checking if an infer var is sized

[WaffleLapkin]

View all comments

That is, when checking if an inference variable (say ?0) is sized, with this PR, we now check for obligations like ?1: Sized, where ?0 and ?1 are transitively related via subtyping (previously we were checking for ?0 = ?1). This is sound because subtyping cannot change sized-ness.

This fixes the 4-th point/issue from #155924. Specifically, it makes this code compile:

fn blah(e: !) {
    let source = Box::new(e);
    let _: Box<dyn Send> = source;
}

This also fixes tests/ui/impl-trait/unsized_coercion.rs on new solver, see #157820 (comment).

r? types

[rustbot]

changes to inspect_obligations.rs

cc @lcnr

[WaffleLapkin]

This is not quite true.

View changes since the review

[lcnr]

how so?

[WaffleLapkin]

As mentioned in the PR description, you can have cases where both the self type and the provided vid are sub/super types of a shared related type.

[lcnr]

ah yeah, the sub relation is "transitively related via subtyping", which is weaker than "is sub or super type"

[lcnr]

sgtm, needs a types team FCP

View changes since this review

[lcnr]

This also fixes tests/ui/impl-trait/unsized_coercion.rs on new solver, although I'm not exactly sure why.

rust/compiler/rustc_hir_typeck/src/coercion.rs

Line 768 in 5ff740e

if self.type_var_is_sized(v) =>

[WaffleLapkin]

(starting a discussion here, as to not pollute the PR comments...

@lcnr re: #157820 (comment)

That is code in the old solver though, but the fix helped with the new solver.

The relevant code is here I think:

rust/compiler/rustc_hir_typeck/src/coercion.rs

Lines 2206 to 2216 in 5ff740e

	// FIXME: structurally normalize?
	if self.fcx.tcx.is_lang_item(pred.def_id(), LangItem::Unsize)
	&& let ty::Dynamic(..) = pred.skip_binder().trait_ref.args.type_at(1).kind()
	&& let ty::Infer(ty::TyVar(vid)) = *pred.self_ty().skip_binder().kind()
	&& self.fcx.type_var_is_sized(vid)
	{
	// We get here when trying to unsize a type variable to a `dyn Trait`,
	// knowing that that variable is sized. Unsizing definitely has to happen in that case.
	// If the variable weren't sized, we may not need an unsizing coercion.
	// In general, we don't want to add coercions too eagerly since it makes error messages much worse.
	ControlFlow::Continue(())

But my confusion is mostly from "why was this already working in the old solver?". The answer is probably that with the new trait solver opaques introduce inference variables differently, so that the Sized goal for them can only be detected if you consider subtyping. But I don't have the knowledge to back this idea up ^^'

View changes since the review

[lcnr]

👍

But my confusion is mostly from "why was this already working in the old solver?"

old solver doesn't eagerly replace opaque types with inference vars when normalizing, so the infer var encountered by the coercion was equal to the opaque instead of a subtype

[lcnr]

if that encountered an infer var at all, the coercion may just have encountered the opaque directly

[WaffleLapkin]

Proposal

When checking if an inference variable is sized, to infer if we need to do a coercion to a trait object or not, consider subtyping. That is, when checking if ?0 is sized, instead of looking for ?1: Sized where ?0 = ?1 (via root_var(a) == root_var(b)), look for ?1: Sized where ?0 and ?1 are transitively related via subtyping (sub_unification_table_root_var(a) == sub_unification_table_root_var(b)).

That is sound, because whatever something is sized, cannot change over subtyping. A: Sized <=> B: Sized when A <: B or A :> B.

This fixes one of the breakages from making Infallible = ! (4-th issue from #155924, breaks only one crate, but this feels reasonable to support). Specifically, this change allows the following code to compile:

fn blah(e: !) {
    let source = Box::new(e);
    let _: Box<dyn Send> = source;
}

// notably this already compiled before
fn bleh(e: !) {
    let _: Box<dyn Send> = Box::new(e);
}

It also fixes tests/ui/impl-trait/unsized_coercion.rs with new solver, which previously couldn't coerce an opaque to a trait object (see #157820 (comment)):

trait Trait {}
impl Trait for u32 {}

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

[lcnr]

For a bit more context, we never used the sub_relations for obligations_for_self_ty and doing so in general is slightly dubious. However, subtyping must not change layout, so it can also not change whether something is Sized, making this entirely correct.

We removed sub_relations in #119989 as using it in generalization made caching for the old trait solver unsound. I then added it back in #140375 for the new solver where we do track it properly.

We do need the sub_relations for opaque type handling with the new solver and for diagnostics. I don't see us ever removing this and once the old solver has been removed, I also want to change the occurs check back to use sub_relations as that is sound once old-solver caching has been removed

@rfcbot fcp merge types

[rust-rfcbot]

@lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @lcnr
• @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.

[BoxyUwU]

I feel somewhat dubious of the back and forthing of sub relations and it feels unfortunate that this FCP is us FCPing something relating to whether or not to have sub relations 😅 In general I feel somewhat nervous about committing to stuff in the old solver based off of "the new solver needs X". The new solver has changed plenty (quite recently even, eager norm :3) so it feels weird to presume how it will wind up looking once we stabilize it.

I agree that realistically we should never have subtyping affect sizedness 🤔 If that did actually exist then &&T <: &&U couldn't hold as that'd change the layout of the inner reference. In theory the only way we could support this would be if we had subtyping between a sized and an unsized type, where the unsized type's metadata is a ZST I guess?

I'm not sure how likely that is to actually be something we want :3 And, tbh, in general I would quite like to move towards a world in which subtyping never affects trait solving (e.g. fixing the current jank around higher ranked subtyping) so I would also be opposed to having such subtyping in the first place (though it remains to be seen if such a world is even workable).

With that said I think I'll tick my box, it generally aligns with where I hope subtyping winds up in the long term. Also, avoiding breakage is nice and it's fairly easy to do in this case. We can always revisit this in the future if it winds up causing problems, realistically this probably isn't a one way door even if it'd be technically breaking to stop doing this?

---

On an actually reviewing this PR's diff POV it seems fine to me, just needs the FCP to go through 👍

[lcnr]

I'm not sure how likely that is to actually be something we want :3 And, tbh, in general I would quite like to move towards a world in which subtyping never affects trait solving (e.g. fixing the current jank around higher ranked subtyping) so I would also be opposed to having such subtyping in the first place (though it remains to be seen if such a world is even workable).

@spastorino is working on this :>

I feel somewhat dubious of the back and forthing of sub relations and it feels unfortunate that this FCP is us FCPing something relating to whether or not to have sub relations 😅 In general I feel somewhat nervous about committing to stuff in the old solver based off of "the new solver needs X". The new solver has changed plenty (quite recently even, eager norm :3) so it feels weird to presume how it will wind up looking once we stabilize it.

I agree that "new solver currently does X" is a poor argument :> it turns out getting things to work is actually hard.

I think here I feel more confident as I really just want to go back to considering sub_relations in generalization as well as doing so avoids a lot of incorrect overflow errors and even hangs when diagnostics want to print the involved types after encountering overflow

[theemathas]

Does this interact with #156770?

[jackh726]

I think the logic here is sound, but the implementation here is scary: this feels very easy to misuse with a stray UseSubtyping::Yes

[lcnr]

Does this interact with #156770?

don't expect so, no. this is likely caused by MIR borrowck not checking sizedness in the right way

[JonathanBrouwer]

@rust-timer build c173b3b

[rust-timer]

Finished benchmarking commit (c173b3b): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -0.6%, secondary 9.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Cycles

Results (secondary 6.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

Results (secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Bootstrap: 491.018s -> 489.205s (-0.37%)
Artifact size: 388.45 MiB -> 388.48 MiB (0.01%)