Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support user format-like macros #9948

Merged
merged 1 commit into from
Nov 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions book/src/SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
53 changes: 53 additions & 0 deletions book/src/attribs.md
Original file line number Diff line number Diff line change
@@ -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.
Comment on lines +12 to +14
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are lints that apply to format! that wouldn't apply to macros marked with the attr, might want to list it as format!/println!/etc or similar

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. format_in_format_args

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alexendoo thx, updated


### 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);
}
}
```
3 changes: 3 additions & 0 deletions clippy_utils/src/attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 5 additions & 2 deletions clippy_utils/src/macros.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
#![allow(clippy::similar_names)] // `expr` and `expn`

use crate::get_unique_attr;
use crate::visitors::{Descend, for_each_expr_without_closures};

use arrayvec::ArrayVec;
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};
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a follow up PR to the rust repo, after this PR is merged and synced

get_unique_attr(cx.sess(), cx.tcx.get_attrs_unchecked(macro_def_id), "format_args").is_some()
}
}

Expand Down
29 changes: 29 additions & 0 deletions tests/ui/format_args_unfixable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
65 changes: 64 additions & 1 deletion tests/ui/format_args_unfixable.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

21 changes: 19 additions & 2 deletions tests/ui/uninlined_format_args.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -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! {
Expand All @@ -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}");
}
21 changes: 19 additions & 2 deletions tests/ui/uninlined_format_args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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! {
Expand All @@ -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);
}
50 changes: 49 additions & 1 deletion tests/ui/uninlined_format_args.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

35 changes: 35 additions & 0 deletions tests/ui/unused_format_specs.1.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -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}");
}
35 changes: 35 additions & 0 deletions tests/ui/unused_format_specs.2.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -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}");
}
Loading