-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
impl Trait diagnostic/test cleanups #50943
Conversation
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
O_o |
Ah no. This is because it's a path into the libstd. |
I adjusted compiletest to also normalize paths into the libstd/libcore |
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.
Love it!
| ^^^^^^^^^^ expected generic parameter, found `impl Trait` | ||
help: try changing the `impl Trait` argument to a generic parameter | ||
| | ||
LL | fn bar<U: Debug><U: Debug>(&self, _: &U); |
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.
Incorrect span for the suggestion. This should be in the bar
for the impl Bar for ()
let new_generics_span = tcx | ||
.sess | ||
.codemap() | ||
.generate_fn_name_span(impl_m.span)? |
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.
Use impl_span
here instead and the output will be correct.
This is because generate_fn_name_span
looks backwards until it finds fn
in the span, instead of checking wether the span starts with fn<WS>
. When using impl_m.span
, this starts in the fn...
, so it goes backwards until it finds the previous piece of code that contains fn
, which in this case is the trait's method. Using impl_span
will break if any part of the snippet contains fn
(like a method called my_fn
).
In order to avoid this there are two options: changing generate_fn_name_span
to peek at the first three chars of the passed span's snippet to see if the first two are fn
and the third is whitespace, and if so assign span
to prev_span
and use _impl_m_span
here instead, or synthesize a new span derived from _impl_m_span
moving forward long enough to leave the fn<WS>
outside of the new span. Either way, it would have to account for the potential leading whitespace and visibility (pub(crate)
, etc) text.
We really need to add spans to all idents, IMO. We keep having to do this kind of brittle span wrangling way too often.
📌 Commit 849c565 has been approved by |
impl Trait diagnostic/test cleanups
I was debugging this yesterday, but didn't get very far. I think the suggestion is correct, the issue is the renderer. We should probably make it a rustfix test so we can see the fixed output |
☀️ Test successful - status-appveyor, status-travis |
…ions, r=petrochencov Underline multiple suggested replacements in the same line <img width="685" alt="screen shot 2018-05-22 at 21 06 48" src="https://user-images.githubusercontent.com/1606434/40403051-174f3180-5e04-11e8-86b6-261630c5ff80.png"> Follow up to rust-lang#50943. Fix rust-lang#50977.
No description provided.