Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tomaka opened this issue Mar 1, 2015 · 21 comments
Closed

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

tomaka opened this issue Mar 1, 2015 · 21 comments
Assignees
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@tomaka
Copy link
Contributor

tomaka commented Mar 1, 2015

I didn't find any duplicate issue, but this problem has probably been there for a long time.

  • The code panic!("{}", 7) compiles and prints thread <main> panicked at "7", ...
  • The code panic!("{} {}", 7) doesn't compile as an argument is missing
  • The code panic!("{}") does compile and prints thread <main> panicked at "{}", ...

The last one doesn't detect potential mistakes, and is inconsistent with println!("{}") which doesn't compile.

@kmcallister kmcallister added A-libs A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) labels Mar 1, 2015
@amaneureka
Copy link

Actually this is not an issue; If you go through the docs (http://doc.rust-lang.org/std/macro.panic!.html)
"The multi-argument form of this macro panics with a string and has the format! syntax for building a string."
So, In the code panic!("{}", 7) you are calling multi-argument form of this macro. Same for panic!("{} {}", 7) Here Format! syntax will be followed, And that is why the code doesn't compile.
Now panic!("{}") This is a single-argument form of this macro, so it gonna to be treated as plain string. Hence it got compiled and prints thread <main> panicked at "{}", ....
In case of println!("{}") it is clearly stated in API docs that, println uses Format! syntax irrespective weather it is a multi-argument or not.

@tomaka
Copy link
Contributor Author

tomaka commented Mar 1, 2015

I know why it compiles, but it's still an issue.

@rprichard
Copy link
Contributor

It'd be nice if we could remove the inconsistency between single-argument and multiple-argument panic! macros. It's easy if we make single-argument panic! call core::panicking::panic_fmt, but that would add massive bloat to the panic! call sites.

This same issue came up in #22335, which tried to optimize write*! and print*! in the same way that panic! is already optimized. After stumbling onto the inconsistency, the author added a format_arg! that evaluates to a &'static str. As a new macro, it needed an RFC.

I'm interested in this stuff right now because I'm working on a bootloader, and the overhead of the panic formatting code is giving me trouble -- http://users.rust-lang.org/t/fixed-overhead-rust-bootloader-and-core-panicking/429/10.

@rprichard
Copy link
Contributor

The same inconsistency also applies to assert!:

    assert!(1 == 2, "{}");          // prints: '{}'
    assert!(1 == 2, "{}", 1234);    // prints: '1234'

Currently the one-argument assert! just calls panic!(concat!("assertion failed: ", stringify!($cond))), so fixing panic! would break assert! on this code:

    assert!("abc" == "{}");         // prints: 'assertion failed: "abc" == "{}"'

Probably easy to fix.

@pnkfelix
Copy link
Member

pnkfelix commented Mar 5, 2015

1.0 polish; P-low. If this gets fixed by 1.0, great; but if we shipped 1.0 with this semantics, its not the end of the world.

@pnkfelix pnkfelix added the P-low Low priority label Mar 5, 2015
@pnkfelix pnkfelix added this to the 1.0 milestone Mar 5, 2015
@rprichard
Copy link
Contributor

If we make single-argument panic! use the formatting system like write!, then we need an alternate syntax for the current single-argument panic! in libstd, so we can still do things like:

panic!({ let x = "{}"; x })
panic!(7)

Maybe add a panic_arg! or panic_any! macro?

The other thing needed for this issue is some way to convert a format string to an output string:

  • A new format_arg! macro. (I suggest format_args_literal! as an alternate name.)
  • Use format_args! and a new method on Arguments to extract the lone string piece. It will probably generate larger code then the previous option, but not worse than the current libstd panic!. We'd like to place the string piece in a static item, like core::panicking::panic does, but we can't have a function call in a static. This approach would generate a lot more code at each call-site, but it ought to be optimized out.

I benchmarked the size overhead of calling panic! with a single-argument on x86_64. It's 52 bytes in libcore and 67 bytes in libstd. Calling core::panicking::panic_fmt takes 110 bytes.

@rprichard
Copy link
Contributor

I found a mistake in a libstd test case that would presumably have been avoided were this issue fixed:

assert_eq!((0 as $T).is_power_of_two(), false);
assert_eq!((1 as $T).is_power_of_two(), true);
assert_eq!((2 as $T).is_power_of_two(), true);
assert_eq!((3 as $T).is_power_of_two(), false);
assert_eq!((4 as $T).is_power_of_two(), true);
assert_eq!((5 as $T).is_power_of_two(), false);
assert!(($T::MAX / 2 + 1).is_power_of_two(), true);

The last line should be assert_eq! for consistency (or maybe they should all use plain assert!). The test still checks the right thing anyway. I'll submit a PR.

What I was trying to find was a place invoking assert! with the intent to panic with a non-string value. That usage would break if panic! were split into two macros. (I did find one place passing a non-constant string as the second assert! argument, but it could be fixed using a "{}" format.)

What if we changed the non-formatting panic! by inserting a token at the front, maybe panic!(msg 7), in which case one could also write assert!(condition, msg 7)?

@alexcrichton
Copy link
Member

triage: P-high

I think the panic!(anything) aspect of panic! will make quite hard to overcome a limitation such as this, and I also think that we should audit whether that's actually an interface we want to support at this time.

@rprichard
Copy link
Contributor

@alexcrichton In case it matters for bookkeeping: GitHub is still labeling this issue P-low, despite your triage: P-high comment.

@rust-highfive rust-highfive added P-medium Medium priority and removed P-low Low priority labels Mar 9, 2015
@rust-highfive rust-highfive modified the milestones: 1.0 beta, 1.0 Mar 9, 2015
@alexcrichton
Copy link
Member

hehe, but it appears to have picked up your comment :)

