Improve `?` error messages per RFC 1859

[steveklabnik]

RFC 1859 laid out specific error messages for the ? operator, but currently they are not implemented (and instead the compiler's internal desugaring is revealed).

UPDATE: Many parts of this are done, but not all. Here is a post with examples that are not yet great.

---

Original text:

try! in main has long been an issue, to the point of trying to adjust the language to support it: rust-lang/rfcs#1176

I was trying out ? today, and hit this error:

error[E0308]: mismatched types
 --> src/main.rs:5:13
  |
5 |     let f = File::open("hello.txt")?;
  |             ^^^^^^^^^^^^^^^^^^^^^^^^ expected (), found enum `std::result::Result`
  |
  = note: expected type `()`
  = note:    found type `std::result::Result<_, _>`

That's the only line, inside of a main() function. Even though I know about this pitfall, and have been using Rust for a long time, I asked a question about it on IRC.

Can we print a better diagnostic here that points out it's the return type of the function that's looking for ()?

[durka]

Now that it's part of the language, we can even print help: the ? operator can only be used in a function that returns a Result.

[steveklabnik]

Yeah, that's exactly what I was hoping.

[nagisa]

Now that it's part of the language, we can even print help: the ? operator can only be used in a function that returns a Result.

Carrier trait.

[durka]

@nagisa can you elaborate? Do we have to wait on this kind of diagnostic until the Carrier stuff shakes out?

[nagisa]

@durka I mean that such diagnostic will make no sense once we implement and accept the carrier trait. First, because then you’ll be able to use ? in function that returns anything implementing Carrier trait (not just Result), but also because it would be possible to implement carrier trait for something like () in case anybody wanted to do so in their standard library.

Implementing such special cased diagnostic now would 100% result in it being forgotten until somebody comes along and complains that diagnostic about Result makes no sense (like it is already a case with a bunch of such help messages).

[durka]

Maybe we can just put a #[rustc_on_unimplemented] on Carrier? It would be neat if that worked, but that attribute has been sorely neglected of late. Regardless I hope that good diagnostics will be taken into account while designing the Carrier trait.

[carols10cents]

As of rustc 1.13.0-nightly (eac41469d 2016-08-30), the error in this case has gotten less understandable, imo:

error[E0277]: the trait bound `(): std::ops::Carrier` is not satisfied
 --> src/main.rs:6:13
  |
6 |     let f = File::open("hello.txt")?;
  |             ^^^^^^^^^^^^^^^^^^^^^^^^ trait `(): std::ops::Carrier` not satisfied
  |
  = note: required by `std::ops::Carrier::from_error`

error: aborting due to previous error

This error message seems to be guiding me towards implementing Carrier on (), but what would be better is what @steveklabnik and @durka suggested: something that mentions the return type is (), and that mentions you can only use ? in a function that returns a Result.

[nagisa]

This is what you can get with rustc_on_unimplemented:

error[E0277]: the trait bound `(): std::ops::Carrier` is not satisfied
 --> test.rs:4:5
  |
4 |     Ok::<()>(42)?;
  |     ^^^^^^^^^^^^^ trait `(): std::ops::Carrier` not satisfied
  |
  = note: the return type `()` cannot be used with the `?` operator
  = note: required by `std::ops::Carrier::from_error`

Bikeshed on wording.

[nikomatsakis]

@nagisa

This is what you can get with rustc_on_unimplemented:

I am not sure if this is good enough. I've long been unsatisfied with rustc_on_implemented for a couple of reasons -- one of them is that the "better" message is not the main thing people see. The other is that the ability to decide when to give the better message is pretty limited: often people write a "better" message for one particular scenario, but the same error (i.e., an unsatisfied trait bound) can appear in other scenarios where that better message sounds wrong.

i.e., here, we talk about return types, but if I wrote some code like:

fn foo<T: Carrier>() { .. .}
foo::<()>() // error

that error message would make any sense to me.

These two things are of course related, since giving a "maybe wrong" message high prominence feels bad.

That said, in this particular case, since Carrier is unstable, we can probably hard-code more -- but it feels like we could do better still. We know that the ? desugars into some particular code. We can probably tag that code and give the resulting trait obligations a more specific "cause", so that we can produce a good error in exactly this situation.

Note that this is valuable even once Carrier gets stabilized.

[durka]

Here's what I have so far:

fn does_not_return_result() -> i32 {
    try!(Ok(42));
}

error[E0308]: mismatched types
  --> src/test/compile-fail/expected-result.rs:14:5
14 |     try!(Ok(42));
   |     ^^^^^^^^^^^^^ expected i32, found enum `std::result::Result`
   |
   = note: `try!` and `?` can only be used in a function that returns a `Result`
   = note: this error originates in a macro outside of the current crate

Need to inhibit that second note. Next step is to actually figure out whether the error came from try! or ?.

[nikomatsakis]

@durka seems good so far!

[carols10cents]

Hi @durka, what do you have so far where? I don't see a PR linked to this issue at all :-/

Is there anyone I could bother to get you the help you need to finish this?

I'd really love for this to get fixed before the new version of the book comes out, and for all the new rustaceans who are hitting this in the meantime <3 <3 <3

[durka]

Hey @carols10cents, I don't really have anything working unfortunately. I'd like to pick it up again, I'll need some help though. I think that #38301 will change things anyway (@bluss why was that closed? is the design work that came out of ongoing somewhere?).

[bluss]

It may look like I did something significant but I didn't think so. This rust-lang/rfcs/issues/1718 is the main thread, and niko proposed another variant of the formulation (which I confirmed compiles and works in rustc).

It does change slightly for the better, it doesn't have a carrier trait anymore.

fn does_not_return_result() -> i32 {
    Ok(42)?
}

error[E0308]: mismatched types
 --> test3.rs:2:5
  |
2 |     Ok(42)?;
  |     ^^^^^^^ expected i32, found enum `std::result::Result`
  |
  = note: expected type `i32`
  = note:    found type `std::result::Result<_, _>`

error[E0308]: mismatched types
 --> test3.rs:1:36
  |
1 |   fn does_not_return_result() -> i32 {
  |  ____________________________________^ starting here...
2 | |     Ok(42)?;
3 | | }
  | |_^ ...ending here: expected i32, found ()
  |
  = note: expected type `i32`
  = note:    found type `()`
help: consider removing this semicolon:
 --> test3.rs:2:12
  |
2 |     Ok(42)?;
  |            ^

error: aborting due to 2 previous errors

Edited, without the semicolon is maybe better:

error[E0308]: mismatched types
 --> test3.rs:2:5
  |
2 |     Ok(42)?
  |     ^^^^^^^ expected i32, found enum `std::result::Result`
  |
  = note: expected type `i32`
  = note:    found type `std::result::Result<_, _>`

error: aborting due to previous error

[carols10cents]

@bluss thanks! That definitely looks better :) I assume if you use ? in main (the most common thing new rustaceans do) that the error message looks like:

error[E0308]: mismatched types
 --> test3.rs:2:5
  |
2 |     Ok(42)?
  |     ^^^^^^^ expected (), found enum `std::result::Result`
  |
  = note: expected type `()`
  = note:    found type `std::result::Result<_, _>`

error: aborting due to previous error

With this implementation, would it still be possible to add this note?

   = note: `try!` and `?` can only be used in a function that returns a `Result`

[ChrisJefferson]

I just wanted to mentio one more option, as a beginner rust user.

I would find it helpful if the error message mentioned something like function should return (), rather than just expected () -- to make clear why we are expecting a ().

[estebank]

Current output on nightly after #43001:

error[E0277]: the trait bound `(): std::ops::Try` is not satisfied
 --> file.rs:6:5
  |
6 |     std::fs::File::open("foo")?;
  |     ---------------------------
  |     |
  |     the `?` operator can only be used in a function that returns `Result` (or another type that implements `std::ops::Try`)
  |     in this macro invocation
  |
  = help: the trait `std::ops::Try` is not implemented for `()`
  = note: required by `std::ops::Try::from_error`

[nikomatsakis]

In order to really implement the full semantics described by RFC 1859, I think we will need to do something better than #43001. In #42526, we discussed that before we really start adding impls of Try for ?, we need to get the error messages under control, so this issue is quite a high priority in that sense.

