Skip to content

Bad diagnostic for match ergonomics #70047

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
GuillaumeGomez opened this issue Mar 16, 2020 · 10 comments
Open

Bad diagnostic for match ergonomics #70047

GuillaumeGomez opened this issue Mar 16, 2020 · 10 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@GuillaumeGomez
Copy link
Member

This is a reopening of #51743 to not lose track of the diagnostic issue which still remains to this day.

Match ergonomics can create quite confusing errors.

For instance:

fn main() {
    let option = MyOption::MySome(String::new());
    option.take();
}

enum MyOption<T> {
    MyNone,
    MySome(T),
}

impl<T> MyOption<T> {
    fn take(&mut self) -> Option<T> {
        match self {
            MyOption::MySome(value) => {
                *self = MyOption::MyNone;
                Some(value)
            },
            MyOption::MyNone => None,
        }
    }
}

(playground)
gives the following error message:

error[E0308]: mismatched types
  --> src/main.rs:16:22
   |
16 |                 Some(value)
   |                      ^^^^^ expected type parameter, found &mut T
   |
   = note: expected type `T`
              found type `&mut T`

This error message is not even helping the user to figure out what the problem is.
Here, it's like if value was of type &mut T instead of T.

@jonas-schievink jonas-schievink added A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 16, 2020
@Centril
Copy link
Contributor

Centril commented Mar 16, 2020

Diagnostic on nightly:

error[E0308]: mismatched types
  --> src/main.rs:16:22
   |
11 | impl<T> MyOption<T> {
   |      - this type parameter
...
16 |                 Some(value)
   |                      ^^^^^ expected type parameter `T`, found `&mut T`
   |
   = note: expected type parameter `T`
           found mutable reference `&mut T`
   = help: type parameters must be constrained to match other types
   = note: for more information, visit https://doc.rust-lang.org/book/ch10-02-traits.html#traits-as-parameters

@Centril
Copy link
Contributor

Centril commented Mar 16, 2020

Do you have any suggestions as to what the error message should say in the best of worlds?

@varkor
Copy link
Member

varkor commented Mar 16, 2020

I imagine some of these confusing errors are really due to #64586. I think we should try to fix that one.

@GuillaumeGomez
Copy link
Member Author

I think a "bigger" error message explaining what the compiler did and what the user needs to do precisely here might be required.

@Centril
Copy link
Contributor

Centril commented Mar 16, 2020

@GuillaumeGomez That's not really actionable as a specification I could go implement though. Could you try to take the diagnostic on nightly and adjust it to your preference?

@antoyo
Copy link
Contributor

antoyo commented Mar 17, 2020

@Centril:
Something along those lines (probably adjusted to take match ergonomics into account, though) that you can get by adding the * to disable the match ergonomics and you get an error that gives a better hint at what to fix:

   Compiling playground v0.0.1 (/playground)
error[E0507]: cannot move out of `self.0` which is behind a mutable reference
  --> src/main.rs:13:15
   |
13 |         match *self {
   |               ^^^^^ help: consider borrowing here: `&*self`
14 |             MyOption::MySome(value) => {
   |                              -----
   |                              |
   |                              data moved here
   |                              move occurs because `value` has type `T`, which does not implement the `Copy` trait

@Centril
Copy link
Contributor

Centril commented Mar 17, 2020

I did some quick investigation of strategies to fix this issue. Unfortunately, the way the type checker is setup today, check_expr_path does not care about the input expected type and is only in synthesis mode (in bidirectional type-checking terms). It is when type checking the Some(_) expression (where _ is a hole for the value path expression) that the actual type error occurs, and so we would have to insert the logic which would consider Res::Local(...) there. But this would also only work for call expressions. We would have to insert similar hacks for many other expression forms, so this would not be a worthwhile change.

@Centril Centril added P-low Low priority E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Mar 17, 2020
@GuillaumeGomez
Copy link
Member Author

This completely goes against the primary purpose of the match ergonomics: making pattern matching borrows simpler. :-/

I don't know how to improve things efficiently considering how complex this would be...

@antoyo
Copy link
Contributor

antoyo commented Mar 17, 2020

What about checking in which context (i.e. expected type) the auto-ref value is used and say something like:

help: it looks like you want to move this value, but match ergonomics auto-ref this value

?

@estebank
Copy link
Contributor

estebank commented Nov 19, 2023

Current output:

error[E0308]: mismatched types
  --> src/main.rs:16:22
   |
11 | impl<T> MyOption<T> {
   |      - expected this type parameter
...
16 |                 Some(value)
   |                 ---- ^^^^^ expected type parameter `T`, found `&mut T`
   |                 |
   |                 arguments to this enum variant are incorrect
   |
   = note: expected type parameter `T`
           found mutable reference `&mut T`
help: the type constructed contains `&mut T` due to the type of the argument passed
  --> src/main.rs:16:17
   |
16 |                 Some(value)
   |                 ^^^^^-----^
   |                      |
   |                      this argument influences the type of `Some`
note: tuple variant defined here
  --> /rustc/6b771f6b5a6c8b03b6322a9c77ac77cb346148f0/library/core/src/option.rs:577:5
error[E0507]: cannot move out of `self` as enum variant `MySome` which is behind a mutable reference
  --> src/main.rs:13:15
   |
13 |         match *self {
   |               ^^^^^
14 |             MyOption::MySome(value) => {
   |                              -----
   |                              |
   |                              data moved here
   |                              move occurs because `value` has type `T`, which does not implement the `Copy` trait
   |
help: consider removing the dereference here
   |
13 -         match *self {
13 +         match self {
   |

# for free to join this conversation on GitHub. Already have an account? # 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. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants