Compilation error reporting: Incomplete match over types

[ghost]

Heya,

I'm currently learning rust through a small systems-related project, with a knowledgeable mentor ( @GuillaumeGomez ) ;)

When (badly) attempting to match over the Result<EnumType, String> returned by one of my functions, I only made a match explicit for one Ok case, and was expecting to have one kitchen-sink case to catch all other cases (This was aimed at testing).

The error I got rightly claimed that I was using incompatible types, but did not make it clear that I was either missing some potential cases (Ok over some other types of my enum) .

I do not know whether this would be easy or feasible, but it could be nice to have a suggestion pointing at the potential error that the match may deserve to be more detailed.

Here is a sample test code:

use std::fs::{self,File};
use std::net::{self,TcpStream,TcpListener};

enum FdImplementor {
    File(File),
    TcpStream(TcpStream),
    TcpListener(TcpListener),
}

fn do_smthg() -> Result<FdImplementor, String> {
    Ok(FdImplementor::File(fs::File::create("/tmp/pathtest.txt").map_err(|_| "failed to create file")?))
}

fn test() -> Result<bool, String> {
    match do_smthg() {
        Ok(FdImplementor::File(file)) => Ok(true),
        e => e
    }
}

fn main() {
    test();
}

And the error:

rustc 1.13.0 (2c6933acc 2016-11-07)
error[E0308]: match arms have incompatible types
  --> <anon>:15:5
   |
15 |     match do_smthg() {
   |     ^ expected bool, found enum `FdImplementor`
   |
   = note: expected type `std::result::Result<bool, std::string::String>`
   = note:    found type `std::result::Result<FdImplementor, std::string::String>`
note: match arm with an incompatible type
  --> <anon>:17:14
   |
17 |         e => e
   |              ^

error: aborting due to previous error

[durka]

I'm not sure what you are asking for here? Once you cover the other cases you will still have to write Err(e) => Err(e) instead of e => e, since the Result types are not the same (it's inferred to Err::<FdImplementor, String>(e) => Err::<bool, String>(e)).

[GuillaumeGomez]

@durka: The thing is that the error isn't very intuitive for beginners because they bind a super specific case and then all the others without thinking (in this case) that Ok(_) is also bound. So maybe a little suggestion from the compiler might come in handy!

[durka]

Indeed if you leave out the bad arm or fix it, the error is amazing:

error[E0004]: non-exhaustive patterns: `Ok(TcpStream(_))` and `TcpListener(_)` not covered
  --> <anon>:15:11
   |
15 |     match do_smthg() {
   |           ^^^^^^^^^^ patterns `Ok(TcpStream(_))` and `TcpListener(_)` not covered

error: aborting due to previous error

so I guess the focus should be improving "match arm with incompatible type" so you can get through to "non-exhaustive patterns".

[durka]

Although... even that has a small bug... it should say "Ok(TcpStream(_)) and Ok(TcpListener(_)) not covered"!

[GuillaumeGomez]

The fact is that they're covered. But it's shadowed by the other arms. But indeed, a similar error/note would be awesome.

[ghost]

Indeed, it would have made much more sense to me as a beginner than a "type mismatch" kind of error.

[estebank]

Current output:

error[E0308]: match arms have incompatible types
  --> src/main.rs:15:5
   |
15 | /     match do_smthg() {
16 | |         Ok(FdImplementor::File(file)) => Ok(true),
17 | |         e => e
18 | |     }
   | |_____^ expected bool, found enum `FdImplementor`
   |
   = note: expected type `std::result::Result<bool, _>`
              found type `std::result::Result<FdImplementor, _>`
note: match arm with an incompatible type
  --> src/main.rs:17:14
   |
17 |         e => e
   |              ^

---

Would it make sense if the output were:

error[E0308]: match arms have incompatible types
  --> src/main.rs:15:5
   |
15 | /     match do_smthg() {
16 | |         Ok(FdImplementor::File(file)) => Ok(true),
17 | |         e => e
18 | |     }
   | |_____^ expected bool, found enum `FdImplementor`
   |
   = note: expected type `std::result::Result<bool, _>`
              found type `std::result::Result<FdImplementor, _>`
note: match arm with an incompatible type
  --> src/main.rs:17:14
   |
17 |         e => e
   |              ^
note: this match arm covers multiple cases
  --> src/main.rs:17:14
   |
17 |         e => e
   |         ^
   = note: cases covered:
           - `Ok(FdImplementor::TcpStream(TcpStream))`
           - `Ok(FdImplementor::TcpListener(TcpListener))`
           - `Err(_)`

Looking at this it looks a bit long, I feel it could be pruned a bit by doing:

error[E0308]: match arms have incompatible types
  --> src/main.rs:15:5
   |
17 |         e => e
   |              ^ expected bool, found enum `FdImplementor`
   = note: expected type `std::result::Result<bool, _>`
              found type `std::result::Result<FdImplementor, _>`
note: this match arm covers multiple cases
  --> src/main.rs:17:14
   |
17 |         e => e
   |         ^
   = note: cases covered:
           - `Ok(FdImplementor::TcpStream(TcpStream))`
           - `Ok(FdImplementor::TcpListener(TcpListener))`
           - `Err(_)`

[ghost]

The last message you are suggesting looks pretty neat, imho.
It provides the information I was missing at the time:

• fact that the second arm matches multiple cases (which was my expectation)
• List of cases covered (neat!)

Note that I'm still a bit wary of the incompatible types and the Result<bool, _> thing, as it was one of the confusing bits for me; but the improvement you suggest is already great !

Long story short: Yes it would make sense :)