Right now, the way that ? is implemented is as follows:

• The AST includes direct support for the ? operator (ExprTry).
• During HIR lowering, ? gets converted to lower-level HIR expressions.
• This means that HIR doesn't directly see the ? operator -- it just sees a call to Try::from_result. This explains the kind of poor error messages at present.
• However, we do keep a bit of extra info -- in particular, the span of that function call includes some annotations indicating it arises from a compiler desugaring. At the moment, this annotation is only used to allow access to unstable APIs (notably Try::from_result) even within stable code. But we can leverage it to give better errors too.
  • At this point, a slight diversion is in order. Each Span in the compiler includes "expansion info" that indicates, if this code arose from macro expansion, precisely what that expansion was.
  • This includes a full backtrace.
  • That backtrace may include "compiler desugarings", in which case we allow access to unstable APIs. For example, this helper function checks whether code in that span should have access to unstable APIs.
  • We would need a similar function like fn is_try_desugaring(&self) -> bool that checks whether the top-most expn-info has info.callee.format equal to ExpnFormat::CompilerDesugaring(x) where x == ?
    • the CompilerDesugaring currently includes an interned string to indicate which one it is. This is kind of hokey. Probably an enum would be better.
• If we had such a helper on spans, then we could use it when reporting trait failures.
  • In particular, let's start with the simplest error case: imagine that we have applied ? to a value of some type (e.g., u32) that does not implement Try. The RFC states then that the error should be: "? cannot be applied to a value of type Foo".
  • To implement this, we can modify the report_selection_error function, which is invoked when a trait selection fails.
  • In this case, the trait selection would be u32: Try.
  • We can then check whether the trait in question is the Try trait
    • It seems like we have to make Try into a lang-item first, by adding it to [this file] and then adding #[lang(try)] to the declaration of the Try trait
    • Then we can get its def-id by doing self.tcx.lang_items.try_trait() and compare that against the def-id of the trait we are selecting
    • Presuming they are the same, we can check whether the span comes from ? desugaring using our helper we added earlier (is_try_desugar)
    • If (a) this is from the try-desugar and (b) this is a match of the Try trait, then we could print the error message proscribed by the RFC.

I think this is roughly the idea. Happy to provide more details where needed. But ping on IRC or gitter if you don't get a response though, as it's easy to lose track of GH notifications.

I think that to start it would be good to target this precise case I outlined, and I would probably do it in a few steps PRs (each of which could be a separate PR):

1. Replace the string in CompilerDesugaring with an enum CompilerDesugaringKind and add a helper is_compiler_desugaring(kind: CompilerDesugaringKind) on Span that will check if it arises from a compile desugaring of a particular kind.
2. Make Try a lang item.
3. Extend the error handling in error-selection as described above.

Once we get that working, we can cover the other cases described in the RFC.

[nikomatsakis]

@huntiep expressed interest in working on this in #42526.

[nikomatsakis]

@huntiep regarding your question from #43984:

How would I differentiate between the first two situations of the RFC?

I think the thing you need to do is to create a second desugaring code, maybe something like CompilerDesugaringKind::QuestionMarkReturn, and then use that span for this code here, basically the parts that correspond to the return. Then we can detect errors that arose due to the return (or break) part of the ? desugaring and give them the custom error message of "try cannot be used in a function that returns". We might even make a distinct code for QuestionMarkBreak so that we can tailor the error message to the case of catch { ... foo? ... } versus just foo?.

[arielb1]

@huntiep feel free to ping me on IRC.

[nikomatsakis]

