From 81dceed8baf324960ea05e9079b749305b16a7cc Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Tue, 28 Mar 2023 17:04:30 -0400 Subject: [PATCH] Support user format-like macros Add support for `#[clippy::format_args]` attribute that can be attached to any macro to indicate that it functions the same as the built-in format macros like `format!`, `println!` and `write!` --- book/src/SUMMARY.md | 1 + book/src/attribs.md | 53 ++++++++++++++++++++++ clippy_utils/src/attrs.rs | 3 ++ clippy_utils/src/macros.rs | 7 ++- tests/ui/format_args_unfixable.rs | 29 ++++++++++++ tests/ui/format_args_unfixable.stderr | 65 ++++++++++++++++++++++++++- tests/ui/uninlined_format_args.fixed | 21 ++++++++- tests/ui/uninlined_format_args.rs | 21 ++++++++- tests/ui/uninlined_format_args.stderr | 50 ++++++++++++++++++++- tests/ui/unused_format_specs.1.fixed | 35 +++++++++++++++ tests/ui/unused_format_specs.2.fixed | 35 +++++++++++++++ tests/ui/unused_format_specs.rs | 35 +++++++++++++++ tests/ui/unused_format_specs.stderr | 60 ++++++++++++++++++++++++- 13 files changed, 406 insertions(+), 9 deletions(-) create mode 100644 book/src/attribs.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index be13fcbe2..19328fdd3 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -7,6 +7,7 @@ - [Configuration](configuration.md) - [Lint Configuration](lint_configuration.md) - [Clippy's Lints](lints.md) +- [Attributes for Crate Authors](attribs.md) - [Continuous Integration](continuous_integration/README.md) - [GitHub Actions](continuous_integration/github_actions.md) - [GitLab CI](continuous_integration/gitlab.md) diff --git a/book/src/attribs.md b/book/src/attribs.md new file mode 100644 index 000000000..cf99497bc --- /dev/null +++ b/book/src/attribs.md @@ -0,0 +1,53 @@ +# Attributes for Crate Authors + +In some cases it is possible to extend Clippy coverage to 3rd party libraries. +To do this, Clippy provides attributes that can be applied to items in the 3rd party crate. + +## `#[clippy::format_args]` + +_Available since Clippy v1.84_ + +This attribute can be added to a macro that supports `format!`, `println!`, or similar syntax. +It tells Clippy that the macro is a formatting macro, and that the arguments to the macro +should be linted as if they were arguments to `format!`. Any lint that would apply to a +`format!` call will also apply to the macro call. The macro may have additional arguments +before the format string, and these will be ignored. + +### Example + +```rust +/// A macro that prints a message if a condition is true. +#[macro_export] +#[clippy::format_args] +macro_rules! print_if { + ($condition:expr, $($args:tt)+) => {{ + if $condition { + println!($($args)+) + } + }}; +} +``` + +## `#[clippy::has_significant_drop]` + +_Available since Clippy v1.60_ + +The `clippy::has_significant_drop` attribute can be added to types whose Drop impls have an important side effect, +such as unlocking a mutex, making it important for users to be able to accurately understand their lifetimes. +When a temporary is returned in a function call in a match scrutinee, its lifetime lasts until the end of the match +block, which may be surprising. + +### Example + +```rust +#[clippy::has_significant_drop] +struct CounterWrapper<'a> { + counter: &'a Counter, +} + +impl<'a> Drop for CounterWrapper<'a> { + fn drop(&mut self) { + self.counter.i.fetch_sub(1, Ordering::Relaxed); + } +} +``` diff --git a/clippy_utils/src/attrs.rs b/clippy_utils/src/attrs.rs index edc9c6ccd..b2a6657ba 100644 --- a/clippy_utils/src/attrs.rs +++ b/clippy_utils/src/attrs.rs @@ -27,7 +27,10 @@ pub const BUILTIN_ATTRIBUTES: &[(&str, DeprecationStatus)] = &[ ("cyclomatic_complexity", DeprecationStatus::Replaced("cognitive_complexity")), ("dump", DeprecationStatus::None), ("msrv", DeprecationStatus::None), + // The following attributes are for the 3rd party crate authors. + // See book/src/attribs.md ("has_significant_drop", DeprecationStatus::None), + ("format_args", DeprecationStatus::None), ]; pub struct LimitStack { diff --git a/clippy_utils/src/macros.rs b/clippy_utils/src/macros.rs index 9c4d19ac1..01261ea6e 100644 --- a/clippy_utils/src/macros.rs +++ b/clippy_utils/src/macros.rs @@ -1,5 +1,6 @@ #![allow(clippy::similar_names)] // `expr` and `expn` +use crate::get_unique_attr; use crate::visitors::{Descend, for_each_expr_without_closures}; use arrayvec::ArrayVec; @@ -7,7 +8,7 @@ use rustc_ast::{FormatArgs, FormatArgument, FormatPlaceholder}; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::sync::{Lrc, OnceLock}; use rustc_hir::{self as hir, Expr, ExprKind, HirId, Node, QPath}; -use rustc_lint::LateContext; +use rustc_lint::{LateContext, LintContext}; use rustc_span::def_id::DefId; use rustc_span::hygiene::{self, MacroKind, SyntaxContext}; use rustc_span::{BytePos, ExpnData, ExpnId, ExpnKind, Span, SpanData, Symbol, sym}; @@ -36,7 +37,9 @@ 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 + // Allow users to tag any macro as being format!-like + // TODO: consider deleting FORMAT_MACRO_DIAG_ITEMS and using just this method + get_unique_attr(cx.sess(), cx.tcx.get_attrs_unchecked(macro_def_id), "format_args").is_some() } } diff --git a/tests/ui/format_args_unfixable.rs b/tests/ui/format_args_unfixable.rs index f04715f4f..7590de375 100644 --- a/tests/ui/format_args_unfixable.rs +++ b/tests/ui/format_args_unfixable.rs @@ -119,3 +119,32 @@ fn test2() { format!("something failed at {}", Location::caller()) ); } + +#[clippy::format_args] +macro_rules! usr_println { + ($target:expr, $($args:tt)*) => {{ + if $target { + println!($($args)*) + } + }}; +} + +fn user_format() { + let error = Error::new(ErrorKind::Other, "bad thing"); + let x = 'x'; + + usr_println!(true, "error: {}", format!("boom at {}", Location::caller())); + //~^ ERROR: `format!` in `usr_println!` args + usr_println!(true, "{}: {}", error, format!("boom at {}", Location::caller())); + //~^ ERROR: `format!` in `usr_println!` args + usr_println!(true, "{:?}: {}", error, format!("boom at {}", Location::caller())); + //~^ ERROR: `format!` in `usr_println!` args + usr_println!(true, "{{}}: {}", format!("boom at {}", Location::caller())); + //~^ ERROR: `format!` in `usr_println!` args + usr_println!(true, r#"error: "{}""#, format!("boom at {}", Location::caller())); + //~^ ERROR: `format!` in `usr_println!` args + usr_println!(true, "error: {}", format!(r#"boom at "{}""#, Location::caller())); + //~^ ERROR: `format!` in `usr_println!` args + usr_println!(true, "error: {}", format!("boom at {} {0}", Location::caller())); + //~^ ERROR: `format!` in `usr_println!` args +} diff --git a/tests/ui/format_args_unfixable.stderr b/tests/ui/format_args_unfixable.stderr index 20cd0bb8c..1b4b683fd 100644 --- a/tests/ui/format_args_unfixable.stderr +++ b/tests/ui/format_args_unfixable.stderr @@ -174,5 +174,68 @@ LL | panic!("error: {}", format!("something failed at {}", Location::caller( = help: combine the `format!(..)` arguments with the outer `panic!(..)` call = help: or consider changing `format!` to `format_args!` -error: aborting due to 18 previous errors +error: `format!` in `usr_println!` args + --> tests/ui/format_args_unfixable.rs:136:5 + | +LL | usr_println!(true, "error: {}", format!("boom at {}", Location::caller())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: combine the `format!(..)` arguments with the outer `usr_println!(..)` call + = help: or consider changing `format!` to `format_args!` + +error: `format!` in `usr_println!` args + --> tests/ui/format_args_unfixable.rs:138:5 + | +LL | usr_println!(true, "{}: {}", error, format!("boom at {}", Location::caller())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: combine the `format!(..)` arguments with the outer `usr_println!(..)` call + = help: or consider changing `format!` to `format_args!` + +error: `format!` in `usr_println!` args + --> tests/ui/format_args_unfixable.rs:140:5 + | +LL | usr_println!(true, "{:?}: {}", error, format!("boom at {}", Location::caller())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: combine the `format!(..)` arguments with the outer `usr_println!(..)` call + = help: or consider changing `format!` to `format_args!` + +error: `format!` in `usr_println!` args + --> tests/ui/format_args_unfixable.rs:142:5 + | +LL | usr_println!(true, "{{}}: {}", format!("boom at {}", Location::caller())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: combine the `format!(..)` arguments with the outer `usr_println!(..)` call + = help: or consider changing `format!` to `format_args!` + +error: `format!` in `usr_println!` args + --> tests/ui/format_args_unfixable.rs:144:5 + | +LL | usr_println!(true, r#"error: "{}""#, format!("boom at {}", Location::caller())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: combine the `format!(..)` arguments with the outer `usr_println!(..)` call + = help: or consider changing `format!` to `format_args!` + +error: `format!` in `usr_println!` args + --> tests/ui/format_args_unfixable.rs:146:5 + | +LL | usr_println!(true, "error: {}", format!(r#"boom at "{}""#, Location::caller())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: combine the `format!(..)` arguments with the outer `usr_println!(..)` call + = help: or consider changing `format!` to `format_args!` + +error: `format!` in `usr_println!` args + --> tests/ui/format_args_unfixable.rs:148:5 + | +LL | usr_println!(true, "error: {}", format!("boom at {} {0}", Location::caller())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: combine the `format!(..)` arguments with the outer `usr_println!(..)` call + = help: or consider changing `format!` to `format_args!` + +error: aborting due to 25 previous errors diff --git a/tests/ui/uninlined_format_args.fixed b/tests/ui/uninlined_format_args.fixed index 3f5b0e52e..111a2e198 100644 --- a/tests/ui/uninlined_format_args.fixed +++ b/tests/ui/uninlined_format_args.fixed @@ -257,8 +257,6 @@ fn tester2() { 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! { @@ -267,3 +265,22 @@ fn tester2() { local_i32, }; } + +#[clippy::format_args] +macro_rules! usr_println { + ($target:expr, $($args:tt)*) => {{ + if $target { + println!($($args)*) + } + }}; +} + +fn user_format() { + let local_i32 = 1; + let local_f64 = 2.0; + + usr_println!(true, "val='{local_i32}'"); + usr_println!(true, "{local_i32}"); + usr_println!(true, "{local_i32:#010x}"); + usr_println!(true, "{local_f64:.1}"); +} diff --git a/tests/ui/uninlined_format_args.rs b/tests/ui/uninlined_format_args.rs index b311aa491..81fe24765 100644 --- a/tests/ui/uninlined_format_args.rs +++ b/tests/ui/uninlined_format_args.rs @@ -262,8 +262,6 @@ fn tester2() { 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! { @@ -272,3 +270,22 @@ fn tester2() { local_i32, }; } + +#[clippy::format_args] +macro_rules! usr_println { + ($target:expr, $($args:tt)*) => {{ + if $target { + println!($($args)*) + } + }}; +} + +fn user_format() { + let local_i32 = 1; + let local_f64 = 2.0; + + usr_println!(true, "val='{}'", local_i32); + usr_println!(true, "{}", local_i32); + usr_println!(true, "{:#010x}", local_i32); + usr_println!(true, "{:.1}", local_f64); +} diff --git a/tests/ui/uninlined_format_args.stderr b/tests/ui/uninlined_format_args.stderr index 5a7ff3bc4..77961fea2 100644 --- a/tests/ui/uninlined_format_args.stderr +++ b/tests/ui/uninlined_format_args.stderr @@ -845,5 +845,53 @@ LL - println!("expand='{}'", local_i32); LL + println!("expand='{local_i32}'"); | -error: aborting due to 71 previous errors +error: variables can be used directly in the `format!` string + --> tests/ui/uninlined_format_args.rs:287:5 + | +LL | usr_println!(true, "val='{}'", local_i32); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: change this to + | +LL - usr_println!(true, "val='{}'", local_i32); +LL + usr_println!(true, "val='{local_i32}'"); + | + +error: variables can be used directly in the `format!` string + --> tests/ui/uninlined_format_args.rs:288:5 + | +LL | usr_println!(true, "{}", local_i32); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: change this to + | +LL - usr_println!(true, "{}", local_i32); +LL + usr_println!(true, "{local_i32}"); + | + +error: variables can be used directly in the `format!` string + --> tests/ui/uninlined_format_args.rs:289:5 + | +LL | usr_println!(true, "{:#010x}", local_i32); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: change this to + | +LL - usr_println!(true, "{:#010x}", local_i32); +LL + usr_println!(true, "{local_i32:#010x}"); + | + +error: variables can be used directly in the `format!` string + --> tests/ui/uninlined_format_args.rs:290:5 + | +LL | usr_println!(true, "{:.1}", local_f64); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: change this to + | +LL - usr_println!(true, "{:.1}", local_f64); +LL + usr_println!(true, "{local_f64:.1}"); + | + +error: aborting due to 75 previous errors diff --git a/tests/ui/unused_format_specs.1.fixed b/tests/ui/unused_format_specs.1.fixed index b7d1cce28..157c2b08d 100644 --- a/tests/ui/unused_format_specs.1.fixed +++ b/tests/ui/unused_format_specs.1.fixed @@ -33,3 +33,38 @@ fn should_not_lint() { let args = format_args!(""); println!("{args}"); } + +#[clippy::format_args] +macro_rules! usr_println { + ($target:expr, $($args:tt)*) => {{ + if $target { + println!($($args)*) + } + }}; +} + +fn should_lint_user() { + // prints `.`, not ` .` + usr_println!(true, "{:5}.", format!("")); + //~^ ERROR: format specifiers have no effect on `format_args!()` + //prints `abcde`, not `abc` + usr_println!(true, "{:.3}", format!("abcde")); + //~^ ERROR: format specifiers have no effect on `format_args!()` + + usr_println!(true, "{}.", format_args_from_macro!()); + //~^ ERROR: format specifiers have no effect on `format_args!()` + + let args = format_args!(""); + usr_println!(true, "{args}"); + //~^ ERROR: format specifiers have no effect on `format_args!()` +} + +fn should_not_lint_user() { + usr_println!(true, "{}", format_args!("")); + // Technically the same as `{}`, but the `format_args` docs specifically mention that you can use + // debug formatting so allow it + usr_println!(true, "{:?}", format_args!("")); + + let args = format_args!(""); + usr_println!(true, "{args}"); +} diff --git a/tests/ui/unused_format_specs.2.fixed b/tests/ui/unused_format_specs.2.fixed index 94bb6b703..92c7b951f 100644 --- a/tests/ui/unused_format_specs.2.fixed +++ b/tests/ui/unused_format_specs.2.fixed @@ -33,3 +33,38 @@ fn should_not_lint() { let args = format_args!(""); println!("{args}"); } + +#[clippy::format_args] +macro_rules! usr_println { + ($target:expr, $($args:tt)*) => {{ + if $target { + println!($($args)*) + } + }}; +} + +fn should_lint_user() { + // prints `.`, not ` .` + usr_println!(true, "{}.", format_args!("")); + //~^ ERROR: format specifiers have no effect on `format_args!()` + //prints `abcde`, not `abc` + usr_println!(true, "{}", format_args!("abcde")); + //~^ ERROR: format specifiers have no effect on `format_args!()` + + usr_println!(true, "{}.", format_args_from_macro!()); + //~^ ERROR: format specifiers have no effect on `format_args!()` + + let args = format_args!(""); + usr_println!(true, "{args}"); + //~^ ERROR: format specifiers have no effect on `format_args!()` +} + +fn should_not_lint_user() { + usr_println!(true, "{}", format_args!("")); + // Technically the same as `{}`, but the `format_args` docs specifically mention that you can use + // debug formatting so allow it + usr_println!(true, "{:?}", format_args!("")); + + let args = format_args!(""); + usr_println!(true, "{args}"); +} diff --git a/tests/ui/unused_format_specs.rs b/tests/ui/unused_format_specs.rs index 2c85e3711..a5df4d8a8 100644 --- a/tests/ui/unused_format_specs.rs +++ b/tests/ui/unused_format_specs.rs @@ -33,3 +33,38 @@ fn should_not_lint() { let args = format_args!(""); println!("{args}"); } + +#[clippy::format_args] +macro_rules! usr_println { + ($target:expr, $($args:tt)*) => {{ + if $target { + println!($($args)*) + } + }}; +} + +fn should_lint_user() { + // prints `.`, not ` .` + usr_println!(true, "{:5}.", format_args!("")); + //~^ ERROR: format specifiers have no effect on `format_args!()` + //prints `abcde`, not `abc` + usr_println!(true, "{:.3}", format_args!("abcde")); + //~^ ERROR: format specifiers have no effect on `format_args!()` + + usr_println!(true, "{:5}.", format_args_from_macro!()); + //~^ ERROR: format specifiers have no effect on `format_args!()` + + let args = format_args!(""); + usr_println!(true, "{args:5}"); + //~^ ERROR: format specifiers have no effect on `format_args!()` +} + +fn should_not_lint_user() { + usr_println!(true, "{}", format_args!("")); + // Technically the same as `{}`, but the `format_args` docs specifically mention that you can use + // debug formatting so allow it + usr_println!(true, "{:?}", format_args!("")); + + let args = format_args!(""); + usr_println!(true, "{args}"); +} diff --git a/tests/ui/unused_format_specs.stderr b/tests/ui/unused_format_specs.stderr index 2b5c81c63..df61d5913 100644 --- a/tests/ui/unused_format_specs.stderr +++ b/tests/ui/unused_format_specs.stderr @@ -58,5 +58,63 @@ LL - println!("{args:5}"); LL + println!("{args}"); | -error: aborting due to 4 previous errors +error: format specifiers have no effect on `format_args!()` + --> tests/ui/unused_format_specs.rs:48:25 + | +LL | usr_println!(true, "{:5}.", format_args!("")); + | ^^^^ + | +help: for the width to apply consider using `format!()` + | +LL | usr_println!(true, "{:5}.", format!("")); + | ~~~~~~ +help: if the current behavior is intentional, remove the format specifiers + | +LL - usr_println!(true, "{:5}.", format_args!("")); +LL + usr_println!(true, "{}.", format_args!("")); + | + +error: format specifiers have no effect on `format_args!()` + --> tests/ui/unused_format_specs.rs:51:25 + | +LL | usr_println!(true, "{:.3}", format_args!("abcde")); + | ^^^^^ + | +help: for the precision to apply consider using `format!()` + | +LL | usr_println!(true, "{:.3}", format!("abcde")); + | ~~~~~~ +help: if the current behavior is intentional, remove the format specifiers + | +LL - usr_println!(true, "{:.3}", format_args!("abcde")); +LL + usr_println!(true, "{}", format_args!("abcde")); + | + +error: format specifiers have no effect on `format_args!()` + --> tests/ui/unused_format_specs.rs:54:25 + | +LL | usr_println!(true, "{:5}.", format_args_from_macro!()); + | ^^^^ + | + = help: for the width to apply consider using `format!()` +help: if the current behavior is intentional, remove the format specifiers + | +LL - usr_println!(true, "{:5}.", format_args_from_macro!()); +LL + usr_println!(true, "{}.", format_args_from_macro!()); + | + +error: format specifiers have no effect on `format_args!()` + --> tests/ui/unused_format_specs.rs:58:25 + | +LL | usr_println!(true, "{args:5}"); + | ^^^^^^^^ + | + = help: for the width to apply consider using `format!()` +help: if the current behavior is intentional, remove the format specifiers + | +LL - usr_println!(true, "{args:5}"); +LL + usr_println!(true, "{args}"); + | + +error: aborting due to 8 previous errors