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 named_arguments_used_positionally suggestion in obscure case #99655

Closed
nyurik opened this issue Jul 24, 2022 · 9 comments
Closed

Incorrect named_arguments_used_positionally suggestion in obscure case #99655

nyurik opened this issue Jul 24, 2022 · 9 comments
Labels
C-bug Category: This is a bug.

Comments

@nyurik
Copy link
Contributor

nyurik commented Jul 24, 2022

This code is not really something anyone SHOULD write, but because rustc catches it and makes a suggestion, it should either handle it correctly, or not suggest anything at all and give a different warning because it is clearly not something anyone should ever write, and thus it is likely a bug.

    let val = 5;
    // same issue for all these cases
    println!("{:0$}", v = val);
    println!("{0:0$}", v = val);
    println!("{:0$.0$}", v = val);
    println!("{0:0$.0$}", v = val);
    // these work correctly, but should probably give a different warning (see below)
    println!("{0:0$.v$}", v = val);
    println!("{0:v$.0$}", v = val);
    println!("{v:0$.0$}", v = val);
    println!("{v:v$.0$}", v = val);
    println!("{v:0$.v$}", v = val);
    println!("{v:v$.v$}", v = val);

Expected

I expected to see this happen (one of these):

  • suggest corrected version
62 |     println!("{v:v$}", v = val);
  • ignore it as part of named_arguments_used_positionally, but warn as part of some new warning like argument_used_multiple_times_as_value_and_as_format_or_precision (the name needs some work :) )

Current behavior

Instead, rustc showed this incorrect suggestion:

warning: named argument `v` is not used by name
  --> src/main.rs:64:23
   |
64 |     println!("{:0$}", v = val);
   |               -----   ^ this named argument is only referred to by position in formatting string
   |               |
   |               this formatting argument uses named argument `v` by position
   |
   = note: `#[warn(named_arguments_used_positionally)]` on by default
help: use the named argument by name to avoid ambiguity
   |
64 |     println!("{v}", v = val);
   |               ~~~

Meta

rustc --version --verbose:

cargo 1.64.0-nightly (d8d30a753 2022-07-19)
release: 1.64.0-nightly
commit-hash: d8d30a75376f78bb0fabe3d28ee9d87aa8035309
commit-date: 2022-07-19
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.83.1-DEV (sys:0.4.55+curl-7.83.1 vendored ssl:OpenSSL/1.1.1n)
os: Linux Mint 20.3 (una) [64-bit]
@nyurik nyurik added the C-bug Category: This is a bug. label Jul 24, 2022
@compiler-errors
Copy link
Member

I think @PrestonFrom is working on this?

@PrestonFrom
Copy link
Contributor

Yea, I am! Sorry, taking a little longer than I expected.

Thank you for the additional test cases!

@compiler-errors
Copy link
Member

No rush @PrestonFrom.

@nyurik
Copy link
Contributor Author

nyurik commented Jul 24, 2022

@PrestonFrom awesome! I have been hacking on rust-lang/rust-clippy#9233 -- a clippy lint to inline print!("{}", var) into print!("{var}"), and there are a lot more test cases including two that broke rustc up until a few days ago (note that that PR is based on another PR, so the actual change is much smaller). It seems the current internal clippy code for the format arg parsing is a bit lacking - it doesn't track precision and width argument spans, nor can it detect if an argument is being used (aliased) by width/precision in addition to the value use. I wonder if there should be some code reuse between the projects. Currently it is all in FormatArgsArg and related.

Thanks @compiler-errors for the connection!

@compiler-errors
Copy link
Member

It seems the current internal clippy code for the format arg parsing is a bit lacking - it doesn't track precision and width argument spans

Lol, yeah the current implementation in rustc doesn't track these spans either. 🤣

@compiler-errors
Copy link
Member

@PrestonFrom did you fix this or does this need to further fixed? (Just asking, no pressure for you to do it if it's not fixed by your previous PRs)

@PrestonFrom
Copy link
Contributor

@compiler-errors Oh, sorry! I believe this is fixed by previous PRs.

@compiler-errors
Copy link
Member

No worries. Just trawling the issues backlog 😄

@PrestonFrom
Copy link
Contributor

Just checked and unless I'm mistaken all the cases in this issue are covered.

Is it okay to close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

3 participants