So, between @huntiep and @arielb1, we now have a lot of progress on this topic. See for example the try-operator-on-main test case (output).

Notably, we now say one of the following:

• the ? operator can only be used in a function that returns Result (or another type that implements std::ops::Try)
• the ? operator can only be applied to values that implement std::ops::Try

I think we can close this issue, right?

[nikomatsakis]

Well, looking through the RFC, there are a few things not covered. These are the heading:

• Suggesting a new return type:
  • The RFC writes, 'At this point, we could likely make a suggestion such as "consider changing the return type to Result<(), Box>".'
  • It's not clear to me that this is a good idea, but it might be, subject to the caveats in the RFC.
• Errors cannot be interconverted -- probably a good idea.
• Consider suggesting the use of catch -- I'm not sure how I feel about this, perhaps the extended error description is the right place to talk about it though. In any case, as catch is not stable, we likely won't want to do this (yet).

[nikomatsakis]

@arielb1 mentioned in the impl Try for Option PR that he was planning on trying to cover the "Errors cannot be interconverted" case. @arielb1 -- as an alternative, if you feel like writing mentoring instructions, this is the place.

[arielb1]

I already opened #44755 for the mentoring instructions

[estebank]

Current output:

error[E0277]: the `?` operator can only be used in a function that returns `Result` (or another type that implements `std::ops::Try`)
 --> src/main.rs:3:5
  |
3 |     std::fs::File::open("foo")?;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot use the `?` operator in a function that returns `()`
  |
  = help: the trait `std::ops::Try` is not implemented for `()`
  = note: required by `std::ops::Try::from_error`

[nikomatsakis]

@estebank what are the "errors cannot be interconverted case"? (See my earlier summary)

[estebank]

It's not great yet:

error[E0277]: the trait bound `usize: std::convert::From<std::io::Error>` is not satisfied
 --> src/main.rs:2:5
  |
2 |     std::fs::File::open("foo")?;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::convert::From<std::io::Error>` is not implemented for `usize`
  |
  = help: the following implementations were found:
            <usize as std::convert::From<u8>>
  = note: required by `std::convert::From::from`

The case where ? is used in a method with default return type is slightly verbose but suggests setting a return type only when returning, not when the ? used on its own (and the suggestion doesn't take the ? return path into consideration for the suggested return type):

error[E0277]: the `?` operator can only be used in a function that returns `Result` (or another type that implements `std::ops::Try`)
 --> src/main.rs:2:5
  |
2 |     std::fs::File::open("foo")?;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot use the `?` operator in a function that returns `()`
  |
  = help: the trait `std::ops::Try` is not implemented for `()`
  = note: required by `std::ops::Try::from_error`

error[E0308]: mismatched types
 --> src/main.rs:3:5
  |
3 |     Ok(())
  |     ^^^^^^ expected (), found enum `std::result::Result`
  |
  = note: expected type `()`
             found type `std::result::Result<(), _>`
help: try adding a semicolon
  |
3 |     Ok(());
  |           ^
help: try adding a return type
  |
1 | fn foo() -> std::result::Result<(), _> {
  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

When a return type is specified, we don't get suggestions:

error[E0277]: the trait bound `std::option::NoneError: std::convert::From<std::io::Error>` is not satisfied
 --> src/main.rs:2:8
  |
2 |     Ok(std::fs::File::open("foo")?)
  |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::convert::From<std::io::Error>` is not implemented for `std::option::NoneError`
  |
  = note: required by `std::convert::From::from`

error[E0308]: mismatched types
 --> src/main.rs:2:5
  |
1 | fn foo() -> Option<()> {
  |             ---------- expected `std::option::Option<()>` because of return type
2 |     Ok(std::fs::File::open("foo")?)
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `std::option::Option`, found enum `std::result::Result`
  |
  = note: expected type `std::option::Option<()>`
             found type `std::result::Result<std::fs::File, _>`

[nikomatsakis]

OK, I updated the head comment to link to your post.