Generate correct suggestion with named arguments used positionally #99660
Conversation
rustbot
added
the
T-compiler
|
(rust-highfive has picked a reviewer for you, use r? to override) |
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
@compiler-errors I just found a bug in how I'm getting the span for width and precision. Please hold off on reviewing! |
This comment has been minimized.
This comment has been minimized.
Address issue rust-lang#99265 by checking each positionally used argument to see if the argument is named and adding a lint to use the name instead. This way, when named arguments are used positionally in a different order than their argument order, the suggested lint is correct. For example: ``` println!("{b} {}", a=1, b=2); ``` This will now generate the suggestion: ``` println!("{b} {a}", a=1, b=2); ``` Additionally, this check now also correctly replaces or inserts only where the positional argument is (or would be if implicit). Also, width and precision are replaced with their argument names when they exists. Since the issues were so closely related, this fix for issue rust-lang#99265 also fixes issue rust-lang#99266. Fixes rust-lang#99265 Fixes rust-lang#99266
|
@compiler-errors Figured out my bug! I think this is ready for a look now. I am wondering if I should do a crater run to make avoid any issues like last time. Is there a way to do that in GitHub or do I need to do it locally? |
|
@compiler-errors thanks for the initial comments! Great idea on both of them. |
| ) { | ||
| let named_arg = names | ||
| .iter() | ||
| .find(|name| name.deref().1.0 == format_argument_index) |
There was a problem hiding this comment.
nit: why is this deref call needed? can you destructure things here, e.g.
.find(|(_, (idx, _))| idx == format_arg_idx)
| return Some(Span::new( | ||
| arg_span.lo() + BytePos(1), | ||
| arg_span.lo() + BytePos(1), | ||
| self.positional_named_arg_span.ctxt(), |
There was a problem hiding this comment.
Why are you using the ctxt and parent from self.positional_named_arg_span?
There was a problem hiding this comment.
You can also probably just do arg_span.with_lo(arg_span.lo() + BytePos(1)).shrink_to_lo()
There was a problem hiding this comment.
First bit was a copy-and-paste error, I think, but the suggested replacement is much better!
| // In the case of a named argument whose position is implicit, there will not be a span | ||
| // to replace. Instead, we insert the name after the `{`, which is the first character | ||
| // of arg_span. | ||
| if let Some(arg_span) = cx.arg_spans.get(self.cur_piece).copied() { |
There was a problem hiding this comment.
nit
| if let Some(arg_span) = cx.arg_spans.get(self.cur_piece).copied() { | |
| if let Some(arg_span) = cx.arg_spans.get(self.cur_piece) { |
| // the lint suggestion, so increment `start` by 1 when `PositionalArgumentType` is | ||
| // `Precision`. | ||
| if ty == PositionalNamedArgType::Precision { | ||
| inner_span_to_replace = inner_span_to_replace.map(|mut is| { |
There was a problem hiding this comment.
tiny microscopic nit: can we do this without mutating, lol
| cur_piece, | ||
| inner_span_to_replace, | ||
| replacement: named_arg.0.clone(), | ||
| positional_named_arg_span: named_arg.1.1.clone(), |
There was a problem hiding this comment.
same here re: destructuring
| i, | ||
| named_arg_type, | ||
| self.curpiece, | ||
| inner_span.clone(), |
There was a problem hiding this comment.
Is inner_span not Copy? If not, then it probably can be.
| inner_span.clone(), | |
| *inner_span, |
There was a problem hiding this comment.
Oh, it is indeed copy!
| @@ -1316,11 +1442,10 @@ pub fn expand_preparsed_format_args( | |||
| } | |||
|
|
|||
| diag.emit(); | |||
| } else if cx.invalid_refs.is_empty() && !named_arguments.is_empty() { | |||
| } else if cx.invalid_refs.is_empty() && cx.ecx.sess.err_count() == 0 { | |||
There was a problem hiding this comment.
Don't we suppress lints if we have errors? Can you describe what case(s) this prevents erroneous lints from being fired?
There was a problem hiding this comment.
I actually don't know one way or another if lints are suppressed when there are errors -- it would make sense if that were the case. However, I had a test fail because it output this lint in addition to the error for positional arguments cannot follow named arguments. (This was in src/test/ui/macros/format-parse-errors.rs.)
|
@compiler-errors Thank you for the comments! I think I go them all. I also added a new method with comment on the logic behind how it determines when a lint should be emitted. If the logic is still not making sense, please let me know! |
|
Thanks, this looks good for now. rollup=never so it's easier to bisect later just in case we have more ICEs, though from the look of this code we probably shouldn't have more errors. @bors r+ rollup=never |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
| | -- ^^ this named argument is only referred to by position in formatting string | ||
| | | | ||
| | this formatting argument uses named argument `_x` by position | ||
| | - ^^ this named argument is only referred to by position in formatting string |
There was a problem hiding this comment.
is the span change only underlining the closing } expected ?
There was a problem hiding this comment.
@lqd
Basically yes, because that's where the named argument gets inserted. But now that you've commented on it, I think it should probably underline both.
I think I will have time to fix this today.
There was a problem hiding this comment.
Took a little longer than I expected!
#99958
|
Finished benchmarking commit (ea6ab1b): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
…rrors Generate correct suggestion with named arguments used positionally Address issue rust-lang#99265 by checking each positionally used argument to see if the argument is named and adding a lint to use the name instead. This way, when named arguments are used positionally in a different order than their argument order, the suggested lint is correct. For example: ``` println!("{b} {}", a=1, b=2); ``` This will now generate the suggestion: ``` println!("{b} {a}", a=1, b=2); ``` Additionally, this check now also correctly replaces or inserts only where the positional argument is (or would be if implicit). Also, width and precision are replaced with their argument names when they exists. Since the issues were so closely related, this fix for issue rust-lang#99265 also fixes issue rust-lang#99266. Fixes rust-lang#99265 Fixes rust-lang#99266
Address issue #99265 by checking each positionally used argument
to see if the argument is named and adding a lint to use the name
instead. This way, when named arguments are used positionally in a
different order than their argument order, the suggested lint is
correct.
For example:
This will now generate the suggestion:
Additionally, this check now also correctly replaces or inserts
only where the positional argument is (or would be if implicit).
Also, width and precision are replaced with their argument names
when they exists.
Since the issues were so closely related, this fix for issue #99265
also fixes issue #99266.
Fixes #99265
Fixes #99266