Fix tuple element tys returned from sized_conditions()

[gurry]

Instead of returning the type of the last element we flip it around and return types of all elements except the last one. This matches the the constraint on tuple types which says that only the last element can be unsized.

Edit
Returning all elements except the last was a mistake. We now return all the elements.

Fixes #121443

[rustbot]

r? @fmease

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

[gurry]

Please see my comment here: #121443 (comment) . Would appreciate if you can validate if this is the right approach. If it is, I can fix the failing/ICEing tests caused by this change.

[oli-obk]

sized_conditions and sized_constraint_for_ty are mostly duplicates of each other. we should probably deduplicate them $somehow.

They are meant to only look at the last type in structs and tuples (the "tail"), as that is the only thing that decides the sizedness of the entire type. This is sufficient in theory, because unsized types anywhere but the last field are unsupported and a hard error in wfcheck. Of course that doesn't help code that relies on wfcheck having bailed out already.

Maybe we should move such checks into stuff the WellFormed obligation checks, and then change the wfcheck check to just register that predcicate instead of manually checking it

[gurry]

Thanks. I'll implement according to your suggestions.

[oli-obk]

Those were just thoughts, I'm not sure this is a straightforward change. Tho the deduplication should probably be done on its own anyway

[gurry]

Yeah, we can do the dedup in a separate PR.

I am not super familiar with the trait selection code, so let me go through it and see what I can do.

[gurry]

I have spent some time familiarizing myself with trait selection code and analyzed #21443 again.

What is happening in the issue is:

1. check_item_type in wfcheck adds both a Sized and a WF obligation for the tuple type in question:

   rust/compiler/rustc_hir_analysis/src/check/wfcheck.rs

   Lines 1252 to 1260 in 8b1af4c

   	wfcx.register_wf_obligation(ty_span, Some(WellFormedLoc::Ty(item_id)), item_ty.into());
   	if forbid_unsized {
   	wfcx.register_bound(
   	traits::ObligationCause::new(ty_span, wfcx.body_def_id, traits::WellFormed(None)),
   	wfcx.param_env,
   	item_ty,
   	tcx.require_lang_item(LangItem::Sized, None),
   	);
   	}

2. During obligation processing the Sized obligation does not fail and causes an Ok entry to be added to the global selection cache for the Sized trait predicate on the tuple type:

   rust/compiler/rustc_trait_selection/src/traits/select/mod.rs

   Lines 396 to 405 in f7cb53e

   	let (candidate, dep_node) =
   	self.in_task(|this| this.candidate_from_obligation_no_cache(stack));
   	debug!("CACHE MISS");
   	self.insert_candidate_cache(
   	stack.obligation.param_env,
   	cache_fresh_trait_pred,
   	dep_node,
   	candidate.clone(),
   	);

   The WF obligation does result in Sized obligations registered for non-last fields of the tuple via it calls wf::obligations->compute:

   rust/compiler/rustc_trait_selection/src/traits/wf.rs

   Lines 627 to 633 in f7cb53e

   	ty::Tuple(tys) => {
   	if let Some((_last, rest)) = tys.split_last() {
   	for &elem in rest {
   	self.require_sized(elem, traits::TupleElem);
   	}
   	}
   	}

   which seems to be exactly what we want. However it has no impact whatsoever on the cache entry added above which stays put.
3. Later on, the fallback phase in typeck tries to select a Sized predicate on the tuple type and finds the one added to the cache above. As a result typeck passes for the type and the ICE occurs during const eval.

Based on the above analysis, the fix in sized_conditions does seem to be the best solution as it should prevent the bad entry from going into the cache. However, in my earlier commit I messed up by returning only the non-last fields from size_conditions whereas I should've returned all the fields.

Now I have pushed a new commit in which sized_conditions which returns all fields. It causes the Sized obligation on the tuple fail during confirmation:

rust/compiler/rustc_trait_selection/src/traits/select/mod.rs

Lines 1270 to 1279 in f7cb53e

	match this.confirm_candidate(stack.obligation, candidate) {
	Ok(selection) => {
	debug!(?selection);
	this.evaluate_predicates_recursively(
	stack.list(),
	selection.nested_obligations().into_iter(),
	)
	}
	Err(..) => Ok(EvaluatedToErr),
	}

which adds an Err entry to the selection cache.

#21443 is now fixed, the ICEs in codegen we saw earlier are gone and all the tests are green. @oli-obk Please let me know what you think and whether this approach can be viable at all.

[gurry]

@rustbot label -S-waiting-on-author +S-waiting-on-review

[oli-obk]

I think this is correct. I am worried there is something this will break out in the wild, but I think it's different from how we accept fn() -> str function pointers in the type system, even if they can't be constructed or used.

@rust-lang/types can you think of any issues with checking that in order for a tuple to be sized, all its fields need to be sized (instead of just checking the last field)

