Add new lint [positional_named_format_parameters]

[miam-miam]

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.

[rust-highfive]

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.

[dswij]

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

[dswij]

Thanks for the patience! The changes in the PR look great to me and is certainly a nice addition! Just some comments

[miam-miam]

Hmmm it looks like the exact same lint was added directly to rustc for release 1.64.0 (rust-lang/rust#98580). What were the chances of that?

[estebank]

@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.

[miam-miam]

Hmmm so if I've understood correctly we should have a lint that prevents you from writing this?

println!("hello {world} {}", world = "world");

[estebank]

Exactly.

[miam-miam]

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: rust-lang/rust#99263 and rust-lang/rust#99480. @dswij Do you know what I can do? Do I just have to wait?

[dswij]

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?

[miam-miam]

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.

[dswij]

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.

[nyurik]

I have been hacking on a var inlining lint available since 1.58 - it is based on another possibly conflicting pr #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!

[miam-miam]

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.

[miam-miam]

Nice! Successfully rebased, @dswij feel free to review me whenever you are free.

[nyurik]

@miam-miam100 I made a tiny suggestion, but the bigger question is how this PR should be merged with the @Alexendoo 's #8518 -- it seems both make a number of significant changes to the clippy_lints/src/write.rs, nearly rewriting it. Would it make sense to perhaps base your PR on top of @Alexendoo 's, so that all of them use late context?

[miam-miam]

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.

[nyurik]

@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 the arg span could be empty, but must be present, whereas width/precision cannot be empty, but could be None)
• 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!

[miam-miam]

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.

[Alexendoo]

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

rust-clippy/clippy_lints/src/lib.rs

Lines 419 to 428 in 53a09d4

	/// Register all pre expansion lints
	///
	/// Pre-expansion lints run before any macro expansion has happened.
	///
	/// Note that due to the architecture of the compiler, currently `cfg_attr` attributes on crate
	/// level (i.e `#![cfg_attr(...)]`) will still be expanded even when using a pre-expansion pass.
	///
	/// Used in `./src/driver.rs`.
	pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore, sess: &Session, conf: &Conf) {
	// NOTE: Do not add any more pre-expansion passes. These should be removed eventually.

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

[nyurik]

@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?

[dswij]

The lint itself looks great, just the lint description that can be improved, IMO.

Can you help squash the commits, and after that, I think it's good to merge.

[Alexendoo]

@nyurik Yeah, just waiting for the next sync that includes rust-lang/rust#99987, I'll ping you in the PR once it's ready

[miam-miam]

Right that's great thanks, I'm currently on holiday so I'll do that when I get back. (a week's time)

[miam-miam]

@dswij All squashed!

[dswij]

Huge thanks for this @miam-miam100! This is a nice lint to have for sure.

@bors r+

[bors]

📌 Commit 9ffddf5 has been approved by dswij

It is now in the queue for this repository.

[bors]

⌛ Testing commit 9ffddf5 with merge 86ac6e8...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing 86ac6e8 to master...