Skip to content
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

Jump to bottom

The never type and diverging type variables #85021

Closed
wants to merge 2 commits into from
Closed

The never type and diverging type variables #85021

wants to merge 2 commits into from

Conversation

lrh2000
Copy link
Contributor

@lrh2000 lrh2000 commented May 7, 2021

Previously, the never type will be replaced by a diverging type variable (generally) only if some coercion occurs. It can cause inconsistent behaviors like

return.foo();      //~ ERROR no method named `foo` found for type `!`
{ return }.foo();  //~ ERROR type annotations needed
let a = (return, );          // The type is `(!, )`.
let a = ({ return }, );      // The type is `(_, )`.
let a: (_, ) = (return, );   // The type is `(_, )`.

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

let a = return;
{ if true { a } else { return } }.foo();

but not for

let a: ! = return;
{ if true { a } else { return } }.foo();

cc @nikomatsakis
cc @Mark-Simulacrum
Discussed at https://rust-lang.zulipchat.com/#narrow/stream/259160-t-lang.2Fproject-never-type/topic/Never.20type.20in.20tuple .

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lrh2000 lrh2000 force-pushed the diverging-tyvars branch from 906e421 to fe7d7f5 Compare May 7, 2021 07:24
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lrh2000 lrh2000 force-pushed the diverging-tyvars branch from fe7d7f5 to 876d91a Compare May 7, 2021 08:18
@petrochenkov
Copy link
Contributor

r? @nikomatsakis

@bors
Copy link
Contributor

bors commented May 14, 2021

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

@lrh2000 lrh2000 force-pushed the diverging-tyvars branch from 876d91a to 77c2e72 Compare May 14, 2021 11:14
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lrh2000 lrh2000 force-pushed the diverging-tyvars branch from 77c2e72 to 0ca1282 Compare May 14, 2021 11:54
@rust-log-analyzer

This comment has been minimized.

Sign in to view
lrh2000 added 2 commits May 14, 2021 20:20
@lrh2000
Always replace the never type by a diverging type variable
3231e54 
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.
@lrh2000
Allow early fallback of diverging type variables in typeck
d918387 
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();
```
@lrh2000 lrh2000 force-pushed the diverging-tyvars branch from 0ca1282 to d918387 Compare May 14, 2021 12:24
@bors
Copy link
Contributor

bors commented May 16, 2021

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

@lovishpuri
Copy link

Resolved conflicts in PR: #85558

@nikomatsakis
Copy link
Contributor

Thanks for the PR @lrh2000 (and for the rebase, @lovishpuri!), sorry I haven't had time to go over it in detail yet.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2021
@camelid camelid removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2021
@camelid camelid added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2021
@jackh726 jackh726 mentioned this pull request Aug 23, 2021
Closed
@inquisitivecrystal inquisitivecrystal added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Aug 24, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2021
@JohnCSimon
Copy link
Member

ping from triage:
@lrh2000 Returning to you to address merge conflicts and switch back to S-waiting-on-review thanks,

@lrh2000
Copy link
Contributor Author

lrh2000 commented Sep 19, 2021

Closed due to inactivity and no positive feedback (so I'm not sure whether it is on the right road).

@lrh2000 lrh2000 closed this Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.