Skip to content

Commit

Permalink
Fix #7903 and to_string_in_format_args false negative
Browse files Browse the repository at this point in the history
  • Loading branch information
@smoelius
smoelius committed Oct 31, 2021
1 parent 310ecb0 commit 5ad10c8
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 39 deletions.
48 changes: 32 additions & 16 deletions clippy_lints/src/format_args.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,17 +95,9 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
if let Some(args) = format_args.args();
then {
for (i, arg) in args.iter().enumerate() {
if !arg.is_display() {
continue;
}
if arg.has_string_formatting() {
continue;
}
if is_aliased(&args, i) {
continue;
}
check_format_in_format_args(cx, outermost_expn_data.call_site, name, arg);
check_to_string_in_format_args(cx, name, arg);
let aliases = aliases(&args, i);
check_format_in_format_args(cx, outermost_expn_data.call_site, name, arg, &aliases);
check_to_string_in_format_args(cx, name, arg, &aliases);
}
}
}
Expand All @@ -120,8 +112,17 @@ fn outermost_expn_data(expn_data: ExpnData) -> ExpnData {
}
}

fn check_format_in_format_args(cx: &LateContext<'_>, call_site: Span, name: Symbol, arg: &FormatArgsArg<'_>) {
fn check_format_in_format_args(
cx: &LateContext<'_>,
call_site: Span,
name: Symbol,
arg: &FormatArgsArg<'_>,
aliases: &[&FormatArgsArg<'_>],
) {
if_chain! {
if arg.is_display();
if !arg.has_string_formatting();
if aliases.is_empty();
if FormatExpn::parse(arg.value).is_some();
if !arg.value.span.ctxt().outer_expn_data().call_site.from_expansion();
then {
Expand All @@ -142,9 +143,17 @@ fn check_format_in_format_args(cx: &LateContext<'_>, call_site: Span, name: Symb
}
}

fn check_to_string_in_format_args<'tcx>(cx: &LateContext<'tcx>, name: Symbol, arg: &FormatArgsArg<'tcx>) {
fn check_to_string_in_format_args<'tcx>(
cx: &LateContext<'tcx>,
name: Symbol,
arg: &FormatArgsArg<'tcx>,
aliases: &[&FormatArgsArg<'tcx>],
) {
let value = arg.value;
if_chain! {
if std::iter::once(&arg)
.chain(aliases)
.all(|arg| arg.is_display() && !arg.has_string_formatting());
if !value.span.from_expansion();
if let ExprKind::MethodCall(_, _, [receiver], _) = value.kind;
if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(value.hir_id);
Expand Down Expand Up @@ -184,12 +193,19 @@ fn check_to_string_in_format_args<'tcx>(cx: &LateContext<'tcx>, name: Symbol, ar
}
}

// Returns true if `args[i]` "refers to" or "is referred to by" another argument.
fn is_aliased(args: &[FormatArgsArg<'_>], i: usize) -> bool {
// Returns the `args[j]` that refer to value of `args[i]`, but that are not `args[i]`.
fn aliases<'a, 'tcx>(args: &'a [FormatArgsArg<'tcx>], i: usize) -> Vec<&'a FormatArgsArg<'tcx>> {
let value = args[i].value;
args.iter()
.enumerate()
.any(|(j, arg)| i != j && std::ptr::eq(value, arg.value))
.filter_map(|(j, arg)| {
if i != j && std::ptr::eq(value, arg.value) {
Some(arg)
} else {
None
}
})
.collect()
}

fn trim_semicolon(cx: &LateContext<'_>, span: Span) -> Span {
Expand Down
28 changes: 25 additions & 3 deletions clippy_utils/src/higher.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use if_chain::if_chain;
use rustc_ast::ast::{self, LitKind};
use rustc_hir as hir;
use rustc_hir::{
Arm, Block, BorrowKind, Expr, ExprKind, HirId, LoopSource, MatchSource, Node, Pat, QPath, StmtKind, UnOp,
Arm, Block, BorrowKind, Expr, ExprKind, HirId, LoopSource, MatchSource, Node, Pat, PatKind, QPath, StmtKind, UnOp,
};
use rustc_lint::LateContext;
use rustc_span::{sym, symbol, ExpnKind, Span, Symbol};
Expand Down Expand Up @@ -513,6 +513,8 @@ pub struct FormatArgsExpn<'tcx> {
pub format_string_parts: &'tcx [Expr<'tcx>],
/// Symbols corresponding to [`Self::format_string_parts`]
pub format_string_symbols: Vec<Symbol>,
/// Match arm patterns, the `arg0`, etc. from the next field `args`
pub arg_names: &'tcx [Pat<'tcx>],
/// Expressions like `ArgumentV1::new(arg0, Debug::fmt)`
pub args: &'tcx [Expr<'tcx>],
/// The final argument passed to `Arguments::new_v1_formatted`, if applicable
Expand Down Expand Up @@ -557,13 +559,15 @@ impl FormatArgsExpn<'tcx> {
_ => None,
})
.collect();
if let PatKind::Tuple(arg_names, None) = arm.pat.kind;
if let ExprKind::Array(args) = arm.body.kind;
then {
Some(FormatArgsExpn {
format_string_span: strs_ref.span,
value_args,
format_string_parts,
format_string_symbols,
arg_names,
args,
fmt_expr,
})
Expand All @@ -587,9 +591,12 @@ impl FormatArgsExpn<'tcx> {
if let Some(position_field) = fields.iter().find(|f| f.ident.name == sym::position);
if let ExprKind::Lit(lit) = &position_field.expr.kind;
if let LitKind::Int(position, _) = lit.node;
if let Ok(i) = usize::try_from(position);
let arg = &self.args[i];
if let ExprKind::Call(_, [arg_name, _]) = arg.kind;
if let Some(j) = self.arg_names.iter().position(|pat| match_arg(pat, arg_name));
then {
let i = usize::try_from(position).unwrap();
Some(FormatArgsArg { value: self.value_args[i], arg: &self.args[i], fmt: Some(fmt) })
Some(FormatArgsArg { value: self.value_args[j], arg, fmt: Some(fmt) })
} else {
None
}
Expand All @@ -611,6 +618,21 @@ impl FormatArgsExpn<'tcx> {
}
}

