Only point out a single function parameter if we have a single arg incompatibility #99646
Conversation
rustbot
added
the
T-compiler
|
r? @estebank (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| LL | fn foo( | ||
| | ^^^ | ||
| ... | ||
| LL | f: i32, |
There was a problem hiding this comment.
We could say "parameter defined here" if we have just one (or maybe just if the signature is multiline)
| // A specific argument should be labeled, instead of all of them | ||
| expected_idx: Option<usize>, |
There was a problem hiding this comment.
this only works if there's a single error in the function arguments, right? would it be possible to extend it to multiple arguments? https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7a212f35d6f07a993c223f3e4662db39
There was a problem hiding this comment.
The further we move from certainty, the more we might mislead if we point at things too confidently. I feel like doing this only for single args for now is a good compromise while we come up with better heuristics. The suggestion being wrong is counteracted by pointing at the whole fn signature, but if we no longer do that then we're implicitly claiming infallibility 😅
There was a problem hiding this comment.
For context, I was hoping this change would make the awful error in https://mobile.twitter.com/joshuayn514/status/1557913836337786880 better.
Maybe we can somehow limit this to cases where the arguments are "close", for some definition of close? like, say, a function pointer where only the arguments differ? :P
(I don't actually have great general suggestions here but I would love that error to be smarter than it is now ...)
| @@ -2225,7 +2225,7 @@ impl<'a> Parser<'a> { | |||
| attrs: attrs.into(), | |||
| ty, | |||
| pat, | |||
| span: lo.to(this.token.span), | |||
| span: lo.to(this.prev_token.span), | |||
There was a problem hiding this comment.
This change means that we no longer include the comma in the param span, right? I don't know if that is a good call 🤔
Maybe we can address the span issue on the other end, by creating a new span that is param.pat.span.to(param.ty.map_or(param.pat.span, |ty| ty.span). I guess it depends on what we've used the param's span for elsewhere (even though there haven't been any visible changes in our test suite from this... I guess I'm fine either way :-/
There was a problem hiding this comment.
I think nobody used it, evidenced by the lack of UI test changes.
This makes it more consistent with regular fn signatures' spans.
There was a problem hiding this comment.
That's fine, let's go ahead with that change then. We can revisit this if we ever need the param + comma (I'm sure, for removal suggestions) :)
| for (idx, param) in params.into_iter().enumerate() { | ||
| if let Some(expected_idx) = expected_idx { | ||
| if idx == expected_idx { | ||
| spans.push_span_label(param.span, ""); | ||
| } | ||
| } else { | ||
| spans.push_span_label(param.span, ""); | ||
| } |
There was a problem hiding this comment.
| for (idx, param) in params.into_iter().enumerate() { | |
| if let Some(expected_idx) = expected_idx { | |
| if idx == expected_idx { | |
| spans.push_span_label(param.span, ""); | |
| } | |
| } else { | |
| spans.push_span_label(param.span, ""); | |
| } | |
| for (idx, param) in params.into_iter().enumerate() | |
| .filter(|(idx, _)| expected_idx.map_or(true, |expected| idx == expected)) | |
| { | |
| spans.push_span_label(param.span, ""); |
|
@bors r+ |
|
📌 Commit 694def26197a8024e814d1e7a63495ca471c1b34 has been approved by It is now in the queue for this repository. |
bors
added
S-waiting-on-bors
|
@bors r- this is old and i can apply these nits later, then r=you again later |
bors
added
S-waiting-on-author
compiler-errors
force-pushed
the
arg-mismatch-single-arg-label
branch
from
694def2 to
c608918
Compare
|
@bors r=estebank |
bors
added
S-waiting-on-bors
…-arg-label, r=estebank Only point out a single function parameter if we have a single arg incompatibility Fixes rust-lang#99635
|
alleviating pressure on bors queue, since this is just diagnostics changes @bors rollup |
…mpiler-errors Rollup of 8 pull requests Successful merges: - rust-lang#99646 (Only point out a single function parameter if we have a single arg incompatibility) - rust-lang#100299 (make `clean::Item::span` return `Option` instead of dummy span) - rust-lang#100335 (Rustdoc-Json: Add `Path` type for traits.) - rust-lang#100367 (Suggest the path separator when a dot is used on a trait) - rust-lang#100431 (Enum variant ctor inherits the stability of the enum variant) - rust-lang#100446 (Suggest removing a semicolon after impl/trait items) - rust-lang#100468 (Use an extensionless `x` script for non-Windows) - rust-lang#100479 (Argument type error improvements) Failed merges: - rust-lang#100483 (Point to generic or arg if it's the self type of unsatisfied projection predicate) r? `@ghost` `@rustbot` modify labels: rollup
…r=estebank
Adjust span of fn argument declaration
Span of a fn argument declaration goes from:
```
fn foo(i : i32 , ...)
^^^^^^^^
```
to:
```
fn foo(i : i32 , ...)
^^^^^^^
```
That is, we don't include the extra spacing up to the trailing comma, which I think is more correct.
cc rust-lang#99646 (comment)
r? `@estebank`
---
The two tests that had dramatic changes in their rendering I think actually are improved, though they are kinda poor spans both before and after the changes. 🤷 Thoughts?
…r=estebank
Adjust span of fn argument declaration
Span of a fn argument declaration goes from:
```
fn foo(i : i32 , ...)
^^^^^^^^
```
to:
```
fn foo(i : i32 , ...)
^^^^^^^
```
That is, we don't include the extra spacing up to the trailing comma, which I think is more correct.
cc rust-lang#99646 (comment)
r? ``@estebank``
---
The two tests that had dramatic changes in their rendering I think actually are improved, though they are kinda poor spans both before and after the changes. 🤷 Thoughts?
Fixes #99635