[estebank]

I've been looking at this and the MatchVisitor that checks exhaustiveness of match arms is run after typechecking that catches this problem. With the current flow of the compiler we either have to duplicate the pattern checking logic (bad), we switch around the order (would need to rework librustc_const_eval/_match to accept non typechecked code), or we settle on the following output (not ideal):

error[E0308]: match arms have incompatible types
  --> src/main.rs:15:5
   |
17 |         e => e
   |              ^ expected bool, found enum `FdImplementor`
   = note: expected type `std::result::Result<bool, _>`
              found type `std::result::Result<FdImplementor, _>`
note: this match arm covers multiple cases
  --> src/main.rs:17:14
   |
17 |         e => e
   |         ^

[arielb1]

@estebank

But this would still be an error if you covered all of the Ok branch:

use std::fs::{self,File};
use std::net::{self,TcpStream,TcpListener};

enum FdImplementor {
    File(File),
    TcpStream(TcpStream),
    TcpListener(TcpListener),
}

fn do_smthg() -> Result<FdImplementor, String> {
    Ok(FdImplementor::File(fs::File::create("/tmp/pathtest.txt").map_err(|_| "failed to create file")?))
}

fn test() -> Result<bool, String> {
    match do_smthg() {
        Ok(_) => Ok(true),
        e => e //~ ERROR still a type error, you still want Err(e)
    }
}

fn main() {
    test();
}

I think we should just recommend wrapping a variant in this case, but I'm not sure how generic to make it.

I don't think exhaustiveness needs to be integrated - after you'll fix the type error, you'll get the correct exhaustiveness error.

[estebank]

Current output:

error[E0308]: match arms have incompatible types
  --> src/main.rs:17:14
   |
15 | /     match do_smthg() {
16 | |         Ok(FdImplementor::File(file)) => Ok(true),
   | |                                          -------- this is found to be of type `std::result::Result<_, _>`
17 | |         e => e
   | |              ^ expected bool, found enum `FdImplementor`
18 | |     }
   | |_____- `match` arms have incompatible types
   |
   = note: expected type `std::result::Result<bool, _>`
              found type `std::result::Result<FdImplementor, _>`

[ghost]

This is indeed much better, in the sense that the "returned" e is described as being the source of the type error. Took me some time to understand it, though (since I didn't get back to this since then, so I'm still a newbie which is good to provide feedback here imho).

I think it can be considered ok to close this. The only difficulty left here for beginners is to understand that the expected return type of std::result::Result<bool,_> is coming from the first arm's resolution; and defined as the expected return type for the match expression, but then that the second arm provides a conflict on that type resolution.

I'll leave it up to you to determine whether you want to close this issue or not at this point, but I personally think this is already a great improvement :) 👍

[estebank]

Closing in view of the output after #60455 closing the last somewhat confusing part (missing types in the match arm):

I see very little improvement we could make over this.

[Joacchim]

@estebank Looks fine, we have both the expected return type, and the expectation vs reality on the mismatching match arm. All info is there, and I agree it looks hard to improve :)