Skip to content

Commit

Permalink
Add new lint [positional_named_format_parameters]
Browse files Browse the repository at this point in the history
  • Loading branch information
miam-miam committed Aug 9, 2022
1 parent 3c7e7db commit 9ffddf5
Show file tree
Hide file tree
Showing 8 changed files with 657 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3823,6 +3823,7 @@ Released 2018-09-13
[`partialeq_ne_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_ne_impl
[`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
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,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),
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,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,
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_suspicious.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,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),
])
129 changes: 123 additions & 6 deletions clippy_lints/src/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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())
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand All @@ -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();
Expand Down Expand Up @@ -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 {
Expand Down
56 changes: 56 additions & 0 deletions tests/ui/positional_named_format_parameters.fixed
Original file line number Diff line number Diff line change
@@ -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);
}
56 changes: 56 additions & 0 deletions tests/ui/positional_named_format_parameters.rs
Original file line number Diff line number Diff line change
@@ -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);
}
Loading

0 comments on commit 9ffddf5

Please sign in to comment.