Fix accidental type inference in array coercion #140283
Conversation
|
rustbot has assigned @petrochenkov. Use |
rustbot
added
S-waiting-on-review
There was a problem hiding this comment.
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.
|
r? types |
rustbot
added
the
T-types
|
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;
};
// I can't come up with an example of function return type for now.
} |
oli-obk
removed
the
T-compiler
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 |
oli-obk
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
adwinwhite
force-pushed
the
fn-pointer-coercion
branch
from
fdbaf03 to
b31fa29
Compare
|
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. |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
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 |
|
☔ The latest upstream changes (presumably #141716) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Array expressions normally lub their element expressions' types to ensure that things like 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 |
|
Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
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. |
rfcbot
added
proposed-final-comment-period
|
should we crater this too? @bors2 try |
|
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot How do I rebase?Assuming
You may also read Please avoid the "Resolve conflicts" button on GitHub. Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how |
I tried this at first, but it resulted in too many errors. I think one reason is that constraints on the old inference variables are missing when we create fresh ones. |
Can you share an example snippet which failed? I would assume that we still constrain the original variable when we actually equate the returned type in the parent. |
|
This is how I fold the type vars in Failed ui tests (Some diagnostics changes are excluded)
Pick one example, pub fn main() {
let s = String::from("bar");
let _ = [("a", Default::default()), (Default::default(), "b"), (&s, &s)];
}Its compilation error: Let's call the expectation type var in Before the change, After the change, |
|
this is surprising to me 🤔 while the expected type in Is the issue here that in the Ah no, the issue is that you're updating the rust/compiler/rustc_hir_typeck/src/expr.rs Lines 1794 to 1797 in 40daf23 |
|
Ah, you're right. I should do the replacement right before |
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
By now, I think using type variable to guide elements' type inference is an accident. pub fn main() {
let s = String::from("bar");
let _ = [("a", Default::default()), (Default::default(), "b"), (&s, &s)]; // compiles.
}Since the type variable is always unified with the first expr's type, it can be a wrong expectation for the remaining expressions. fn foo() {}
fn bar() {}
fn main() {
let _ = [foo, if false { bar } else { foo }]; // type mismatch when trying to coerce `bar` into `foo`.
}On the contrary, pub fn main() {
let s = String::from("bar");
let _ = if false { // `if` uses `NoExpectation` for typecking when the expected_ty is a type variable.
("a", Default::default())
} else if false {
(Default::default(), "b")
} else {
(&s, &s) // incompatible types: expected `(_, &str)`, found `(&String, &String)`
};
}fn foo() {}
fn bar() {}
fn main() {
let _ = if false { // compiles.
foo
} else {
if false {
bar
} else {
foo
}
};
}As for mentioned tuple coercion support, fn foo() {}
fn bar() {}
fn mk<T>() -> T { todo!() }
fn main() {
let mut x = (mk(),);
x = match false {
true => (foo,),
false => (bar,),
}
}it's a non-trivial task. Tuples can be nested arbitrarily. As we try to refine the LUB type at each new coercion site, we need to apply adjustments to each field of previous expressions recursively. This seems to complicate coercion a lot. |
| ty::Array(ty, _) | ty::Slice(ty) => Some(ty), | ||
| // Avoid using a type variable as the coerce_to type, as it may resolve | ||
| // during the first coercion instead of being based on the common type. | ||
| ty::Array(ty, _) | ty::Slice(ty) if !ty.is_ty_var() => Some(ty), |
There was a problem hiding this comment.
u need to structurally resolve the ty again before this check 🤔 try_structurally_resolve_type only structurally resolves the outermost type constructor
|
hmm, I don't feel like I've got a good grasp of the way this works rn (and it's definitely not in cache for me rn) I do think treating array expressions teh same as @rfcbot fcp reviewed |
adwinwhite
force-pushed
the
fn-pointer-coercion
branch
from
b31fa29 to
6ab8f33
Compare
|
Filed a separate issue #145048 to track the nested coercion bug. |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
ah wait, can you actually try to perfectly match the if/match behavior 🤔 it feels weird to weaken inference without going all the way here. We can crater and look at the actual regressions in the wild |
|
@rfcbot concern try going all the way, no expectation |
|
@bors try |
Fix accidental type inference in array coercion
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed (CI). Failed jobs:
|
Fixes #136420.
If the expectation is a type variable, we should avoid resolving it to the first element's type.
We can create a free type variable instead which is only used in this
CoerceMany.check_expr_matchwhereCoerceManyis also used does the same.