-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add explanation to type mismatch involving type params and assoc types #63907
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
@rust-lang/wg-diagnostics can I get some eyeballs on this? I would like feedback on the copy, verbosity and ideas. |
- change `foo` to return an argument of type `T`: | ||
``` | ||
impl<T> Trait<T> for X { | ||
fn foo(&self, x: T) -> T { x } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one confused me since it hinges on changes to the trait def as well as the impl
src/librustc/ty/error.rs
Outdated
db.note("you might be missing a type parameter or trait bound"); | ||
} | ||
(ty::Param(_), _) | (_, ty::Param(_)) => { | ||
db.help("given a type parameter `T` and a method `foo`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we gate the long explanations (both of them) under the teaching flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, but I'm getting more and more convinced that --teach
will not be as useful as focusing on improving the existing default errors. I would like help editing and pruning down the error. We could even give this case a different error code to have somewhere to put these explanations. Then we could have --teach
just embed the error code text below the error when they are flagged as not having special support for it... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just think we need to keep in mind folks who have already learned the concept and don't want to be spammed down with very long messages (which are useful to beginners but not familiar folks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
I think it would be better if we just had a section in the book that we could link to explaining this case. Having at most two lines, including link, of text should be reasonable (and is my unofficial cutoff for these kind of messages).
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sgtm
I'll be getting back to this over the weekend. |
b42c4bf
to
479ce39
Compare
@bors r+ |
📌 Commit 479ce39 has been approved by |
…-obk Add explanation to type mismatch involving type params and assoc types CC rust-lang#63711
Rollup of 9 pull requests Successful merges: - #63907 (Add explanation to type mismatch involving type params and assoc types) - #64615 (rustbuild: Turn down compression on exe installers) - #64617 (rustbuild: Turn down compression on msi installers) - #64618 (rustbuild: Improve output of `dist` step) - #64619 (Fixes #63962. Hint about missing tuple parentheses in patterns) - #64634 (Update to LLVM 9.0.0) - #64635 (Allow using fn pointers in const fn with unleash miri) - #64660 (unify errors for tuple/struct variants) - #64664 (fully remove AstBuilder) Failed merges: r? @ghost
CC #63711