Skip to content
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

Incorrect suggestion with named_arguments_used_positionally lint #99265

Closed
compiler-errors opened this issue Jul 15, 2022 · 3 comments · Fixed by #99660
Closed

Incorrect suggestion with named_arguments_used_positionally lint #99265

compiler-errors opened this issue Jul 15, 2022 · 3 comments · Fixed by #99660
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@compiler-errors
Copy link
Member

compiler-errors commented Jul 15, 2022

Code:

fn main() {
    println!("{b} {}", a=1, b=2);
}

The current output is:

warning: named argument `a` is not used by name
 --> /home/michael/test.rs:2:24
  |
2 |     println!("{b} {}", a=1, b=2);
  |               ---      ^ this named argument is only referred to by position in formatting string
  |               |
  |               this formatting argument uses named argument `a` by position
  |
  = note: `#[warn(named_arguments_used_positionally)]` on by default
help: use the named argument by name to avoid ambiguity
  |
2 |     println!("{a} {}", a=1, b=2);
  |               ~~~

Ideally it would look like:

  = note: `#[warn(named_arguments_used_positionally)]` on by default
help: use the named argument by name to avoid ambiguity
  |
2 |     println!("{b} {a}", a=1, b=2);
  |                    +
  1. The suggestion is not exactly correct. Instead of replacing the first named arg with the suggestion, we should replace the first implicitly positional argument.
  2. It would also be nice if we could tighten the span on the structured suggestion (see ~~~ -> + in the before and after)
@compiler-errors compiler-errors added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 15, 2022
@compiler-errors
Copy link
Member Author

cc @PrestonFrom

@PrestonFrom
Copy link
Contributor

@rustbot claim

@PrestonFrom
Copy link
Contributor

@compiler-errors Thank you for cc'ing me on this!

bors added a commit to rust-lang-ci/rust that referenced this issue Jul 16, 2022
Fix ICE in `named_arguments_used_positionally` lint

Fixes rust-lang#99261
Fixes rust-lang#99289
Fixes rust-lang#99284
Fixes rust-lang#99273
Fixes rust-lang#99297
Fixes rust-lang#99271

This match pattern:

```
 FormatSpec { width: Count::CountIsName(s, _), .. }
| FormatSpec { precision: Count::CountIsName(s, _), .. }
```

does not account for when both `width` and `precision` are both `Count::CountIsName`, so split the check for these two fields into two separate `if let`.

Also, remove any future potential for ICEs by removing the index operator altogether.

---

It is still suspicious that this indexing was broken and caused the ICE, as opposed to just causing a spurious lint message.

cc `@PrestonFrom,` who may be familiar with this code because of implementing the lint this touches, perhaps you'd like to look into why named arguments in `FormatSpec.precision` seem to have indices that don't correspond to a span in `Context.arg_spans`?

Edit: Opened rust-lang#99265 to track a (related?) incorrect argument indexing issue.
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 29, 2022
…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
@bors bors closed this as completed in 3330c7d Jul 29, 2022
flip1995 pushed a commit to flip1995/rust that referenced this issue Aug 11, 2022
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
flip1995 pushed a commit to flip1995/rust that referenced this issue Aug 11, 2022
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants