delay unexpected successful goal during ambiguity reporting

[amirHdev]

View all comments

fixes #161669

During final ambiguity reporting a stalled obligation can unexpectedly reevaluate successfully after an earlier compilation error
Instead of immediately ICEing when this happens emit a delayed compiler bug and continue reporting the ambiguity

[rustbot]

Some changes occurred to the core trait solver

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

[rustbot]

r? @nnethercote

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 75 candidates
• Random selection from 20 candidates

[adwinwhite]

It should be a bigger problem if we can have stale stalled-on? 🤔

[amirHdev]

It should be a bigger problem if we can have stale stalled-on? 🤔

I don’t think this necessarily means the normal goal_remains_stalled path is broken
that path already checks the cached stalled_on and this ICE happens later when collect_remaining_errors_impl drains the remaining pending obligations and treats them as ambiguities

the fix here is to stop treating pending obligations there and reevaluate the drained obligation without cached stalled_on

[nnethercote]

I will pass this on to someone who knows more about this stuff than I do.

r? @adwinwhite

[ShoyuVanilla]

the fix here is to stop treating pending obligations there and reevaluate the drained obligation without cached stalled_on

But I guess using stalled_on is generally correct. Why they become stale in the related issue?

[amirHdev]

But I guess using stalled_on is generally correct. Why they become stale in the related issue?

by stale I mean stale at the final error collection point
not necessarily wrong when it was recorded

the obligation is still in the pending list but when collect_remaining_errors_impl drains it for diagnostics evaluating the same goal can already return Yes
that is what the ICE is about. we are collecting ambiguity errors but the drained goal is successful

It only rechecks drained pending obligations at the diagnostics boundary instead of blindly turning all of them into Ambiguity

[ShoyuVanilla]

I mean, what makes them stale at the final error collection point? If the goal is stalled on some conditions, e.g. some vars or opaque types and those conditions still remain unchanged, what could make goal evaluation successful then?

I'm not denying what's actually happening. What I want to say is it would be better to dig into the root cause rather than fixing the surfaced ICE directly

[amirHdev]

What I know right now is only the symptom
the obligation is still in pending when we collect ambiguity errors but a fresh evaluation at that point can succeed.
I don't yet know what state transition makes that possible and I agree with your point. if everything recorded in stalled_on is still unchanged then the goal should not suddenly evaluate to Yes

The real thing to check is whether goal_remains_stalled(stalled_on) still says the obligation is WontMakeProgress at final collection time. If it does while a fresh evaluation succeeds then the root bug is probably that stalled_on missed some dependency condition. and if it does not then the obligation became runnable but fulfillment did not reprocess it before final error collection.

[amirHdev]

for the obligations that fresh evaluate to Yes during final collection the cached stalled_on had no concrete wake-up dependency:
stalled_vars = []
sub_roots = []

goal_remains_stalled(stalled_on) still returned true and fulfillment skipped re-evaluating them but evaluating the same goal without the cached stalled_on returned Yes

I tested moving the fix earlier by not reusing stalled_on when it records no concrete dependency. with the final collection filtering removed the original repro no longer ICEs and only reports the normal diagnostics

I think the right fix is to make this a stalled cache issue instead of handling it only in collect_remaining_errors_impl.

does that direction sound right?

[ShoyuVanilla]

Hmm, I think the most problematic thing is that we are getting an empty stalled_on but getting different result in the end.

I guess the following is the relevant lines that allegedly builds an empty stalled_on (we do build stalled_on in fn goal_stalled_on_args(..) as well but it never builds an empty stalled_on, I think)

rust/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs

Lines 819 to 881 in 71238e2

	HasChanged::No => Some(self.build_stalled_on(
	canonical_goal,
	maybe_info,
	orig_values,
	succeeded_in_erased,
	)),
	},
	};
	Ok((
	normalization_nested_goals,
	GoalEvaluation { goal, certainty, has_changed, stalled_on },
	))
	}
	fn build_stalled_on(
	&self,
	canonical_goal: I::CanonicalInput,
	maybe_info: MaybeInfo,
	stalled_vars: ThinVec<I::GenericArg>,
	previously_succeeded_in_erased: SucceededInErased<I>,
	) -> GoalStalledOn<I> {
	// Remove the canonicalized universal vars, since we only care about stalled existentials.
	let mut sub_roots = ThinVec::new();
	let stalled_vars = stalled_vars
	.into_iter()
	.filter_map(|arg| match arg.kind() {
	// Lifetimes can never stall goals.
	ty::GenericArgKind::Lifetime(_) => None,
	ty::GenericArgKind::Type(ty) => match ty.kind() {
	ty::Infer(ty::TyVar(vid)) => {
	sub_roots.push(self.delegate.sub_unification_table_root_var(vid));
	Some(TyOrConstInferVar::Ty(vid))
	}
	ty::Infer(ty::IntVar(vid)) => Some(TyOrConstInferVar::TyInt(vid)),
	ty::Infer(ty::FloatVar(vid)) => Some(TyOrConstInferVar::TyFloat(vid)),
	ty::Param(_) | ty::Placeholder(_) => None,
	_ => unreachable!("unexpected orig_value: {ty:?}"),
	},
	ty::GenericArgKind::Const(ct) => match ct.kind() {
	ty::ConstKind::Infer(ty::InferConst::Var(v)) => {
	Some(TyOrConstInferVar::Const(v))
	}
	ty::ConstKind::Param(_) | ty::ConstKind::Placeholder(_) => None,
	_ => unreachable!("unexpected orig_value: {ct:?}"),
	},
	})
	.collect();
	GoalStalledOn {
	stalled_vars,
	sub_roots,
	stalled_maybe_info: maybe_info,
	opaques: GoalStalledOnOpaques::Yes {
	num_opaques_in_storage: canonical_goal
	.canonical
	.value
	.predefined_opaques_in_body
	.len(),
	previously_succeeded_in_erased,
	},
	}
	}

So, to build an empty stalled_on, we shouldn't have any interesting things in the goal modulo canonicalization. The goal shouldn't have anything like infer var, type param or placeholder. So the goal itself should be very boring (such as i32: Debug or <Vec<()> as IntoIterator>::Item = ()) and there won't be any new fact can be discovered about the goal; if we once evaluated it as ambiguous, it will ever remain ambiguous.

But what really happens doesn't match with this. So I guess there are some possibilities like..

• We might be evaluating the stalled goal with different conditions, such as different typing mode?
• We might be dropping some necessary values when building stalled_on wrongly?

[amirHdev]

I checked the two possibilities you mentioned
for the projection obligation the direct issue does not look like a typing-mode mismatch. the cached state is built in Typeck and later checked in Typeck

