non_fmt_panics cannot migrate to 2021 for assert with non-string payload

[ehuss]

I tried this code:

fn f() {
    assert!(true, 123);
}

I expected to see this happen: This includes a machine-applicable fix to migrate to 2021.

Instead, this happened: The suggestion is MaybeIncorrect preventing automatic migration.

Part of the problem is that there isn't an easy way to convert this to panic_any. For example, it could convert to:

fn f() {
    if !(expr) { ::std::panic::panic_any(123); }
}

which is pretty awful and probably has other downsides. Trying to format with {:?} is also probably not a reliable option, since it would require the payload to impl Debug, and the user's code may be relying on catching the panic and inspecting the payload.

I don't know what is really practical here.

Meta

rustc 1.55.0-nightly (014026d1a 2021-07-19)

[m-ou-se]

I'm not sure there's much we can/should do automatically here. In these cases, the user probably meant to have a "{}", or a "{:?}" or probably a more descriptive message like "{:?} must be 123 or higher". Or worse: they meant to use assert_eq instead of assert:

assert!(v.is_empty(), true); // throws a Box<Any> containing `true` if v is not empty

[ehuss]

Yea, this is likely a "won't fix", though I think it would be good to leave this open for tracking purposes.

I'm wondering if it would make sense to include known migration limitations like this in the edition guide?

[m-ou-se]

Working on this now, to make the "{}", suggestion MachineApplicable when the argument is a &str or String.

[m-ou-se]

im 15.0.0 and structopt 0.3.22 both failed in the edition crater run on this issue. However, in both cases they were using a non-string as panic payload. An Error struct in structopt, and a usize in im.

Both of those would have resulted in panicked at 'Box<dyn Any>' on failure. So those are cases where this lint discovered a bug that needs manual fixing.

[nrc]

I think I just hit something similar, I got the following error message:

warning: panic message is not a string literal
 --> src/main.rs:8:12
  |
8 |     panic!(f);
  |            ^
  |
  = note: `#[warn(non_fmt_panics)]` on by default
  = note: this usage of panic!() is deprecated; it will be a hard error in Rust 2021
  = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/panic-macro-consistency.html>
help: add a "{}" format string to Display the message
  |
8 |     panic!("{}", f);
  |            +++++
help: or use std::panic::panic_any instead
  |
8 |     std::panic::panic_any(f);
  |     ~~~~~~~~~~~~~~~~~~~~~

But the suggestion doesn't work because f does not implement Display, only Debug. In this case the suggestion would better to be {:?}. I'm not sure if we can customise the suggestion based on the type of the argument to panic!? Otherwise, I think most of the time it is more likley for Debug to be implemented than Display, so {:?} might be a better suggestion?

[m-ou-se]

Yeah, the "{}", suggestion was more intended as "you need some sort of format string here", which doesn't have to be Display, but that's not reflected well in the message.

I think we can just check if either Display or Debug is implemented and suggest something that works. #87982 looks at the type too to see if the argument is a string. Checking if the type has a trait implemented should be doable there too. I'll look into that soon.

Edit: filed #87999 for that.