[lcnr]

Checking that all tuple fields are Sized for (T, U): Sized is not wrong, and would even match the user written impl (if it were possible to write Sized impls), however, it is likely less efficient and may not fix the underlying issue in #121443.

From what I can tell the root issue here is that we do not abort evaluation even though typeck failed. I don't know why this is the case but both TEST and TEST2 should fail typeck as (T, U): Sized already requires T: Sized for wf, as explained by @gurry. That should taint the MIR, preventing ctfe from ever trying to compute the layout of an unsized type

[oli-obk]

From what I can tell the root issue here is that we do not abort evaluation even though typeck failed.

it's not a typeck error, but a wfcheck error, and that happens in a separate query invocation tree. There is no way to carry information from wfcheck to typeck without causing lots of cycles.

[lcnr]

why does typeck not check that the return type is well-formed? We should check that in typeck as well 🤔

[gurry]

why does typeck not check that the return type is well-formed? We should check that in typeck as well 🤔

Not well formed, but typeck does check that the the return type is sized. It does so by adding a Sized obligation over here:

rust/compiler/rustc_hir_typeck/src/lib.rs

Line 256 in 7606c13

fcx.require_type_is_sized(expected_type, body.value.span, traits::ConstSized);

which is then checked here:

rust/compiler/rustc_hir_typeck/src/lib.rs

Line 266 in 7606c13

fcx.type_inference_fallback();

Unfortunately this check passes because it finds the Sized predicate entry sitting in selection cache which was added earlier during wfcheck (see my previous comment).

[oli-obk]

Maybe we should move such checks into stuff the WellFormed obligation checks, and then change the wfcheck check to just register that predcicate instead of manually checking it

We could still try this scheme (and then make typeck do the wfchecks instead of wfcheck)

[gurry]

Maybe we should move such checks into stuff the WellFormed obligation checks, and then change the wfcheck check to just register that predcicate instead of manually checking it

Sorry, I didn't fully understand what you meant😟(mostly due to my new-ness to this area).

wfcheck doesn't seem to be doing any manual checks for tuples even now. All it does is add a WellFormed obligation. When that WF obligation is processed in process_obligations it doesn't do any direct WF checks either. It simply adds child obligations for the sizedness of all non-last fields over here:

rust/compiler/rustc_trait_selection/src/traits/wf.rs

Lines 627 to 633 in f7cb53e

	ty::Tuple(tys) => {
	if let Some((_last, rest)) = tys.split_last() {
	for &elem in rest {
	self.require_sized(elem, traits::TupleElem);
	}
	}
	}

which is exactly what we would like. However, these child obligations do not have the desired effect. They do not prevent a Sized trait predicate for the tuple type from being added to the cache.

Please elaborate a bit more on what you had in mind and I'll investigate further.

[oli-obk]

They do not prevent a Sized trait predicate for the tuple type from being added to the cache.

that's fine. What lcnr was proposing was to liberally add additional WellFormed obligations in typeck, instead of just adding Sized obligations.

[gurry]

that's fine. What lcnr was proposing was to liberally add additional WellFormed obligations in typeck, instead of just adding Sized obligations.

If we only register a WellFormed obligation in typeck, I'm afraid it won't have any effect, because the previous WellFormed obligation registered by wfcheck didn't either. The main problem here is the bad Sized entry sitting in the selection cache (which by way comes from from the Sized obligation registered by wfcheck). If we want to do any additional checks in typeck, we will have to somehow get rid of that cache entry first.

[oli-obk]

The main problem here is the bad Sized entry sitting in the selection cache (which by way comes from from the Sized obligation registered by wfcheck). If we want to do any additional checks in typeck, we will have to somehow get rid of that cache entry first.

I assumed that WellFormed((T, U)) got expanded to various things, among them T: Sized, which would be exactly what we'd need

[gurry]

If we only register a WellFormed obligation in typeck, I'm afraid it won't have any effect, because the previous WellFormed obligation registered by wfcheck didn't either. The main problem here is the bad Sized entry sitting in the selection cache (which by way comes from from the Sized obligation registered by wfcheck). If we want to do any additional checks in typeck, we will have to somehow get rid of that cache entry first.

And I should add, the sized_conditions change in this PR is aimed precisely at preventing that bad entry from getting into the cache in the first place.

[oli-obk]

because the previous WellFormed obligation registered by wfcheck didn't either

