RFC: Plan to make core and std's panic identical. by m-ou-se · Pull Request #300...
source link: https://github.com/rust-lang/rfcs/pull/3007
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
Contributor
petrochenkov commented on Oct 25
However, that would break too much existing code.
[Citation needed]
I think the first thing that needs to be here is a crater run.
Then we can break cases that are rare enough in practice (that may mean all of them).
I think the first thing that needs to be here is a crater run.
I'm not sure if a crater run would be enough here. I expect a lot of uses of core::panic!() to be in embedded code, which is unfortunately mostly closed-source.
Edit: Here's already a small preview of what crater will find: https://grep.app/search?q=panic%21%5C%28%5Cs%2A%5B%5E%5Cs%22%29%5D®exp=true&filter[lang][0]=Rust
Contributor
petrochenkov commented on Oct 26
Edit: Here's already a small preview of what crater will find: https://grep.app/search?q=panic%21%5C%28%5Cs%2A%5B%5E%5Cs%22%29%5D®exp=true&filter[lang][0]=Rust
Nothing in the search looks like an incompatible use of a format string, in particular (panic!("{}") or panic!("{{")).
So it's very possible that we can make behavior at least in this case (first argument is a string literal) identical.
Nothing in the search looks like an incompatible use of a format string
That's because that regex specifically looks for panic!() invocations that do not start with a string literal.
Searching for invocations of panic with only one argument that's both a string literal and contains braces is a lot harder. Here's an incomplete try (also note that grep.app searches through only a relatively small set of repositories): https://grep.app/search?q=panic%21%5C%28%5Cs%2A%22%5B%5E%22%5D%2A%7B%5B%5E%22%5D%2A%22%5Cs%2A%5C%29®exp=true&filter[lang][0]=Rust
Cases like that could clearly use a warning. But making them an error would unecessarily break them.
Contributor
petrochenkov commented on Oct 26
Ok, what I want to convey is that keeping compiler simple and minimizing edition differences is more important that preventing a couple of obscure crates from breaking.
That's how breaking bug fixes were treated historically and continue being treated now.
Anyway, we need some reference breakage numbers from crater.
So it's very possible that we can make behavior at least in this case (first argument is a string literal) identical.
Still throwing panic!(x) as is but also using panic!("..") as format string will just make things more inconsistent. Then panic!("{}") is used as a format string, but const X: &str = "{}"; panic!(X); is not. (But will be once const format_args becomes a thing (?).)
We're still going to need an edition gate and a lint for panic!(non_string) anyway. Special casing panic!(one_arg) to have panic!(string_literal) to be interpreted as a format string in Rust 2018 will make things more complicated, not less. (Note that just $_:literal in the macro_rules wouldn't work, because that also matches other literals like 123 and does not match concat!("{}", "\n").)
Note that the idea of shifting panic!(expr) into a new function was also discussed lightly in the RFC 2795 tracking issue, e.g rust-lang/rust#67984 (comment).
One argument I found compelling - besides all the consistency / simplification arguments - is that panic! is a macro because it does string formatting, and in the panic-with-any case there isn't actually any formatting going on.
I particularly agree with
💔 Problem 3: panic!(123), panic!(&".."), panic!(b".."), etc. are probably mistakes, but compile fine with std.
Even as an experienced Rust developer, I still often do this when I'm thinking more about what I'm debugging than the debug code I'm hacking in. I often worry Rust beginners debugging their first program will not easily understand what they did wrong when they see error messages reading along the lines of panicked at Box<Any>.
All for removing differences between core and std, and for advancing progress toward capturing format string arguments.
For the sake of completeness, one alternative that I've not yet seen proposed would be to introduce a new alternative to panic! with the correct semantics. Obviously this would be a much more drastic step, but risks no breakage whatsoever.
Specifically:
- Only for Rust 2021, we apply the breaking changes as in the previous section.
Aaron1011 on Oct 27 •
Member
How will this interact with macro expansion? For example, if I have the following code:
// crate2018 (2018 edition):
macro_rules! custom_panic {
($($arg:tt),*) => {
panic!($($arg),*)
}
}
fn foo() {
custom_panic!("Weird format: {}");
}
// crate2021 (2021 edition)
fn bar() {
crate2018::custom_panic!("Weird format: {}");
}what should the result be?
m-ou-se on Oct 27
Author
Member
That's a good question. We could either look at the edition of the span of panic or at the edition of the span of "Weird format: {}".
I think the former would make most sense. Then the macro doesn't change its behaviour until it is updated to Rust 2021 itself.
What do you think?
Aaron1011 on Oct 27 •
Member
That sounds reasonable to me. Note that this will be a 'best effort' approach to preventing breakage. With a somewhat contrived macro, this approach can be made to fail:
macro_rules! custom_invoke {
($name:ident) => {
$name!("My panic: {}")
}
}
fn main() {
custom_invoke!(panic)
}That is, a higher-order macro could use the ident panic from a 2021 edition crate, causing the new behavior to be used.
While this particular macro seems very unrealistic, there are some places in rustc that actually use higher-order macros. Unfortunately, I think we need to accept risk of breakage if this RFC is accepted. However, this would require a crate to be bumped to the 2021 edition, so we will never break a 2018-edition-only crate graph.
m-ou-se on Oct 27 •
Author
Member
That'd still make sense right? You're now passing the macro the 2021 panic, not the 2018 panic. With this style of macro, it's the responsibility of the caller to provide it with the name of a macro that works. Just like custom_invoke!(println) also wouldn't compile. So (like you said) the breakage in this example is in the 2021 crate that calls the macro, not in the 2018 one that defines it.
Aaron1011 on Oct 27 •
Member
With proc macros, it may not be as clear that crate2021 is to blame. Using Span::resolved_at, you could get a panic ident that appears to come from a 2021 crate, even if the literal panic never appears in a 2021 crate.
To be clear, I woud be shocked if this ever came up in practice. However, I think it would be worth adding an note to the RFC that this is idended to mitigate cross-edition breakage, not prevent it entirely.
Diggsey on Oct 27
Contributor
@Aaron1011 isn't this true for all edition changes though? If I write a proc-macro that attaches the wrong spans, then it may stop working when a crate using that macro upgrades to a new edition.
Perhaps there should be an exception to the stability rules for bugs that cause code to be tagged with the wrong edition.
If progress is made on implementing future-incompat-report, we could enable it for the lint referenced by this RFC.
rust-lang/rust#67984 (comment)
We would like to see a concrete proposal with a migration plan.
Here it is. :)
Contributor
nikomatsakis commented on Oct 28
I'm nominating for discussion in the @rust-lang/lang triage meeting, although I think this may also be considered a @rust-lang/libs RFC.
- Instead of the last step, we could also simply break `assert!(expr, non_string_literal)` in all editions.
This usage is probably way less common than `panic!(non_string_literal)`.
Comment on lines
+184 to +185
Thanks for working on this @m-ou-se! We discussed this in the recent Libs meeting and felt like this was a reasonable path forward to making our panicking macros behave consistently. I also quite liked the problem-solution organization style 
This work is somewhat time-critical to catch the 2021 edition, so to get eyes and input from the wider team let's kick off a FCP!
@rfcbot fcp merge
Team member @KodrAus has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
Contributor
nikomatsakis commented on Nov 2
@rfcbot reviewed
We discussed this in our @rust-lang/lang backlog bonanza meeting and also felt quite good about it.
Member
Author
m-ou-se commented 19 days ago
@BurntSushi @pnkfelix @withoutboats Ping. 
Special limited time offer: 50% off your next checkmark if you click today!*
*Terms and conditions apply. Offer valid for up to 1 (one) checkmark per customer.
rfcbot commented 19 days ago
This is now entering its final comment period, as per the review above.
rfcbot commented 9 days ago
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
The RFC will be merged soon.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK