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 28, 2022
1 parent 1207480 commit d12944f
Show file tree
Hide file tree
Showing 8 changed files with 267 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, NamedInline, 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 @@ -187,7 +187,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
99 changes: 93 additions & 6 deletions tests/ui/uninlined_format_args.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,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 @@ -159,10 +159,6 @@ fn tester(fn_arg: i32) {
}
}

fn main() {
tester(42);
}

#[clippy::msrv = "1.57"]
fn _under_msrv() {
let local_i32 = 1;
Expand All @@ -174,3 +170,94 @@ 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_good_macro {
($fmt:literal $(, $e:expr)* $(,)?) => {
println!($fmt $(, $e)*)
}
}

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

macro_rules! my_bad_macro2 {
($fmt:literal) => {
let s = $fmt.clone();
println!("{}", s);
};
($fmt:literal, $($e:expr)+) => {
println!($fmt, $($e,)*)
};
}

// This abomination was suggested by @Alexendoo, may the Rust gods have mercy on their soul...
// https://github.com/rust-lang/rust-clippy/pull/9948#issuecomment-1327965962
macro_rules! used_twice {
(
large = $large:literal,
small = $small:literal,
$val:expr,
) => {
if $val < 5 {
println!($small, $val);
} else {
println!($large, $val);
}
};
}

fn tester2() {
let local_i32 = 1;
my_println2_args!(true, "{local_i32}");
my_println2!(true, "{local_i32}");
my_concat!("{}", local_i32);
my_good_macro!("{local_i32}");
my_good_macro!("{local_i32}",);

// FIXME: Broken false positives, currently unhandled
// my_bad_macro!("{}", local_i32);
// my_bad_macro2!("{}", local_i32);
// used_twice! {
// large = "large value: {}",
// small = "small value: {}",
// 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();
}
95 changes: 91 additions & 4 deletions tests/ui/uninlined_format_args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,6 @@ fn tester(fn_arg: i32) {
}
}

fn main() {
tester(42);
}

#[clippy::msrv = "1.57"]
fn _under_msrv() {
let local_i32 = 1;
Expand All @@ -179,3 +175,94 @@ 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_good_macro {
($fmt:literal $(, $e:expr)* $(,)?) => {
println!($fmt $(, $e)*)
}
}

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

macro_rules! my_bad_macro2 {
($fmt:literal) => {
let s = $fmt.clone();
println!("{}", s);
};
($fmt:literal, $($e:expr)+) => {
println!($fmt, $($e,)*)
};
}

// This abomination was suggested by @Alexendoo, may the Rust gods have mercy on their soul...
// https://github.com/rust-lang/rust-clippy/pull/9948#issuecomment-1327965962
macro_rules! used_twice {
(
large = $large:literal,
small = $small:literal,
$val:expr,
) => {
if $val < 5 {
println!($small, $val);
} else {
println!($large, $val);
}
};
}

fn tester2() {
let local_i32 = 1;
my_println2_args!(true, "{}", local_i32);
my_println2!(true, "{}", local_i32);
my_concat!("{}", local_i32);
my_good_macro!("{}", local_i32);
my_good_macro!("{}", local_i32,);

// FIXME: Broken false positives, currently unhandled
// my_bad_macro!("{}", local_i32);
// my_bad_macro2!("{}", local_i32);
// used_twice! {
// large = "large value: {}",
// small = "small value: {}",
// 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();
}
Loading

0 comments on commit d12944f

Please sign in to comment.