Better underline for E0057,E0060,E0061 #36222
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jonathandturner (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. |
| @@ -2449,6 +2449,12 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |||
| let mut expected_arg_tys = expected_arg_tys; | |||
| let expected_arg_count = fn_inputs.len(); | |||
|
|
|||
| let sp_args = if args.len() > 0 { | |||
| Span { lo: args[0].span.lo, ..args[args.len()-1].span} | |||
There was a problem hiding this comment.
Sadly, we can't safely mix spans like this because some arguments may come from incompatible locations (like from macros).
|
I added a comment here about not being able to mix spans directly. Instead, we'll need to use the span information that's already been calculated, or add it when it's safe to add. If the latter is required, you should chat with some people in #rust-internals and see if they have ideas of how best to add it. |
|
I cannot find any usable span, I'm asking on irc. |
|
This is what I can achieve with a MultiSpan constructed from the args span |
acrrd
force-pushed
the
better_underline_E0057
branch
from
b7a58bf to
cd9476e
Compare
|
☔ The latest upstream changes (presumably #36303) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@nrc - is it okay to merge spans if those spans are on the same line? See the above screenshot. It seems like there are cases we could safely merge. |
acrrd
force-pushed
the
better_underline_E0057
branch
from
cd9476e to
a2b8dd8
Compare
| vec!(sp) | ||
| }; | ||
|
|
||
| fn parameter_count_error<'tcx>(sess: &Session, spans: Vec<Span>, fn_inputs: &[Ty<'tcx>], |
There was a problem hiding this comment.
Can we use an array/slice for spans instead of a Vec here?
There was a problem hiding this comment.
The field of MultiSpan is a Vec so we have to construct a Vec somewhere.
|
I'm chatting with @nrc about what to do here, and we have some ideas. RustConf is currently going on, so it may take us a few more days to get back to this. |
|
ok! |
|
@acrrd - wow, sorry for the looong delay. I just landed a new method on CodeMap you can use to merge spans called |
acrrd
force-pushed
the
better_underline_E0057
branch
2 times, most recently
from
8732f14 to
e6f2e01
Compare
|
I merge spans until possible, this allow to underline at least some arguments also if the call is on multiple lines. |
|
@acrrd - sounds good You may want to also run tidy on your code: And fix the errors it has. Looks like there are some in the current PR. |
acrrd
force-pushed
the
better_underline_E0057
branch
from
e6f2e01 to
01d4c95
Compare
|
Fixed |
|
Great! The one last thing is to create a test for this in src/test/ui. You can see some of the tests there and how to do them (specifically in src/test/ui/spans). After you add a test I think we're good to go. |
acrrd
force-pushed
the
better_underline_E0057
branch
from
01d4c95 to
d0c6172
Compare
|
Test added |
|
Thanks! @bors: r+ rollup |
|
📌 Commit d0c6172 has been approved by |
…llaumeGomez Better underline for E0057,E0060,E0061 Fix rust-lang#35214 Part of rust-lang#35233 r? @jonathandturner
Fix #35214
Part of #35233
r? @jonathandturner