Improving error messages

[aepsil0n]

While coding some Rust, I was a bit horrified by the error messages of some generic-heavy code. So I thought about, what might be viable ways to improve on it, as it was not as clear, as I feel it could have been. Consider it an ad-hoc case study in usability. I hope it is at least a bit informative.

Here is my example code fragment:

boundary_stepper_1d({
    // more code...
    move |grid, boundaries| /* closure body */
}, state)

To provide some context, I was in the process of refactoring it and decided to swap the two arguments to the outer function boundary_stepper_1d. But I forgot the instance above. Nothing that was particularly complicated about it, but as I was doing too many things at once (I know), I lost track of that fact for a second.

Now here’s what the compiler told me:

error[E0277]: the trait bound `timely::dataflow::Stream<timely::dataflow::scopes::Child<'_, timely::dataflow::scopes::Root<timely_communication::allocator::generic::Generic>, u64>, ({integer}, timely_cfd::geometry::Grid<nalgebra::Matrix<f64, nalgebra::U2, nalgebra::U1, nalgebra::MatrixArray<f64, nalgebra::U2, nalgebra::U1>>>)>: std::ops::Fn<(timely_cfd::geometry::Grid<_>, [_; 2])>` is not satisfied
  --> examples/isothermal_sod_shocktube.rs:19:17
   |
