non_fmt_panics suggestion applied many times when it appears inside a macro_rules

[m-ou-se]

This showed up in a small crater run for the 2021 edition migrations:

• https://crater-reports.s3.amazonaws.com/pr-87190-2/try%23d8627d19850b63702b76089d7b055730e9ddca14/reg/image-0.23.14/log.txt
• https://crater-reports.s3.amazonaws.com/pr-87190-2/try%23d8627d19850b63702b76089d7b055730e9ddca14/reg/mockers-0.21.0/log.txt

The suggestion gets applied for every usage of the macro, resulting in:

                 panic!("{}", "{}", "{}", "{}", "{}", "{}", "{}", "{}", err)

[m-ou-se]

@estebank Is this a known problem with suggestions inside macro_rules definitions?

[estebank]

Oh, that definitely looks like a previously existing outstanding bug :-/

I also noticed this other issue as well:

[INFO] [stdout] error: format argument must be a string literal
[INFO] [stdout]    --> src/imageops/sample.rs:904:21
[INFO] [stdout]     |
[INFO] [stdout] 904 |                     format!("alpha value: {}, {:?}", alpha, filter)
[INFO] [stdout]     |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stdout]     |
[INFO] [stdout]     = note: this error originates in the macro `format` (in Nightly builds, run with -Z macro-backtrace for more info)
[INFO] [stdout] help: you might be missing a string literal to format with
[INFO] [stdout]     |
[INFO] [stdout] 904 |                     "{}", format!("alpha value: {}, {:?}", alpha, filter)
[INFO] [stdout]     |                     +++++

[m-ou-se]

That other issue looks confusing because the surrounding assert!( and ) are on separate lines:

                assert!(
                    alpha != 254 && alpha != 253,
                    format!("alpha value: {}, {:?}", alpha, filter)
                );

but the error is correct.

(It could suggest removing format!( and ) instead of adding "{}", though, like the non_fmt_panics lint does in 2015/2018 code:

warning

[INFO] [stdout] warning: panic message is not a string literal
[INFO] [stdout]    --> src/imageops/sample.rs:904:21
[INFO] [stdout]     |
[INFO] [stdout] 904 |                     format!("alpha value: {}, {:?}", alpha, filter)
[INFO] [stdout]     |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stdout]     |
[INFO] [stdout]     = note: this usage of assert!() is deprecated; it will be a hard error in Rust 2021
[INFO] [stdout]     = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/panic-macro-consistency.html>
[INFO] [stdout]     = note: the assert!() macro supports formatting, so there's no need for the format!() macro here
[INFO] [stdout] help: remove the `format!(..)` macro call
[INFO] [stdout]     |
[INFO] [stdout] 904 -                     format!("alpha value: {}, {:?}", alpha, filter)
[INFO] [stdout] 904 +                     "alpha value: {}, {:?}", alpha, filter
[INFO] [stdout]     | 

)

[m-ou-se]

Oh, that definitely looks like a previously existing outstanding bug :-/

@estebank So I assume there's no existing mechanism for 'deduplicating' suggestions like that, such that they only appear on the first macro invocation or something? :(

[estebank]

I believe we have a mechanism to do that, but it might be relying on the Span information, and because these spans have different provenance they are considered "different". We could maybe make rustfix smarter here instead :-/

What's the timeline for the edition?

[m-ou-se]

What's the timeline for the edition?

We have 17 days until 1.56 branches off. In 10'000 crates we only ran into this twice though, so it doesn't seem like a big issue; definitely not a blocker.

[m-ou-se]

It seems to work fine though when the suggestion is to replace panic! by panic_any, even if it's duplicated six times. I'm guessing whatever makes that work isn't working for inserting "{}", , because it's inserting (replacing an empty span) rather than replacing something.

[m-ou-se]

https://twitter.com/artichokeruby/status/1427720656817332227

This same thing happened with 2018 edition fix ups for anonymous lifetime parameters for Debug impls defined in macros.

[estebank]

Less than ideal, but I think it is acceptable to go out of the door with this unaddressed. We should fix it soonish regardless.

[m-ou-se]

This is an instance of rust-lang/cargo#13027.

Closing this as duplicate.