Skip to content

Fix accidental type inference in array coercion #140283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+69 −1
Open

Fix accidental type inference in array coercion #140283

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ab8f33
fix accidental type variable resolution in array coercion
adwinwhite Aug 7, 2025
668b954
add a test for weakened closure signature inference
adwinwhite Sep 24, 2025
9d0fe04
style: remove trailing empty lines in test
adwinwhite Sep 24, 2025
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
13 changes: 12 additions & 1 deletion compiler/rustc_hir_typeck/src/expr.rs
View file
Open in desktop
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 •
edited
Loading

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
38 changes: 38 additions & 0 deletions tests/ui/coercion/coerce-many-with-ty-var.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
//@ 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,
});


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

}
Loading