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

Binop type mismatch error could be better #38564

Closed
brson opened this issue Dec 23, 2016 · 15 comments
Closed

Binop type mismatch error could be better #38564

brson opened this issue Dec 23, 2016 · 15 comments
Labels
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.

Comments

@brson
Copy link
Contributor

brson commented Dec 23, 2016

From the new book.

let x: i8 = 5;
let y: Option<i8> = Some(5);

let sum = x + y;

If we run this code, we get an error message like this:

error[E0277]: the trait bound `i8: std::ops::Add<std::option::Option<i8>>` is not satisfied
 -->
  |
7 | let sum = x + y;
  |           ^^^^^
  |

This error could be a lot more newbie friendly. The next section of the book even has to apologize and explain:

"Intense! What this error message is trying to say is that Rust does not understand how to add an Option and an i8, since they're different types."

cc @jonathandturner @rust-lang/docs

@brson brson 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. T-tools labels Dec 23, 2016
@brson
Copy link
Contributor Author

brson commented Dec 23, 2016

On the other hand, one might argue that it's important for users to understand how to interpret this error. It seems like binops though could be special case to say "The operator + is not implemented for ...".

@sanmai-NL
Copy link

I also think unimplemented operators in general could be described more accessibly in the way you described @brson.

The error message you gave as an example would be optimal, I think, if it pointed out instead that it pertains to x, even though the whole context is x + y. I expect that a novice programmer will then understand the problem more readily.

@clarfonthey
Copy link
Contributor

clarfonthey commented Dec 23, 2016

I think that the general gist of the error is good, but it should add something mentioning that the reason why it needs this trait is because the function add is not implemented.

Perhaps we could explain this using the regular trait-not-found error and do something like:

"The compiler converts this operation into the form x.add(y), using the Add trait. Because this trait isn't implemented, the operation could not be performed."

That way, it tells users 1) what the compiler actually is doing with the operation and 2) doesn't add any special case for this error, just adds on information in addition to the trait error to help the user understand the conversion.

@killercup
Copy link
Member

killercup commented Dec 24, 2016

I think as a first step it would be helpful to add type annotations to x and y. For example:

error[E0277]: the trait bound `i8: std::ops::Add<std::option::Option<i8>>` is not satisfied
 -->
  |
7 | let sum = x + y;
  |           ^   - `std::option::Option<i8>`
  |           |   
  |          `i8`

This way, people can at least recognize the Option<i8> in the error message as the type of y. I think this might be helpful in other "trait bound not satisfied" cases as well.

For operators in particular, it might be nice to add a sentence saying + becomes std::ops::Add::add. Alternatively, add an annotation to the +, but I think this will make it very noisy.

@steveklabnik steveklabnik removed the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Mar 10, 2017
@steveklabnik steveklabnik added A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools and removed A-docs labels Mar 23, 2017
bors added a commit that referenced this issue Apr 10, 2017
Explicit help message for binop type mismatch

When trying to do `1 + Some(2)`, or some other binary operation on two
types different types without an appropriate trait implementation, provide
an explicit help message:

```rust
help: `{integer} + std::option::Option<{integer}>` has no implementation
```

Re: #39579, #38564, #37626, #39942, #34698.
@oli-obk
Copy link
Contributor

oli-obk commented May 8, 2017

This has now a

note: no implementation for i8 + std::option::Option<i8>

@Mark-Simulacrum
Copy link
Member

The new error I think closes this, since we note that i8 + std::option::Option<i8> isn't implemented directly.

rustc 1.19.0-dev (5dfcd85fd 2017-05-19)
error[E0277]: the trait bound `i8: std::ops::Add<std::option::Option<i8>>` is not satisfied
 --> test.rs:5:11
  |
5 | let sum = x + y;
  |           ^^^^^ the trait `std::ops::Add<std::option::Option<i8>>` is not implemented for `i8`
  |
  = note: no implementation for `i8 + std::option::Option<i8>`

error: aborting due to previous error

@sophiajt
Copy link
Contributor

@Mark-Simulacrum - not to be a spoil sport, but this error doesn't feel beginner friendly, yet, which was the original bug.

@sophiajt
Copy link
Contributor

sophiajt commented May 21, 2017

Specifically, a beginner friendly message probably doesn't talk about traits at all. Though we want users to understand traits as they grow in their knowledge of Rust, having to learn about them in your first few minutes of Rust is probably too much to ask.

We may want to simplify well-known traits (and their errors) to the point of being easier to read by new users. We can still leave a note to help more advanced folks, but something like this:

error[E0277]: Adding incompatible types
 -->
  |
7 | let sum = x + y;
  |           ^^^^^ can't add i8 and std::option::Option<i8>
  |
= note: trait `std::ops::Add<std::option::Option<i8>>` is not implemented for `i8`

aside: would be nice to get rid of all the std:: stuff, since well, it's std 😛

error[E0277]: Adding incompatible types
 -->
  |
7 | let sum = x + y;
  |           ^^^^^ can't add i8 and Option<i8>
  |
= note: trait `ops::Add<Option<i8>>` is not implemented for `i8`

(in this particular error, we can be even more specific, not mention traits, but instead point them at match statements or the like to get the i8 out of the Option)

@Mark-Simulacrum
Copy link
Member

Hm, yeah, true. Reopening. I worry about having this much of a special case, but I agree it's probably worth it.

@sophiajt
Copy link
Contributor

Yeah, definitely a good thing to worry about.

Just realised something. You actually can't do what the error note says in this message, right? Because of coherence you won't be able to implement that trait, if I understand correctly.

@Mark-Simulacrum
Copy link
Member

Not sure I follow you. I don't see anything about "maybe implement this" in the error. Perhaps we could detect the impossible case and special case it, though I worry that's overly ambitious -- technically, any two types that expose some form of mutable access to internals could be "added" through newtypes.

@sophiajt
Copy link
Contributor

sophiajt commented May 21, 2017

Heh, I've been doing Rust for a year and a half, and I'm still not 100% on exactly how the newtype trick is supposed to work. ;)

For me the main point is we should be cognisant of just how much someone needs to understand to get benefit from the error message. There's a big benefit to having simpler phrases like can't add i8 and Option<i8>.

@clarfonthey
Copy link
Contributor

I agree with swapping the note and annotation. Perhaps an extra note could just state "the value inside an Option can't be manipulated directly; did you mean to use map or expect"

@steveklabnik steveklabnik removed the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Aug 30, 2017
@estebank
Copy link
Contributor

Current output:

error[E0277]: cannot add `std::option::Option<i8>` to `i8`
 --> src/main.rs:5:13
  |
5 | let sum = x + y;
  |             ^ no implementation for `i8 + std::option::Option<i8>`
  |
  = help: the trait `std::ops::Add<std::option::Option<i8>>` is not implemented for `i8`

Further suggestions about using Option::map would be interesting to add.

The reverse doesn't trigger E0277, but E0369 instead:

error[E0369]: binary operation `+` cannot be applied to type `std::option::Option<i8>`
 --> src/main.rs:6:11
  |
6 | let sum = y + x;
  |           ^^^^^
  |
  = note: an implementation of `std::ops::Add` might be missing for `std::option::Option<i8>`

@estebank
Copy link
Contributor

estebank commented Apr 28, 2019

I'm going to close this issue as I see very little we could improve over the following, other than maybe unifying them both:

error[E0277]: cannot add `std::option::Option<i8>` to `i8`
 --> src/main.rs:5:13
  |
5 | let sum = x + y;
  |             ^ no implementation for `i8 + std::option::Option<i8>`
  |
  = help: the trait `std::ops::Add<std::option::Option<i8>>` is not implemented for `i8`
error[E0369]: binary operation `+` cannot be applied to type `std::option::Option<i8>`
 --> src/main.rs:5:13
  |
5 | let sum = y + x;
  |           - ^ - i8
  |           |
  |           std::option::Option<i8>
  |
  = note: an implementation of `std::ops::Add` might be missing for `std::option::Option<i8>`

Filed #60497 to track the wording change.

@estebank estebank closed this as completed May 3, 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. 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

10 participants