Diverging tyvars #85558
Diverging tyvars #85558
Conversation
Previously, the never type will be replaced by a diverging
type variable (generally) only if some coercion occurs. It
can cause inconsistent behaviors like
```rust
return.foo(); //~ ERROR no method named `foo` found for type `!`
{ return }.foo(); //~ ERROR type annotations needed
```
```rust
let a = (return, ); // The type is `(!, )`.
let a = ({ return }, ); // The type is `(_, )`.
let a: (_, ) = (return, ); // The type is `(_, )`.
```
With this commit, the never type will be replaced by a
diverging type variable just at the end of the type check
for every expression, even if no coercion occurs. Thus
the problems above get solved and the consistency should
be improved.
Previously, we issue "type annotations needed" when types
must be known at some point but the resolution failed,
even if the type variables are just some diverging ones.
A typical example is `{ return }.foo()`.
With this commit, the information about diverging is recorded
in the unification table, so that we can check whether
performing the fallback affects other non-diverging type
variables. If it doesn't, we will safely perform the fallback
and we won't issue "type annotations needed" anymore.
Note lots of auxiliary type variables should be ignored during
the check, which is done with the help of `TypeVariableOriginKind`.
As a result, "type annotations needed" will be issued for
```rust
let a = return;
{ if true { a } else { return } }.foo();
```
but not for
```rust
let a: ! = return;
{ if true { a } else { return } }.foo();
```
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
☔ The latest upstream changes (presumably #85804) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #86226) made this pull request unmergeable. Please resolve the merge conflicts. |
JohnCSimon
added
S-waiting-on-review
JohnCSimon
added
S-waiting-on-review
JohnCSimon
added
S-waiting-on-review
joelpalmer
added
S-waiting-on-review
| @@ -1,3 +1,3 @@ | |||
| fn main() { | |||
| return.is_failure //~ ERROR no field `is_failure` on type `!` | |||
| return.is_failure //~ ERROR no field `is_failure` on type `()` | |||
There was a problem hiding this comment.
Haven't looked much at the implementation here, but this change alone signals something is probably not correct with this PR; the type of return should be !, not ()
|
Is this just a rebase of #85021? |
|
Going to reassign to Niko, since he is assigned to the other PR r? @nikomatsakis |
|
cc @Mark-Simulacrum we should discuss this pr =) I can't quite remember what it does |
inquisitivecrystal
added
the
T-compiler
JohnCSimon
added
S-waiting-on-review
crlf0710
added
S-waiting-on-review
|
Reviewed in Team Compiler meeting on Zulip |
|
Apologies for the long wait -- taking a look at this PR, I think in broad strokes the goal is not yet clear to me. While there is some "inconsistency" in the handling of It is true that we try to infer the type of the block (vs. a literal return expression), which causes an inference failure rather than the method resolution failure, but I'm not sure that this is actually a problem in practice? Such code is pretty rare, I'd expect, and introducing more code that "cares" about never type in particular seems unfortunate. The second change which affects whether we indicate inference failed (i.e., type annotations needed) also seems somewhat similar in practice -- do we have real-world(ish) code examples of that change improving things? It might also be a rebase error -- not clear if the diff github showing is accurate -- but the changes to the MIR-opt tests do concern me a little. Ideally I think we'd not see changes to compiling code based on changes to inference, since that means we're at least potentially altering the runtime behavior. Can you say a little more about what motivated this patch? Is it just to try to close out the particular issue around whether never-typed expressions sometimes get inference variables and sometimes don't? |
Mark-Simulacrum
added
S-waiting-on-author
|
Ping from triage: |
|
Ping from triage: @rustbot label: +S-inactive |
rustbot
added
the
S-inactive
Rebased with master and resolved conflicts.
-- Copied from #85021 --
Previously, the never type will be replaced by a diverging type variable (generally) only if some coercion occurs. It can cause inconsistent behaviors like
With the first commit, the never type will be replaced by a diverging type variable just at the end of the type check for every expression, even if no coercion occurs. Thus the problems above get solved and the consistency should be improved.
Then, another problem is we'll issue too many "type annotations needed". They are issued when types must be known at some point but the resolution failed, even if the type variables are just some diverging ones. A typical example is
{ return }.foo().With the second commit, the information about diverging is recorded in the unification table, so that we can check whether performing the fallback affects other non-diverging type variables. If it doesn't, we will safely perform the fallback and we won't issue "type annotations needed" anymore.
As a result, "type annotations needed" will be issued for
but not for
Discussed at https://rust-lang.zulipchat.com/#narrow/stream/259160-t-lang.2Fproject-never-type/topic/Never.20type.20in.20tuple .