Better error message when trying to write default impls #49372

Phlosioneer · 2018-03-26T04:47:05Z

Previously, if you tried to write this (using the specialization
feature flag):

default impl PartialEq {
...
}

The compiler would give you the mysterious warning "inherent impls
cannot be default". What it really means is that you're trying to
write an impl for a Structure or Trait Object, and that cannot
be "default". However, one of the ways to encounter this error
(as shown by the above example) is when you forget to write "for
MyType".

This PR adds a help message that reads "maybe missing a for
keyword?" This is useful, actionable advice that will help any user
identify their mistake, and doesn't get in the way or mislead any
user that really meant to use the "default" keyword for this weird
purpose. In particular, this help message will be useful for any
users who don't know the "inherent impl" terminology, and/or users
who forget that inherent impls CAN be written for traits (they apply
to the trait objects). Both of these are somewhat confusing, seldom-
used concepts; a one-line error message without any error number for
longer explanation is NOT the place to introduce these ideas.

I wasn't quite sure what grammar / wording to use. I'm open to suggestions. CC @rust-lang/docs (I hope I'm doing that notation right)

(Apparently not. :( )

Previously, if you tried to write this (using the specialization feature flag): default impl PartialEq<MyType> { ... } The compiler would give you the mysterious warning "inherent impls cannot be default". What it really means is that you're trying to write an impl for a Structure or *Trait Object*, and that cannot be "default". However, one of the ways to encounter this error (as shown by the above example) is when you forget to write "for MyType". This PR adds a help message that reads "maybe missing a `for` keyword?" This is useful, actionable advice that will help any user identify their mistake, and doesn't get in the way or mislead any user that really meant to use the "default" keyword for this weird purpose. In particular, this help message will be useful for any users who don't know the "inherent impl" terminology, and/or users who forget that inherent impls CAN be written for traits (they apply to the trait objects). Both of these are somewhat confusing, seldom- used concepts; a one-line error message without any error number for longer explanation is NOT the place to introduce these ideas.

pietroalbini · 2018-03-26T11:01:10Z

Highfive failed to pick a reviewer for this PR.

r? @estebank

estebank

I would prefer a suggestion if it could be provided, with text close to

help: if you meant to write an `impl` for `PartialEq`, use the `for` keyword: `default impl for PartialEq`

(I hope I'm doing that notation right)

Only members of the org can CC teams.

CC @rust-lang/docs

I'm gonna be offline for two weeks. Apologies in advance for the unresponsiveness.

estebank · 2018-03-27T01:11:47Z

src/librustc_passes/ast_validation.rs

-                    self.err_handler().span_err(item.span, "inherent impls cannot be default");
+                    self.err_handler()
+                        .struct_span_err(item.span, "inherent impls cannot be default")
+                        .help("maybe a missing `for` keyword?");


This is creating a DiagnosticBuilder but never has .emit() called on it, which causes an Internal Compiler Error in test compile-fail/specialization/defaultimpl/validation.rs.

Assign the result of the struct_span_err to a mutable variable err and then err.help(...) and err.emit().

Oh, okay. Sorry, I'm not very familiar with internals. And there's no rush for feedback, thanks for the warning.

petrochenkov · 2018-03-27T08:12:07Z

one of the ways to encounter this error
(as shown by the above example) is when you forget to write "for
MyType".

And another way is to actually write default impl MyStruct<T> { ... } in attempt to use specialization on inherent methods.
You can't recommend for without checking that X in default impl X { ... } is actually a trait.

Phlosioneer · 2018-03-27T19:57:37Z

True... I don't know that we can do that here, though. Isn't this AST validation, before we've resolved trait names and the like?

Also, I don't know how to do that; I'm not very familiar with the rustc internals. Do you happen to know how to do that, or someplace I can start looking?

shepmaster · 2018-04-07T19:45:56Z

Since @estebank is out, randomly assigning to...

r? @nagisa

nagisa · 2018-04-08T18:19:46Z

Rather than trying to be helpful here and phrasing your suggestion as a question, I’d recommend using a note and informing the user that only trait implementations may be annotated with default or some similar wording.

pietroalbini · 2018-04-16T10:30:18Z

Ping from triage @Phlosioneer! The reviewer left some comments, could you address them?

pietroalbini · 2018-04-23T10:36:06Z

Ping from triage @nagisa! The author addressed your concerns, could you review this again?

nagisa · 2018-04-23T10:37:15Z

@bors r+

bors · 2018-04-23T10:37:16Z

📌 Commit c61e641 has been approved by nagisa

bors · 2018-04-23T10:40:15Z

⌛ Testing commit c61e641 with merge 4640615...

…e, r=nagisa Better error message when trying to write default impls Previously, if you tried to write this (using the specialization feature flag): default impl PartialEq<MyType> { ... } The compiler would give you the mysterious warning "inherent impls cannot be default". What it really means is that you're trying to write an impl for a Structure or *Trait Object*, and that cannot be "default". However, one of the ways to encounter this error (as shown by the above example) is when you forget to write "for MyType". This PR adds a help message that reads "maybe missing a `for` keyword?" This is useful, actionable advice that will help any user identify their mistake, and doesn't get in the way or mislead any user that really meant to use the "default" keyword for this weird purpose. In particular, this help message will be useful for any users who don't know the "inherent impl" terminology, and/or users who forget that inherent impls CAN be written for traits (they apply to the trait objects). Both of these are somewhat confusing, seldom- used concepts; a one-line error message without any error number for longer explanation is NOT the place to introduce these ideas. I wasn't quite sure what grammar / wording to use. I'm open to suggestions. CC @rust-lang/docs (I hope I'm doing that notation right) (Apparently not. :( )

bors · 2018-04-23T13:07:31Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 4640615 to master...

pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 26, 2018

rust-highfive assigned estebank Mar 26, 2018

estebank requested changes Mar 27, 2018

View reviewed changes

Add emit call to error message

686682d

rust-highfive assigned nagisa and unassigned estebank Apr 7, 2018

pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2018

Changed help message to note

c61e641

pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2018

bors merged commit c61e641 into rust-lang:master Apr 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error message when trying to write default impls #49372

Better error message when trying to write default impls #49372

Phlosioneer commented Mar 26, 2018 •

edited

Loading

pietroalbini commented Mar 26, 2018

estebank left a comment

estebank Mar 27, 2018

Phlosioneer Mar 27, 2018

petrochenkov commented Mar 27, 2018

Phlosioneer commented Mar 27, 2018

shepmaster commented Apr 7, 2018

nagisa commented Apr 8, 2018

pietroalbini commented Apr 16, 2018

pietroalbini commented Apr 23, 2018

nagisa commented Apr 23, 2018

bors commented Apr 23, 2018

bors commented Apr 23, 2018

bors commented Apr 23, 2018

Better error message when trying to write default impls #49372

Better error message when trying to write default impls #49372

Conversation

Phlosioneer commented Mar 26, 2018 • edited Loading

pietroalbini commented Mar 26, 2018

estebank left a comment

Choose a reason for hiding this comment

estebank Mar 27, 2018

Choose a reason for hiding this comment

Phlosioneer Mar 27, 2018

Choose a reason for hiding this comment

petrochenkov commented Mar 27, 2018

Phlosioneer commented Mar 27, 2018

shepmaster commented Apr 7, 2018

nagisa commented Apr 8, 2018

pietroalbini commented Apr 16, 2018

pietroalbini commented Apr 23, 2018

nagisa commented Apr 23, 2018

bors commented Apr 23, 2018

bors commented Apr 23, 2018

bors commented Apr 23, 2018

Phlosioneer commented Mar 26, 2018 •

edited

Loading