Explicit help message for binop type mismatch #40565

estebank · 2017-03-16T02:23:16Z

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:

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

Re: #39579, #38564, #37626, #39942, #34698.

rust-highfive · 2017-03-16T02:23:28Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

estebank · 2017-03-16T04:23:05Z

CCing the people involved in the referenced tickets: @beamspease, @abonander, @brson, @killercup, @oli-obk, @frewsxcv

Could I get some feedback on the current output of this PR?

abonander · 2017-03-16T04:47:49Z

I like it. What does it print for PartialOrd operators? Same as for PartialEq?

This isn't specific to this PR, I know, but I kind of wish that error messages didn't use full paths for types, because that's just noise. I would like to see error messages use type names based on how they're imported, but resolving that is nontrivial.

estebank · 2017-03-16T05:08:36Z

What does it print for PartialOrd operators?

@abonander, at the moment it's keeping the current output for PartialOrd, << and >>.

oli-obk · 2017-03-16T05:35:55Z

src/librustc/traits/error_reporting.rs

+                                                  op,
+                                                  trait_ref.substs()[1]));
+                            } else if &trait_def == "std::cmp::PartialEq" {
+                                err.help(&format!("can't compare `{}` against `{:?}`",


Both could be Display format specifiers

Kind doesn't implement Display and using Ty instead would not work for cases with impl.

"with" instead of "against"

oli-obk · 2017-03-16T05:37:50Z

src/librustc/traits/error_reporting.rs

-                            // similar impls.
-                            let impl_candidates = self.find_similar_impl_candidates(trait_ref);
-                            self.report_similar_impl_candidates(impl_candidates, &mut err);
+                            let binops: FxHashMap<&str, &str> = [


This won't work on no_std.

Could you clarify what wouldn't work?

In a crate with no_std, these path start with core

Added core::* versions to the list. Feels hacky...

Use lang items please - tcx.lang_items.add_trait etc. - instead of guessing stuff.

oli-obk · 2017-03-16T05:51:35Z

src/test/ui/span/multiline-span-simple.stderr

-             <&'a u32 as std::ops::Add<u32>>
-             <u32 as std::ops::Add<&'a u32>>
-             <&'b u32 as std::ops::Add<&'a u32>>
+   = help: `u32 + ()` has no implementation


The suggestions should still show up. Maybe even in the Format oft this PR?

I felt that for these cases in particular, in order to keep it newbie friendly, it was better to simplify the text in order to make the error less daunting.

In case of &a + a I think it would be very helpful to see the hints

Fair enough. Should I only show them in the &a + a case then?

Would be enough for the cases I've run into with ppl I've taught rust

oli-obk · 2017-03-16T05:54:53Z

src/test/ui/mismatched_types/binops.stderr

+13 |     1 as usize - Some(1);
+   |     ^^^^^^^^^^^^^^^^^^^^ the trait `std::ops::Sub<std::option::Option<{integer}>>` is not implemented for `usize`
+   |
+   = help: `usize - std::option::Option<{integer}>` has no implementation


The note is already duplicating the main error message. You could remove the note and make your help a note

It makes sense. Something along the lines of this?:

error[E0277]: the trait bound `usize: std::ops::Sub<std::option::Option<{integer}>>` is not satisfied --> $DIR/binops.rs:13:5 | 13 | 1 as usize - Some(1); | ^^^^^^^^^^^^^^^^^^^^ no implementation for `usize - std::option::Option<{integer}>`

nagisa · 2017-03-16T07:02:26Z

Message like this can be introduced without changing the compiler by adding a #[on_unimplemented] attribute onto the traits.

estebank · 2017-03-16T20:22:41Z

Message like this can be introduced without changing the compiler by adding a #[on_unimplemented] attribute onto the traits.

That wouldn't let me remove the "alternative implementations" recomendation or replace the span label.

estebank · 2017-03-22T03:39:55Z

Current output:

beamspease · 2017-03-22T05:30:51Z

Very nice

estebank · 2017-04-03T23:04:14Z

Ping.

CC @jonathandturner, @GuillaumeGomez, @nikomatsakis

sophiajt · 2017-04-03T23:10:03Z

Oh I'm glad you're tackling this one!

Can you add tests for a couple more operators? I like the way it looks, but just wanted to make sure other operators looked like it seems like they should.

GuillaumeGomez · 2017-04-04T09:27:47Z

Awesome! :D

arielb1 · 2017-04-04T19:39:32Z

@estebank

What's the problem with using #[rustc_on_unimplemented] again? That it does not affect the primary label? Maybe rustc_on_unimplemented should be a label instead of being a note?

I don't see why the rendering here should be different then for, e.g., Sized.

estebank · 2017-04-04T22:57:11Z

@arielb1, I really want to remove the suggestions because for these cases it is more confusing than helpful:

error[E0277]: the trait bound `{integer}: std::ops::Add<std::option::Option<{integer}>>` is not satisfied
 --> <anon>:3:5
  |
3 |     1 + Some(1);
  |     ^^^^^^^^^^^ the trait `std::ops::Add<std::option::Option<{integer}>>` is not implemented for `{integer}`
  |
  = help: the following implementations were found:
            <std::borrow::Cow<'a, str> as std::ops::Add<&'a str>>
            <std::borrow::Cow<'a, str> as std::ops::Add>
            <u32 as std::ops::Add>
            <&'a u32 as std::ops::Add<u32>>
          and 106 others

error[E0277]: the trait bound `usize: std::ops::Sub<std::option::Option<{integer}>>` is not satisfied
 --> <anon>:4:5
  |
4 |     1 as usize - Some(1);
  |     ^^^^^^^^^^^^^^^^^^^^ the trait `std::ops::Sub<std::option::Option<{integer}>>` is not implemented for `usize`
  |
  = help: the following implementations were found:
            <usize as std::ops::Sub>
            <&'a usize as std::ops::Sub<usize>>
            <usize as std::ops::Sub<&'a usize>>
            <&'b usize as std::ops::Sub<&'a usize>>

error[E0277]: the trait bound `{integer}: std::cmp::PartialEq<std::result::Result<{integer}, _>>` is not satisfied
 --> <anon>:5:5
  |
5 |     1 == Ok(1);
  |     ^^^^^^^^^^ the trait `std::cmp::PartialEq<std::result::Result<{integer}, _>>` is not implemented for `{integer}`
  |
  = help: the following implementations were found:
            <&'a A as std::cmp::PartialEq<&'b B>>
            <&'a mut A as std::cmp::PartialEq<&'b mut B>>
            <&'a A as std::cmp::PartialEq<&'b mut B>>
            <&'a mut A as std::cmp::PartialEq<&'b B>>
          and 697 others

vs

error[E0277]: the trait bound `{integer}: std::ops::Add<std::option::Option<{integer}>>` is not satisfied
  --> $DIR/binops.rs:12:5
   |
12 |     1 + Some(1);
   |     ^^^^^^^^^^^ no implementation for `{integer} + std::option::Option<{integer}>`

error[E0277]: the trait bound `usize: std::ops::Sub<std::option::Option<{integer}>>` is not satisfied
  --> $DIR/binops.rs:13:5
   |
13 |     1 as usize - Some(1);
   |     ^^^^^^^^^^^^^^^^^^^^ no implementation for `usize - std::option::Option<{integer}>`

error[E0277]: the trait bound `{integer}: std::cmp::PartialEq<std::result::Result<{integer}, _>>` is not satisfied
  --> $DIR/binops.rs:14:5
   |
14 |     1 == Ok(1);
   |     ^^^^^^^^^^ can't compare `{integer}` with `std::result::Result<{integer}, _>`

Using rustc_on_unimplemented the output would look this way:

error[E0277]: the trait bound `{integer}: std::ops::Add<std::option::Option<{integer}>>` is not satisfied
 --> <anon>:3:5
  |
3 |     1 + Some(1);
  |     ^^^^^^^^^^^ the trait `std::ops::Add<std::option::Option<{integer}>>` is not implemented for `{integer}`
  |
  = help: the following implementations were found:
            <std::borrow::Cow<'a, str> as std::ops::Add<&'a str>>
            <std::borrow::Cow<'a, str> as std::ops::Add>
            <u32 as std::ops::Add>
            <&'a u32 as std::ops::Add<u32>>
          and 106 others
  = note: no implementation for `{integer} + std::option::Option<{integer}>`

error[E0277]: the trait bound `usize: std::ops::Sub<std::option::Option<{integer}>>` is not satisfied
 --> <anon>:4:5
  |
4 |     1 as usize - Some(1);
  |     ^^^^^^^^^^^^^^^^^^^^ the trait `std::ops::Sub<std::option::Option<{integer}>>` is not implemented for `usize`
  |
  = help: the following implementations were found:
            <usize as std::ops::Sub>
            <&'a usize as std::ops::Sub<usize>>
            <usize as std::ops::Sub<&'a usize>>
            <&'b usize as std::ops::Sub<&'a usize>>
  = note: no implementation for `usize + std::option::Option<{integer}>`

error[E0277]: the trait bound `{integer}: std::cmp::PartialEq<std::result::Result<{integer}, _>>` is not satisfied
 --> <anon>:5:5
  |
5 |     1 == Ok(1);
  |     ^^^^^^^^^^ the trait `std::cmp::PartialEq<std::result::Result<{integer}, _>>` is not implemented for `{integer}`
  |
  = help: the following implementations were found:
            <&'a A as std::cmp::PartialEq<&'b B>>
            <&'a mut A as std::cmp::PartialEq<&'b mut B>>
            <&'a A as std::cmp::PartialEq<&'b mut B>>
            <&'a mut A as std::cmp::PartialEq<&'b B>>
          and 697 others
  = note: no implementation for `{integer} + std::cmp::Result<{integer}, _>`

which I feel is not approachable enough.

arielb1 · 2017-04-06T14:32:59Z

@estebank

But if you use rustc_on_unimplemented, the "following implementations were found" message is suppressed.

estebank · 2017-04-07T14:44:22Z

You're right, of course. I don't think there's a way to get the types involved on rustc_on_unimplemented, though, to accomplish the same kind of message proposed on this PR.

arielb1 · 2017-04-08T12:55:11Z

@estebank

What's the problem with adding something like:

#[rustc_on_unimplemented = "no implementation for `{Self} + {RHS}`"]

If there is something missing, can it be added to rustc_on_unimplemented? It might be useful to other people too.

estebank · 2017-04-09T21:18:55Z

@arielb1 I'm so sorry, I only now see what you mean. I had completely missed that this attribute takes arguments. Then, the difference between this PR and your suggestion is much smaller than I thought:

error[E0277]: the trait bound `{integer}: std::ops::Add<std::option::Option<{integer}>>` is not satisfied
  --> $DIR/binops.rs:12:5
   |
12 |     1 + Some(1);
   |     ^^^^^^^^^^^ no implementation for `{integer} + std::option::Option<{integer}>`

error[E0277]: the trait bound `usize: std::ops::Sub<std::option::Option<{integer}>>` is not satisfied
  --> $DIR/binops.rs:13:5
   |
13 |     1 as usize - Some(1);
   |     ^^^^^^^^^^^^^^^^^^^^ no implementation for `usize - std::option::Option<{integer}>`

error[E0277]: the trait bound `{integer}: std::cmp::PartialEq<std::result::Result<{integer}, _>>` is not satisfied
  --> $DIR/binops.rs:14:5
   |
14 |     1 == Ok(1);
   |     ^^^^^^^^^^ can't compare `{integer}` with `std::result::Result<{integer}, _>`

vs

error[E0277]: the trait bound `{integer}: std::ops::Add<std::option::Option<{integer}>>` is not satisfied
 --> <anon>:3:5
  |
3 |     1 + Some(1);
  |     ^^^^^^^^^^^ the trait `std::ops::Add<std::option::Option<{integer}>>` is not implemented for `{integer}`
  |
  = note: no implementation for `{integer} + std::option::Option<{integer}>`

error[E0277]: the trait bound `usize: std::ops::Sub<std::option::Option<{integer}>>` is not satisfied
 --> <anon>:4:5
  |
4 |     1 as usize - Some(1);
  |     ^^^^^^^^^^^^^^^^^^^^ the trait `std::ops::Sub<std::option::Option<{integer}>>` is not implemented for `usize`
  |
  = note: no implementation for `usize + std::option::Option<{integer}>`

error[E0277]: the trait bound `{integer}: std::cmp::PartialEq<std::result::Result<{integer}, _>>` is not satisfied
 --> <anon>:5:5
  |
5 |     1 == Ok(1);
  |     ^^^^^^^^^^ the trait `std::cmp::PartialEq<std::result::Result<{integer}, _>>` is not implemented for `{integer}`
  |
  = note: can't compare `{integer}` with `std::result::Result<{integer}, _>`

I still believe the first version is nicer for newcomers, but the second option is already a huge improvement over the status quo. I'll be updating this PR to use rustc_on_unimplemented instead. Sorry for the misunderstanding on my part.

arielb1 · 2017-04-10T13:22:02Z

src/librustc/traits/error_reporting.rs

@@ -487,15 +487,15 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                                  obligation: &PredicateObligation<'tcx>,
                                  error: &SelectionError<'tcx>)
    {
-        let span = obligation.cause.span;
+        let sp = obligation.cause.span;


Could you change this back to span and squash?

arielb1 · 2017-04-10T13:24:18Z

src/librustc/traits/error_reporting.rs

@@ -544,25 +537,28 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                            // which is somewhat confusing.
                            err.help(&format!("consider adding a `where {}` bound",
                                                trait_ref.to_predicate()));
-                        } else if let Some(s) = self.on_unimplemented_note(trait_ref,
-                                                                            obligation) {
+                        } else if let Some(s) = self.on_unimplemented_note(trait_ref, obligation) {
                            // If it has a custom "#[rustc_on_unimplemented]"
                            // error message, let's display it!
                            err.note(&s);


We can change all on_unimplemented messages to be labels by making this a .span_label call. Not sure it's a good idea.

Probably a bad idea to do that for all cases, but I've been considering the possibility of having a way to configure wether it is shown as a note or as the span's label. I'm thinking that it could either accomplished by changing the parsing of attributes to accept extra arguments, or simply have it change behavior when the rustc_on_unimplemented text starts with label: . It might be too more magic than I'm comfortable with, though.

arielb1 · 2017-04-10T13:24:29Z

r+ modulo squash.

estebank · 2017-04-10T21:25:39Z

@bors r=arielb1

@arielb1 squashed into single commit.

bors · 2017-04-10T21:25:40Z

📌 Commit 9aa75a8 has been approved by arielb1

estebank · 2017-04-10T21:26:35Z

@bors r-

arielb1 · 2017-04-10T21:28:34Z

@bors r+

bors · 2017-04-10T21:28:35Z

📌 Commit 9aa75a8 has been approved by arielb1

When trying to do a binary operation with missing implementation, for example `1 + Some(2)`, provide an explicit help message: ``` note: no implementation for `{integer} + std::option::Option<{integer}>` ``` Use `rustc_on_unimplemented` for the suggestions. Move cfail test to ui.

estebank · 2017-04-10T21:29:41Z

@bors r=arielb1

hadn't seen the comment about sp -> span, changed.

bors · 2017-04-10T21:29:42Z

📌 Commit be8787d has been approved by arielb1

bors · 2017-04-10T21:56:18Z

⌛ Testing commit be8787d with merge 730e5ad...

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.

bors · 2017-04-11T01:25:22Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 730e5ad to master...

rust-highfive assigned pnkfelix Mar 16, 2017

estebank force-pushed the binops-help branch 3 times, most recently from c8cea56 to f8e0cc5 Compare March 16, 2017 04:18

oli-obk reviewed Mar 16, 2017

View reviewed changes

estebank force-pushed the binops-help branch from 2c1da95 to b2013d2 Compare March 16, 2017 16:00

estebank changed the title ~~Explicit help message for binop type missmatch~~ Explicit help message for binop type mismatch Mar 21, 2017

estebank mentioned this pull request Mar 21, 2017

Better error messages for &T == T #40660

Closed

estebank force-pushed the binops-help branch from 31cfe47 to 73abfb2 Compare April 7, 2017 14:40

estebank force-pushed the binops-help branch from 73abfb2 to 90f3997 Compare April 7, 2017 21:57

estebank force-pushed the binops-help branch 2 times, most recently from 4aab465 to 23af7b4 Compare April 10, 2017 10:02

arielb1 reviewed Apr 10, 2017

View reviewed changes

estebank force-pushed the binops-help branch from 23af7b4 to 9aa75a8 Compare April 10, 2017 21:21

estebank force-pushed the binops-help branch from 9aa75a8 to be8787d Compare April 10, 2017 21:28

bors merged commit be8787d into rust-lang:master Apr 11, 2017

estebank mentioned this pull request May 2, 2017

Add better error message when == operator is badly used #41559

Merged

estebank deleted the binops-help branch November 9, 2023 05:26

Explicit help message for binop type mismatch #40565

Explicit help message for binop type mismatch #40565

Conversation

estebank commented Mar 16, 2017 • edited Loading

rust-highfive commented Mar 16, 2017

estebank commented Mar 16, 2017

abonander commented Mar 16, 2017 • edited Loading

estebank commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa commented Mar 16, 2017

estebank commented Mar 16, 2017

estebank commented Mar 22, 2017

beamspease commented Mar 22, 2017

estebank commented Apr 3, 2017

sophiajt commented Apr 3, 2017

GuillaumeGomez commented Apr 4, 2017

arielb1 commented Apr 4, 2017 • edited Loading

estebank commented Apr 4, 2017 • edited Loading

arielb1 commented Apr 6, 2017

estebank commented Apr 7, 2017

arielb1 commented Apr 8, 2017 • edited Loading

estebank commented Apr 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Apr 10, 2017

estebank commented Apr 10, 2017

bors commented Apr 10, 2017

estebank commented Apr 10, 2017

arielb1 commented Apr 10, 2017

bors commented Apr 10, 2017

estebank commented Apr 10, 2017

bors commented Apr 10, 2017

bors commented Apr 10, 2017

bors commented Apr 11, 2017

estebank commented Mar 16, 2017 •

edited

Loading

abonander commented Mar 16, 2017 •

edited

Loading

arielb1 commented Apr 4, 2017 •

edited

Loading

estebank commented Apr 4, 2017 •

edited

Loading

arielb1 commented Apr 8, 2017 •

edited

Loading

estebank commented Apr 9, 2017 •

edited

Loading