Skip to content

Commit

Permalink
Process all format-like macros
Browse files Browse the repository at this point in the history
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!(...)`
  • Loading branch information
nyurik committed Nov 25, 2022
1 parent 6d0b4e3 commit 60f8dcc
Show file tree
Hide file tree
Showing 8 changed files with 213 additions and 59 deletions.
8 changes: 2 additions & 6 deletions clippy_dev/src/new_lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,9 +368,7 @@ fn create_lint_for_ty(lint: &LintData<'_>, enable_msrv: bool, ty: &str) -> io::R
}}
todo!();
}}
"#,
context_import = context_import,
name_upper = name_upper,
"#
);
} else {
let _ = writedoc!(
Expand All @@ -384,9 +382,7 @@ fn create_lint_for_ty(lint: &LintData<'_>, enable_msrv: bool, ty: &str) -> io::R
pub(super) fn check(cx: &{context_import}) {{
todo!();
}}
"#,
context_import = context_import,
name_upper = name_upper,
"#
);
}

Expand Down
14 changes: 5 additions & 9 deletions clippy_dev/src/update_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -537,17 +537,13 @@ fn declare_deprecated(name: &str, path: &Path, reason: &str) -> io::Result<()> {
/// Nothing. This lint has been deprecated.
///
/// ### Deprecation reason
/// {}
#[clippy::version = \"{}\"]
pub {},
\"{}\"
/// {deprecation_reason}
#[clippy::version = \"{version}\"]
pub {name},
\"{reason}\"
}}
",
deprecation_reason,
version,
name,
reason,
"
)
}

Expand Down
3 changes: 1 addition & 2 deletions clippy_lints/src/format_args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::is_diag_trait_item;
use clippy_utils::macros::FormatParamKind::{Implicit, Named, Numbered, Starred};
use clippy_utils::macros::{
is_format_macro, is_panic, root_macro_call, Count, FormatArg, FormatArgsExpn, FormatParam, FormatParamUsage,
is_panic, root_macro_call, Count, FormatArg, FormatArgsExpn, FormatParam, FormatParamUsage,
};
use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::source::snippet_opt;
Expand Down Expand Up @@ -177,7 +177,6 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
&& let expr_expn_data = expr.span.ctxt().outer_expn_data()
&& let outermost_expn_data = outermost_expn_data(expr_expn_data)
&& let Some(macro_def_id) = outermost_expn_data.macro_def_id
&& is_format_macro(cx, macro_def_id)
&& let ExpnKind::Macro(_, name) = outermost_expn_data.kind
{
for arg in &format_args.args {
Expand Down
4 changes: 1 addition & 3 deletions clippy_lints/src/format_impl.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg};
use clippy_utils::macros::{is_format_macro, root_macro_call_first_node, FormatArg, FormatArgsExpn};
use clippy_utils::macros::{root_macro_call_first_node, FormatArg, FormatArgsExpn};
use clippy_utils::{get_parent_as_impl, is_diag_trait_item, path_to_local, peel_ref_operators};
use if_chain::if_chain;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -165,9 +165,7 @@ fn check_self_in_format_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>,
// Check each arg in format calls - do we ever use Display on self (directly or via deref)?
if_chain! {
if let Some(outer_macro) = root_macro_call_first_node(cx, expr);
if let macro_def_id = outer_macro.def_id;
if let Some(format_args) = FormatArgsExpn::find_nested(cx, expr, outer_macro.expn);
if is_format_macro(cx, macro_def_id);
then {
for arg in format_args.args {
if arg.format.r#trait != impl_trait.name {
Expand Down
27 changes: 0 additions & 27 deletions clippy_utils/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,33 +19,6 @@ use rustc_span::{sym, BytePos, ExpnData, ExpnId, ExpnKind, Pos, Span, SpanData,
use std::iter::{once, zip};
use std::ops::ControlFlow;

const FORMAT_MACRO_DIAG_ITEMS: &[Symbol] = &[
sym::assert_eq_macro,
sym::assert_macro,
sym::assert_ne_macro,
sym::debug_assert_eq_macro,
sym::debug_assert_macro,
sym::debug_assert_ne_macro,
sym::eprint_macro,
sym::eprintln_macro,
sym::format_args_macro,
sym::format_macro,
sym::print_macro,
sym::println_macro,
sym::std_panic_macro,
sym::write_macro,
sym::writeln_macro,
];

/// Returns true if a given Macro `DefId` is a format macro (e.g. `println!`)
pub fn is_format_macro(cx: &LateContext<'_>, macro_def_id: DefId) -> bool {
if let Some(name) = cx.tcx.get_diagnostic_name(macro_def_id) {
FORMAT_MACRO_DIAG_ITEMS.contains(&name)
} else {
false
}
}

/// A macro call, like `vec![1, 2, 3]`.
///
/// Use `tcx.item_name(macro_call.def_id)` to get the macro name.
Expand Down
66 changes: 60 additions & 6 deletions tests/ui/uninlined_format_args.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,8 @@ fn tester(fn_arg: i32) {
println!("{local_i32:width$.prec$}");
println!("{width:width$.prec$}");
println!("{}", format!("{local_i32}"));
my_println!("{}", local_i32);
my_println_args!("{}", local_i32);
my_println!("{local_i32}");
my_println_args!("{local_i32}");

// these should NOT be modified by the lint
println!(concat!("nope ", "{}"), local_i32);
Expand Down Expand Up @@ -160,10 +160,6 @@ fn tester(fn_arg: i32) {
}
}

fn main() {
tester(42);
}

fn _under_msrv() {
#![clippy::msrv = "1.57"]
let local_i32 = 1;
Expand All @@ -175,3 +171,61 @@ fn _meets_msrv() {
let local_i32 = 1;
println!("expand='{local_i32}'");
}

macro_rules! _internal {
($($args:tt)*) => {
println!("{}", format_args!($($args)*))
};
}

macro_rules! my_println2 {
($target:expr, $($args:tt)+) => {{
if $target {
_internal!($($args)+)
}
}};
}

macro_rules! my_println2_args {
($target:expr, $($args:tt)+) => {{
if $target {
_internal!("foo: {}", format_args!($($args)+))
}
}};
}

macro_rules! my_concat {
($fmt:literal $(, $e:expr)*) => {
println!(concat!("ERROR: ", $fmt), $($e,)*)
}
}

macro_rules! my_bad_macro {
($fmt:literal, $($e:expr),*) => {
println!($fmt, $($e,)*)
}
}

macro_rules! my_good_macro {
($fmt:literal $(, $e:expr)* $(,)?) => {
println!($fmt $(, $e)*)
}
}

fn tester2() {
let local_i32 = 1;
my_println2_args!(true, "{local_i32}");
my_println2!(true, "{local_i32}");
my_concat!("{}", local_i32);
my_bad_macro!("{local_i32}");
// my_bad_macro!("{}", local_i32, ); // <-- this is a syntax error with my_bad_macro! call
my_good_macro!("{local_i32}");
my_good_macro!("{local_i32}", );
}

// Add new tests right above this line to keep existing test line numbers intact.
// The main function is the only one that be kept at the end.
fn main() {
tester(42);
tester2();
}
62 changes: 58 additions & 4 deletions tests/ui/uninlined_format_args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,6 @@ fn tester(fn_arg: i32) {
}
}

fn main() {
tester(42);
}

fn _under_msrv() {
#![clippy::msrv = "1.57"]
let local_i32 = 1;
Expand All @@ -180,3 +176,61 @@ fn _meets_msrv() {
let local_i32 = 1;
println!("expand='{}'", local_i32);
}

macro_rules! _internal {
($($args:tt)*) => {
println!("{}", format_args!($($args)*))
};
}

macro_rules! my_println2 {
($target:expr, $($args:tt)+) => {{
if $target {
_internal!($($args)+)
}
}};
}

macro_rules! my_println2_args {
($target:expr, $($args:tt)+) => {{
if $target {
_internal!("foo: {}", format_args!($($args)+))
}
}};
}

macro_rules! my_concat {
($fmt:literal $(, $e:expr)*) => {
println!(concat!("ERROR: ", $fmt), $($e,)*)
}
}

macro_rules! my_bad_macro {
($fmt:literal, $($e:expr),*) => {
println!($fmt, $($e,)*)
}
}

macro_rules! my_good_macro {
($fmt:literal $(, $e:expr)* $(,)?) => {
println!($fmt $(, $e)*)
}
}

fn tester2() {
let local_i32 = 1;
my_println2_args!(true, "{}", local_i32);
my_println2!(true, "{}", local_i32);
my_concat!("{}", local_i32);
my_bad_macro!("{}", local_i32);
// my_bad_macro!("{}", local_i32, ); // <-- this is a syntax error with my_bad_macro! call
my_good_macro!("{}", local_i32);
my_good_macro!("{}", local_i32, );
}

// Add new tests right above this line to keep existing test line numbers intact.
// The main function is the only one that be kept at the end.
fn main() {
tester(42);
tester2();
}
88 changes: 86 additions & 2 deletions tests/ui/uninlined_format_args.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -834,6 +834,30 @@ LL - println!("{}", format!("{}", local_i32));
LL + println!("{}", format!("{local_i32}"));
|

error: variables can be used directly in the `format!` string
--> $DIR/uninlined_format_args.rs:127:5
|
LL | my_println!("{}", local_i32);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: change this to
|
LL - my_println!("{}", local_i32);
LL + my_println!("{local_i32}");
|

error: variables can be used directly in the `format!` string
--> $DIR/uninlined_format_args.rs:128:5
|
LL | my_println_args!("{}", local_i32);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: change this to
|
LL - my_println_args!("{}", local_i32);
LL + my_println_args!("{local_i32}");
|

error: variables can be used directly in the `format!` string
--> $DIR/uninlined_format_args.rs:144:5
|
Expand Down Expand Up @@ -893,7 +917,7 @@ LL + panic!("p3 {local_i32}");
|

error: variables can be used directly in the `format!` string
--> $DIR/uninlined_format_args.rs:181:5
--> $DIR/uninlined_format_args.rs:177:5
|
LL | println!("expand='{}'", local_i32);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -904,5 +928,65 @@ LL - println!("expand='{}'", local_i32);
LL + println!("expand='{local_i32}'");
|

error: aborting due to 76 previous errors
error: variables can be used directly in the `format!` string
--> $DIR/uninlined_format_args.rs:222:5
|
LL | my_println2_args!(true, "{}", local_i32);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: change this to
|
LL - my_println2_args!(true, "{}", local_i32);
LL + my_println2_args!(true, "{local_i32}");
|

error: variables can be used directly in the `format!` string
--> $DIR/uninlined_format_args.rs:223:5
|
LL | my_println2!(true, "{}", local_i32);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: change this to
|
LL - my_println2!(true, "{}", local_i32);
LL + my_println2!(true, "{local_i32}");
|

error: variables can be used directly in the `format!` string
--> $DIR/uninlined_format_args.rs:225:5
|
LL | my_bad_macro!("{}", local_i32);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: change this to
|
LL - my_bad_macro!("{}", local_i32);
LL + my_bad_macro!("{local_i32}");
|

error: variables can be used directly in the `format!` string
--> $DIR/uninlined_format_args.rs:227:5
|
LL | my_good_macro!("{}", local_i32);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: change this to
|
LL - my_good_macro!("{}", local_i32);
LL + my_good_macro!("{local_i32}");
|

error: variables can be used directly in the `format!` string
--> $DIR/uninlined_format_args.rs:228:5
|
LL | my_good_macro!("{}", local_i32, );
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: change this to
|
LL - my_good_macro!("{}", local_i32, );
LL + my_good_macro!("{local_i32}", );
|

error: aborting due to 83 previous errors

0 comments on commit 60f8dcc

Please sign in to comment.