Skip to content

Commit ef69735

Browse files
smoeliussmoelius
committed
Address review comments
1 parent 99d4a1d commit ef69735

File tree

8 files changed

+198
-133
lines changed

8 files changed

+198
-133
lines changed

‎clippy_lints/src/format_args.rs

Lines changed: 55 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
22
use clippy_utils::higher::{FormatArgsArg, FormatArgsExpn, FormatExpn};
33
use clippy_utils::source::snippet_opt;
44
use clippy_utils::ty::implements_trait;
5-
use clippy_utils::{get_trait_def_id, is_diag_trait_item, match_def_path, paths};
5+
use clippy_utils::{is_diag_trait_item, match_def_path, paths};
66
use if_chain::if_chain;
77
use rustc_errors::Applicability;
88
use rustc_hir::{Expr, ExprKind};
@@ -64,18 +64,18 @@ declare_clippy_lint! {
6464
declare_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
6565

6666
const FORMAT_MACROS: &[&[&str]] = &[
67-
&["alloc", "macros", "format"],
68-
&["core", "macros", "assert_eq"],
69-
&["core", "macros", "assert_ne"],
70-
&["core", "macros", "write"],
71-
&["core", "macros", "writeln"],
72-
&["core", "macros", "builtin", "assert"],
73-
&["core", "macros", "builtin", "format_args"],
74-
&["std", "macros", "eprint"],
75-
&["std", "macros", "eprintln"],
76-
&["std", "macros", "panic"],
77-
&["std", "macros", "print"],
78-
&["std", "macros", "println"],
67+
&paths::FORMAT_MACRO,
68+
&paths::FORMAT_ARGS_MACRO,
69+
&paths::ASSERT_EQ_MACRO,
70+
&paths::ASSERT_MACRO,
71+
&paths::ASSERT_NE_MACRO,
72+
&paths::EPRINT_MACRO,
73+
&paths::EPRINTLN_MACRO,
74+
&paths::PANIC_MACRO,
75+
&paths::PRINT_MACRO,
76+
&paths::PRINTLN_MACRO,
77+
&paths::WRITE_MACRO,
78+
&paths::WRITELN_MACRO,
7979
];
8080

8181
impl<'tcx> LateLintPass<'tcx> for FormatArgs {
@@ -116,49 +116,50 @@ fn outermost_expn_data(expn_data: ExpnData) -> ExpnData {
116116
}
117117

118118
fn check_format_in_format_args(cx: &LateContext<'_>, call_site: Span, name: Symbol, arg: &FormatArgsArg<'_>) {
119-
if FormatExpn::parse(arg.value).is_some() {
120-
span_lint_and_then(
121-
cx,
122-
FORMAT_IN_FORMAT_ARGS,
123-
trim_semicolon(cx, call_site),
124-
&format!("`format!` in `{}!` args", name),
125-
|diag| {
126-
diag.help(&format!(
127-
"combine the `format!(..)` arguments with the outer `{}!(..)` call",
128-
name
129-
));
130-
diag.help("or consider changing `format!` to `format_args!`");
131-
},
132-
);
119+
if_chain! {
120+
if FormatExpn::parse(arg.value).is_some();
121+
if !arg.value.span.ctxt().outer_expn_data().call_site.from_expansion();
122+
then {
123+
span_lint_and_then(
124+
cx,
125+
FORMAT_IN_FORMAT_ARGS,
126+
trim_semicolon(cx, call_site),
127+
&format!("`format!` in `{}!` args", name),
128+
|diag| {
129+
diag.help(&format!(
130+
"combine the `format!(..)` arguments with the outer `{}!(..)` call",
131+
name
132+
));
133+
diag.help("or consider changing `format!` to `format_args!`");
134+
},
135+
);
136+
}
133137
}
134138
}
135139

136140
fn check_to_string_in_format_args<'tcx>(cx: &LateContext<'tcx>, name: Symbol, arg: &FormatArgsArg<'tcx>) {
137141
let value = arg.value;
138142
if_chain! {
143+
if !value.span.from_expansion();
139144
if let ExprKind::MethodCall(_, _, [receiver], _) = value.kind;
140145
if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(value.hir_id);
141146
if is_diag_trait_item(cx, method_def_id, sym::ToString);
142147
let receiver_ty = cx.typeck_results().expr_ty(receiver);
143148
if let Some(display_trait_id) = cx.tcx.get_diagnostic_item(sym::Display);
144-
if let Some(value_snippet) = snippet_opt(cx, value.span);
145-
if let Some(dot) = value_snippet.rfind('.');
146149
if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
147150
then {
148-
let (n_derefs, target) = count_derefs(
151+
let (n_overloaded_derefs, target) = count_overloaded_derefs(
152+
0,
149153
0,
150154
receiver_ty,
151155
&mut cx.typeck_results().expr_adjustments(receiver).iter(),
152156
);
153157
if implements_trait(cx, target, display_trait_id, &[]) {
154-
if n_derefs == 0 {
155-
let span = value.span.with_lo(
156-
value.span.lo() + BytePos(u32::try_from(dot).unwrap())
157-
);
158+
if n_overloaded_derefs == 0 {
158159
span_lint_and_sugg(
159160
cx,
160161
TO_STRING_IN_FORMAT_ARGS,
161-
span,
162+
value.span.with_lo(receiver.span.hi()),
162163
&format!("`to_string` applied to a type that implements `Display` in `{}!` args", name),
163164
"remove this",
164165
String::new(),
@@ -171,7 +172,7 @@ fn check_to_string_in_format_args<'tcx>(cx: &LateContext<'tcx>, name: Symbol, ar
171172
value.span,
172173
&format!("`to_string` applied to a type that implements `Display` in `{}!` args", name),
173174
"use this",
174-
format!("{:*>width$}{}", "", receiver_snippet, width = n_derefs),
175+
format!("{:*>width$}{}", "", receiver_snippet, width = n_overloaded_derefs),
175176
Applicability::MachineApplicable,
176177
);
177178
}
@@ -195,17 +196,31 @@ fn trim_semicolon(cx: &LateContext<'_>, span: Span) -> Span {
195196
})
196197
}
197198

198-
fn count_derefs<'tcx, I>(n: usize, ty: Ty<'tcx>, iter: &mut I) -> (usize, Ty<'tcx>)
199+
fn count_overloaded_derefs<'tcx, I>(
200+
n_total: usize,
201+
n_overloaded: usize,
202+
ty: Ty<'tcx>,
203+
iter: &mut I,
204+
) -> (usize, Ty<'tcx>)
199205
where
200206
I: Iterator<Item = &'tcx Adjustment<'tcx>>,
201207
{
202208
if let Some(Adjustment {
203-
kind: Adjust::Deref(Some(_)),
209+
kind: Adjust::Deref(overloaded_deref),
204210
target,
205211
}) = iter.next()
206212
{
207-
count_derefs(n + 1, target, iter)
213+
count_overloaded_derefs(
214+
n_total + 1,
215+
if overloaded_deref.is_some() {
216+
n_total + 1
217+
} else {
218+
n_overloaded
219+
},
220+
target,
221+
iter,
222+
)
208223
} else {
209-
(n, ty)
224+
(n_overloaded, ty)
210225
}
211226
}

‎clippy_utils/src/higher.rs

Lines changed: 30 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -575,35 +575,39 @@ impl FormatArgsExpn<'tcx> {
575575

576576
/// Returns a vector of `FormatArgsArg`.
577577
pub fn args(&self) -> Option<Vec<FormatArgsArg<'tcx>>> {
578-
if_chain! {
579-
if let Some(expr) = self.fmt_expr;
580-
if let ExprKind::AddrOf(BorrowKind::Ref, _, expr) = expr.kind;
581-
if let ExprKind::Array(exprs) = expr.kind;
582-
then {
583-
return exprs.iter().map(|fmt| {
584-
if_chain! {
585-
// struct `core::fmt::rt::v1::Argument`
586-
if let ExprKind::Struct(_, fields, _) = fmt.kind;
587-
if let Some(position_field) = fields.iter().find(|f| f.ident.name == sym::position);
588-
if let ExprKind::Lit(lit) = &position_field.expr.kind;
589-
if let LitKind::Int(position, _) = lit.node;
590-
then {
591-
let i = usize::try_from(position).unwrap();
592-
Some(FormatArgsArg { value: self.value_args[i], arg: &self.args[i], fmt: Some(fmt) })
593-
} else {
594-
None
578+
if let Some(expr) = self.fmt_expr {
579+
if_chain! {
580+
if let ExprKind::AddrOf(BorrowKind::Ref, _, expr) = expr.kind;
581+
if let ExprKind::Array(exprs) = expr.kind;
582+
then {
583+
exprs.iter().map(|fmt| {
584+
if_chain! {
585+
// struct `core::fmt::rt::v1::Argument`
586+
if let ExprKind::Struct(_, fields, _) = fmt.kind;
587+
if let Some(position_field) = fields.iter().find(|f| f.ident.name == sym::position);
588+
if let ExprKind::Lit(lit) = &position_field.expr.kind;
589+
if let LitKind::Int(position, _) = lit.node;
590+
then {
591+
let i = usize::try_from(position).unwrap();
592+
Some(FormatArgsArg { value: self.value_args[i], arg: &self.args[i], fmt: Some(fmt) })
593+
} else {
594+
None
595+
}
595596
}
596-
}
597-
}).collect()
597+
}).collect()
598+
} else {
599+
None
600+
}
598601
}
602+
} else {
603+
Some(
604+
self.value_args
605+
.iter()
606+
.zip(self.args.iter())
607+
.map(|(value, arg)| FormatArgsArg { value, arg, fmt: None })
608+
.collect(),
609+
)
599610
}
600-
Some(
601-
self.value_args
602-
.iter()
603-
.zip(self.args.iter())
604-
.map(|(value, arg)| FormatArgsArg { value, arg, fmt: None })
605-
.collect(),
606-
)
607611
}
608612
}
609613

‎clippy_utils/src/paths.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@ pub const APPLICABILITY_VALUES: [[&str; 3]; 4] = [
1717
#[cfg(feature = "metadata-collector-lint")]
1818
pub const DIAGNOSTIC_BUILDER: [&str; 3] = ["rustc_errors", "diagnostic_builder", "DiagnosticBuilder"];
1919
pub const ARC_PTR_EQ: [&str; 4] = ["alloc", "sync", "Arc", "ptr_eq"];
20+
pub const ASSERT_EQ_MACRO: [&str; 3] = ["core", "macros", "assert_eq"];
21+
pub const ASSERT_MACRO: [&str; 4] = ["core", "macros", "builtin", "assert"];
22+
pub const ASSERT_NE_MACRO: [&str; 3] = ["core", "macros", "assert_ne"];
2023
pub const ASMUT_TRAIT: [&str; 3] = ["core", "convert", "AsMut"];
2124
pub const ASREF_TRAIT: [&str; 3] = ["core", "convert", "AsRef"];
2225
pub(super) const BEGIN_PANIC: [&str; 3] = ["std", "panicking", "begin_panic"];
@@ -42,11 +45,15 @@ pub const DROP: [&str; 3] = ["core", "mem", "drop"];
4245
pub const DURATION: [&str; 3] = ["core", "time", "Duration"];
4346
#[cfg(feature = "internal-lints")]
4447
pub const EARLY_CONTEXT: [&str; 2] = ["rustc_lint", "EarlyContext"];
48+
pub const EPRINT_MACRO: [&str; 3] = ["std", "macros", "eprint"];
49+
pub const EPRINTLN_MACRO: [&str; 3] = ["std", "macros", "eprintln"];
4550
pub const EXIT: [&str; 3] = ["std", "process", "exit"];
4651
pub const F32_EPSILON: [&str; 4] = ["core", "f32", "<impl f32>", "EPSILON"];
4752
pub const F64_EPSILON: [&str; 4] = ["core", "f64", "<impl f64>", "EPSILON"];
4853
pub const FILE: [&str; 3] = ["std", "fs", "File"];
4954
pub const FILE_TYPE: [&str; 3] = ["std", "fs", "FileType"];
55+
pub const FORMAT_ARGS_MACRO: [&str; 4] = ["core", "macros", "builtin", "format_args"];
56+
pub const FORMAT_MACRO: [&str; 3] = ["alloc", "macros", "format"];
5057
pub const FROM_FROM: [&str; 4] = ["core", "convert", "From", "from"];
5158
pub const FROM_ITERATOR: [&str; 5] = ["core", "iter", "traits", "collect", "FromIterator"];
5259
pub const FROM_ITERATOR_METHOD: [&str; 6] = ["core", "iter", "traits", "collect", "FromIterator", "from_iter"];
@@ -99,6 +106,7 @@ pub(super) const PANICKING_PANIC: [&str; 3] = ["core", "panicking", "panic"];
99106
pub(super) const PANICKING_PANIC_FMT: [&str; 3] = ["core", "panicking", "panic_fmt"];
100107
pub(super) const PANICKING_PANIC_STR: [&str; 3] = ["core", "panicking", "panic_str"];
101108
pub(super) const PANIC_ANY: [&str; 3] = ["std", "panic", "panic_any"];
109+
pub const PANIC_MACRO: [&str; 3] = ["std", "macros", "panic"];
102110
pub const PARKING_LOT_MUTEX_GUARD: [&str; 2] = ["parking_lot", "MutexGuard"];
103111
pub const PARKING_LOT_RWLOCK_READ_GUARD: [&str; 2] = ["parking_lot", "RwLockReadGuard"];
104112
pub const PARKING_LOT_RWLOCK_WRITE_GUARD: [&str; 2] = ["parking_lot", "RwLockWriteGuard"];
@@ -109,6 +117,8 @@ pub const PERMISSIONS_FROM_MODE: [&str; 6] = ["std", "os", "unix", "fs", "Permis
109117
pub const POLL: [&str; 4] = ["core", "task", "poll", "Poll"];
110118
pub const POLL_PENDING: [&str; 5] = ["core", "task", "poll", "Poll", "Pending"];
111119
pub const POLL_READY: [&str; 5] = ["core", "task", "poll", "Poll", "Ready"];
120+
pub const PRINT_MACRO: [&str; 3] = ["std", "macros", "print"];
121+
pub const PRINTLN_MACRO: [&str; 3] = ["std", "macros", "println"];
112122
pub const PTR_COPY: [&str; 3] = ["core", "intrinsics", "copy"];
113123
pub const PTR_COPY_NONOVERLAPPING: [&str; 3] = ["core", "intrinsics", "copy_nonoverlapping"];
114124
pub const PTR_EQ: [&str; 3] = ["core", "ptr", "eq"];
@@ -185,3 +195,5 @@ pub const VEC_NEW: [&str; 4] = ["alloc", "vec", "Vec", "new"];
185195
pub const VEC_RESIZE: [&str; 4] = ["alloc", "vec", "Vec", "resize"];
186196
pub const WEAK_ARC: [&str; 3] = ["alloc", "sync", "Weak"];
187197
pub const WEAK_RC: [&str; 3] = ["alloc", "rc", "Weak"];
198+
pub const WRITE_MACRO: [&str; 3] = ["core", "macros", "write"];
199+
pub const WRITELN_MACRO: [&str; 3] = ["core", "macros", "writeln"];

‎tests/ui/format_args.fixed

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,12 @@ impl Deref for X {
2929
}
3030
}
3131

32-
struct Y(X);
32+
struct Y<'a>(&'a X);
3333

34-
impl Deref for Y {
35-
type Target = X;
34+
impl<'a> Deref for Y<'a> {
35+
type Target = &'a X;
3636

37-
fn deref(&self) -> &X {
37+
fn deref(&self) -> &Self::Target {
3838
&self.0
3939
}
4040
}
@@ -55,8 +55,22 @@ impl std::fmt::Display for Z {
5555
}
5656
}
5757

58+
macro_rules! my_macro {
59+
() => {
60+
// here be dragons, do not enter (or lint)
61+
println!("error: something failed at {}", Location::caller().to_string());
62+
};
63+
}
64+
65+
macro_rules! my_other_macro {
66+
() => {
67+
Location::caller().to_string()
68+
};
69+
}
70+
5871
fn main() {
5972
let x = 'x';
73+
let x_ref = &x;
6074

6175
let _ = format!("error: something failed at {}", Location::caller());
6276
let _ = write!(
@@ -79,16 +93,12 @@ fn main() {
7993
assert_ne!(0, 0, "error: something failed at {}", Location::caller());
8094
panic!("error: something failed at {}", Location::caller());
8195
println!("{}", *X(1));
82-
println!("{}", **Y(X(1)));
96+
println!("{}", ***Y(&X(1)));
8397
println!("{}", Z(1));
98+
println!("{}", x_ref);
8499

85100
println!("error: something failed at {}", Somewhere.to_string());
86101
println!("{} and again {0}", x.to_string());
87-
}
88-
89-
macro_rules! my_macro {
90-
() => {
91-
// here be dragons, do not enter (or lint)
92-
println!("error: something failed at {}", Location::caller().to_string());
93-
};
102+
my_macro!();
103+
println!("error: something failed at {}", my_other_macro!());
94104
}

‎tests/ui/format_args.rs

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,12 @@ impl Deref for X {
2929
}
3030
}
3131

32-
struct Y(X);
32+
struct Y<'a>(&'a X);
3333

34-
impl Deref for Y {
35-
type Target = X;
34+
impl<'a> Deref for Y<'a> {
35+
type Target = &'a X;
3636

37-
fn deref(&self) -> &X {
37+
fn deref(&self) -> &Self::Target {
3838
&self.0
3939
}
4040
}
@@ -55,8 +55,22 @@ impl std::fmt::Display for Z {
5555
}
5656
}
5757

58+
macro_rules! my_macro {
59+
() => {
60+
// here be dragons, do not enter (or lint)
61+
println!("error: something failed at {}", Location::caller().to_string());
62+
};
63+
}
64+
65+
macro_rules! my_other_macro {
66+
() => {
67+
Location::caller().to_string()
68+
};
69+
}
70+
5871
fn main() {
5972
let x = 'x';
73+
let x_ref = &x;
6074

6175
let _ = format!("error: something failed at {}", Location::caller().to_string());
6276
let _ = write!(
@@ -79,16 +93,12 @@ fn main() {
7993
assert_ne!(0, 0, "error: something failed at {}", Location::caller().to_string());
8094
panic!("error: something failed at {}", Location::caller().to_string());
8195
println!("{}", X(1).to_string());
82-
println!("{}", Y(X(1)).to_string());
96+
println!("{}", Y(&X(1)).to_string());
8397
println!("{}", Z(1).to_string());
98+
println!("{}", x_ref.to_string());
8499

85100
println!("error: something failed at {}", Somewhere.to_string());
86101
println!("{} and again {0}", x.to_string());
87-
}
88-
89-
macro_rules! my_macro {
90-
() => {
91-
// here be dragons, do not enter (or lint)
92-
println!("error: something failed at {}", Location::caller().to_string());
93-
};
102+
my_macro!();
103+
println!("error: something failed at {}", my_other_macro!());
94104
}

0 commit comments

Comments
 (0)