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

Specific Error Message or Note for Final Expression being Loop and Return not Unit #39968

Closed
Havvy opened this issue Feb 20, 2017 · 7 comments · Fixed by #66101
Closed

Specific Error Message or Note for Final Expression being Loop and Return not Unit #39968

Havvy opened this issue Feb 20, 2017 · 7 comments · Fixed by #66101
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-diagnostics Working group: Diagnostics

Comments

@Havvy
Copy link
Contributor

Havvy commented Feb 20, 2017

fn loop_ending() -> i32 {
    loop {
        if false { break; }
        return 42;
    }
}

produces

error[E0308]: mismatched types
 --> <anon>:2:5
  |
2 |       loop {
  |  _____^ starting here...
3 | |         if false { break; }
4 | |         return 42;
5 | |     }
  | |_____^ ...ending here: expected i32, found ()
  |
  = note: expected type `i32`
  = note:    found type `()`

error: aborting due to previous error

But the compiler can check that the final expression is a loop and must always evaluate into (), so we can recommend more advanced information.

This has caused real confusion.

@Aatch Aatch added the A-diagnostics Area: Messages for errors, warnings, and lints label Feb 21, 2017
@steveklabnik steveklabnik added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
@nikomatsakis
Copy link
Contributor

What would you think we ought to suggest?

@nikomatsakis
Copy link
Contributor

(In other words, I'm trying to understand what users might be going for when writing code like this... I guess we could say something like "no value returned along this control-flow path" or something?)

@Havvy
Copy link
Contributor Author

Havvy commented Mar 24, 2017

Perhaps saying that the i32 is the expected type because the loop is the implicit return. I forgot about break value; actually being a thing when I initially posted the issue, though for loops also have the issue.

I guess the issue might actually be that we're not showing where the i32 came from.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Sep 22, 2017

Another example (Derived from #18390):

fn main() {
    let _ = foo();
}

fn foo() -> i32 {
    let x = 5;
    match x {
        1 => {
            return 1
        },
        2 => {
            return 2
        },
        3 => {
            return 3
        },
        4 => {
            return 4
        },
        5 => {
            return 5
        },
        _ => { }
    }
}

gives:

error[E0308]: mismatched types
  --> src/main.rs:23:14
   |
23 |         _ => { }
   |              ^^^ expected i32, found ()
   |
   = note: expected type `i32`
              found type `()`

I guess the key point in both cases is that the user is writing more "C-style" code and not expecting to make use of the "implicit return" here, so the error is sort of surprising.

@nikomatsakis nikomatsakis added E-needs-mentor WG-diagnostics Working group: Diagnostics labels Sep 22, 2017
@nikomatsakis
Copy link
Contributor

It seems to me that we could cover this by a similar rule to the one we use to suggest removing a semicolon -- in particular, if the expected type is not unit, but we are returning unit from a tail expression, and that tail expression is some kind of "control-flow" structure, we might point to the end of it and say something like "no return along this path":

error[E0308]: no return along this path
 --> src/main.rs:3:20
  |
3 |         if false { break; }
  |                    ^^^^^ following this path, no value is ever returned
  |

This will require a bit more state to be plumbed around, I imagine, not entirely sure what (and I'm not entirely sure where the error should be reported -- on the break, as I showed above?).

@estebank
Copy link
Contributor

estebank commented Nov 23, 2017

The current output is even worse, IMO:

error[E0308]: mismatched types
 --> src/main.rs:3:20
  |
3 |         if false { break; }
  |                    ^^^^^ expected (), found i32
  |
  = note: expected type `()`
             found type `i32`

Centril added a commit to Centril/rust that referenced this issue Sep 27, 2019
…atthewjasper

Account for tail expressions when pointing at return type

When there's a type mismatch we make an effort to check if it was
caused by a function's return type. This logic now makes sure to
only point at the return type if the error happens in a tail
expression.

Turn `walk_parent_nodes` method into an iterator.

CC rust-lang#39968, CC rust-lang#40799.
Centril added a commit to Centril/rust that referenced this issue Sep 28, 2019
…atthewjasper

Account for tail expressions when pointing at return type

When there's a type mismatch we make an effort to check if it was
caused by a function's return type. This logic now makes sure to
only point at the return type if the error happens in a tail
expression.

Turn `walk_parent_nodes` method into an iterator.

CC rust-lang#39968, CC rust-lang#40799.
Centril added a commit to Centril/rust that referenced this issue Sep 28, 2019
…atthewjasper

Account for tail expressions when pointing at return type

When there's a type mismatch we make an effort to check if it was
caused by a function's return type. This logic now makes sure to
only point at the return type if the error happens in a tail
expression.

Turn `walk_parent_nodes` method into an iterator.

CC rust-lang#39968, CC rust-lang#40799.
@estebank
Copy link
Contributor

Current output:

error[E0308]: mismatched types
 --> src/lib.rs:3:20
  |
3 |         if false { break; }
  |                    ^^^^^
  |                    |
  |                    expected i32, found ()
  |                    help: give it a value of the expected type: `break 42`
  |
  = note: expected type `i32`
             found type `()`

@estebank estebank added D-confusing Diagnostics: Confusing error or lint that should be reworked. D-papercut Diagnostics: An error or lint that needs small tweaks. labels Oct 11, 2019
Centril added a commit to Centril/rust that referenced this issue Nov 5, 2019
Tweak type mismatch caused by break on tail expr

When `break;` has a type mismatch because the `Destination` points at a tail
expression with an obligation flowing from a return type, point at the
return type.

Fix rust-lang#39968.
Centril added a commit to Centril/rust that referenced this issue Nov 6, 2019
Tweak type mismatch caused by break on tail expr

When `break;` has a type mismatch because the `Destination` points at a tail
expression with an obligation flowing from a return type, point at the
return type.

Fix rust-lang#39968.
@bors bors closed this as completed in b0f258b Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-diagnostics Working group: Diagnostics
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants