Emit warning when named arguments are used positionally in format #98580

PrestonFrom · 2022-06-27T15:48:26Z

Addresses Issue 98466 by emitting an error if a named argument
is used like a position argument (i.e. the name is not used in
the string to be formatted).

Fixes #98466

rust-highfive · 2022-06-27T15:48:29Z

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

PrestonFrom · 2022-06-27T15:49:29Z

r? @estebank

I'm not sure if I should override who rustbot picked to review this, but I thought you might have a little more context.

estebank · 2022-06-27T16:57:38Z

@bors try

bors · 2022-06-27T16:57:47Z

⌛ Trying commit aa875b67b4e6be07f55065129a3b4426d720de28 with merge 768af4cba83356e997782d009ecca0564739e9fc...

PrestonFrom · 2022-06-27T17:02:39Z

Uh-oh! I will look into this! I must have missed something when compiling locally.

bors · 2022-06-27T17:23:04Z

💔 Test failed - checks-actions

estebank · 2022-06-27T20:37:01Z

The fact that we have failures in our own codebase makes me think that this error is way too common for an outright error. We'll have to turn it into a warn by default lint very likely. Still interested in gauging what the real incidence is.

PrestonFrom · 2022-06-27T20:39:50Z

@estebank Looking into this, it turns out I made a bad assumption in how I was checking for unused named arguments, and the error should not have been emitted! I'll let you know when I figure out how to do this correctly. Sorry for the delay!

Mark-Simulacrum · 2022-06-27T22:35:11Z

FWIW, this has been discussed by T-lang in the past and at the time there was positive sentiment towards a lint on this (#45256 (comment)).

PrestonFrom · 2022-06-27T22:37:26Z

@Mark-Simulacrum thank you for sharing this! Would I be correct in thinking that this means a warning should be emitted instead of an error?

Mark-Simulacrum · 2022-06-27T22:40:04Z

I would expect so, it should be a lint though -- maybe even we can reuse something like unused variable for it, but @estebank can say more on the naming front.

PrestonFrom · 2022-06-27T22:54:28Z

Oh, I see -- this should not be done in format.rs at all, but in rustc_lint then. (Or should it be in clippy?)

estebank · 2022-06-28T01:14:04Z

The new error shows that the "liveness" check needs to account for the formatting position (when used after the :, instead of before it).

estebank · 2022-06-28T01:16:01Z

compiler/rustc_builtin_macros/src/format.rs

+            match a.format {
+                FormatSpec { width: Count::CountIsName(s, _), .. } => {
+                    used_argument_names.insert(s);
+                }
+                _ => {}


Oh, it seems like you were doing that already? 🤔

It turns out I missed the same case but for precision!

estebank · 2022-06-28T01:29:43Z

Oh, I see -- this should not be done in format.rs at all, but in rustc_lint then. (Or should it be in clippy?)

It shouldn't need to be in clippy, I think this warrants a rustc lint. To add a lint you have to declare it and you can (and will have to in this case) create it from a place other than rustc_lint. I think you can take a look at lint_unicode_text_flow for one example of a "non-standard" builtin lint that requires state to be kept. You would have to add a variant to BuiltinLintDiagnostics, and that would keep around enough info (spans and symbols) to be able to provide an appropriate suggestion (likely naming it in the format string would suffice).

PrestonFrom · 2022-06-28T05:04:54Z

@estebank Thank you! I will take a look at lint_unicode_text_flow!

estebank · 2022-07-01T02:04:47Z

src/test/ui/macros/issue98466.stderr

+error: named argument is not used by name
+  --> $DIR/issue98466.rs:4:26
+   |
+LL |     println!("_x is {}", _x = 5);
+   |                          ^^


Ideally the end result would look something like this:

error: named string formatting argument is not used by name --> $DIR/issue98466.rs:4:26 | LL | println!("_x is {}", _x = 5); | -- ^^ this named argument is only referred to by position in the formatting string | | | this formatting argument uses named argument `_x` by position | = note: lint `#[deny(named_formatting_arg_by_pos)]` enabled by default help: use the named argument by name to avoid ambiguity | LL | println!("_x is {_x}", _x = 5); | ++

That seems reasonable to me! Still working on getting the lint to actually be emitted.

What I ended up with looks a little different -- since I used span_suggestion the help part got rolled in with the suggestion. Should I be using a different method?

Also, the lint is currently Warn instead of Deny, since it seems like it's been valid to use named arguments positionally for some years. Would Deny be better?

PrestonFrom · 2022-07-08T04:41:55Z

@estebank Thank you for the review and suggestions! I believe I have addressed them all and squashed (after running x.py fmt).

estebank · 2022-07-08T17:55:59Z

compiler/rustc_builtin_macros/src/format.rs

+        .iter()
+        .map(|piece| {
+            let mut piece = piece.clone();


This can also be written as

Suggested change

.iter()

.map(|piece| {

let mut piece = piece.clone();

.iter()

.cloned()

.map(|mut piece| {

@estebank Thanks for the suggestion! Did you want me to fix this? (I wasn't sure since it was in the queue.)

There's no need right now if you can't get to it, but keep it in mind for the future. If you do fix it, I can reapprove :)

I force pushed the fix -- I hope that was what was expected!

estebank · 2022-07-08T17:57:18Z

@bors r+

bors · 2022-07-08T17:57:20Z

📌 Commit abf6741408fb31701168c6150c077e98cdc82b4b has been approved by estebank

It is now in the queue for this repository.

PrestonFrom · 2022-07-11T22:47:32Z

@estebank I hope I didn’t break anything by doing this, but I updated the name of the PR since it’s a warning instead of an error now. I also pushed the change to use cloned. Do I need another approval from you or do I just wait for bors now?

bors · 2022-07-13T14:11:09Z

☔ The latest upstream changes (presumably #99203) made this pull request unmergeable. Please resolve the merge conflicts.

PrestonFrom · 2022-07-13T14:30:12Z

Oh I forgot the no merging rule. I'll rebase and force push.

Addresses Issue 98466 by emitting a warning if a named argument is used like a position argument (i.e. the name is not used in the string to be formatted). Fixes rust-lang#98466

PrestonFrom · 2022-07-13T22:23:35Z

I think this is good now!

estebank · 2022-07-13T23:37:50Z

@bors r+

bors · 2022-07-13T23:37:52Z

📌 Commit 1219f72 has been approved by estebank

It is now in the queue for this repository.

Rollup of 6 pull requests Successful merges: - rust-lang#98072 (Add provider API to error trait) - rust-lang#98580 (Emit warning when named arguments are used positionally in format) - rust-lang#99000 (Move abstract const to middle) - rust-lang#99192 (Fix spans for asm diagnostics) - rust-lang#99222 (Better error message for generic_const_exprs inference failure) - rust-lang#99236 (solaris: unbreak build on native platform) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned cjgillot Jun 27, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 27, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2022

rust-highfive assigned estebank and unassigned cjgillot Jun 27, 2022

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2022

This comment has been minimized.

Sign in to view

estebank reviewed Jun 28, 2022

View reviewed changes

estebank reviewed Jul 1, 2022

View reviewed changes

PrestonFrom force-pushed the issue_98466 branch from d67fece to 5d4f269 Compare July 3, 2022 06:46

This comment has been minimized.

Sign in to view

PrestonFrom force-pushed the issue_98466 branch 2 times, most recently from f20ee9f to abf6741 Compare July 8, 2022 04:39

PrestonFrom requested a review from estebank July 8, 2022 04:43

estebank reviewed Jul 8, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 8, 2022

This was referenced Jul 8, 2022

Assignment inside format_args! is ignored #45256

Closed

Named format arguments can be used as positional #93415

Closed

PrestonFrom force-pushed the issue_98466 branch from abf6741 to c2048e0 Compare July 8, 2022 18:18

PrestonFrom changed the title ~~Emit error when named arguments are used positionally in format~~ Emit warning when named arguments are used positionally in format Jul 11, 2022

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 13, 2022

Emit warning when named arguments are used positionally in format

1219f72

Addresses Issue 98466 by emitting a warning if a named argument is used like a position argument (i.e. the name is not used in the string to be formatted). Fixes rust-lang#98466

PrestonFrom force-pushed the issue_98466 branch from e7bafa3 to 1219f72 Compare July 13, 2022 21:34

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 13, 2022

Dylan-DPC mentioned this pull request Jul 14, 2022

Rollup of 6 pull requests #99242

Merged

bors merged commit 8c5c983 into rust-lang:master Jul 14, 2022

rustbot added this to the 1.64.0 milestone Jul 14, 2022

miam-miam mentioned this pull request Jul 18, 2022

Add new lint [positional_named_format_parameters] rust-lang/rust-clippy#9040

Merged

Emit warning when named arguments are used positionally in format #98580

Emit warning when named arguments are used positionally in format #98580

Conversation

PrestonFrom commented Jun 27, 2022

rust-highfive commented Jun 27, 2022

PrestonFrom commented Jun 27, 2022

estebank commented Jun 27, 2022

bors commented Jun 27, 2022

PrestonFrom commented Jun 27, 2022

This comment has been minimized.

bors commented Jun 27, 2022

This comment has been minimized.

estebank commented Jun 27, 2022

PrestonFrom commented Jun 27, 2022

Mark-Simulacrum commented Jun 27, 2022

PrestonFrom commented Jun 27, 2022

Mark-Simulacrum commented Jun 27, 2022

PrestonFrom commented Jun 27, 2022

This comment has been minimized.

estebank commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Jun 28, 2022

PrestonFrom commented Jun 28, 2022

estebank Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

PrestonFrom commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Jul 8, 2022

bors commented Jul 8, 2022

PrestonFrom commented Jul 11, 2022

bors commented Jul 13, 2022

PrestonFrom commented Jul 13, 2022

PrestonFrom commented Jul 13, 2022

estebank commented Jul 13, 2022

bors commented Jul 13, 2022

estebank Jul 1, 2022 •

edited

Loading