the more suspicious part is build_stalled_on. in this case the canonical values are regions only:
['?4, '?2, '?0]

those all go through the lifetime arm and are not tracked and the goal is not actually boring after canonicalization. it still depends on region state but that state is not represented in the cached stalled_on

[ShoyuVanilla]

Interesting. So maybe the lifetimes are actually making the goal stalled in this case, unlike our comment 🤔

[amirHdev]

I tried the broader version first but it turned out to be too aggressive. it also affected coroutine-stalled goals.

the current one keeps cached stalled states when stalled_on_coroutines is set

[ShoyuVanilla]

I looked into this a bit and I found the actual problem.

// Edition >= 2021
trait MyTrait {}
impl MyTrait for () {}

impl<'de> DeserTrait<'de> for &'de DeserStruct {}

trait DeserTrait<'de> {}
struct DeserStruct;

impl DeserTrait<'_> for &'static MyTrait {}

fn test() -> impl Send {
    testfn(&DeserStruct)
}

fn testfn<'de, D: DeserTrait<'de>>(_deserializer: D) -> impl MyTrait + 'static {}

fn main() {}

So this is your test code that ICEs.

To prove Projection({opaque#test}) == {opaque#testfn::<'de, &'?x DeserStruct>}, you go down into the nested goal &'?x DeserStruct: DeserTrait<'de>, to satisfy testfn's where-bounds.

Normally, this should be proved via the implementation impl<'de> DeserTrait<'de> for &'de DeserStruct {} and this requires the region constraint '?x = 'de.

But we have malformed another impl impl DeserTrait<'_> for &'static MyTrait {} and this results in impl DeserTrait<'_> for &'static {error type} {} b/c we can't lower MyTrait to a proper type while HIR lowering.

And this becomes another candidate as we equate the args for the trait goal and the impl candidates to check whether it's a candidate and the error type can be equated with any type:

rust/compiler/rustc_next_trait_solver/src/solve/trait_goals.rs

Lines 60 to 125 in b924f94

	fn consider_impl_candidate(
	ecx: &mut EvalCtxt<'_, D>,
	goal: Goal<I, TraitClause<I>>,
	goal_trait_ref: TraitRef<I>,
	impl_def_id: I::ImplId,
	then: impl FnOnce(&mut EvalCtxt<'_, D>) -> QueryResultOrRerunNonErased<I>,
	) -> Result<Candidate<I>, NoSolutionOrRerunNonErased> {
	let cx = ecx.cx();
	let impl_trait_ref = cx.impl_trait_ref(impl_def_id);
	if !DeepRejectCtxt::relate_rigid_infer(ecx.cx())
	.args_may_unify(goal_trait_ref.args, impl_trait_ref.skip_binder().args)
	{
	return Err(NoSolution.into());
	}
	// For every `default impl`, there's always a non-default `impl` that will *also* apply.
	// There's no reason to register a candidate for this impl, since it is *not* proof that
	// the trait goal holds.
	if cx.impl_is_default(impl_def_id) {
	return Err(NoSolution.into());
	}
	match (cx.impl_polarity(impl_def_id), goal.predicate.polarity) {
	// Impl matches polarity
	(ty::ImplPolarity::Positive, ty::ClausePolarity::Positive)
	| (ty::ImplPolarity::Negative, ty::ClausePolarity::Negative) => {}
	// Impl doesn't match polarity
	(ty::ImplPolarity::Positive, ty::ClausePolarity::Negative)
	| (ty::ImplPolarity::Negative, ty::ClausePolarity::Positive) => {
	return Err(NoSolution.into());
	}
	}
	if ecx.typing_mode().is_reflection() && !cx.is_fully_generic_for_reflection(impl_def_id) {
	return Err(NoSolution.into());
	}
	ecx.probe_trait_candidate(CandidateSource::Impl(impl_def_id)).enter(|ecx| {
	let impl_args = ecx.fresh_args_for_item(impl_def_id.into());
	ecx.record_impl_args(impl_args);
	let impl_trait_ref = impl_trait_ref.instantiate(cx, impl_args).skip_norm_wip();
	ecx.eq(goal.param_env, goal_trait_ref, impl_trait_ref)?;
	let where_clause_bounds = cx
	.clauses_of(impl_def_id.into())
	.iter_instantiated(cx, impl_args)
	.map(Unnormalized::skip_norm_wip)
	.map(|clause| goal.with(cx, clause));
	ecx.add_goals(GoalSource::ImplWhereBound, where_clause_bounds)?;
	// We currently elaborate all supertrait outlives obligations from impls.
	// This can be removed when we actually do coinduction correctly, and prove
	// all supertrait obligations unconditionally.
	ecx.add_goals(
	GoalSource::Misc,
	cx.impl_super_outlives(impl_def_id)
	.iter_instantiated(cx, impl_args)
	.map(Unnormalized::skip_norm_wip)
	.map(|pred| goal.with(cx, pred)),
	)?;
	then(ecx)
	})
	}

rust/compiler/rustc_type_ir/src/relate/combine.rs

Lines 51 to 55 in b924f94

	match (a.kind(), b.kind()) {
	(ty::Error(e), _) | (_, ty::Error(e)) => {
	infcx.set_tainted_by_errors(e);
	return Ok(Ty::new_error(infcx.cx(), e));
	}

So, this becomes another candidates with region constraint '?x: 'static as we equate &'?x DeserStruct with &'static {error type} and thus '?x with 'static.

Thus, candidates are:

• impl<'de> DeserTrait<'de> for &'de DeserStruct {}, with '?x = 'de
• impl DeserTrait<'_> for &'static {error type} {}, with ?x: 'static

The certainties of these candidates are all Yes so we flounder with ambiguity and this is why this goal is stalled:

rust/compiler/rustc_next_trait_solver/src/solve/trait_goals.rs

Lines 1616 to 1620 in b924f94

	if let Some((response, _)) = self.try_merge_candidates(&candidates) {
	Ok((response, Some(proven_via)))
	} else {
	self.flounder(&candidates).map(|r| (r, None))
	}

And as you've found out, all the args in this goals are lifetimes, so we get an empty stalled_on.

But during typeck, we end up erase 'de with '{erased} and equate '?x with it.

So, when we finish the typeck collecting the errors the same stalled goal have the previous two candidates:

• impl<'de> DeserTrait<'de> for &'de DeserStruct {}, with '?x = 'de
• impl DeserTrait<'_> for &'static {error type} {}, with ?x: 'static

But this time, as ?x and 'de are equated/erased with `'{erased}' so they become:

• impl<'de> DeserTrait<'de> for &'de DeserStruct {}, with '{erased} = '{erased}
• impl DeserTrait<'_> for &'static {error type} {}, with ?erased: 'static

And '{erased} = {erased}' constraint is trivial, so we have

• impl<'de> DeserTrait<'de> for &'de DeserStruct {}, no region constraint
• impl DeserTrait<'_> for &'static {error type} {}, with ?erased: 'static

This suddenly makes candidate merging return an always applicable candidate instead of ambiguity

rust/compiler/rustc_next_trait_solver/src/solve/mod.rs

Lines 306 to 320 in b924f94

	fn try_merge_candidates(
	&mut self,
	candidates: &[Candidate<I>],
	) -> Option<(CanonicalResponse<I>, MergeCandidateInfo)> {
	if candidates.is_empty() {
	return None;
	}
	let always_applicable = candidates.iter().enumerate().find(|(_, candidate)| {
	candidate.result.value.certainty == Certainty::Yes
	&& has_no_inference_or_external_constraints(candidate.result)
	});
	if let Some((i, c)) = always_applicable {
	return Some((c.result, MergeCandidateInfo::AlwaysApplicable(i)));
	}

And this is why this ICE happens

[ShoyuVanilla]

So, what should we do?

I think we shouldn't consider stalled_on as stale one as they shouldn't be stale and if so something wrong is happening and we shouldn't hide them under the rug.

I think we shouldn't ignore the empty stalled_on as empty stalled on might really mean nothing can (except for new opaques?) progress this goal and while reevaluating those things every time might be no harm in correctness, they can harm the perf.

I think we simply emit delayed bugs in the very line currently emits ICEs as there must be other error and if not it will be a real bug. Or even better, we can do so only if the current InferCtxt is tainted by errors.

[amirHdev]

Thanks that explains the state transition I was missing.

I updated the patch to avoid fixing this as a stalled_on cache issue.

Instead of dropping the error-type impl candidate the new version keeps it available for diagnostics but disables the always applicable merge fast path when any impl candidate header references an error type

[adwinwhite]

gonna pass the review to @ShoyuVanilla for obvious reasons :>
r? ShoyuVanilla

[ShoyuVanilla]

Yeah, I felt a bit worried about the second commit 1c6c74b bc the problematic situation is the opposite direction, i.e. the erronous candidate makes the candidate select flounder. And it might worsen the rust-analyzer's type inference as it oftentimes has candidates contains error type/region/consts due to some implementation flaws/limitations. I think alternatively, we could drop erronous candidates from for_each_relevant_impl's implementation in the rustc_middle side

Could you squash the commits and change the PR and commit title accordingly?

View changes since this review

[amirHdev]

One thing I think is worth checking.
span_delayed_bug is unconditional here and we then return an ambiguity error which emits E0284
could that end up suppressing the delayed bug even if there wasn't already an error before this branch?

that seems a little different from your earlier point that there should already be another error here otherwise it should stay a real bug

I tried set guard on infcx.tainted_by_errors() but it's None for this repro even though E0782 was already emitted 🤦🏻

Would infcx.dcx().has_errors() be the better check here keeping the existing span_bug! when there wasn't a prior error?

[ShoyuVanilla]

One thing I think is worth checking.
span_delayed_bug is unconditional here and we then return an ambiguity error which emits E0284
could that end up suppressing the delayed bug even if there wasn't already an error before this branch?

Yeah, fair point. I was thinking of emitting a delayed bug and then emitting no fulfillment error for that obligation, so that having no other error would end up triggering the ICE eventually but yeah, missed that while reviewing the actual code 😅

[ShoyuVanilla]

Some nits

View changes since this review

[ShoyuVanilla]

I think the test file name should be changed as it's not actually about stale stalled.
And could you remove the issue number from the filename and add a comment like // Regression test for <https://github.com/rust-lang/rust/issues/161669> instead?

[ShoyuVanilla]

This feels a bit awkward as this is only used for collect_remaining_errors in next-solver.
Yeah, it's a pain that current API forces us to handle solver error as a real fulfillment error 🤔

How about moving evaluating the pending obligations and filtering for delayed bug here

rust/compiler/rustc_trait_selection/src/solve/fulfill.rs

Line 408 in 32d94cc

.map(|(obligation, _)| NextSolverError::Ambiguity(obligation))

, make NextSolverError::Ambiguity carry extra fields for error mapping and comment why we need this?

That would be still ugly but I guess that might be less puzzling

[ShoyuVanilla]

👍

[ShoyuVanilla]

Thanks!

@bors r+

View changes since this review

[rust-bors]

📌 Commit d742397 has been approved by ShoyuVanilla

It is now in the queue for this repository.