Do not use non-wf input expectations from fudge when checking function calls

[ShoyuVanilla]

View all comments

cc #149379

r? lcnr

FCP: Do not use non-wf input expectations from fudge when checking function calls

What is fudging?

rust/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

Lines 168 to 170 in 71e0027

	/// Generic function that factors out common logic from function calls,
	/// method calls and overloaded operators.
	pub(in super::super) fn check_argument_types(

Consider this coercion site:

let _: Box<dyn Fn(&str) -> usize> = Box::new(|s| s.len());

We rely on the expectation to eagerly infer the type of s to be &str. However, dyn Fn(&str) -> usize is not a valid type as the argument of Box::new, as it is not Sized.

Fudging is the mechanism we use to propagate the expectation through the Box::new call without constraining its generic parameter.

Fudging computes the expected argument types by acting as if we're able to propagate the expected return type directly through the function, without any coercions on the return site.

Given that we may actually want to coerce afterwards, we cannot actually commit any constraints here. We therefore compute the expectations for the function arguments in a probe and rely on fudging to be able to name any inference variables created inside of the probe.

After the fudging step, we weaken the resulting expectation if it is an unsized type in the following lines:

rust/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

Line 354 in 71e0027

let expectation = Expectation::rvalue_hint(self, expected_input_ty);

rust/compiler/rustc_hir_typeck/src/expectation.rs

Lines 77 to 89 in 71e0027

	pub(super) fn rvalue_hint(fcx: &FnCtxt<'a, 'tcx>, ty: Ty<'tcx>) -> Expectation<'tcx> {
	let span = match ty.kind() {
	ty::Adt(adt_def, _) => fcx.tcx.def_span(adt_def.did()),
	_ => fcx.tcx.def_span(fcx.body_id),
	};
	let cause = ObligationCause::misc(span, fcx.body_id);
	// FIXME: This is not right, even in the old solver...
	match fcx.tcx.struct_tail_raw(ty, &cause, |ty| ty, || {}).kind() {
	ty::Slice(_) | ty::Str | ty::Dynamic(..) => ExpectRvalueLikeUnsized(ty),
	_ => ExpectHasType(ty),
	}
	}

Because function arguments must be Sized, this weakening prevents us from applying wrong, unsized coercions to them.

How fudging currently goes wrong

We have an opened issue for tracking such cases: #149379.

Broadly, the failures seem to fall into two buckets.

We may end up with non–well-formed expectations

Fudging can produce an expected type that is not well-formed.

That would eventually result in an error failing the well-formedness check, either when we do the coercion with the expected argument types, or when we select the remaining obligations.

fn foo<T>(x: (T, ())) -> Box<T> {
    Box::new(x.0)
}

fn main() {
    // We use `(dyn Send, ())` as the expectation the argument.
    let _: Box<dyn Send> = foo(((), ()));
}

Weakening fudged expectation is not covering all the cases

fn field_to_box<T>(x: &(T,)) -> &T {
    &x.0
}
fn main() {
    // `Expectation::rvalue_hint` only checks whether the whole argument
    // itself is `Sized`. It does not check whether the function requires
    // its generic parameters to be `Sized`.
    let _: &dyn Send = field_to_box(&(1,));
}

What this PR fixes

One of the problematic cases of the issue

This PR fixes the first case, by simply checking well-formedness of the each expected argument types inside the fudge scope.

This is a reasonable change because:

• Non well-formed expectation would result in a well-formedness error so not using such expectation wouldn't make any previously compiled code being not compiled anymore
• Dropping a non well-formed expectation does not mean we stop providing expectations for argument coercions altogether.
  If fudging fails, we still fall back to using the types from the function signature as expectations in the usual path:
  

  rust/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

  Lines 330 to 336 in 71e0027

  	// If there are no external expectations at the call site, just use the types from the function defn
  	let expected_input_tys = if let Some(expected_input_tys) = expected_input_tys {
  	assert_eq!(expected_input_tys.len(), formal_input_tys.len());
  	expected_input_tys
  	} else {
  	formal_input_tys.clone()
  	};

Related tests

• Fixes tests/ui/coercion/fudge-inference/fn-ret-trait-object-propagated-to-inputs-issue-149379-1.rs

Limited for -Znext-solver

Separately (and not directly tied to the above issue), this PR also fixes a next-solver regression tracked at: rust-lang/trait-system-refactor-initiative#259.

That regression was introduced by #149320, which started normalizing expected function input types inside the fudge scope.
Because all inference-variable relationships and pending obligations are dropped out when we exit the fudge scope, we drop the ambiguous goal used to normalize, leaving us with an unconstrained inference variable in the expectation.

This PR fixes that by normalizing the expectation outside the fudge scope, so the resulting normalization obligations are preserved to the InferCtxt's ObligationCtxt.

Related tests

• Fixes tests/ui/traits/next-solver/fudge-inference/do-not-drop-ambig-normalization.rs

Does this PR break anything

I don't think there should be any breakage affecting the old solver.

The separate expectation normalization change only affecting the new solver does break an existing test.
This is unfortunate but I think this change should be done because

• The broken test also doesn't compile with the old solver
• The expectation normalization change is necessary to compile stuff supported on stable

What this PR doesn't fix

This PR doesn't fix the second case -- Weakening fudged expectation is not covering all the cases.

@lcnr has suggested the following solution for that problem:

check whether a function where-bound errors without an out coercion, if so, weaken the expectation to ExpectRvalueLikeUnsized

I experimented with this and it works well in many cases.
However, on the old solver, checking where-bounds cannot reliably be treated as speculative: if we hit an overflow while checking the where-bounds, the old solver can fail the entire compilation rather than merely treating the check as failed and relaxing the expectation.

Since this second class of issues affects both the old solver and the next-solver, it seems preferable to keep the conservative behavior for now, at least until the next-solver is stabilized, rather than introducing a next-solver-only relaxation that might create new regressions and complicate stabilization efforts.

Related tests

• Does NOT Fix tests/ui/coercion/fudge-inference/expectated-input-not-satisfying-fn-bounds-issue-89299.rs
• Does NOT Fix tests/ui/coercion/fudge-inference/expectated-input-not-satisfying-fn-bounds-issue-149881.rs
• Does NOT Fix tests/ui/coercion/fudge-inference/fn-ret-trait-object-propagated-to-inputs-issue-149379-3.rs

[lcnr]

I feel like this PR is currently doing multiple somewhat distinct changes in the same commit. Please split them up into separate commits (or PRs even)

View changes since this review

[lcnr]

why this?

[ShoyuVanilla]

My intention was to check well-formedness of expected input tys and I thought this would do the thing as they are equated with formal input tys.
But it might be more correct to check them separately outside the fudge

[ShoyuVanilla]

Kinda related to #150316 (comment), I think this former implementation would be simpler and correct 😅

[ShoyuVanilla]

I feel like this PR is currently doing multiple somewhat distinct changes in the same commit. Please split them up into separate commits (or PRs even)

Yeah, I tried to do two things, checking well-formedness of expected input tys and whether they meet the callee's where bounds 😅

I'll break them into three commits, one for adding tests from related issues, wf check, and where bound check, with blessed test output to visualize the influence of each changes.

[rust-bors]

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

[ShoyuVanilla]

Sorry for the ping 😅 I've split the changes into multiple commits and blessed tests for each commit. Would this be okay?

[lcnr]

why and then instead of filter?

[ShoyuVanilla]

Yeah, this is very absurd 😅 I guess I was trying to do more things but just ended up something equivalent to filter

[lcnr]

separete question, why a separate op at all, instead of doing this in the existing and_then above?

[ShoyuVanilla]

I once considered doing this check inside the fudge scope here but thought that it might make more sense doing wf check outside of the fudge.
If you are meaning doing this after the fudge_inference_if_ok().ok() call, yeah, there's no reason to not to do so 😅

[lcnr]

the second change isn't necessary to avoid any breakage when switching solvers, is it?

I think to fix rust-lang/trait-system-refactor-initiative#259 we should just do f5acf22

I think these sorts of improvements are generally good, but don't like improvements which are only limited to the new solver. I would drop the second commit and then get back to that one once the new solver is stable

View changes since this review

[ShoyuVanilla]

Yeah, moving generalization outside of the fudge fixes the issue but it regresses back an improvement made by next-solver as said in #t-types/trait-system-refactor > diesel benchmark doesn't compile @ 💬

I'll try combining it with the third commit of this PR and if it doesn't fix the regression, I think this should be closed and revisited once the new solver is stabilized 😄

[ShoyuVanilla]

Yeah, moving generalization outside of the fudge fixes the issue but it regresses back an improvement made by next-solver as said in #t-types/trait-system-refactor > diesel benchmark doesn't compile @ 💬

I'll try combining it with the third commit of this PR and if it doesn't fix the regression, I think this should be closed and revisited once the new solver is stabilized 😄

Oops, the third commit + f5acf22 still regresses back tests/ui/traits/next-solver/lazy-nested-obligations-2.rs 😅 Closing this for now

[lcnr]

The first change is also old solver, is it not? That one seems cool and I think I'd like to FCP it rn

[ShoyuVanilla]

Oh, I misread your comment 😅 You meant effd66a by the word second commit. Then I'll reopen this with it

[lcnr]

I think 9f3e4d5 is clearly good and doesn't depend on nexct solver, I think effd66a should not land for now as it's limited to new solver for now

[ShoyuVanilla]

Yeah, I understood what you meant by now but I should have written "without it" or "with the first commit" instead of "with it" 😅 Anyway, I'll try that once I get home

[lcnr]

do you want to also add the move normalize out of fudge to your PR?

[lcnr]

more specifically, do you want to integrate https://github.com/lcnr/rust/tree/fudge-norm-outside into your PR? It's annoying to break tests only to unbreak them right afterwards

[ShoyuVanilla]

I think that's good to do so but I can't do it rn because I'm just going out. I'll be back home two or three hours later 😅

[ShoyuVanilla]

Squashed both commits https://github.com/lcnr/rust/tree/fudge-norm-outside into one and updated tests.
Sorry for the consecutive force push noises. I was confused between this and the other branch 😅

[lcnr]

we need to types FCP your wf checking commit here, gonna write that later this week/start of next week

[ShoyuVanilla]

we need to types FCP your wf checking commit here, gonna write that later this week/start of next week

Let me know if there’s anything I can help with on this

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[lcnr]

Suggested change

	// expectation may cause type inference errors.
	// expectation may cause type inference errors, see #150316.

[lcnr]

is this actually true, especially now that we always resolve things to their root vars, it is unnecessary though :3

[ShoyuVanilla]

Yeah, this isn't true anymore because of that root var thing + moving generalization out of fudge 😅

[lcnr]

@rfcbot fcp merge types

[rust-rfcbot]

Team member @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.