how did we end up getting an error then? We do get the error the size for values of type (dyn FnOnce() -> u8 + 'static) cannot be known at compilation time before we ICE

[oli-obk]

if a type is not well-formed, we can probably prove all kinds of broken things for it. So the (T, U): Sized check assumes that (T, U) is well formed, and thus it only needs to check U for sizedness. Any field but the last does not affect the Sizedness of the tuple, it affects the well-formedness.

[oli-obk]

That said, ideally we'd just do this properly with the change that you did here, but that is only part of the story, there may be other ways in which those tuples (or other types) may not be well formed, and cause ICEs subsequently.

That said, let's at least check the claim that this change is a perf issue

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[gurry]

I assumed that WellFormed((T, U)) got expanded to various things, among them T: Sized, which would be exactly what we'd need

Yes it did. The processing of WellFormed((T, U)) in process_obligation constitutes of calling wf::obligations which in turn calls compute which does this:

rust/compiler/rustc_trait_selection/src/traits/wf.rs

Lines 627 to 633 in f7cb53e

	ty::Tuple(tys) => {
	if let Some((_last, rest)) = tys.split_last() {
	for &elem in rest {
	self.require_sized(elem, traits::TupleElem);
	}
	}
	}

i.e. it adds Sized obligations for all non-last fields which would be T: Sized in our case.

And this T: Sized does fail. Unfortunately it doesn't prevent an Ok entry for (T, U): Sized from being added to the selection cache.

Ideally the failure of a child obligations should add an Err for the parent obligation. But the code in process_obligations doesn't do any such thing.

The only place where something like this occurs is in confirm_candidate deeper in the call hierachy. But at that place the above parent-child relation is not known. The only parent child relation known there is the one returned by sized_conditions. Hence this change.

[gurry]

because the previous WellFormed obligation registered by wfcheck didn't either

how did we end up getting an error then? We do get the error the size for values of type (dyn FnOnce() -> u8 + 'static) cannot be known at compilation time before we ICE

Yes, this error is the T: Sized failing. But, crucially, (T, U): Sized still didn't fail i.e. T: Sized failure did not trigger a failure of the parent.

[oli-obk]

We don't actually need (T, U): Sized to fail, any failure, as long as it happens in typeck, will taint the typeck results and prevent const eval from running at all.

Are you saying that the implied T: Sized obligation failing will not cause an error if we're inserting a WellFormed((T, U)) obligation?

[gurry]

We don't actually need (T, U): Sized to fail, any failure, as long as it happens in typeck, will taint the typeck results and prevent const eval from running at all.

True. The immediate need is only to have typeck fail. We don't really care about (T, U): Sized directly. But what makes (T, U): Sized of interest is that if it fails it guarantees that typeck will also fail (see below).

Are you saying that the implied T: Sized obligation failing will not cause an error if we're inserting a WellFormed((T, U)) obligation?

T: Sized is indeed failing and it is showing an error to the user, which is the one you mentioned above. It is just not causing typeck to fail. That is because typeck is NOT checking for T: Sized failure. It is checking for (T, U): Sized failure.

And all of this is happening regardless of the presence of WellFormed((T, U)) i.e. it does not have any impact. So we can ignore it

But this given me an idea - what if we made typeck to check for T: Sized instead of just (T, U): Sized? In other words, what if typeck checked all the non-last fields of a tuple for sizedness?

Now that it is slowly dawning on me, isn't that what you were suggesting originally? Or may be lcnr suggested that?

[gurry]

if a type is not well-formed, we can probably prove all kinds of broken things for it. So the (T, U): Sized check assumes that (T, U) is well formed, and thus it only needs to check U for sizedness. Any field but the last does not affect the Sizedness of the tuple, it affects the well-formedness.

That means (T, U): Sized implies (T, U) is WF implies T: Sized

Or, transitively, (T, U): Sized implies T: Sized

If so, T: Sized NOT holding must imply (T, U): Sized should NOT hold. This is exactly what we're trying to ensure with this PR.

[lcnr]

We imply that (T, U): Sized is correct if WellFormed((T, U)) holds. This means that (T, U): Sized is unspecified if WellFormed((T, U)) does not hold. (T, U): Sized does not imply WellFormed((T, U)).

@oli-obk, what kind of cycles do you get if you also taint the MIR if wfcheck failed?

[oli-obk]

what kind of cycles do you get if you also taint the MIR if wfcheck failed?

Mostly const eval cycles iirc, but let's actually try it and document it xD

Ah, but also type_of cycles, especially around opaques

[gurry]

We imply that (T, U): Sized is correct if WellFormed((T, U)) holds. This means that (T, U): Sized is unspecified if WellFormed((T, U)) does not hold. (T, U): Sized does not imply WellFormed((T, U)).

Just curious. What does (T, U): Sized being correct mean and how is it different from it holding? Aren't they the same thing?

[oli-obk]

Just curious. What does (T, U): Sized being correct mean and how is it different from it holding? Aren't they the same thing?

The situation right now is that (T, U): Sized holds, even if T is unsized, but it is not correct, because (T, U) is not well-formed if T is unsized.

[gurry]

@oli-obk @lcnr I have implemented the alternate solution proposed by you which relies on doing WF check in typeck: #122078. If that solution is better we can abandon this PR.

[oli-obk]

Jup, I prefer the other PR. Thanks!