Rust 2021 (seemingly) mistakenly warns 'panic message contains braces'

[mocsy]

Later versions of rust mistake macros like https://crates.io/crates/contracts requires as a panic message.

Given the following code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=babeae07316ceb37dd7291ff889e9ea3

<code>

The current output is:

warning: panic message contains braces
  --> src/main.rs:4:16
   |
4 |     #[requires(key.starts_with('{'))]
   |                ^^^
   |
   = note: `#[warn(non_fmt_panic)]` on by default
   = note: this message is not used as a format string, but will be in Rust 2021
help: add a "{}" format string to use the message literally
   |
4 |     #[requires("{}", key.starts_with('{'))]
   |                ^^^^^

Ideally the output should look like:
Nothing, it's not a panic message, it's an expression.

<proposed output>

[ehuss]

Hm, I'm uncertain how that should be handled.

The requires attribute essentially expands to:

assert!(expr, concat!("Pre-condition of parse violated: ", stringify!(key.starts_with('{'))))

which expands to

assert!(expr, "Pre-condition of parse violated: key.starts_with('{')")

This ends up with a bare { in the panic string. Indeed this code will fail to compile in 2021.

The contracts crate could be updated to generate the assert macros with the proper formatting (assert!(expr, "{}", "the message")). However, it would be desirable if that was not required. I'm not entirely familiar with proc-macro edition hygiene. I would actually expect this to not warn at all, since I would expect the code generated by the proc-macro to be tagged with the 2018 edition.

[nikomatsakis]

Indeed, I think that proc-macro hygiene doesn't work all that well. To make it work, presumably, one would have to use the proper spans -- and this is a bit questionable, because the span for this time probably comes from the Rust 2021 crate, but the Rust 2018 crate is the one turning it into a string.

In general, macro selection etc is not especially hygiene friendly, so I'm not totally sure how this winds up interacting.

[m-ou-se]

Aw, I put effort into making sure the suggestions are handled correctly in macro expansions as well, but apparently it doesn't work here at all. (Suggesting the "{}", in the macro call.) I'll take a look at improving that.

However, it would be desirable if that was not required. I'm not entirely familiar with proc-macro edition hygiene. I would actually expect this to not warn at all, since I would expect the code generated by the proc-macro to be tagged with the 2018 edition.

It does see this as a 2015/2018 panic. The hygiene works correctly. This warning is for 2015/2018 code. In 2021 code, such a panic/assert call wouldn't compile at all.

[ehuss]

It does see this as a 2015/2018 panic. The hygiene works correctly. This warning is for 2015/2018 code. In 2021 code, such a panic/assert call wouldn't compile at all.

So, just to clarify: The issue is that contracts uses quote_spanned, which tags that entire section of code with the edition of the caller (2021 in this case)? (At least, in my limited testing, it seems to not fail if I use quote! instead of quote_spanned!).

Also, I noticed that calling any macro (macro_rules, proc_macro, etc.) from another crate triggers the warning. I don't think it should be doing that, though, since there's nothing the caller can do about it, and it doesn't actually fail? That is, defining a macro in 2018 like this:

#[macro_export]
macro_rules! foo {
    () => {assert!(true, "{")};
}

And calling that from a 2021 crate triggers a warning, but there's nothing to do since it works correctly.

[Nemo157]

It does see this as a 2015/2018 panic. The hygiene works correctly. This warning is for 2015/2018 code. In 2021 code, such a panic/assert call wouldn't compile at all.

Testing the playground code in an edition = "2021" crate it fails to compile, so I don't think the hygiene is correct (but it sounds like that is an implementation issue that contracts is applying the incorrect span to the panic! macro call).

[m-ou-se]

So, just to clarify: The issue is that contracts uses quote_spanned, which tags that entire section of code with the edition of the caller (2021 in this case)? (At least, in my limited testing, it seems to not fail if I use quote! instead of quote_spanned!).

Ah, yes. The span of the format string itself doesn't really matter, but the call site of the assert!() call itself does matter. So if they used quote_spanned only for the format string, but regular quote for the surrounding tokens, it'd be fine.

Also, I noticed that calling any macro (macro_rules, proc_macro, etc.) from another crate triggers the warning.

Yeah it shouldn't give warnings you can't fix in your current crate, so that part is a bug. It's a bit tricky, because if you have e.g.

macro_rules! mypanic {
    ($($t:tt)*) => { assert!(false, $($t:tt)*) };
}

then a warning about mypanic!("{}") probably missing an argument (even though it compiles) is still good. Surprised to see that your example still ends up in a warning. I thought I had handled those cases, but apparently I misremember.

[m-ou-se]

Warning about issues that are entirely in another crate is solved by #87965
(It still warns about mypanic!("{}") in the example above, but not if a!() expands to a panic including format string that would trigger the lint.)

The problem with #[requires(key.starts_with('{'))] with the contracts crate isn't really something to solve fully: That proc macro applies the same span to all tokens, effectively claiming that not the macro but the user wrote assert!(expr, concat!(...)), in exactly the spot where the user wrote key.starts_with('{'). The compiler trusts that information, and repots to the user that the assert!() 'that the user wrote' is incorrect. The proper fix is to fix the contracts crate to not claim the assert!( part was written by the user. #87967 is a band aid to detect the most basic cases like this and avoid giving an invalid suggestion.

[m-ou-se]

The latest nightly no longer produces the wrong suggestion.

The error is still produced, but that's not a problem that rustc can solve. That's up to the contracts crate, to use the right spans.

Closing this.