19 |                 boundary_stepper_1d({
   |                 ^^^^^^^^^^^^^^^^^^^ the trait `std::ops::Fn<(timely_cfd::geometry::Grid<_>, [_; 2])>` is not implemented for `timely::dataflow::Stream<timely::dataflow::scopes::Child<'_, timely::dataflow::scopes::Root<timely_communication::allocator::generic::Generic>, u64>, ({integer}, timely_cfd::geometry::Grid<nalgebra::Matrix<f64, nalgebra::U2, nalgebra::U1, nalgebra::MatrixArray<f64, nalgebra::U2, nalgebra::U1>>>)>`
   |
   = note: required because of the requirements on the impl of `std::ops::Fn<(timely_cfd::geometry::Grid<_>, [_; 2])>` for `&timely::dataflow::Stream<timely::dataflow::scopes::Child<'_, timely::dataflow::scopes::Root<timely_communication::allocator::generic::Generic>, u64>, ({integer}, timely_cfd::geometry::Grid<nalgebra::Matrix<f64, nalgebra::U2, nalgebra::U1, nalgebra::MatrixArray<f64, nalgebra::U2, nalgebra::U1>>>)>`
   = note: required by `timely_cfd::dataflow::boundary_stepper_1d`

A lot of information to process there, most of which is irrelevant to me. The essence is that a Stream<_> is not Fn(_). As soon as that is clear, it makes sense, that I mixed up something, as that should obviously not be true.

What is most distracting here to me are all the fully qualified paths. When I strip these out by hand, I get this:

error[E0277]: the trait bound `Stream<Child<'_, Root<Generic>, u64>, ({integer}, Grid<Matrix<f64, U2, U1, MatrixArray<f64, U2, U1>>>)>: Fn<(Grid<_>, [_; 2])>` is not satisfied
  --> examples/isothermal_sod_shocktube.rs:19:17
   |
19 |                 boundary_stepper_1d({
   |                 ^^^^^^^^^^^^^^^^^^^ the trait `Fn<(Grid<_>, [_; 2])>` is not implemented for `Stream<Child<'_, Root<Generic>, u64>, ({integer}, Grid<Matrix<f64, U2, U1, MatrixArray<f64, U2, U1>>>)>`
   |
   = note: required because of the requirements on the impl of `Fn<(Grid<_>, [_; 2])>` for `&Stream<Child<'_, Root<Generic>, u64>, ({integer}, Grid<Matrix<f64, U2, U1, MatrixArray<f64, U2, U1>>>)>`
   = note: required by `boundary_stepper_1d`

In this case, all the names are unique in the current context, so there is no chance to confuse them. Some of these names are already explicitly in scope, so they can not clash. And I would rarely refer to the others using their fully qualified name in practice. So the names that I see above are not the ones I’m familiar with from the code.

In this particular case, there is also a type alias in scope, Vector2<T> that refers to the more complex Matrix<T, ...>.

Replacing Matrix by Vector2 it would read:

error[E0277]: the trait bound `Stream<Child<'_, Root<Generic>, u64>, ({integer}, Grid<Vector2<f64>>)>: Fn<(Grid<_>, [_; 2])>` is not satisfied
  --> examples/isothermal_sod_shocktube.rs:19:17
   |
19 |                 boundary_stepper_1d({
   |                 ^^^^^^^^^^^^^^^^^^^ the trait `Fn<(Grid<_>, [_; 2])>` is not implemented for `Stream<Child<'_, Root<Generic>, u64>, ({integer}, Grid<Vector2<f64>>)>`
   |
   = note: required because of the requirements on the impl of `Fn<(Grid<_>, [_; 2])>` for `&Stream<Child<'_, Root<Generic>, u64>, ({integer}, Grid<Vector2<f64>>)>`
   = note: required by `boundary_stepper_1d`

And that one actually fits on my screen again

It would be nice, if the compiler was using the same wording for those types as I usually would write down in code and only disambiguate, when necessary instead of using the fully qualified name. Do you think, it would be sensible to make the error messages more terse by leaving out the parts that the programmer does not care about? Or is there too much risk of not providing enough relevant information in other situations?

[GolDDranks]

I have been agonising over these same things lately. I think your points are spot on, and I have some of my own too. To collect them up:

1. Using non-fully qualified path names where the types are unambiguous makes the messages a LOT cleaner.
2. Using types as used in the codebase makes it easier to connect the dots. That means that if an alias is being used, use that alias.
3. Sometimes the error messages write associated types “over-carefully” open: <Type as Trait>::AssocType, where there is no ambiguity, and Type::AssocType would do. (Often it demands the author to write <Type as Trait>::AssocType to be sure, but sometimes not, and those times it shouldn’t use the verbose form in the error messages.) Edit: actually, it might be even better to demand the author to write the fully qualified trait path like it often does, since the code is more resistant to breakages that way, but when printing errors, it has the luxury of unambiguating only where needed, so it could be a lot more relaxed and not unambiguate so much.
4. Sometimes two types are defined to be equal using where clauses containing bounds on associated types like this: where Type: Trait<AssocType = AnotherType>. In this case, it might be sense to talk of the shorter and simpler AnotherType instead of <Type as Trait>::AssocType, but I wonder if some equalities might be counterintuitive instead; there might be tricky corner cases.
5. There should be better tooling for pretty-printing complex types, indented if needs to be. I’m not saying that should be the default, as it could be visually very distracting and take space, but it should be an option.

[jsgf]

@GolDDranks

There should be better tooling for pretty-printing complex types, indented if needs to be. I’m not saying that should be the default, as it could be visually very distracting and take space, but it should be an option.

As I've been working on async code which generates enormous types, I've been thinking that we really need some kind of interactive error explorer. For example, if the error is something along the lines of "expected type X, found type Y" - where X and Y are huge - then we'd want to at first focus on the parts of the types that are different, and then maybe work out from there to get more context.

Of course from there, what we'd really like to do is relate the types to the code. Why did the compiler infer the "expected" and "found" types - either (or both) could be wrong, so we need to try and work that out in context.

One way to produce this would be via RLS, but it would be also useful if a non-interactive compile (such as in a build farm) can produce enough info to allow the error to be interactively inspected later on.

[oli-obk]

Sounds like you want https://github.com/nrc/rustw

[aepsil0n]

Hm… I don’t know about this. I really like the immediacy of the command-line interface.

A web app feels like overkill just to understand an error message. After all, I’m complaining about this, because it takes me 30 s instead of 5 s to see the obvious issue, that is happening, as there is so much to read.

What I want, are not complex analysis tools, but a clear explanation of the root cause of the problem at a single glance. I know this not always possible to achieve. The best user experience is provided by an error message, when the smallest possible amount of time is spent until it’s gone and code actually does what I want it to.

[GolDDranks]

I agree; the error messages are the primary thing we should strive to improve. https://www.reddit.com/r/rust/comments/6zl1dn/improved_lifetime_error_messages_on_nightly/ is a prime example of this.

I think that interactive tooling is a nice bonus and definitely a valuable thing to have, but there is still low-hanging fruits in the compiler error messages.

[jsgf]

Thanks for the pointer! In-editor would be better, but this might be fun to play with.

[jsgf]

@GolDDranks

I think that interactive tooling is a nice bonus and definitely a valuable thing to have, but there is still low-hanging fruits in the compiler error messages.

I'm specifically thinking about type problems and type inference. The compiler is trying to prove consistency over a set of type rules. If it is consistent then everyone is happy, but if its inconsistent then it will just present the inconsistency it found - but that may not be the inconsistency that actually happened in your internal model of the system. It will report "expected X found Y", and you're immediately thinking something like:

• Why did the compiler expect X? That's not what I was expecting.
• Likewise Y?
• Where did it get X and Y from anyway, I was expecting Z?
• If the compiler has X and Y, where did they come from? What type does it think all the other expressions around here are?
• etc, etc

I'm not saying this is something that happens with every error message or even every type error. But I've found it's pretty common to get large complex errors and be stuck for some time trying to work out what on-line change is needed to make it all work. The only tools the language offers is being able to insert let x: MyType<_> = style bindings to pin down some of the type inference, which can be pretty helpful (but type ascription would be nicer).

[aepsil0n]

These issues tend to become more and more confusing the longer your function gets. So early refactoring helps, as it forces you to write down types to give the compiler some orientation

[jsgf]

Yep. But unfortunately that’s not possible in stable Rust without making semantic changes (ie, boxed trait objects all over the place). And error messages around impl Trait could definitely do with some improvement.

[ekuber]

@GolDDranks

1. Using non-fully qualified path names where the types are unambiguous makes the messages a LOT cleaner.
2. Using types as used in the codebase makes it easier to connect the dots. That means that if an alias is being used, use that alias.

I completely agree. This request has appeared multiple times, rightfully so. #21934 keeps track of the discussion around this. I can't promise anything in the short term, but I've been ruminating about how to implement this in such a way that will work with incremental compilation (the internal state of the compiler would allow you to keep a separate per scope mapping for local names, but this would depend on NodeIds which, as I understand it, change on subsequent compilations, so it is not suitable to do this).

@GolDDranks

3. Sometimes the error messages write associated types “over-carefully” open: <Type as Trait>::AssocType, where there is no ambiguity, and Type::AssocType would do. (Often it demands the author to write <Type as Trait>::AssocType to be sure, but sometimes not, and those times it shouldn’t use the verbose form in the error messages.) Edit: actually, it might be even better to demand the author to write the fully qualified trait path like it often does, since the code is more resistant to breakages that way, but when printing errors, it has the luxury of unambiguating only where needed, so it could be a lot more relaxed and not unambiguate so much.

This should be possible to do now with little effort. I can't come up with any objection to this, so I will look into writing mentoring instructions for it when I create this ticket.

@GolDDranks

5. There should be better tooling for pretty-printing complex types, indented if needs to be. I’m not saying that should be the default, as it could be visually very distracting and take space, but it should be an option.

Currently there's some type elision going on when possible (think Some(3) != Ok(3) => Option<_> != Result<_, _>, but there's plenty of improvement that can be done in this front. Some relevant tickets with plenty of background about some different alternatives: #21025 , #39906 , #43943 and #40186 .

@jsgf

Of course from there, what we’d really like to do is relate the types to the code. Why did the compiler infer the “expected” and “found” types - either (or both) could be wrong, so we need to try and work that out in context.

This is something I've looked into. The compiler would need to start tracking more information than it does now during typeck to keep the span for the first cause of the current type requirement. There is some tracking of this already, but it is lost in some boundaries. Having said that, the compiler should be pointing out with spans something that would read along the lines of expected X, found Y, because x was assigned foo() that returns type argument Z that impls Y but not X (contrived example). #14007 tracks this request.

@jsgf

And error messages around impl Trait could definitely do with some improvement.

I have filed #44530 to deal with the first issue you're likely to come across when trying impl Trait. Feel free to add comments and expand on desired output, as well as point out other cases that you would like addressed.

[aepsil0n]

@ekuber

I completely agree. This request has appeared multiple times, rightfully so. #21934 keeps track of the discussion around this. I can’t promise anything in the short term, but I’ve been ruminating about how to implement this in such a way that will work with incremental compilation (the internal state of the compiler would allow you to keep a separate per scope mapping for local names, but this would depend on NodeIds which, as I understand it, change on subsequent compilations, so it is not suitable to do this).

I would arguably go further than just using the names in scope. Due to type inference it happens quite a lot that you are using names that you are aware of, but have not pulled into scope, as it was not necessary to refer to them explicitly.

As long as a name is unambiguous in the context of the error message, it should be shortened imho. Perhaps add a note like "Add refers to std::ops::Add here" below the message, if it has not been imported, to make sure no useful information is lost.

[ekuber]

@aepsil0n

As long as a name is unambiguous in the context of the error message, it should be shortened imho. Perhaps add a note like “Add refers to std::ops::Add here” below the message, if it has not been imported, to make sure no useful information is lost.

That sounds like it might be reasonable, depending on the actual implementation.

---

One thing to add is probably that the json output should have enough type information so that even though the text contains Add, tools can have a way to refer to desugar that to std::ops::Add.

[ekuber]

A first real stab at referring to items with the local name instead of the fully qualified path: #44642

error[E0308]: mismatched types
 --> file.rs:7:24
  |
7 |     let y: Option<X> = Ok(());
  |                        ^^^^^^ expected enum `std::option::Option`, found enum `std::result::Result`
  |
  = note: expected type `Option<X>`
             found type `Result<(), _>`
  = help: here are some functions which might fulfill your needs:
          - .err()
          - .unwrap_err()