Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions compiler/rustc_hir_typeck/src/coercion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1381,6 +1381,7 @@ pub fn can_coerce<'tcx>(
/// - WARNING: I don't believe this final type is guaranteed to be
/// related to your initial `expected_ty` in any particular way,
/// although it will typically be a subtype, so you should check it.
/// Check the note below for more details.
/// - Invoking `complete()` may cause us to go and adjust the "adjustments" on
/// previously coerced expressions.
///
Expand All @@ -1394,6 +1395,28 @@ pub fn can_coerce<'tcx>(
/// }
/// let final_ty = coerce.complete(fcx);
/// ```
///
/// NOTE: Why does the `expected_ty` participate in the LUB?
/// When coercing, each branch should use the following expectations for type inference:
/// - The branch can be coerced to the expected type of the match/if/whatever.
/// - The branch can be coercion lub'd with the types of the previous branches.
/// Ideally we'd have some sort of `Expectation::ParticipatesInCoerceLub(ongoing_lub_ty, final_ty)`,
/// but adding and using this feels very challenging.
/// What we instead do is to use the expected type of the match/if/whatever as
/// the initial coercion lub. This allows us to use the lub of "expected type of match" with
/// "types from previous branches" as the coercion target, which can contains both expectations.
///
/// Two concerns with this approach:
/// - We may have incompatible `final_ty` if that lub is different from the expected
/// type of the match. However, in this case coercing the final type of the
/// `CoerceMany` to its expected type would have error'd anyways, so we don't care.
/// - We may constrain the `expected_ty` too early. For some branches with
/// type `a` and `b`, we end up with `(a lub expected_ty) lub b` instead of
/// `(a lub b) lub expected_ty`. They should be the same type. However,
/// `a lub expected_ty` may constrain inference variables in `expected_ty`.
/// In this case the difference does matter and we get actually incorrect results.
/// FIXME: Ideally we'd compute the final type without unnecessarily constraining
/// the expected type of the match when computing the types of its branches.
pub(crate) struct CoerceMany<'tcx, 'exprs, E: AsCoercionSite> {
expected_ty: Ty<'tcx>,
final_ty: Option<Ty<'tcx>>,
Expand Down
13 changes: 12 additions & 1 deletion compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1817,11 +1817,22 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let element_ty = if !args.is_empty() {
let coerce_to = expected
.to_option(self)
.and_then(|uty| self.try_structurally_resolve_type(expr.span, uty).builtin_index())
.and_then(|uty| {
self.try_structurally_resolve_type(expr.span, uty)
.builtin_index()
// Avoid using the original type variable as the coerce_to type, as it may resolve
// during the first coercion instead of being the LUB type.
.filter(|t| !self.try_structurally_resolve_type(expr.span, *t).is_ty_var())
})
.unwrap_or_else(|| self.next_ty_var(expr.span));
let mut coerce = CoerceMany::with_coercion_sites(coerce_to, args);
assert_eq!(self.diverges.get(), Diverges::Maybe);
for e in args {
// FIXME: the element expectation should use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this FIXME 🤔 If we have an expected type of some infer var and we turn that into NoExpectation that ought to only affect closure signature inference, or if we were to be able to make inference progress and resolve it to more than just an infer var 🤔

Both of those cases ought to already be broken under this PR in a lot of cases though. It also seems like the examples that broke didn't actually care about either of those things.

Reading further it sounds like the original impl was wrong? Would like to see this comment updated with more of an explanation of what went wrong here.

Copy link
Contributor Author

@adwinwhite adwinwhite Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This FIXME is about this issue.
To make array coercion consistent with if and match, we ought not to use ty var as expectation here.
But a lot of code already depends on the behavior that ty var expectation gets resolved in CoerceMany thus it can guide array element's typeck. E.g. the example in my linked comment and others.
This comment isn't out of sync with the code. Perhaps I should word it better or omit it to avoid confusion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, what i don't understand is why using the try_structurally_resolve_and_adjust_for_branches function would break anything. That should only affect the case where we have an unconstrained infer var, but the case we care about supporting is one where that variable has been inferred to some type like fn(...) in which case we wouldn't replace the expectation with NoExpectation.

I agree that as the CoerceMany "makes progress" we need to propagate that information into the expectation of the element expressions as otherwise nested coercions will fail.

Did you try to implement this previously and found it broke stuff, do you still have that impl around for me to look at?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can break code like this,

pub fn main() {
    let s = String::from("bar");
    let _ = [("a", Default::default()), (Default::default(), "b"), (&s, &s)];
}

Currently we're using an infer var as expectation for array element. With coercions of the first two elements, the infer var is known to be (&str, &str) so it can guide the typeck of the third element, specifically the tuple coercion (&String, &String) -> (&str, &str).

If we use try_structurally_resolve_and_adjust_for_branches, the third element would have NoExpectation as expectation and its typeck result would be (&String, &String) which can't be coerced into or from (&str, &str) in CoerceMany.

The key point is that field-wise tuple coercion is supported in typeck but not in CoerceMany.

This branch breaks the code above, but fixes #145048.

// `try_structurally_resolve_and_adjust_for_branches` just like in `if` and `match`.
// While that fixes nested coercion, it will break [some
// code like this](https://github.com/rust-lang/rust/pull/140283#issuecomment-2958776528).
// If we find a way to support recursive tuple coercion, this break can be avoided.
let e_ty = self.check_expr_with_hint(e, coerce_to);
Comment on lines +1835 to 1836
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels somewhat weird 🤔 I would expect we use coerce.merged_ty() here to typeck each element with an expectation of the currently known coerce-LUB'd type of the array element, rather than whatever the first element happend to have the type of.

Not sure of a test where this would actually matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, coerce.merged_ty() is better than the first element's type.
But it's still best effort. We can only know the true LUB type after all typeck is done, not before.

It may introduce the problem that adjusted types of array elements are not the same since merged_ty() can change gradually.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah 👍 My thoughts here is that we've already given up on having the expected type stay the same across branches. If we wanted this we'd do what match/if do and use the original expectation adjusted for branches for each expr we check.

Instead here we're accepting that we need the expectation to change across element expressions as too much code relies on that happening in order to have nested coercions work properly. From this POV using the (in progress) LUB type seems more principled than the type of the first element's expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just replaced coerce_to with merged_ty() and it doesn't break any ui test.
The adjusted types are not a concern since all exprs are always adjusted according to the latest merged_ty() so they'll have the same type.
This change should enhance something. I just can't come up with an example.

let cause = self.misc(e.span);
coerce.coerce(self, &cause, e, e_ty);
Expand Down
7 changes: 7 additions & 0 deletions tests/ui/coercion/closure-in-array.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// Weakened closure sig inference by #140283.
fn foo<F: FnOnce(&str) -> usize, const N: usize>(x: [F; N]) {}

fn main() {
foo([|s| s.len()])
//~^ ERROR: type annotations needed
}
14 changes: 14 additions & 0 deletions tests/ui/coercion/closure-in-array.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error[E0282]: type annotations needed
--> $DIR/closure-in-array.rs:5:11
|
LL | foo([|s| s.len()])
| ^ - type must be known at this point
|
help: consider giving this closure parameter an explicit type
|
LL | foo([|s: /* Type */| s.len()])
| ++++++++++++

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0282`.
36 changes: 36 additions & 0 deletions tests/ui/coercion/coerce-many-with-ty-var.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
//@ run-pass
// Check that least upper bound coercions don't resolve type variable merely based on the first
// coercion. Check issue #136420.

fn foo() {}
fn bar() {}

fn infer<T>(_: T) {}

fn infer_array_element<T>(_: [T; 2]) {}

fn main() {
// Previously the element type's ty var will be unified with `foo`.
let _: [_; 2] = [foo, bar];
infer_array_element([foo, bar]);

let _ = if false {
foo
} else {
bar
};
infer(if false {
foo
} else {
bar
});

let _ = match false {
true => foo,
false => bar,
};
infer(match false {
true => foo,
false => bar,
});
}
Loading