Fix accidental type inference in array coercion

[adwinwhite]

Fixes #136420.

If the expectation of array element is a type variable, we should avoid resolving it to the first element's type and wait until LUB coercion is completed.
We create a free type variable instead which is only used in this CoerceMany.

check_expr_match and check_expr_if where CoerceMany is also used do the same.

FCP Proposal:

Array expressions normally lub their element expressions' types to ensure that things like [5, 5_u8] work and don't result in type mismatches. When invoking a generic function fn foo<T>(_: [T; N]) with an array expression, we end up with an infer var for the element type of the array in the signature. So when typecking the first array element we compare its type with the infer var and thus subsequently require all other elements to be the same type.

This PR changes that to instead fall back to "not knowing" that the argument type is array of infer var, but just having an infer var for the entire argument. Thus we typeck the array expression normally, lubbing the element expressions, and then in the end comparing the array expression's type with the array of infer var type.

Things like

fn foo() {}
fn bar() {} 
fn f<T>(_: [T; 2]) {}

f([foo, bar]);

and

struct Foo;
struct Bar;
trait Trait {}
impl Trait for Foo {}
impl Trait for Bar {} 
fn f<T>(_: [T; 2]) {}

f([&Foo, &Bar as &dyn Trait]);

Remaining inconsistency with if and match(#145048):

The typeck of array always uses the element coercion target type as the expectation of element exprs while if and match use NoExpectation if the expected type is an infer var.
This causes that array doesn't support nested coercion.

fn foo() {}
fn bar() {}
fn main() {
    let _ =  [foo, if false { bar } else { foo }]; // type mismatch when trying to coerce `bar` into `foo` in if-then branch coercion.
}

But we can't simply change this behavior to be the same as if and match since many code depends on using the first element's type as expectation.

[rustbot]

r? @petrochenkov

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

[jieyouxu]

Please add a test to demonstrate the effect of this change. Especially since with this change, we would be accepting more code than on master (this is the snippet in the original issue):

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

fn main() {
    let _a = if true { foo } else { bar };
    let _b = vec![foo, bar];
    let _c = [foo, bar];
    d(if true { foo } else { bar });
    e(vec![foo, bar]);
    f([foo, bar]);  // <- this PR now accepts this
}

fn d<T>(_: T) {}
fn e<T>(_: Vec<T>) {}
fn f<T>(_: [T; 2]) {}

whereas on master this snippet does not compile w/

error[E0308]: mismatched types
  --> src/main.rs:10:7
   |
10 |     f([foo, bar]);
   |     - ^^^^^^^^^^ expected `[fn() {foo}; 2]`, found `[fn(); 2]`
   |     |
   |     arguments to this function are incorrect
   |
   = note: expected array `[fn() {foo}; 2]`
              found array `[fn(); 2]`
note: function defined here
  --> src/main.rs:15:4
   |
15 | fn f<T>(_: [T; 2]) {}
   |    ^    ---------

For more information about this error, try `rustc --explain E0308`.

I'm surprised there are no ui test diffs.

[petrochenkov]

r? types

[adwinwhite]

There're some similar errors, but I'm unsure whether it's okay to allow these code. The Rust Reference.

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

fn main() {
    let block_var = 'a: { // Easy to fix, but not specified by the Rust Reference.
        if false {
            break 'a foo;
        }
        break 'a bar;
    };

    let loop_var = loop {  // Easy to fix, but not specified by the Rust Reference.
        if false {
            break foo;
        }
        break bar;
    };

    let closure_var = || { // More complicated. But this should work according to the Rust Reference.
        if false {
            return foo;
        }
        return bar;
    };
}

[oli-obk]

There're some similar errors, but I'm unsure whether it's okay to allow these code

Yea I think these all should work, too. Please fix the easy ones and add tests for all of them if we don't already have any

[adwinwhite]

Turns out the ‘easy’ cases aren’t so easy after all. I can't fix the inference regressions for now, but I might revisit them later once I understand type inference better.

[adwinwhite]

@rustbot ready

[oli-obk]

I am not going to get to this in the next 10 days. I'll need to review the general state of inference here and write a T-types FCP text

[bors]

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

[oli-obk]

Array expressions normally lub their element expressions' types to ensure that things like [5, 5_u8] work and don't result in type mismatches. When invoking a generic function fn foo<T>(_: [T; N]) with an array expression, we end up with an infer var for the element type of the array in the signature. So when typecking the first array element we compare its type with the infer var and thus subsequently require all other elements to be the same type.

This PR changes that to instead fall back to "not knowing" that the argument type is array of infer var, but just having an infer var for the entire argument. Thus we typeck the array expression normally, lubbing the element expressions, and then in the end comparing the array expression's type with the array of infer var type.

Things like

