Skip to content

Commit 39fadaf

Browse files
nyuriknyurik
committed
Process all format-like macros
Remove the `is_format_macro()` function and calls to it because now it seems to be working just fine without it, properly handling all other use cases, such as `log::warn!(...)`
1 parent 911864d commit 39fadaf

File tree

8 files changed

+267
-59
lines changed

8 files changed

+267
-59
lines changed

‎clippy_dev/src/new_lint.rs

+2-6
Original file line numberDiff line numberDiff line change
@@ -368,9 +368,7 @@ fn create_lint_for_ty(lint: &LintData<'_>, enable_msrv: bool, ty: &str) -> io::R
368368
}}
369369
todo!();
370370
}}
371-
"#,
372-
context_import = context_import,
373-
name_upper = name_upper,
371+
"#
374372
);
375373
} else {
376374
let _ = writedoc!(
@@ -384,9 +382,7 @@ fn create_lint_for_ty(lint: &LintData<'_>, enable_msrv: bool, ty: &str) -> io::R
384382
pub(super) fn check(cx: &{context_import}) {{
385383
todo!();
386384
}}
387-
"#,
388-
context_import = context_import,
389-
name_upper = name_upper,
385+
"#
390386
);
391387
}
392388

‎clippy_dev/src/update_lints.rs

+5-9
Original file line numberDiff line numberDiff line change
@@ -537,17 +537,13 @@ fn declare_deprecated(name: &str, path: &Path, reason: &str) -> io::Result<()> {
537537
/// Nothing. This lint has been deprecated.
538538
///
539539
/// ### Deprecation reason
540-
/// {}
541-
#[clippy::version = \"{}\"]
542-
pub {},
543-
\"{}\"
540+
/// {deprecation_reason}
541+
#[clippy::version = \"{version}\"]
542+
pub {name},
543+
\"{reason}\"
544544
}}
545545
546-
",
547-
deprecation_reason,
548-
version,
549-
name,
550-
reason,
546+
"
551547
)
552548
}
553549

‎clippy_lints/src/format_args.rs

