rust-lang
Add new lint [`positional_named_format_parameters`]
Please write a short comment explaining your change (or "none" for internal only changes)
changelog: Add new lint [positional_named_format_parameters] to warn when named parameters in format strings are used as positional arguments.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dswij (or someone else) soon.
Please see the contribution instructions for more information.
Thanks for the PR and welcome!
Sorry, haven't had the time to take a look at this. I will get to it asap this week when I have the time
Hmmm it looks like the exact same lint was added directly to rustc for release 1.64.0 (https://github.com/rust-lang/rust/pull/98580). What were the chances of that?
@miam-miam100 it might still be worth it to have a clippy lint against named params ever being referred to positionally, which would be noisier/stricter than what this PR and rustc do. That way rustc avoids false positives but there's still a mechanism to detect "iffy" cases.
Hmmm so if I've understood correctly we should have a lint that prevents you from writing this?
println!("hello {world} {}", world = "world");
I don't know what rustc version clippy is using but it looks like my two errors are due to these two PRs not being merged in yet: https://github.com/rust-lang/rust/pull/99263 and https://github.com/rust-lang/rust/pull/99480. @dswij Do you know what I can do? Do I just have to wait?
I believe clippy uses the toolchain version defined here: https://github.com/rust-lang/rust-clippy/blob/master/rust-toolchain.
The job failed when trying to apply the fix for the lint 😕. Did this pass on your local machine?
The changes do not currently pass on my local machine as the toolchain version clippy uses does not include the changes I need for the lint to work correctly. However you are correct in saying that the rust fix file was incorrectly written and has now been fixed.
That makes sense. We can wait for the weekly sync then. The next scheduled one is pretty soon (by the next 24 hours). We can come back and rebase the PR afterward.
I have been hacking on a var inlining lint available since 1.58 - it is based on another possibly conflicting pr https://github.com/rust-lang/rust-clippy/pull/9233
I could really use the code 1)to check if an argument is an alias to another argument or width or precision, 2) get the spans of all three possible usages so I can inline vars and 3) possibly share the same code between rustc and clippy (keep all buggy parsing code in one place 😀). I have written some test cases you might want to use, eg with some unexpected whitespaces.
Awesome to see so much format-related work!
That's awesome, that does not sound all too easy so I wish you the best of luck on that! I added the test cases to my code which look like they should all pass once we update the nightly version.
Nice! Successfully rebased, @dswij feel free to review me whenever you are free.
Hmmm I'm not sure that is a great idea since we don't know when that PR will land and this PR does not really benefit from the late pass in any way.
@miam-miam100 my interest is getting the #9233 in, which I was basing on #8518. I am not yet certain I need to base my lint on #8518, or if I can do it all with the early context. Perhaps your PR could be refactored a bit so that we can have shared functions to parse params and get back the spans? My main requirement:
- detect format!(...) - the string and all arguments
- find spans for each part of the parameter:
{<arg>:<width>$.<precision>}-->struct FormatStrArg { arg: Span, widthArg: Optional<Span>, precisionArg: Optional<Span> }(note that theargspan could be empty, but must be present, whereas width/precision cannot be empty, but could beNone) - for each arg and non-missing width/precision in
FormatStrArg, find corresponding argument that can be analyzed checked if it is it a local var / a fn parameter, or is it something else.
Do you think your functions could produce this result? If so, I could base my work on top of your PR that doesn't try to change the overall structure of the format code. Thx!
Ah well fortunately for you, all those things are directly exposed by rustc.
All the arg, width and precision spans are given, as you can see here (https://github.com/miam-miam100/rust-clippy/blob/631525a21fa3b4bd0c495da11c3b1c0eec84c5ff/clippy_lints/src/write.rs#L543).
As for checking if the corresponding argument is a local var (checking if it is an ident) you can do that here https://github.com/miam-miam100/rust-clippy/blob/631525a21fa3b4bd0c495da11c3b1c0eec84c5ff/clippy_lints/src/write.rs#L667.
From what you've told me you should just be able to check that there and get the whole lint working without any extra work.
To explain the situation a bit, write.rs currently uses a pre-expansion pass, a variant of an early pass that runs before macro expansion has happened https://github.com/rust-lang/rust-clippy/blob/53a09d48557119d2b2a875d77a284b470a95e1c8/clippy_lints/src/lib.rs#L419-L428
The plan is to move away from the pre-expansion pass as it has a few issues, as an example for this lint:
// lib.rs
#![allow(clippy::positional_named_format_parameters)]
mod foo;
fn x() {
// this would still trigger the lint, ignoring the crate wide allow
println!("hello {world} {}", world = "world");
}
Since it would be pretty unlikely someone will want to allow this lint, I'm fine with it being added to write.rs in the meanwhile. As you say the rewrite may take a while to be merged as it's a rather large change
For #9233 I could see people wanting to allow it crate wide however, either due to personal taste or to avoid churn. For that reason I think it should stick to a late pass. Some time in the next few days I'll see if it's feasible to split off the clippy_utils::macro changes into their own PR so you don't have to wait for the whole thing @nyurik, as well as adding the missing details you need
@Alexendoo thank you for the detailed answer! What are you thoughts about integrating the new rustc format string parsing capabilities like rustc_parse_format::Parser, i.e. as used in here?
@nyurik Yeah, just waiting for the next sync that includes https://github.com/rust-lang/rust/pull/99987, I'll ping you in the PR once it's ready
Right that's great thanks, I'm currently on holiday so I'll do that when I get back. (a week's time)
Huge thanks for this @miam-miam100! This is a nice lint to have for sure.
@bors r+
:pushpin: Commit 9ffddf563c0380abbfc7b77274d89a1cb71573ae has been approved by dswij
It is now in the queue for this repository.
:hourglass: Testing commit 9ffddf563c0380abbfc7b77274d89a1cb71573ae with merge 86ac6e88a837a4fbf0532ef03a79ebc69da9afd7...
:sunny: Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test Approved by: dswij Pushing 86ac6e88a837a4fbf0532ef03a79ebc69da9afd7 to master...