/// Returns true if `pat` and `arg_name` are the same ident.
fn match_arg(pat: &Pat<'_>, arg_name: &Expr<'_>) -> bool {
if_chain! {
if let PatKind::Binding(_, _, ident, _) = pat.kind;
if let ExprKind::Path(QPath::Resolved(_, path)) = &arg_name.kind;
if let [segment] = path.segments;
if ident == segment.ident;
then {
true
} else {
false
}
}
}

/// Type representing a `FormatArgsExpn`'s format arguments
pub struct FormatArgsArg<'tcx> {
/// An element of `value_args` according to `position`
Expand Down
11 changes: 10 additions & 1 deletion tests/ui/format_args.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#![allow(unused_variables)]
#![allow(clippy::assertions_on_constants)]
#![allow(clippy::eq_op)]
#![allow(clippy::print_literal)]
#![warn(clippy::to_string_in_format_args)]

use std::io::{stdout, Write};
Expand Down Expand Up @@ -97,9 +98,17 @@ fn main() {
println!("{}", Z(1));
println!("{}", **x);
println!("{}", ***x_ref);
println!("{} and again {0}", **x);
// https://github.com/rust-lang/rust-clippy/issues/7903
println!("{foo}{foo}", foo = "foo");
println!("{foo}{bar}", foo = "foo", bar = "bar");
println!("{foo}{bar}", foo = "foo", bar = "bar");
println!("{foo}{bar}", bar = "bar", foo = "foo");
println!("{foo}{bar}", bar = "bar", foo = "foo");

println!("error: something failed at {}", Somewhere.to_string());
println!("{} and again {0}", x.to_string());
my_macro!();
println!("error: something failed at {}", my_other_macro!());
// https://github.com/rust-lang/rust-clippy/issues/7903
println!("{foo}{foo:?}", foo = "foo".to_string());
}
11 changes: 10 additions & 1 deletion tests/ui/format_args.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#![allow(unused_variables)]
#![allow(clippy::assertions_on_constants)]
#![allow(clippy::eq_op)]
#![allow(clippy::print_literal)]
#![warn(clippy::to_string_in_format_args)]

