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

core::panic!() cannot be used everywhere std::panic!() can. #80846

Closed
m-ou-se opened this issue Jan 9, 2021 · 5 comments
Closed

core::panic!() cannot be used everywhere std::panic!() can. #80846

m-ou-se opened this issue Jan 9, 2021 · 5 comments
Assignees
Labels
C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@m-ou-se
Copy link
Member

m-ou-se commented Jan 9, 2021

This compiles:

fn main() {
    let x = &5;
    x = &std::panic!()
}

This does not:

fn main() {
    let x = &5;
    x = &core::panic!()
}
error[E0308]: mismatched types
 --> src/main.rs:3:9
  |
3 |     x = &core::panic!()
  |         ^^^^^^^^^^^^^^^ expected integer, found `!`
  |
  = note: expected reference `&{integer}`
             found reference `&!`

The cause seems to be that std::panic!() expands to { some_function(..) } while core::panic!() expands to some_function(..), without { .. }. Adding { .. } in the core::panic!() macro_rules 'fixes' it.

@m-ou-se m-ou-se added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Jan 9, 2021
@m-ou-se m-ou-se self-assigned this Jan 9, 2021
@m-ou-se
Copy link
Member Author

m-ou-se commented Jan 10, 2021

#80848 is an attempt at fixing this, by adding { .. } in the macro_rules! panic in core.

But as mentioned in #80848 (comment), the braces were not added in std::panic!() for this purpose, and seem to be left there by accident.

Is this a bug? Should this behave the same with and without those braces?

@eddyb
Copy link
Member

eddyb commented Jan 11, 2021

The never type never ceases to surprise, does it? cc @nikomatsakis

I'd be worried about changing typeck behavior here, personally, I'm not even sure it would survive crater, and could have subtler implications still. Adding the braces to core::panic! should be fine.

bors added a commit to rust-lang-ci/rust that referenced this issue 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 issue 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.
@bstrie
Copy link
Contributor

bstrie commented Feb 7, 2021

I see that #80162, which listed this issue as a blocker, has been closed. Does that mean this can be closed too?

@m-ou-se
Copy link
Member Author

m-ou-se commented Feb 7, 2021

For that PR we decided to just change it for edition 2021, but this problem still exists in the current editions. It's not a big problem, but it'd be good to figure out why this difference exists.

@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 3, 2021

Closing this issue because I don't care enough, especially since this problem is no longer there in edition 2021.

If anyone does care about this issue in edition 2015/2018, feel free to re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. 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.

3 participants