fn foo() {}
fn bar() {} 
fn f<T>(_: [T; 2]) {}

f([foo, bar]);

and

struct Foo;
struct Bar;
trait Trait {}
impl Trait for Foo {}
impl Trait for Bar {} 
fn f<T>(_: [T; 2]) {}

f([&Foo, &Bar as &dyn Trait]);

@rfcbot merge

[rfcbot]

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @compiler-errors
• @jackh726
• @lcnr
• @nikomatsakis
• @oli-obk
• @spastorino

Concerns:

• try going all the way, no expectation resolved by Fix accidental type inference in array coercion #140283 (comment)

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.

[adwinwhite]

@adwinwhite do you still have your changes when writing #140283 (comment). Talking about this with @BoxyUwU right now and I don't quite get why this test fails with my proposed change 🤔

fn foo() {}
fn bar() {}
fn main() {
    let _ =  [foo, if false { bar } else { foo }]; // type mismatch when trying to coerce `bar` into `foo`.
}

Probably this one. I have too many local branches on this issue and the differences are subtle so I can't be sure.

This test fails with current compiler as well.
We have a ty var expectation for array element. And it gets unified with foo when coercing the first array element. Then we have foo as expectation for the if expression. But we can't coerce bar into foo as the first coercion in CoerceMany doesn't use LUB. It doesn't matter whether we're using the original ty var or another ty var as expectation as long as it's a ty var.

[rustbot]

⚠️ Warning ⚠️

• This PR is based on an upstream commit that is older than 28 days.

  It's recommended to update your branch according to the rustc-dev-guide.

[craterbot]

🎉 Experiment pr-140283 is completed!
📊 4 regressed and 6 fixed (704163 total)
📊 1676 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-140283/retry-regressed-list.txt ↩

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[lcnr]

Can you add a comment to CoerceMany that explains why the expected_ty participates in the lub?

My understanding is: 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::ParticipatsInCoerceLub(ongoing_lub_ty, final_ty), but adding and using this feels very challenging.

What we instead do is use use the expected type of the match as the initial coercion lub and as the expected type for each branch.

This allows us to use the lub of "expected type of match" with "types from previous branches" as the expectation. Now, this is obviously wrong if that lub is different from the expected type of the match. However, in that case coercing the final type of the CoerceMany to its expected type would have error'd anyways, so we don't care.

A bigger issue is that for some branches with type aand b, we end up with (a lub expected_ty) lub b instead of (a lub b) lub expected_ty. This 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 errors.

Ideally we'd compute the expected type without unnecessarily constraining the expected type of the match when computing the expected type of its branches.

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[adwinwhite]

If Coerce.coerce doesn't unify the target type, it'd be nice. But we do rely on this mechanism to resolve infer vars in expectation. Perhaps we can disable unifying the infer var target type only in CoerceMany. One day I might try, one day.

[BoxyUwU]

fn foo() {}
fn bar() {} 
fn f<T>(_: T]) {}

fn main() {
    f(loop {
        if true {
            break foo;
        } else {
            break bar;
        }
    });
}

this seems to demonstrate the same behaviour with loop in which we have some ?x expectation and eagerly infer it to foo instead of properly lubbing things 🤔 I assume this PR does not fix this?

[adwinwhite]

Yeah, this is an inherent problem of CoerceMany. loop/block/ret all have this kind of bug.
See my comment above for examples.
However they can't be fixed by the same trivial solution. There're some inference regressions.

[RalfJung]

This seems to be on the critical path to make #148190 work without even more terrible hacks. So I was wondering, what's the status here and what are the next steps to make progress? :)

[safinaskar]

@adwinwhite , please, do some minimal change, which fixes bugs, which block @RalfJung . This is needed to finally get rid of ShadowInitBox (we all want this).

We need this to work:

fn id<T>(x: Box<T>) -> Box<T> { x }

fn main() {
    <[_]>::into_vec(id::<[_; _]>(Box::new([&String::new(), "abc"])));
}

And this:

fn main() {
    let functions: [_; _] = [
        |x: i32| -> i32 { x + 3 },
        |x: i32| -> i32 { x + 3 },
    ];

    let string = String::new();
    let a: [_; _] = [&string, "abc"];
    let b: [_; _] = ["abc", &string];
}

Everything else can be fixed later.

Please mention me when answering, I'm not subscribed

[adwinwhite]

@RalfJung @safinaskar I think I have answered all questions from the reviewers but not sure whether all concerns are resolved. I'd appreciate the reviewers' judgment on whether this is ready or needs changes.

[RalfJung]

@safinaskar please slow down a little. "We need this to work" is building up pressure that we have no right to put on anyone here.

@adwinwhite thanks! Oli had to reduce his reviewing so let's assign someone else who was already involved in the discussions above:
r? @lcnr
(Or @BoxyUwU could you take it?)