use std::io::{stdout, Write};
Expand Down Expand Up @@ -97,9 +98,17 @@ fn main() {
println!("{}", Z(1).to_string());
println!("{}", x.to_string());
println!("{}", x_ref.to_string());
println!("{} and again {0}", x.to_string());
// https://github.com/rust-lang/rust-clippy/issues/7903
println!("{foo}{foo}", foo = "foo".to_string());
println!("{foo}{bar}", foo = "foo".to_string(), bar = "bar");
println!("{foo}{bar}", foo = "foo", bar = "bar".to_string());
println!("{foo}{bar}", bar = "bar".to_string(), foo = "foo");
println!("{foo}{bar}", bar = "bar", foo = "foo".to_string());

println!("error: something failed at {}", Somewhere.to_string());
println!("{} and again {0}", x.to_string());
my_macro!();
println!("error: something failed at {}", my_other_macro!());
// https://github.com/rust-lang/rust-clippy/issues/7903
println!("{foo}{foo:?}", foo = "foo".to_string());
}
72 changes: 54 additions & 18 deletions tests/ui/format_args.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,106 +1,142 @@
error: `to_string` applied to a type that implements `Display` in `format!` args
--> $DIR/format_args.rs:75:72
--> $DIR/format_args.rs:76:72
|
LL | let _ = format!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this
|
= note: `-D clippy::to-string-in-format-args` implied by `-D warnings`

error: `to_string` applied to a type that implements `Display` in `write!` args
--> $DIR/format_args.rs:79:27
--> $DIR/format_args.rs:80:27
|
LL | Location::caller().to_string()
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `writeln!` args
--> $DIR/format_args.rs:84:27
--> $DIR/format_args.rs:85:27
|
LL | Location::caller().to_string()
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `print!` args
--> $DIR/format_args.rs:86:63
--> $DIR/format_args.rs:87:63
|
LL | print!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:87:65
--> $DIR/format_args.rs:88:65
|
LL | println!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `eprint!` args
--> $DIR/format_args.rs:88:64
--> $DIR/format_args.rs:89:64
|
LL | eprint!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `eprintln!` args
--> $DIR/format_args.rs:89:66
--> $DIR/format_args.rs:90:66
|
LL | eprintln!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `format_args!` args
--> $DIR/format_args.rs:90:77
--> $DIR/format_args.rs:91:77
|
LL | let _ = format_args!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `assert!` args
--> $DIR/format_args.rs:91:70
--> $DIR/format_args.rs:92:70
|
LL | assert!(true, "error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `assert_eq!` args
--> $DIR/format_args.rs:92:73
--> $DIR/format_args.rs:93:73
|
LL | assert_eq!(0, 0, "error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `assert_ne!` args
--> $DIR/format_args.rs:93:73
--> $DIR/format_args.rs:94:73
|
LL | assert_ne!(0, 0, "error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `panic!` args
--> $DIR/format_args.rs:94:63
--> $DIR/format_args.rs:95:63
|
LL | panic!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:95:20
--> $DIR/format_args.rs:96:20
|
LL | println!("{}", X(1).to_string());
| ^^^^^^^^^^^^^^^^ help: use this: `*X(1)`

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:96:20
--> $DIR/format_args.rs:97:20
|
LL | println!("{}", Y(&X(1)).to_string());
| ^^^^^^^^^^^^^^^^^^^^ help: use this: `***Y(&X(1))`

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:97:24
--> $DIR/format_args.rs:98:24
|
LL | println!("{}", Z(1).to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:98:20
--> $DIR/format_args.rs:99:20
|
LL | println!("{}", x.to_string());
| ^^^^^^^^^^^^^ help: use this: `**x`

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:99:20
--> $DIR/format_args.rs:100:20
|
LL | println!("{}", x_ref.to_string());
| ^^^^^^^^^^^^^^^^^ help: use this: `***x_ref`

error: aborting due to 17 previous errors
error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:101:34
|
LL | println!("{} and again {0}", x.to_string());
| ^^^^^^^^^^^^^ help: use this: `**x`

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:103:39
|
LL | println!("{foo}{foo}", foo = "foo".to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:104:39
|
LL | println!("{foo}{bar}", foo = "foo".to_string(), bar = "bar");
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:105:52
|
LL | println!("{foo}{bar}", foo = "foo", bar = "bar".to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:106:39
|
LL | println!("{foo}{bar}", bar = "bar".to_string(), foo = "foo");
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:107:52
|
LL | println!("{foo}{bar}", bar = "bar", foo = "foo".to_string());
| ^^^^^^^^^^^^ help: remove this

error: aborting due to 23 previous errors

0 comments on commit 5ad10c8

Please sign in to comment.