-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Include type of missing trait methods in error #36371
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
cc0c258
to
d53289f
Compare
@@ -62,6 +62,27 @@ pub enum Node<'ast> { | |||
NodeTyParam(&'ast TyParam) | |||
} | |||
|
|||
impl<'ast> Node<'ast> { |
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.
Add spans to everything?
Hmm, while it is nice to see the type, I'm not sure if this change is quite handling it right. The error messages themselves look awfully busy to me now, but maybe colors would help there. But moreover the types are using our internal type-printing, which is probably not what you would want to use when defining the method. To pick one example:
you would probably want:
but this will also depend on what imports you have around (e.g., is I'm not sure what's the best way to handle this, unfortunately. :( |
@estebank can you maybe give the current messages too, just for comparison purposes? |
For the first case: error: main function not found
error[E0046]: not all trait items implemented, missing: `fmt`
--> file4.rs:17:1
|
17 | impl std::fmt::Display for A {
| ^ missing `fmt` in implementation
error[E0046]: not all trait items implemented, missing: `Err`, `from_str`
--> file4.rs:19:1
|
19 | impl FromStr for A{}
| ^^^^^^^^^^^^^^^^^^^^ missing `Err`, `from_str` in implementation
error[E0046]: not all trait items implemented, missing: `Foo`, `foo`, `bar`, `bay`
--> file4.rs:21:1
|
21 | impl X<usize> for A {
| ^ missing `Foo`, `foo`, `bar`, `bay` in implementation
error: aborting due to 3 previous errors and for the second case: error: main function not found
error[E0186]: method `fmt` has a `&self` declaration in the trait, but not in the impl
--> file5.rs:4:5
|
4 | fn fmt() -> () {}
| ^^^^^^^^^^^^^^^^^ expected `&self` in impl
error: aborting due to previous error I agree with the original tickets that showing the signature for the missing implementations is very much needed, otherwise you must go around fishing around the docs.
Neither am I. Looking at the current context to identify that for example I could rework this so as to only show the signature for methods for which we have spans for now. |
@estebank - there has been some recent work on E0186 which I think helps some. Here's one example:
It's similar in spirit, I think, to what @nikomatsakis is saying about using the simpler type if we can. We may want to do something similar for E0046 as well. For example, instead of:
have:
|
Ooh, I like this! It gives you the stuff to copy-and-paste while making it clear where it comes from. (Of course, it won't work across crates.) |
@nikomatsakis - interesting point about working across crates. I'm curious what we might able to print instead. I'm guessing we have the trait information sitting in the rlib since any consumer of a crate can implement a trait with their own types. Maybe there's a way to print it back out again. @alexcrichton - do you know if ^ is possible? |
@jonathandturner I know rustdoc at least reconstructs information like trait signatures from metadata. I don't believe it's lossless though as I think there's at least a bug with lifetime parameters. For simple cases though if rustdoc can recreate a trait signature then surely the compiler error messages can! |
I like the idea of showing the annotated span for the definitions that are not from a different crate, but I'd like to show a reconstructed signature for other crates' definitions. I took a look the code that formats method declarations for rustdoc, and it can't be used directly in the compiler, as it works on rustdoc's own AST and is tied to HTML output only. I feel like I could move the presentation logic up onto the compiler, but as |
I've been looking at the Is there any way at this time to display multiline spans? If not, do we want to add it? Otherwise we end up with a pretty substandard note: note: missing trait item definition
--> missing-impls.rs:10:5
|
10 | fn bar();
| ^^^^^^^^^ `bar` from trait
note: missing trait item definition
--> missing-impls.rs:11:5
|
11 | fn bay<
| ^ `bay` from trait
|
@estebank - there currently isn't a way to display multispans as a "single thing" in the error message, though it does use multispans to draw the error display. What specifically are you trying to do? |
@jonathandturner I feel that showing the complete original span for missing items would help particularly well for the most problematic of the method definitions, those too long to be readable without formatting. By showing the entire multiline span you're taking advantage of the original definition's formatting, which we can assume to be readable most of the time:
Also, if adding a multiline span message is desirable, a presentation like the following might be desirable:
|
I agree that this looks good in this case. I'm not sure how well it composes with errors that have other spans in them as well though... we've been striving for just one snippet, with labels collected together, versus the way your examples are formatted, where there are two independent snippets, right? Maybe that goal doesn't make sense here though. |
@nikomatsakis - do you know if there is a way to pretty print this so that we don't have all the whitespace and linebreaks? |
For a given file `foo.rs`: ```rust use std::str::FromStr; struct A {} trait X<T> { type Foo; const BAR: u32 = 128; fn foo() -> T; fn bar(); fn bay< 'lifetime, TypeParameterA >( a : usize, b: u8 ); } impl std::fmt::Display for A { } impl FromStr for A{} impl X<usize> for A { } ``` Provide the following output: ```bash error: main function not found error[E0046]: not all trait items implemented, missing: `fmt` --> foo.rs:18:1 | 18 | impl std::fmt::Display for A { | ^ missing `fmt` in implementation | = note: fn fmt(&Self, &mut std::fmt::Formatter<'_>) -> std::result::Result<(), std::fmt::Error>; error[E0046]: not all trait items implemented, missing: `Err`, `from_str` --> foo.rs:20:1 | 20 | impl FromStr for A{} | ^^^^^^^^^^^^^^^^^^^^ missing `Err`, `from_str` in implementation | = note: type Err; = note: fn from_str(&str) -> std::result::Result<Self, <Self as std::str::FromStr>::Err>; error[E0046]: not all trait items implemented, missing: `Foo`, `foo`, `bar`, `bay` --> foo.rs:22:1 | 22 | impl X<usize> for A { | ^ missing `Foo`, `foo`, `bar`, `bay` in implementation | = note: type Foo; = note: fn foo() -> T; = note: fn bar(); = note: fn bay<'lifetime, TypeParameterA>(a: usize, b: u8); error: aborting due to 3 previous errors ``` Fixes rust-lang#24626 For a given file `foo.rs`: ```rust struct A {} impl std::fmt::Display for A { fn fmt() -> () {} } ``` provide the expected method signature: ```bash error: main function not found error[E0186]: method `fmt` has a `&self` declaration in the trait, but not in the impl --> foo.rs:4:5 | 4 | fn fmt() -> () {} | ^^^^^^^^^^^^^^^^^ expected `&self` in impl | = note: Expected signature: fn fmt(&Self, &mut std::fmt::Formatter<'_>) -> std::result::Result<(), std::fmt::Error>; = note: Found signature: fn fmt(); error: aborting due to previous error ``` Fixes rust-lang#28011
I've been messing around to see what it would look like and although it is far from perfect, I think it is in the right direction: Here there're three different The full multiline support was relatively easy to add, while the addition for them will take some extra work. I'm mildly worried about this change since every place that expects multiline spans to be shown as just the first line will now show the entire span (which could be huge) and would need to be changed to check wether it is multiline and in that case only use the first char of the first line. When I looked a couple of weeks back for user facing type printing, I couldn't find any, but I'll look again. |
@nikomatsakis I've been thinking about how it would be best to present annotated multiline spans in a composable way, and visually I arrived on:
If you think that there're other places where it'd be beneficial to show labeled multiline spans, I'll go ahead and implement this. The other concern I have is who should be deciding wether to show the span. The way I see it there're two options:
Thoughts? |
Since we're recreating the type signature, instead of printing this:
Can we print this?
|
I'm confused, I thought the point was that we wouldn't be recreating the signature? But rather highlighting the original text? (In general, I agree that if we can print original text, it's a better thing to do.) |
I think that if we get some attractive way to highlight multiline spans, that's probably ok, though we do (I think) in general want to be avoiding those in favor of highlighting some representative thing. The use of the first character was seen as a reasonable stopgap while we went and tried to improve the code not to give multiline spans: but there hasn't been much progress on that and it's hard in general. :( FWIW, the idea of moving into a "bracket" on the RHS seems visually nice, though it of course means we can't clearly indicate where the bracket begins/ends in the line. For a method definition, that's fine of course. For other things, it's less good. Consider something like: foo(1 + bar(x,
y),
z); where we want to highlight the
This can of course be ambiguous if the extent of the expression is unclear, and the number of complaints makes it clear that this can be confusing in practice. Putting it on the side might look like this. I am avoiding unicode because many terminals can't handle it, so in the past we've tried to stick to ASCII.
It seems clear that this too is ambiguous and potentially confusing, particularly absent colors. In the past (in LALRPOP) I tried something like:
but I think that is really confusing too =). Your idea of adding in the
thoughts? |
☔ The latest upstream changes (presumably #37269) made this pull request unmergeable. Please resolve the merge conflicts. |
I'm a bit confused as to the status of this PR. It seems like we're still doing a lot of active debate about what it should do -- I'm inclined to say let's close it and we can move this conversation maybe into an issue, or internals, or something? (Right now it shows up each time I sweep my assigned PRs...) |
I'm okay with the design moving to an issue |
Sorry for being unresponsive, I've been away offline for the past two weeks. I've created https://internals.rust-lang.org/t/proposal-for-multiline-span-comments/4242 for continuing this conversation. Feel free to close the PR. I will continue working regardless, and will create a new PR once a consensus arrises in internals. |
Just one thing, currently what would be displayed is just
|
…-back, r=nikomatsakis Include type of missing trait methods in error Provide either a span pointing to the original definition of missing trait items, or a message with the inferred definitions. Fixes rust-lang#24626. Follow up to PR rust-lang#36371. If PR rust-lang#37369 lands, missing trait items that present a multiline span will be able to show the entirety of the item definition on the error itself, instead of just the first line.
For a given file
foo.rs
:Provide the following output:
Fixes #24626
For a given file
foo.rs
:provide the expected method signature:
Fixes #28011