+1-2
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::is_diag_trait_item;
33
use clippy_utils::macros::FormatParamKind::{Implicit, Named, NamedInline, Numbered, Starred};
44
use clippy_utils::macros::{
5-
is_format_macro, is_panic, root_macro_call, Count, FormatArg, FormatArgsExpn, FormatParam, FormatParamUsage,
5+
is_panic, root_macro_call, Count, FormatArg, FormatArgsExpn, FormatParam, FormatParamUsage,
66
};
77
use clippy_utils::msrvs::{self, Msrv};
88
use clippy_utils::source::snippet_opt;
@@ -187,7 +187,6 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
187187
&& let expr_expn_data = expr.span.ctxt().outer_expn_data()
188188
&& let outermost_expn_data = outermost_expn_data(expr_expn_data)
189189
&& let Some(macro_def_id) = outermost_expn_data.macro_def_id
190-
&& is_format_macro(cx, macro_def_id)
191190
&& let ExpnKind::Macro(_, name) = outermost_expn_data.kind
192191
{
193192
for arg in &format_args.args {

‎clippy_lints/src/format_impl.rs

+1-3
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg};
2-
use clippy_utils::macros::{is_format_macro, root_macro_call_first_node, FormatArg, FormatArgsExpn};
2+
use clippy_utils::macros::{root_macro_call_first_node, FormatArg, FormatArgsExpn};
33
use clippy_utils::{get_parent_as_impl, is_diag_trait_item, path_to_local, peel_ref_operators};
44
use if_chain::if_chain;
55
use rustc_errors::Applicability;
@@ -165,9 +165,7 @@ fn check_self_in_format_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>,
165165
// Check each arg in format calls - do we ever use Display on self (directly or via deref)?
166166
if_chain! {
167167
if let Some(outer_macro) = root_macro_call_first_node(cx, expr);
168-
if let macro_def_id = outer_macro.def_id;
169168
if let Some(format_args) = FormatArgsExpn::find_nested(cx, expr, outer_macro.expn);
170-
if is_format_macro(cx, macro_def_id);
171169
then {
172170
for arg in format_args.args {
173171
if arg.format.r#trait != impl_trait.name {

‎clippy_utils/src/macros.rs

-27
Original file line numberDiff line numberDiff line change
@@ -19,33 +19,6 @@ use rustc_span::{sym, BytePos, ExpnData, ExpnId, ExpnKind, Pos, Span, SpanData,
1919
use std::iter::{once, zip};
2020
use std::ops::ControlFlow;
2121

22-
const FORMAT_MACRO_DIAG_ITEMS: &[Symbol] = &[
23-
sym::assert_eq_macro,
24-
sym::assert_macro,
25-
sym::assert_ne_macro,
26-
sym::debug_assert_eq_macro,
27-
sym::debug_assert_macro,
28-
sym::debug_assert_ne_macro,
29-
sym::eprint_macro,
30-
sym::eprintln_macro,
31-
sym::format_args_macro,
32-
sym::format_macro,
33-
sym::print_macro,
34-
sym::println_macro,
35-
sym::std_panic_macro,
36-
sym::write_macro,
37-
sym::writeln_macro,
38-
];
39-
40-
/// Returns true if a given Macro `DefId` is a format macro (e.g. `println!`)
41-
pub fn is_format_macro(cx: &LateContext<'_>, macro_def_id: DefId) -> bool {
42-
if let Some(name) = cx.tcx.get_diagnostic_name(macro_def_id) {
43-
FORMAT_MACRO_DIAG_ITEMS.contains(&name)
44-
} else {
45-
false
46-
}
47-
}
48-
4922
/// A macro call, like `vec![1, 2, 3]`.
5023
///
5124
/// Use `tcx.item_name(macro_call.def_id)` to get the macro name.

‎tests/ui/uninlined_format_args.fixed

+93-6
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,8 @@ fn tester(fn_arg: i32) {
120120
println!("{local_i32:width$.prec$}");
121121
println!("{width:width$.prec$}");
122122
println!("{}", format!("{local_i32}"));
123-
my_println!("{}", local_i32);
124-
my_println_args!("{}", local_i32);
123+
my_println!("{local_i32}");
124+
my_println_args!("{local_i32}");
125125

126126
// these should NOT be modified by the lint
127127
println!(concat!("nope ", "{}"), local_i32);
@@ -159,10 +159,6 @@ fn tester(fn_arg: i32) {
159159
}
160160
}
161161

162-
fn main() {
163-
tester(42);
164-
}
165-
166162
#[clippy::msrv = "1.57"]
167163
fn _under_msrv() {
168164
let local_i32 = 1;
@@ -174,3 +170,94 @@ fn _meets_msrv() {
174170
let local_i32 = 1;
175171
println!("expand='{local_i32}'");
176172
}
173+
174+
macro_rules! _internal {
175+
($($args:tt)*) => {
176+
println!("{}", format_args!($($args)*))
177+
};
178+
}
179+
180+
macro_rules! my_println2 {
181+
($target:expr, $($args:tt)+) => {{
182+
if $target {
183+
_internal!($($args)+)
184+
}
185+
}};
186+
}
187+
188+
macro_rules! my_println2_args {
189+
($target:expr, $($args:tt)+) => {{
190+
if $target {
191+
_internal!("foo: {}", format_args!($($args)+))
192+
}
193+
}};
194+
}
195+
196+
macro_rules! my_concat {
197+
($fmt:literal $(, $e:expr)*) => {
198+
println!(concat!("ERROR: ", $fmt), $($e,)*)
199+
}
200+
}
201+
202+
macro_rules! my_good_macro {
203+
($fmt:literal $(, $e:expr)* $(,)?) => {
204+
println!($fmt $(, $e)*)
205+
}
206+
}
207+
208+
macro_rules! my_bad_macro {
209+
($fmt:literal, $($e:expr),*) => {
210+
println!($fmt, $($e,)*)
211+
}
212+
}
213+
214+
macro_rules! my_bad_macro2 {
215+
($fmt:literal) => {
216+
let s = $fmt.clone();
217+
println!("{}", s);
218+
};
219+
($fmt:literal, $($e:expr)+) => {
220+
println!($fmt, $($e,)*)
221+
};
222+
}
223+
224+
// This abomination was suggested by @Alexendoo, may the Rust gods have mercy on their soul...
225+
// https://github.com/rust-lang/rust-clippy/pull/9948#issuecomment-1327965962
226+
macro_rules! used_twice {
227+
(
228+
large = $large:literal,
229+
small = $small:literal,
230+
$val:expr,
231+
) => {
232+
if $val < 5 {
233+
println!($small, $val);
234+
} else {
235+
println!($large, $val);
236+
}
237+
};
238+
}
239+
240+
fn tester2() {
241+
let local_i32 = 1;
242+
my_println2_args!(true, "{local_i32}");
243+
my_println2!(true, "{local_i32}");
244+
my_concat!("{}", local_i32);
245+
my_good_macro!("{local_i32}");
246+
my_good_macro!("{local_i32}",);
247+
248+
// FIXME: Broken false positives, currently unhandled
249+
// my_bad_macro!("{}", local_i32);
250+
// my_bad_macro2!("{}", local_i32);
251+
// used_twice! {
252+
// large = "large value: {}",
253+
// small = "small value: {}",
254+
// local_i32,
255+
// };
256+
}
257+
258+
// Add new tests right above this line to keep existing test line numbers intact.
259+
// The main function is the only one that be kept at the end.
260+
fn main() {
261+
tester(42);
262+
tester2();
263+
}

‎tests/ui/uninlined_format_args.rs

+91-4
Original file line numberDiff line numberDiff line change
@@ -164,10 +164,6 @@ fn tester(fn_arg: i32) {
164164
}
165165
}
166166

167-
fn main() {
168-
tester(42);
169-
}
170-
171167
#[clippy::msrv = "1.57"]
172168
fn _under_msrv() {
173169
let local_i32 = 1;
@@ -179,3 +175,94 @@ fn _meets_msrv() {
179175
let local_i32 = 1;
180176
println!("expand='{}'", local_i32);
181177
}
178+
179+
macro_rules! _internal {
180+
($($args:tt)*) => {
181+
println!("{}", format_args!($($args)*))
182+
};
183+
}
184+
185+
macro_rules! my_println2 {
186+
($target:expr, $($args:tt)+) => {{
187+
if $target {
188+
_internal!($($args)+)
189+
}
190+
}};
191+
}
192+
193+
macro_rules! my_println2_args {
194+
($target:expr, $($args:tt)+) => {{
195+
if $target {
196+
_internal!("foo: {}", format_args!($($args)+))
197+
}
198+
}};
199+
}
200+
201+
macro_rules! my_concat {
202+
($fmt:literal $(, $e:expr)*) => {
203+
println!(concat!("ERROR: ", $fmt), $($e,)*)
204+
}
205+
}
206+
207+
macro_rules! my_good_macro {
208+
($fmt:literal $(, $e:expr)* $(,)?) => {
209+
println!($fmt $(, $e)*)
210+
}
211+
}
212+
213+
macro_rules! my_bad_macro {
214+
($fmt:literal, $($e:expr),*) => {
215+
println!($fmt, $($e,)*)
216+
}
217+
}
218+
219+
macro_rules! my_bad_macro2 {
220+
($fmt:literal) => {
221+
let s = $fmt.clone();
222+
println!("{}", s);
223+
};
224+
($fmt:literal, $($e:expr)+) => {
225+
println!($fmt, $($e,)*)
226+
};
227+
}
228+
229+
// This abomination was suggested by @Alexendoo, may the Rust gods have mercy on their soul...
230+
// https://github.com/rust-lang/rust-clippy/pull/9948#issuecomment-1327965962
231+
macro_rules! used_twice {
232+
(
233+
large = $large:literal,
234+
small = $small:literal,
235+
$val:expr,
236+
) => {
237+
if $val < 5 {
238+
println!($small, $val);
239+
} else {
240+
println!($large, $val);
241+
}
242+
};
243+
}
244+
245+
fn tester2() {
246+
let local_i32 = 1;
247+
my_println2_args!(true, "{}", local_i32);
248+
my_println2!(true, "{}", local_i32);
249+
my_concat!("{}", local_i32);
250+
my_good_macro!("{}", local_i32);
251+
my_good_macro!("{}", local_i32,);
252+
253+
// FIXME: Broken false positives, currently unhandled
254+
// my_bad_macro!("{}", local_i32);
255+
// my_bad_macro2!("{}", local_i32);
256+
// used_twice! {
257+
// large = "large value: {}",
258+
// small = "small value: {}",
259+
// local_i32,
260+
// };
261+
}
262+
263+
// Add new tests right above this line to keep existing test line numbers intact.
264+
// The main function is the only one that be kept at the end.
265+
fn main() {
266+
tester(42);
267+
tester2();
268+
}

0 commit comments

Comments
 (0)