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

RFC: Plan to make core and std's panic identical. #3007

Merged
merged 3 commits into from
Jan 13, 2021

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Oct 25, 2020

@petrochenkov
Copy link
Contributor

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).

@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 25, 2020

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&regexp=true&filter[lang][0]=Rust

@petrochenkov
Copy link
Contributor

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&regexp=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.

@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 25, 2020

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&regexp=true&filter[lang][0]=Rust

Cases like that could clearly use a warning. But making them an error would unecessarily break them.

@petrochenkov
Copy link
Contributor

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.

@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 25, 2020

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").)

@davidhewitt
Copy link
Contributor

davidhewitt commented Oct 26, 2020

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>.

@bstrie
Copy link
Contributor

bstrie commented Oct 26, 2020

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.
Copy link
Member

@Aaron1011 Aaron1011 Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@Aaron1011 Aaron1011 Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@m-ou-se m-ou-se Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Aaron1011 Aaron1011 Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. However, there have been a bunch of proc-macro API stabilizations since the 2018 edition, so I don't think it's really come up before.

@Aaron1011
Copy link
Member

If progress is made on implementing future-incompat-report, we could enable it for the lint referenced by this RFC.

@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 27, 2020

cc @rust-lang/lang

rust-lang/rust#67984 (comment)

We would like to see a concrete proposal with a migration plan.

Here it is. :)

@nikomatsakis nikomatsakis added I-nominated T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. labels Oct 27, 2020
@nikomatsakis
Copy link
Contributor

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.

Comment on lines +184 to +185
- 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)`.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to do a crater run for this failed before it even started, because that change didn't even get past the @bors try run. It caused a compilation error while compiling this part of clap:

https://github.com/clap-rs/clap/blob/0c7da9f5b32bcd6968a70258a4868d439fbc1fc3/src/app/parser.rs#L181-L184

Apparently this behaviour of assert!() is relied upon.

text/0000-panic-plan.md Outdated Show resolved Hide resolved
Co-authored-by: Ashley Mannix <[email protected]>
@KodrAus
Copy link
Contributor

KodrAus commented Oct 30, 2020

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

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 30, 2020

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.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Oct 30, 2020
@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

We discussed this in our @rust-lang/lang backlog bonanza meeting and also felt quite good about it.

@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Nov 6, 2020
@m-ou-se
Copy link
Member Author

m-ou-se commented Nov 30, 2020

@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 rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Dec 1, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Dec 1, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Dec 1, 2020
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Dec 11, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Dec 11, 2020

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.

@KodrAus KodrAus merged commit 3669e8c into rust-lang:master Jan 13, 2021
@m-ou-se m-ou-se deleted the panic branch January 13, 2021 20:31
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 1, 2021
Implement Rust 2021 panic

This implements the Rust 2021 versions of `panic!()`. See rust-lang#80162 and rust-lang/rfcs#3007.

It does so by replacing `{std, core}::panic!()` by a bulitin macro that expands to either `$crate::panic::panic_2015!(..)` or `$crate::panic::panic_2021!(..)` depending on the edition of the caller.

This does not yet make std's panic an alias for core's panic on Rust 2021 as the RFC proposes. That will be a separate change: rust-lang@c5273bd That change is blocked on figuring out what to do with rust-lang#80846 first.
Manishearth pushed a commit to Manishearth/rust-clippy that referenced this pull request Feb 3, 2021
Implement Rust 2021 panic

This implements the Rust 2021 versions of `panic!()`. See rust-lang/rust#80162 and rust-lang/rfcs#3007.

It does so by replacing `{std, core}::panic!()` by a bulitin macro that expands to either `$crate::panic::panic_2015!(..)` or `$crate::panic::panic_2021!(..)` depending on the edition of the caller.

This does not yet make std's panic an alias for core's panic on Rust 2021 as the RFC proposes. That will be a separate change: rust-lang/rust@c5273bd That change is blocked on figuring out what to do with rust-lang/rust#80846 first.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. Libs-Tracked Libs issues that are tracked on the team's project board. T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants