Auto merge of #9040 - miam-miam100:unused_named_parameter, r=dswij

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.

Author: bors

CHANGELOG.md

@@ -3975,6 +3975,7 @@ Released 2018-09-13
 [`partialeq_to_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_to_none
 [`path_buf_push_overwrite`]: https://rust-lang.github.io/rust-clippy/master/index.html#path_buf_push_overwrite
 [`pattern_type_mismatch`]: https://rust-lang.github.io/rust-clippy/master/index.html#pattern_type_mismatch
+[`positional_named_format_parameters`]: https://rust-lang.github.io/rust-clippy/master/index.html#positional_named_format_parameters
 [`possible_missing_comma`]: https://rust-lang.github.io/rust-clippy/master/index.html#possible_missing_comma
 [`precedence`]: https://rust-lang.github.io/rust-clippy/master/index.html#precedence
 [`print_in_format_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#print_in_format_impl

clippy_lints/src/lib.register_all.rs

@@ -344,6 +344,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(vec::USELESS_VEC),
     LintId::of(vec_init_then_push::VEC_INIT_THEN_PUSH),
     LintId::of(vec_resize_to_zero::VEC_RESIZE_TO_ZERO),
+    LintId::of(write::POSITIONAL_NAMED_FORMAT_PARAMETERS),
     LintId::of(write::PRINTLN_EMPTY_STRING),
     LintId::of(write::PRINT_LITERAL),
     LintId::of(write::PRINT_WITH_NEWLINE),

clippy_lints/src/lib.register_lints.rs

@@ -587,6 +587,7 @@ store.register_lints(&[
     verbose_file_reads::VERBOSE_FILE_READS,
     wildcard_imports::ENUM_GLOB_USE,
     wildcard_imports::WILDCARD_IMPORTS,
+    write::POSITIONAL_NAMED_FORMAT_PARAMETERS,
     write::PRINTLN_EMPTY_STRING,
     write::PRINT_LITERAL,
     write::PRINT_STDERR,

clippy_lints/src/lib.register_suspicious.rs

@@ -32,4 +32,5 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec!
     LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
     LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),
     LintId::of(swap_ptr_to_ref::SWAP_PTR_TO_REF),
+    LintId::of(write::POSITIONAL_NAMED_FORMAT_PARAMETERS),
 ])

clippy_lints/src/write.rs

@@ -3,8 +3,9 @@ use std::iter;
 use std::ops::{Deref, Range};
 
 use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_and_then};
-use clippy_utils::source::{snippet_opt, snippet_with_applicability};
+use clippy_utils::source::{snippet, snippet_opt, snippet_with_applicability};
 use rustc_ast::ast::{Expr, ExprKind, Impl, Item, ItemKind, MacCall, Path, StrLit, StrStyle};
+use rustc_ast::ptr::P;
 use rustc_ast::token::{self, LitKind};
 use rustc_ast::tokenstream::TokenStream;
 use rustc_errors::{Applicability, DiagnosticBuilder};
@@ -256,6 +257,28 @@ declare_clippy_lint! {
     "writing a literal with a format string"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// This lint warns when a named parameter in a format string is used as a positional one.
+    ///
+    /// ### Why is this bad?
+    /// It may be confused for an assignment and obfuscates which parameter is being used.
+    ///
+    /// ### Example
+    /// ```rust
+    /// println!("{}", x = 10);
+    /// ```
+    ///
+    /// Use instead:
+    /// ```rust
+    /// println!("{x}", x = 10);
+    /// ```
+    #[clippy::version = "1.63.0"]
+    pub POSITIONAL_NAMED_FORMAT_PARAMETERS,
+    suspicious,
+    "named parameter in a format string is used positionally"
+}
+
 #[derive(Default)]
 pub struct Write {
     in_debug_impl: bool,
@@ -270,7 +293,8 @@ impl_lint_pass!(Write => [
     PRINT_LITERAL,
     WRITE_WITH_NEWLINE,
     WRITELN_EMPTY_STRING,
-    WRITE_LITERAL
+    WRITE_LITERAL,
+    POSITIONAL_NAMED_FORMAT_PARAMETERS,
 ]);
 
 impl EarlyLintPass for Write {
@@ -408,6 +432,7 @@ fn newline_span(fmtstr: &StrLit) -> (Span, bool) {
 #[derive(Default)]
 struct SimpleFormatArgs {
     unnamed: Vec<Vec<Span>>,
+    complex_unnamed: Vec<Vec<Span>>,
     named: Vec<(Symbol, Vec<Span>)>,
 }
 impl SimpleFormatArgs {
@@ -419,6 +444,10 @@ impl SimpleFormatArgs {
         })
     }
 
+    fn get_complex_unnamed(&self) -> impl Iterator<Item = &[Span]> {
+        self.complex_unnamed.iter().map(Vec::as_slice)
+    }
+
     fn get_named(&self, n: &Path) -> &[Span] {
         self.named.iter().find(|x| *n == x.0).map_or(&[], |x| x.1.as_slice())
     }
@@ -479,6 +508,61 @@ impl SimpleFormatArgs {
             },
         };
     }
+
+    fn push_to_complex(&mut self, span: Span, position: usize) {
+        if self.complex_unnamed.len() <= position {
+            self.complex_unnamed.resize_with(position, Vec::new);
+            self.complex_unnamed.push(vec![span]);
+        } else {
+            let args: &mut Vec<Span> = &mut self.complex_unnamed[position];
+            args.push(span);
+        }
+    }
+
+    fn push_complex(
+        &mut self,
+        cx: &EarlyContext<'_>,
+        arg: rustc_parse_format::Argument<'_>,
+        str_lit_span: Span,
+        fmt_span: Span,
+    ) {
+        use rustc_parse_format::{ArgumentImplicitlyIs, ArgumentIs, CountIsParam};
+
+        let snippet = snippet_opt(cx, fmt_span);
+
+        let end = snippet
+            .as_ref()
+            .and_then(|s| s.find(':'))
+            .or_else(|| fmt_span.hi().0.checked_sub(fmt_span.lo().0 + 1).map(|u| u as usize));
+
+        if let (ArgumentIs(n) | ArgumentImplicitlyIs(n), Some(end)) = (arg.position, end) {
+            let span = fmt_span.from_inner(InnerSpan::new(1, end));
+            self.push_to_complex(span, n);
+        };
+
+        if let (CountIsParam(n), Some(span)) = (arg.format.precision, arg.format.precision_span) {
+            // We need to do this hack as precision spans should be converted from .* to .foo$
+            let hack = if snippet.as_ref().and_then(|s| s.find('*')).is_some() {
+                0
+            } else {
+                1
+            };
+
+            let span = str_lit_span.from_inner(InnerSpan {
+                start: span.start + 1,
+                end: span.end - hack,
+            });
+            self.push_to_complex(span, n);
+        };
+
+        if let (CountIsParam(n), Some(span)) = (arg.format.width, arg.format.width_span) {
+            let span = str_lit_span.from_inner(InnerSpan {
+                start: span.start,
+                end: span.end - 1,
+            });
+            self.push_to_complex(span, n);
+        };
+    }
 }
 
 impl Write {
@@ -511,8 +595,8 @@ impl Write {
                 // FIXME: modify rustc's fmt string parser to give us the current span
                 span_lint(cx, USE_DEBUG, span, "use of `Debug`-based formatting");
             }
-
             args.push(arg, span);
+            args.push_complex(cx, arg, str_lit.span, span);
         }
 
         parser.errors.is_empty().then_some(args)
@@ -566,6 +650,7 @@ impl Write {
 
         let lint = if is_write { WRITE_LITERAL } else { PRINT_LITERAL };
         let mut unnamed_args = args.get_unnamed();
+        let mut complex_unnamed_args = args.get_complex_unnamed();
         loop {
             if !parser.eat(&token::Comma) {
                 return (Some(fmtstr), expr);
@@ -577,11 +662,20 @@ impl Write {
             } else {
                 return (Some(fmtstr), None);
             };
+            let complex_unnamed_arg = complex_unnamed_args.next();
+
             let (fmt_spans, lit) = match &token_expr.kind {
                 ExprKind::Lit(lit) => (unnamed_args.next().unwrap_or(&[]), lit),
-                ExprKind::Assign(lhs, rhs, _) => match (&lhs.kind, &rhs.kind) {
-                    (ExprKind::Path(_, p), ExprKind::Lit(lit)) => (args.get_named(p), lit),
-                    _ => continue,
+                ExprKind::Assign(lhs, rhs, _) => {
+                    if let Some(span) = complex_unnamed_arg {
+                        for x in span {
+                            Self::report_positional_named_param(cx, *x, lhs, rhs);
+                        }
+                    }
+                    match (&lhs.kind, &rhs.kind) {
+                        (ExprKind::Path(_, p), ExprKind::Lit(lit)) => (args.get_named(p), lit),
+                        _ => continue,
+                    }
                 },
                 _ => {
                     unnamed_args.next();
@@ -637,6 +731,29 @@ impl Write {
         }
     }
 
+    fn report_positional_named_param(cx: &EarlyContext<'_>, span: Span, lhs: &P<Expr>, _rhs: &P<Expr>) {
+        if let ExprKind::Path(_, _p) = &lhs.kind {
+            let mut applicability = Applicability::MachineApplicable;
+            let name = snippet_with_applicability(cx, lhs.span, "name", &mut applicability);
+            // We need to do this hack as precision spans should be converted from .* to .foo$
+            let hack = snippet(cx, span, "").contains('*');
+
+            span_lint_and_sugg(
+                cx,
+                POSITIONAL_NAMED_FORMAT_PARAMETERS,
+                span,
+                &format!("named parameter {} is used as a positional parameter", name),
+                "replace it with",
+                if hack {
+                    format!("{}$", name)
+                } else {
+                    format!("{}", name)
+                },
+                applicability,
+            );
+        };
+    }
+
     fn lint_println_empty_string(&self, cx: &EarlyContext<'_>, mac: &MacCall) {
         if let (Some(fmt_str), _) = self.check_tts(cx, mac.args.inner_tokens(), false) {
             if fmt_str.symbol == kw::Empty {

tests/ui/positional_named_format_parameters.fixed

@@ -0,0 +1,56 @@
+// run-rustfix
+#![allow(unused_must_use)]
+#![allow(named_arguments_used_positionally)] // Unstable at time of writing.
+#![warn(clippy::positional_named_format_parameters)]
+
+use std::io::Write;
+
+fn main() {
+    let mut v = Vec::new();
+    let hello = "Hello";
+
+    println!("{hello:.foo$}", foo = 2);
+    writeln!(v, "{hello:.foo$}", foo = 2);
+
+    // Warnings
+    println!("{zero} {one:?}", zero = 0, one = 1);
+    println!("This is a test {zero} {one:?}", zero = 0, one = 1);
+    println!("Hello {one} is {two:.zero$}", zero = 5, one = hello, two = 0.01);
+    println!("Hello {one:zero$}!", zero = 5, one = 1);
+    println!("Hello {zero:one$}!", zero = 4, one = 1);
+    println!("Hello {zero:0one$}!", zero = 4, one = 1);
+    println!("Hello is {one:.zero$}", zero = 5, one = 0.01);
+    println!("Hello is {one:<6.zero$}", zero = 5, one = 0.01);
+    println!("{zero}, `{two:>8.one$}` has 3", zero = hello, one = 3, two = hello);
+    println!("Hello {one} is {two:.zero$}", zero = 5, one = hello, two = 0.01);
+    println!("Hello {world} {world}!", world = 5);
+
+    writeln!(v, "{zero} {one:?}", zero = 0, one = 1);
+    writeln!(v, "This is a test {zero} {one:?}", zero = 0, one = 1);
+    writeln!(v, "Hello {one} is {two:.zero$}", zero = 5, one = hello, two = 0.01);
+    writeln!(v, "Hello {one:zero$}!", zero = 4, one = 1);
+    writeln!(v, "Hello {zero:one$}!", zero = 4, one = 1);
+    writeln!(v, "Hello {zero:0one$}!", zero = 4, one = 1);
+    writeln!(v, "Hello is {one:.zero$}", zero = 3, one = 0.01);
+    writeln!(v, "Hello is {one:<6.zero$}", zero = 2, one = 0.01);
+    writeln!(v, "{zero}, `{two:>8.one$}` has 3", zero = hello, one = 3, two = hello);
+    writeln!(v, "Hello {one} is {two:.zero$}", zero = 1, one = hello, two = 0.01);
+    writeln!(v, "Hello {world} {world}!", world = 0);
+
+    // Tests from other files
+    println!("{w:w$}", w = 1);
+    println!("{p:.p$}", p = 1);
+    println!("{v}", v = 1);
+    println!("{v:v$}", v = 1);
+    println!("{v:v$}", v = 1);
+    println!("{v:v$.v$}", v = 1);
+    println!("{v:v$.v$}", v = 1);
+    println!("{v:v$.v$}", v = 1);
+    println!("{v:v$.v$}", v = 1);
+    println!("{v:v$.v$}", v = 1);
+    println!("{v:v$.v$}", v = 1);
+    println!("{v:v$.v$}", v = 1);
+    println!("{w:w$}", w = 1);
+    println!("{p:.p$}", p = 1);
+    println!("{:p$.w$}", 1, w = 1, p = 1);
+}

tests/ui/positional_named_format_parameters.rs

@@ -0,0 +1,56 @@
+// run-rustfix
+#![allow(unused_must_use)]
+#![allow(named_arguments_used_positionally)] // Unstable at time of writing.
+#![warn(clippy::positional_named_format_parameters)]
+
+use std::io::Write;
+
+fn main() {
+    let mut v = Vec::new();
+    let hello = "Hello";
+
+    println!("{hello:.foo$}", foo = 2);
+    writeln!(v, "{hello:.foo$}", foo = 2);
+
+    // Warnings
+    println!("{} {1:?}", zero = 0, one = 1);
+    println!("This is a test { } {000001:?}", zero = 0, one = 1);
+    println!("Hello {1} is {2:.0$}", zero = 5, one = hello, two = 0.01);
+    println!("Hello {1:0$}!", zero = 5, one = 1);
+    println!("Hello {0:1$}!", zero = 4, one = 1);
+    println!("Hello {0:01$}!", zero = 4, one = 1);
+    println!("Hello is {1:.*}", zero = 5, one = 0.01);
+    println!("Hello is {:<6.*}", zero = 5, one = 0.01);
+    println!("{}, `{two:>8.*}` has 3", zero = hello, one = 3, two = hello);
+    println!("Hello {1} is {2:.0$}", zero = 5, one = hello, two = 0.01);
+    println!("Hello {world} {}!", world = 5);
+
+    writeln!(v, "{} {1:?}", zero = 0, one = 1);
+    writeln!(v, "This is a test { } {000001:?}", zero = 0, one = 1);
+    writeln!(v, "Hello {1} is {2:.0$}", zero = 5, one = hello, two = 0.01);
+    writeln!(v, "Hello {1:0$}!", zero = 4, one = 1);
+    writeln!(v, "Hello {0:1$}!", zero = 4, one = 1);
+    writeln!(v, "Hello {0:01$}!", zero = 4, one = 1);
+    writeln!(v, "Hello is {1:.*}", zero = 3, one = 0.01);
+    writeln!(v, "Hello is {:<6.*}", zero = 2, one = 0.01);
+    writeln!(v, "{}, `{two:>8.*}` has 3", zero = hello, one = 3, two = hello);
+    writeln!(v, "Hello {1} is {2:.0$}", zero = 1, one = hello, two = 0.01);
+    writeln!(v, "Hello {world} {}!", world = 0);
+
+    // Tests from other files
+    println!("{:w$}", w = 1);
+    println!("{:.p$}", p = 1);
+    println!("{}", v = 1);
+    println!("{:0$}", v = 1);
+    println!("{0:0$}", v = 1);
+    println!("{:0$.0$}", v = 1);
+    println!("{0:0$.0$}", v = 1);
+    println!("{0:0$.v$}", v = 1);
+    println!("{0:v$.0$}", v = 1);
+    println!("{v:0$.0$}", v = 1);
+    println!("{v:v$.0$}", v = 1);
+    println!("{v:0$.v$}", v = 1);
+    println!("{:w$}", w = 1);
+    println!("{:.p$}", p = 1);
+    println!("{:p$.w$}", 1, w = 1, p = 1);
+}