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

Add std::panic::panic_any. #74622

Merged
merged 4 commits into from
Oct 31, 2020
Merged

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Jul 22, 2020

The discussion of #67984 lead to the conclusion that there should be a macro or function separate from std::panic!() for throwing arbitrary payloads, to make it possible to deprecate or disallow (in edition 2021) std::panic!(arbitrary_payload).

Alternative names:

  • panic_with!(..)
  • start_unwind(..) (panicking doesn't always unwind)
  • throw!(..)
  • panic_throwing!(..)
  • panic_with_value(..)
  • panic_value(..)
  • panic_with(..)
  • panic_box(..)
  • panic(..)

The equivalent (private, unstable) function in libstd is called std::panicking::begin_panic.

I suggest panic_any, because it allows for any (Any + Send) type.

Tracking issue: #78500

@rust-highfive
Copy link
Collaborator

r? @shepmaster

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2020
@daboross
Copy link
Contributor

daboross commented Jul 22, 2020

To bikeshed the name a bit more, what about panic_with_value, referring to the fact that one can provide any value? I don't think we necessarily need a short name here, given how infrequently used it is (though if there were a short name which fits well, that would be better).

I'd personally prefer panic_with_value over panic_box (or panic_with_box) just because the fact that it can transmit anything seems slightly more intuitive than the fact that that transmission is done through a box? Though if it did take a Box<dyn Any + Send + 'static>, rather than taking T and boxing it as an implementation detail, I think panic_box (or I think I'd still prefer panic_with_box) could fit well.

src/libstd/panic.rs Outdated Show resolved Hide resolved
src/libstd/panic.rs Outdated Show resolved Hide resolved
src/libstd/panic.rs Outdated Show resolved Hide resolved
src/libstd/panic.rs Outdated Show resolved Hide resolved
@shepmaster
Copy link
Member

My own 2¢ on the name — I'd want panic_value!

@nikomatsakis nikomatsakis added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jul 27, 2020
@nikomatsakis
Copy link
Contributor

Nominating to make @rust-lang/lang members aware of this.

@nikomatsakis
Copy link
Contributor

Regarding naming, I agree that something based on "value" is preferable to "box". The fact that boxing occurs doesn't feel like the important thing for users to keep in mind. I'm happy with panic_with_value, panic_value, or even panic_with, although I raised a concern on that last one.

@nikomatsakis nikomatsakis added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jul 27, 2020
@nikomatsakis
Copy link
Contributor

That said, we discussed this in the @rust-lang/lang meeting today and we felt like this sort of bikeshedding is really a @rust-lang/libs affair, so i'm going to retag this as T-libs.

@joshtriplett
Copy link
Member

joshtriplett commented Jul 27, 2020

Could we separate the idea of having a separate macro for "panic with this specific value" from the "there's one argument and it's a string literal" case? I'd love to see the 2021-edition panic! on a string literal always treat it as a format string if it contains any format arguments, and emit a compile-time error if those arguments aren't satisfied. That seems separable from "what do we do if there's one argument and it isn't a string literal".

@SimonSapin
Copy link
Contributor

to deprecate or disallow (in edition 2021) std::panic!(arbitrary_payload).

That doesn’t seem possible. As far as I know we cannot make macros behave differently based on the edition of the call site. (Editions are per-crate, so std itself is in one edition.)

@Mark-Simulacrum
Copy link
Member

It would require compiler magic, I suppose, but only in a more general way, by adding something like #[deprecated(since = "edition2021")]. That would be straightforward to add to the compiler I think and is also desirable for the MIN/MAX constants and such.

@bors
Copy link
Contributor

bors commented Jul 28, 2020

☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jul 29, 2020

So, it feels like we need to discuss this PR in the context of the full plan. As I understood it, the plan was this:

  • In 2021 Edition:
    • std::panic! requires a string literal and literal is treated as a format string
    • std::panic_with_value accepts only a value
  • In 2018 Edition:
    • std::panic! gets a migration warning if it has no arguments and there is a suggestion to rewrite to panic_with_value -- this is a bit debatable, and we should discuss the particulars, but it seems strictly necessary from my POV to avoid changing runtime semantics

I forget what we would do about core::panic...

The hope was indeed that in 2021 Edition crates, the panic! macro would always begin with a string literal, and that literal would be treated as a format string. It was mentioned that this could be achieved by having the prelude depend on the edition, such that we had panic! route to a distinct macro in the 2021 Edition. It certainly seems possible for us to do it in some way, but we'd have to think about the best impl strategy.

@Mark-Simulacrum
Copy link
Member

core::panic! can't accept arbitrary expressions in the single-argument case, only &str, so I would suspect something like this:

  • In 2021 Edition:
    • core::panic! requires a string literal and literal is treated as a format string
      • We may need to explicitly optimize the "identity" case to not bring in format infrastructure, though, for code size reasons.
  • In 2018 Edition:
    • core::panic!("{:?}") and other single-argument but format strings emit future-compat warnings
      * if you want this, you should write core::panic!("{}", "{:?}") or core::panic!("{{:?}}")

@m-ou-se
Copy link
Member Author

m-ou-se commented Jul 29, 2020

We may need to explicitly optimize the "identity" case to not bring in format infrastructure, though, for code size reasons.

That's already taken care of by #74056

@sfackler
Copy link
Member

Why does panic_with_value! need to be a macro at all rather than a normal function?

@nikomatsakis
Copy link
Contributor

@sfackler it doesn't, I think that was me just typing wrong

@Dylan-DPC-zz
Copy link

@m-ou-se if you can resolve the conflicts, we can push this forward

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2020
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 28, 2020
library/std/src/panic.rs Outdated Show resolved Hide resolved
Co-authored-by: Camelid <camelidcamel@gmail.com>
@m-ou-se m-ou-se mentioned this pull request Oct 28, 2020
3 tasks
@KodrAus
Copy link
Contributor

KodrAus commented Oct 30, 2020

We discussed this in the recent Libs meeting and felt happy with the name panic_any. We felt happy to merge this and move discussion over to the RFC itself. With that in mind...

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 30, 2020

📌 Commit b48fee0 has been approved by KodrAus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2020
@camelid camelid added the A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows label Oct 30, 2020
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Oct 30, 2020
… r=KodrAus

Add std::panic::panic_any.

The discussion of rust-lang#67984 lead to the conclusion that there should be a macro or function separate from `std::panic!()` for throwing arbitrary payloads, to make it possible to deprecate or disallow (in edition 2021) `std::panic!(arbitrary_payload)`.

Alternative names:

- `panic_with!(..)`
- ~~`start_unwind(..)`~~ (panicking doesn't always unwind)
- `throw!(..)`
- `panic_throwing!(..)`
- `panic_with_value(..)`
- `panic_value(..)`
- `panic_with(..)`
- `panic_box(..)`
- `panic(..)`

The equivalent (private, unstable) function in `libstd` is called `std::panicking::begin_panic`.

I suggest `panic_any`, because it allows for any (`Any + Send`) type.

_Tracking issue: #78500_
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 31, 2020
… r=KodrAus

Add std::panic::panic_any.

The discussion of rust-lang#67984 lead to the conclusion that there should be a macro or function separate from `std::panic!()` for throwing arbitrary payloads, to make it possible to deprecate or disallow (in edition 2021) `std::panic!(arbitrary_payload)`.

Alternative names:

- `panic_with!(..)`
- ~~`start_unwind(..)`~~ (panicking doesn't always unwind)
- `throw!(..)`
- `panic_throwing!(..)`
- `panic_with_value(..)`
- `panic_value(..)`
- `panic_with(..)`
- `panic_box(..)`
- `panic(..)`

The equivalent (private, unstable) function in `libstd` is called `std::panicking::begin_panic`.

I suggest `panic_any`, because it allows for any (`Any + Send`) type.

_Tracking issue: #78500_
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Oct 31, 2020
… r=KodrAus

Add std::panic::panic_any.

The discussion of rust-lang#67984 lead to the conclusion that there should be a macro or function separate from `std::panic!()` for throwing arbitrary payloads, to make it possible to deprecate or disallow (in edition 2021) `std::panic!(arbitrary_payload)`.

Alternative names:

- `panic_with!(..)`
- ~~`start_unwind(..)`~~ (panicking doesn't always unwind)
- `throw!(..)`
- `panic_throwing!(..)`
- `panic_with_value(..)`
- `panic_value(..)`
- `panic_with(..)`
- `panic_box(..)`
- `panic(..)`

The equivalent (private, unstable) function in `libstd` is called `std::panicking::begin_panic`.

I suggest `panic_any`, because it allows for any (`Any + Send`) type.

_Tracking issue: #78500_
This was referenced Oct 31, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 31, 2020
Rollup of 7 pull requests

Successful merges:

 - rust-lang#74622 (Add std::panic::panic_any.)
 - rust-lang#77099 (make exp_m1 and ln_1p examples more representative of use)
 - rust-lang#78526 (Strip tokens from trait and impl items before printing AST JSON)
 - rust-lang#78550 (x.py setup: Create config.toml in the current directory, not the top-level directory)
 - rust-lang#78577 (validator: Extend aliasing check to a call terminator)
 - rust-lang#78581 (Constantify more BTreeMap and BTreeSet functions)
 - rust-lang#78587 (parser: Cleanup `LazyTokenStream` and avoid some clones)

Failed merges:

r? `@ghost`
@bors bors merged commit 76b8b00 into rust-lang:master Oct 31, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 31, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 20, 2020
…nt, r=estebank

Add lint for panic!("{}")

This adds a lint that warns about `panic!("{}")`.

`panic!(msg)` invocations with a single argument use their argument as panic payload literally, without using it as a format string. The same holds for `assert!(expr, msg)`.

This lints checks if `msg` is a string literal (after expansion), and warns in case it contained braces. It suggests to insert `"{}", ` to use the message literally, or to add arguments to use it as a format string.

![image](https://user-images.githubusercontent.com/783247/96643867-79eb1080-1328-11eb-8d4e-a5586837c70a.png)

This lint is also a good starting point for adding warnings about `panic!(not_a_string)` later, once [`panic_any()`](rust-lang#74622) becomes a stable alternative.
@m-ou-se m-ou-se deleted the panic-box branch November 30, 2020 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.