@nrc nrc modified the milestones: 1.0 beta, 1.0 Mar 11, 2015
@brson
Copy link
Contributor

brson commented Apr 7, 2015

We need a patch to appear for this very soon for it to happen.

@brson
Copy link
Contributor

brson commented Apr 7, 2015

I guess we also need a firm decision.

@rprichard
Copy link
Contributor

Another question to consider: if we change single-argument panic! to use a formatting string literal, does it continue to have a &str panic value or does it have a String panic value like the multiple-argument panic!? (i.e. what does JoinHandle::join() return?)

I'm happy to write a patch, but I feel like it needs an RFC, and I haven't seen any discussion of this issue. Does it need to be posted on the forum?

@bombless
Copy link
Contributor

bombless commented Apr 8, 2015

It prints panicked at 'Box<Any>'

I'd rather add a panic_fmt! macro.

@rprichard
Copy link
Contributor

I created a post on the forum. http://internals.rust-lang.org/t/panic-shouldnt-compile/1851

@pnkfelix
Copy link
Member

pnkfelix commented Apr 9, 2015

Majority of the team says they want to preserve much of the existing behavor -- i.e. support passing a single arbitrary value, but also support the formatting form.

The main strategy that was supported was to catch this error case (where a string-literal template string is being passed as the single argument, and it has placeholder braces), as a special case (perhaps via a lint) and only reject that case. E.g. any non string-literal unary input to panic! would continue to be accepted.

@pnkfelix pnkfelix self-assigned this Apr 9, 2015
@pnkfelix
Copy link
Member

(I think I have something put together to address this.)

pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 13, 2015
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.
bors added a commit that referenced this issue Apr 13, 2015
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 added a commit that referenced this issue Apr 13, 2015
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.
pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 15, 2015
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.
pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 15, 2015
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.
bors added a commit that referenced this issue Apr 15, 2015
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.
pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 17, 2015
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.
pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 18, 2015
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.
@pnkfelix
Copy link
Member

(The backwards-compatible strategy for addressing this would be to just warn when format string literal is passed to unary panic!. I had something hacked up for this in PR #24370, but I ran into trouble getting it past the stability checker -- which probably represents a bug somewhere in the macro system and/or stability checker. Anyway, again, that strategy would be a backwards-compatible fix to this.)

@steveklabnik
Copy link
Member

Triage: this does reproduce today, printing

thread '<main>' panicked at '{}', foo.rs:2

@brson brson added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 14, 2016
@alexcrichton
Copy link
Member

This is very similar to the recently closed #30143, so I'm going to tag this as such (rust-2-breakage-wishlist) and close.

@colt-browning
Copy link

This is fixed in Rust 2021, so no rust-2-breakage-wishlist label is needed.

@fee1-dead fee1-dead removed the rust-2-breakage-wishlist In the wishlist of breaking changes that requires rust 2.0 label Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.