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.

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

Point at coercion reason for `if` expressions without else clause if caused by return type #58981

Merged
merged 3 commits into from Mar 22, 2019

Conversation

Projects
None yet
6 participants
@estebank
Copy link
Contributor

commented Mar 7, 2019

error[E0317]: if may be missing an else clause
  --> $DIR/if-without-else-as-fn-expr.rs:2:5
   |
LL |   fn foo(bar: usize) -> usize {
   |                         ----- found `usize` because of this return type
LL | /     if bar % 5 == 0 {
LL | |         return 3;
LL | |     }
   | |_____^ expected (), found usize
   |
   = note: expected type `()`
              found type `usize`
   = note: `if` expressions without `else` must evaluate to `()`

Fix #25228.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Mar 7, 2019

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@estebank

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

@rust-highfive rust-highfive assigned davidtwco and unassigned pnkfelix Mar 7, 2019

@estebank estebank changed the title Point at coercion reason for if exprs without else clause Point at coercion reason for `if` expressions without else clause if caused by return type Mar 7, 2019

@davidtwco
Copy link
Member

left a comment

I'm unsure whether this was just me getting confused, but here are some thoughts.

|
= note: expected type `()`
found type `usize`
= note: `if` expressions without `else` must evaluate to `()`

This comment has been minimized.

Copy link
@davidtwco

davidtwco Mar 7, 2019

Member

I found this error message was still quite confusing initially. It's clear to me that the function will return () and not usize when the if condition isn't true and that this is the error, but perhaps outwith the if may be missing an else clause part of the error, this isn't well articulated. I think there are two reasons for this:

  • It appears to me that we should expect a usize (due to the return type) and have found a () (due to the missing else block). Currently the error suggests the reverse, which is correct only when we consider what would be required to have the if without an else be valid.

  • While the note is factually correct, I initially interpreted it as a suggestion rather than a statement of fact - that if I were to change the if block to evaluate to () then this would fix the error, but that isn't the case. Rewording this to something like `if` expressions can only omit an `else` block when they are expected to evaluate to `()` may help clear that up.

I think something along these lines could be more helpful for this particular test:

error[E0317]: if may be missing an else clause
  --> $DIR/if-without-else-as-fn-expr.rs:2:5
   |
LL |   fn foo(bar: usize) -> usize {
   |                         ----- expected `usize` because of this return type
LL | /     if bar % 5 == 0 {
LL | |         return 3;
LL | |     }
   | |_____^ expected usize, found ()
   |
   = note: expected type `usize`
              found type `()`
   = note: this function evaluates to `()` when the `if` condition is not `true`...
   = note: ...consider adding a `else` block that evaluates to `usize`.

More generally, I think when an if expression is expected to evaluate to () then the note as in this PR currently is excellent, but if it doesn't match, we should instead explain that a implicit else { } block is returning the wrong type, and an else block should be added.

This comment has been minimized.

Copy link
@estebank

estebank Mar 7, 2019

Author Contributor

@davidtwco let me know what you think of the new output. It's not exactly the same you requested, as I was trying to avoid incorrect diagnostics in some cases where the coercion is failing from within the arm, not outside.

This comment has been minimized.

Copy link
@davidtwco

davidtwco Mar 8, 2019

Member

Thanks, that's perfect.

estebank added some commits Mar 7, 2019

Point at coercion reason for if exprs without else clause
```
error[E0317]: if may be missing an else clause
  --> $DIR/if-without-else-as-fn-expr.rs:2:5
   |
LL |   fn foo(bar: usize) -> usize {
   |                         ----- found `usize` because of this return type
LL | /     if bar % 5 == 0 {
LL | |         return 3;
LL | |     }
   | |_____^ expected (), found usize
   |
   = note: expected type `()`
              found type `usize`
   = note: `if` expressions without `else` must evaluate to `()`
```

@estebank estebank force-pushed the estebank:elseless-if branch from 2727cdb to ffa40cb Mar 7, 2019

@davidtwco

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

📌 Commit dcaec88 has been approved by davidtwco

bors added a commit that referenced this pull request Mar 21, 2019

Auto merge of #58981 - estebank:elseless-if, r=davidtwco
Point at coercion reason for `if` expressions without else clause if caused by return type

```
error[E0317]: if may be missing an else clause
  --> $DIR/if-without-else-as-fn-expr.rs:2:5
   |
LL |   fn foo(bar: usize) -> usize {
   |                         ----- found `usize` because of this return type
LL | /     if bar % 5 == 0 {
LL | |         return 3;
LL | |     }
   | |_____^ expected (), found usize
   |
   = note: expected type `()`
              found type `usize`
   = note: `if` expressions without `else` must evaluate to `()`
```

Fix #25228.
@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

⌛️ Testing commit dcaec88 with merge 86466a3...

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: davidtwco
Pushing 86466a3 to master...

@bors bors added the merged-by-bors label Mar 22, 2019

@bors bors merged commit dcaec88 into rust-lang:master Mar 22, 2019

1 check passed

homu Test successful
Details

Centril added a commit to Centril/rust that referenced this pull request Mar 30, 2019

Centril added a commit to Centril/rust that referenced this pull request Apr 1, 2019

Centril added a commit to Centril/rust that referenced this pull request Apr 4, 2019

Centril added a commit to Centril/rust that referenced this pull request Apr 4, 2019

Centril added a commit to Centril/rust that referenced this pull request Apr 17, 2019

Centril added a commit to Centril/rust that referenced this pull request Apr 22, 2019

Centril added a commit to Centril/rust that referenced this pull request Apr 24, 2019

Centril added a commit to Centril/rust that referenced this pull request Apr 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.