Ensure a sole string-literal passed to `panic!` is not a fmt string. #24370

pnkfelix · 2015-04-13T10:35:14Z

Ensure a sole string-literal passed to panic! is not a fmt string.

To accomplish this, adds ensure_not_fmt_string_literal! macro that will warn during compilation if its expression argument is a fmt string literal.

Since this is making a certain kind of use of panic! illegal, it is a:

[breaking-change]

In particular, a panic like this:

panic!("Is it stringified code: { or is it a ill-formed fmt arg? }");

must be rewritten; one easy rewrite is to add parentheses:

panic!(("Is it stringified code: { or is it a ill-formed fmt arg? }"));

Fix #22932.

rust-highfive · 2015-04-13T10:35:26Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2015-04-13T16:57:19Z

src/test/compile-fail/issue-22932.rs

+}
+
+pub fn f0_a() {
+    ensure_not_fmt_string_literal!("`f0_a`", "this does not work {}");


Can this macro be put behind a feature gate as well?

yes good idea.

alexcrichton · 2015-04-13T16:58:31Z

This has the potential to be a large-ish breaking change, so it may be useful to gather metrics and see how much breaks. This is a pretty minor problem that may not be worth fixing if it breaks too much.

pnkfelix · 2015-04-13T17:03:49Z

@bors try

bors · 2015-04-13T17:03:51Z

⌛ Trying commit ac56d7f with merge 2fe4066...

Ensure a sole string-literal passed to `panic!` is not a fmt string. To accomplish this, adds `ensure_not_fmt_string_literal!` macro that will fail the compile if its expression argument is a fmt string literal. Since this is making a certain kind of use of `panic!` illegal, it is a: [breaking-change] In particular, a panic like this: ```rust panic!("Is it stringified code: { or is it a ill-formed fmt arg? }"); ``` must be rewritten; one easy rewrite is to add parentheses: ```rust panic!(("Is it stringified code: { or is it a ill-formed fmt arg? }")); ``` ---- Fix #22932.

bors · 2015-04-13T17:55:48Z

💔 Test failed - try-mac

pnkfelix · 2015-04-13T21:25:16Z

@brson argh i'm sorry this clearly does not build. I blame the slow build times on my horribly messed up laptop. :(

pnkfelix · 2015-04-13T21:33:35Z

@bors try

bors · 2015-04-13T21:33:37Z

⌛ Trying commit 21e580f with merge 2b32efb...

pnkfelix · 2015-04-13T21:39:28Z

@bors try force

bors · 2015-04-13T21:39:30Z

⌛ Trying commit 08079d5 with merge 0ec30ad...

Ensure a sole string-literal passed to `panic!` is not a fmt string. To accomplish this, adds `ensure_not_fmt_string_literal!` macro that will fail the compile if its expression argument is a fmt string literal. Since this is making a certain kind of use of `panic!` illegal, it is a: [breaking-change] In particular, a panic like this: ```rust panic!("Is it stringified code: { or is it a ill-formed fmt arg? }"); ``` must be rewritten; one easy rewrite is to add parentheses: ```rust panic!(("Is it stringified code: { or is it a ill-formed fmt arg? }")); ``` ---- Fix #22932.

bors · 2015-04-13T22:40:11Z

💔 Test failed - try-bsd

pnkfelix · 2015-04-14T10:27:22Z

(I do not yet understand why the try-bsd build failed in the manner above. Maybe I need to look into that.)

@brson was kind enough to run his regression tool on this.

So on the one hand, the reported breakage seems ... pretty minimal? Just two crates on crates.io seem to regress due to this change. In particular:

1st in capnp:

         Compiling capnp v0.2.0 (file:///home/crate)
      src/private/layout.rs:946:9: 947:70 error: panic! input cannot be format string literal
      src/private/layout.rs:946         assert!(element_size != InlineComposite,
      src/private/layout.rs:947                 "Use get_struct_list_{element,field}() for structs");
      error: aborting due to previous error
      Could not compile `capnp`.

2nd in mustache:

         Compiling mustache v0.5.0 (file:///home/crate)
      src/parser.rs:327:26: 327:60 error: panic! input cannot be format string literal
      src/parser.rs:327                 } else { panic!("unbalanced \"{\" in tag"); }
                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      error: aborting due to previous error
      Could not compile `mustache`.

But on the other hand, looking at this again, I would probably be pretty annoyed to get an outright compile failure for the two cases above, as opposed to a lint warning.

I'm going to see if I can easily rejigger this into a lint instead of a inescapable failure.

alexcrichton · 2015-04-14T16:29:26Z

@pnkfelix hm what's the rationale for having this be a lint? As in, when the lint warning fires and I change my code, what benefit is reaped from the modification?

pnkfelix · 2015-04-14T18:08:38Z

@alexcrichton the benefit is that you have a chance to see that you intended to actually pass arguments.

I cannot stop people who blindly wrap the literal in parens, but at least that stands out visually compared to assert!(cond, "input {} was messed up");

Update: to be clear: The macro being added here would cause a warning to be emitted on the above assert!, and one would be expected to rewrite as either:

assert!(cond, "input {} was messed up", heres_the_input);

or as:

assert!(cond, ("input {} was messed up")); // as in, yes, I really meant `{}` to be printed

alexcrichton · 2015-04-14T18:21:24Z

@pnkfelix ah right good point!

pnkfelix · 2015-04-15T09:39:04Z

@bors try

bors · 2015-04-15T09:39:06Z

🔒 Merge conflict

pnkfelix · 2015-04-15T10:24:35Z

@bors try

bors · 2015-04-15T10:24:38Z

⌛ Trying commit 12706c9 with merge f637fda...

Ensure a sole string-literal passed to `panic!` is not a fmt string. To accomplish this, adds `ensure_not_fmt_string_literal!` macro that will fail the compile if its expression argument is a fmt string literal. Since this is making a certain kind of use of `panic!` illegal, it is a: [breaking-change] In particular, a panic like this: ```rust panic!("Is it stringified code: { or is it a ill-formed fmt arg? }"); ``` must be rewritten; one easy rewrite is to add parentheses: ```rust panic!(("Is it stringified code: { or is it a ill-formed fmt arg? }")); ``` ---- Fix #22932.

bors · 2015-04-15T11:32:16Z

💔 Test failed - try-linux

To accomplish this, adds `ensure_not_fmt_string_literal!` macro that will fail the compile if its argument is a fmt string literal. `ensure_not_fmt_string_literal!` takes a name along with expr; this allows for better error messages at its usage sites (like `panic!`). ---- Since this is making a certain kind of use of `panic!` illegal, it is a: [breaking-change] In particular, a panic like this: ```rust panic!("Is it stringified code: { or is it a ill-formed fmt arg? }"); ``` must be rewritten; one easy rewrite is to add parentheses: ```rust panic!(("Is it stringified code: { or is it a ill-formed fmt arg? }")); ``` ---- Fix rust-lang#22932.

As a drive-by, attempt to treat the macro as unstable by having it emit a reference to an unstable constant in its expansion. We *do* allow such an expansion when it occurs within `panic!`, for discussion, see large comment explaining the hack in the code.

pnkfelix · 2015-04-17T14:06:26Z

At this point I've tried several different strategies for enforcing and checking the unstable-ness of the helper macro, but none have worked (they either fail to reject the macro, or they reject too much, like invocations of panic!). @huonw and I discussed, and we think the inability to encode this may be symptoms of bugs in the underlying macro system. Not sure yet.

For now, I am planning to just give the helper macro a very ugly, obscure name, and file a bug for investigating the other problems later, but not holding up this PR based on that.

nikomatsakis · 2015-04-17T16:55:16Z

On Fri, Apr 17, 2015 at 07:06:32AM -0700, Felix S Klock II wrote:

For now, I am planning to just give the helper macro a very ugly, obscure name, and file a bug for investigating the other problems later, but not holding up this PR based on that.

my opinion is we could live with this helper macro indefinitely it push came to shove

alexcrichton · 2015-04-17T16:57:49Z

We have a few other macros in the standard library that start with two underscores, and that's generally enough of a deterrent to say "we're not providing guarantees about this"

…s internally. Unfortunately, i am having issues getting this to actually work. I repeatedly see: ``` failures: [run-fail] run-fail/explicit-panic-msg.rs ``` Namely: ``` failures: ---- [run-fail] run-fail/explicit-panic-msg.rs stdout ---- error: compilation failed! status: exit code: 101 command: x86_64-apple-darwin/stage2/bin/rustc /Users/fklock/Dev/Mozilla/rust-panic/src/test/run-fail/explicit-panic-msg.rs -L x86_64-apple-darwin/test/run-fail/ --target=x86_64-apple-darwin -L x86_64-apple-darwin/test/run-fail/explicit-panic-msg.stage2-x86_64-apple-darwinlibaux -C prefer-dynamic -o x86_64-apple-darwin/test/run-fail/explicit-panic-msg.stage2-x86_64-apple-darwin --cfg rtopt --cfg debug -L x86_64-apple-darwin/rt stdout: ------------------------------------------ ------------------------------------------ stderr: ------------------------------------------ <std macros>:2:26: 2:57 error: use of unstable library feature 'core': internal to format_args! <std macros>:2 $ crate:: fmt:: format ( format_args ! ( $ ( $ arg ) * ) ) ) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ note: in expansion of __unstable_rustc_ensure_not_fmt_string_literal! <std macros>:4:1: 4:78 note: expansion site <std macros>:1:1: 12:23 note: in expansion of panic! /Users/fklock/Dev/Mozilla/rust-panic/src/test/run-fail/explicit-panic-msg.rs:18:5: 18:37 note: expansion site <std macros>:2:26: 2:57 help: add #![feature(core)] to the crate attributes to enable <std macros>:2:26: 2:57 error: use of unstable library feature 'core': internal to format_args! <std macros>:2 $ crate:: fmt:: format ( format_args ! ( $ ( $ arg ) * ) ) ) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ note: in expansion of __unstable_rustc_ensure_not_fmt_string_literal! <std macros>:4:1: 4:78 note: expansion site <std macros>:1:1: 12:23 note: in expansion of panic! /Users/fklock/Dev/Mozilla/rust-panic/src/test/run-fail/explicit-panic-msg.rs:18:5: 18:37 note: expansion site <std macros>:2:26: 2:57 help: add #![feature(core)] to the crate attributes to enable error: aborting due to 2 previous errors ------------------------------------------ ``` and I have not yet figured out why this happens even now, nor how to resolve it.

nikomatsakis · 2015-04-18T15:09:13Z

I'm not clear on whether this is still waiting for review or what. At this point, it's just a warning, right? So it's not backwards incompatible? The stability code is not really my area of expertise, so I'll change this to:

r? @alexcrichton

pnkfelix · 2015-04-18T16:51:15Z

Its not done yet, unfortunately; I believe there is still some other fallout I need to address in a manner analogous to b58359a -- but yes, my current plan is for just a warning, which means we probably can land it at any time. (I would like it to end up in the beta nonetheless, but I don't know what our threshold is going to be about what warrants cherry-picking.)

alexcrichton · 2015-04-20T15:53:32Z

src/test/run-fail/explicit-panic-msg.rs

@@ -15,5 +15,6 @@
 fn main() {
    let mut a = 1;
    if 1 == 1 { a = 2; }
-    panic!(format!("woooo{}", "o"));
+    let msg = format!("woooo{}", "o");
+    panic!(msg);


Was this necessary to change?

Ah wait I see the commit now, how come this was necessary to change?

Because otherwise I get stability warnings saying that the internals of float are unstable.

I assume this is due to a bug in either my macro, the expander, or in the way we lint stability via the chain of spans associated with the expansion. I was/am not sure if this is a deal-breaker for this change; maybe I will have to track down that bug first (and presumably let this wait until 1.x to land.)

on reflection, the benefit of the warning (catching the programming mistake where one passes a format literal string to panic! but forgets the associated format arguments) does not warrant breaking even code like the example above.

I still think the fact that my macro did not work represents a bug somewhere, perhaps in the macro system or in the stability checker.

But for now I am closing this PR; we can put in this warning later (i.e. after 1.0).

rust-highfive assigned nikomatsakis Apr 13, 2015

pnkfelix force-pushed the fsk-unary-panic branch from e4d1e6b to ac56d7f Compare April 13, 2015 10:39

alexcrichton reviewed Apr 13, 2015
View reviewed changes

pnkfelix force-pushed the fsk-unary-panic branch from 21e580f to 08079d5 Compare April 13, 2015 21:39

pnkfelix force-pushed the fsk-unary-panic branch from 08079d5 to 4ca0ba6 Compare April 15, 2015 09:37

pnkfelix force-pushed the fsk-unary-panic branch from 4ca0ba6 to 12706c9 Compare April 15, 2015 10:24

pnkfelix added 6 commits April 17, 2015 15:55

Prepare for change to unary panic! rejecting fmt string literals.

0bc20e1

rename macro to make it less palatable for use outside stdlib.

3f85887

bug fix.

840afc3

Regression test.

f77c62b

pnkfelix mentioned this pull request Apr 17, 2015

difficulty: unstable helper macro used by stable macro_rules macro #24534

Closed

pnkfelix added 2 commits April 17, 2015 22:24

allow helper macro to expand to unstable things, if necessary.

ee540bc

pnkfelix force-pushed the fsk-unary-panic branch from c53b532 to 4efe144 Compare April 17, 2015 20:26

pnkfelix added 3 commits April 18, 2015 11:20

remove attempts to encode macro's instability from implementation.

5575da2

update unit test for checking unary panic input.

0a46037

workaround bug in macro expansion.

b58359a

rust-highfive assigned alexcrichton and unassigned nikomatsakis Apr 18, 2015

alexcrichton reviewed Apr 20, 2015
View reviewed changes

pnkfelix closed this Apr 24, 2015

pnkfelix mentioned this pull request Apr 24, 2015

panic!("{}") shouldn't compile #22932

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure a sole string-literal passed to `panic!` is not a fmt string. #24370

Ensure a sole string-literal passed to `panic!` is not a fmt string. #24370

pnkfelix commented Apr 13, 2015

rust-highfive commented Apr 13, 2015

alexcrichton Apr 13, 2015

pnkfelix Apr 13, 2015

alexcrichton commented Apr 13, 2015

pnkfelix commented Apr 13, 2015

bors commented Apr 13, 2015

bors commented Apr 13, 2015

pnkfelix commented Apr 13, 2015

pnkfelix commented Apr 13, 2015

bors commented Apr 13, 2015

pnkfelix commented Apr 13, 2015

bors commented Apr 13, 2015

bors commented Apr 13, 2015

pnkfelix commented Apr 14, 2015

alexcrichton commented Apr 14, 2015

pnkfelix commented Apr 14, 2015

alexcrichton commented Apr 14, 2015

pnkfelix commented Apr 15, 2015

bors commented Apr 15, 2015

pnkfelix commented Apr 15, 2015

bors commented Apr 15, 2015

bors commented Apr 15, 2015

pnkfelix commented Apr 17, 2015

nikomatsakis commented Apr 17, 2015

alexcrichton commented Apr 17, 2015

nikomatsakis commented Apr 18, 2015

pnkfelix commented Apr 18, 2015

alexcrichton Apr 20, 2015

alexcrichton Apr 20, 2015

pnkfelix Apr 20, 2015

pnkfelix Apr 24, 2015

Ensure a sole string-literal passed to panic! is not a fmt string. #24370

Ensure a sole string-literal passed to panic! is not a fmt string. #24370

Conversation

pnkfelix commented Apr 13, 2015

rust-highfive commented Apr 13, 2015

alexcrichton Apr 13, 2015

Choose a reason for hiding this comment

pnkfelix Apr 13, 2015

Choose a reason for hiding this comment

alexcrichton commented Apr 13, 2015

pnkfelix commented Apr 13, 2015

bors commented Apr 13, 2015

bors commented Apr 13, 2015

pnkfelix commented Apr 13, 2015

pnkfelix commented Apr 13, 2015

bors commented Apr 13, 2015

pnkfelix commented Apr 13, 2015

bors commented Apr 13, 2015

bors commented Apr 13, 2015

pnkfelix commented Apr 14, 2015

alexcrichton commented Apr 14, 2015

pnkfelix commented Apr 14, 2015

alexcrichton commented Apr 14, 2015

pnkfelix commented Apr 15, 2015

bors commented Apr 15, 2015

pnkfelix commented Apr 15, 2015

bors commented Apr 15, 2015

bors commented Apr 15, 2015

pnkfelix commented Apr 17, 2015

nikomatsakis commented Apr 17, 2015

alexcrichton commented Apr 17, 2015

nikomatsakis commented Apr 18, 2015

pnkfelix commented Apr 18, 2015

alexcrichton Apr 20, 2015

Choose a reason for hiding this comment

alexcrichton Apr 20, 2015

Choose a reason for hiding this comment

pnkfelix Apr 20, 2015

Choose a reason for hiding this comment

pnkfelix Apr 24, 2015

Choose a reason for hiding this comment

Ensure a sole string-literal passed to `panic!` is not a fmt string. #24370

Ensure a sole string-literal passed to `panic!` is not a fmt string. #24370