Skip to content

Commit

Permalink
Auto merge of rust-lang#8857 - smoelius:fix-8855, r=flip1995
Browse files Browse the repository at this point in the history
Add test for rust-lang#8855

Fix rust-lang#8855

Here is what I think is going on.

First, the expression `format!("{:>6} {:>6}", a, b.to_string())` expands to:
```rust
{
    let res =
        ::alloc::fmt::format(::core::fmt::Arguments::new_v1_formatted(&["",
                            " "],
                &[::core::fmt::ArgumentV1::new_display(&a),
                            ::core::fmt::ArgumentV1::new_display(&b.to_string())],
                &[::core::fmt::rt::v1::Argument {
                                position: 0usize,
                                format: ::core::fmt::rt::v1::FormatSpec {
                                    fill: ' ',
                                    align: ::core::fmt::rt::v1::Alignment::Right,
                                    flags: 0u32,
                                    precision: ::core::fmt::rt::v1::Count::Implied,
                                    width: ::core::fmt::rt::v1::Count::Is(6usize),
                                },
                            },
                            ::core::fmt::rt::v1::Argument {
                                position: 1usize,
                                format: ::core::fmt::rt::v1::FormatSpec {
                                    fill: ' ',
                                    align: ::core::fmt::rt::v1::Alignment::Right,
                                    flags: 0u32,
                                    precision: ::core::fmt::rt::v1::Count::Implied,
                                    width: ::core::fmt::rt::v1::Count::Is(6usize),
                                },
                            }], unsafe { ::core::fmt::UnsafeArg::new() }));
    res
}
```
When I dump the expressions that get past the call to `has_string_formatting` [here](https://github.com/rust-lang/rust-clippy/blob/b312ad7d0cf0f30be2bd4658b71a3520a2e76709/clippy_lints/src/format_args.rs#L83), I see more than I would expect.

In particular, I see this subexpression of the above:
```
                &[::core::fmt::ArgumentV1::new_display(&a),
                            ::core::fmt::ArgumentV1::new_display(&b.to_string())],
```

This suggests to me that more expressions are getting past [this call](https://github.com/rust-lang/rust-clippy/blob/b312ad7d0cf0f30be2bd4658b71a3520a2e76709/clippy_lints/src/format_args.rs#L71) to `FormatArgsExpn::parse` than should.

Those expressions are then visited, but no `::core::fmt::rt::v1::Argument`s are found and pushed [here](https://github.com/rust-lang/rust-clippy/blob/b312ad7d0cf0f30be2bd4658b71a3520a2e76709/clippy_utils/src/macros.rs#L407).

As a result, the expressions appear unformatted, hence, the false positive.

My proposed fix is to restrict `FormatArgsExpn::parse` so that it only matches `Call` expressions.

cc: `@akanalytics`

changelog: none
  • Loading branch information
bors committed Aug 20, 2022
2 parents 5820add + 6f3d398 commit 41309df
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 1 deletion.
24 changes: 24 additions & 0 deletions tests/ui/format_args.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -122,3 +122,27 @@ fn issue8643(vendor_id: usize, product_id: usize, name: &str) {
name
);
}

// https://github.com/rust-lang/rust-clippy/issues/8855
mod issue_8855 {
#![allow(dead_code)]

struct A {}

impl std::fmt::Display for A {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "test")
}
}

fn main() {
let a = A {};
let b = A {};

let x = format!("{} {}", a, b);
dbg!(x);

let x = format!("{:>6} {:>6}", a, b.to_string());
dbg!(x);
}
}
24 changes: 24 additions & 0 deletions tests/ui/format_args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,3 +122,27 @@ fn issue8643(vendor_id: usize, product_id: usize, name: &str) {
name
);
}

// https://github.com/rust-lang/rust-clippy/issues/8855
mod issue_8855 {
#![allow(dead_code)]

struct A {}

impl std::fmt::Display for A {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "test")
}
}

fn main() {
let a = A {};
let b = A {};

let x = format!("{} {}", a, b.to_string());
dbg!(x);

let x = format!("{:>6} {:>6}", a, b.to_string());
dbg!(x);
}
}
8 changes: 7 additions & 1 deletion tests/ui/format_args.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -126,5 +126,11 @@ error: `to_string` applied to a type that implements `Display` in `println!` arg
LL | println!("{foo}{bar}", bar = "bar", foo = "foo".to_string());
| ^^^^^^^^^^^^ help: remove this

error: aborting due to 21 previous errors
error: `to_string` applied to a type that implements `Display` in `format!` args
--> $DIR/format_args.rs:142:38
|
LL | let x = format!("{} {}", a, b.to_string());
| ^^^^^^^^^^^^ help: remove this

error: aborting due to 22 previous errors

0 comments on commit 41309df

Please sign in to comment.