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

assert! in edition 2018 has a non-hygienic use of panic! #56389

Closed
Nemo157 opened this issue Nov 30, 2018 · 10 comments
Closed

assert! in edition 2018 has a non-hygienic use of panic! #56389

Nemo157 opened this issue Nov 30, 2018 · 10 comments

Comments

@Nemo157
Copy link
Member

Nemo157 commented Nov 30, 2018

This code runs fine on edition 2015, and has no warnings with #![warn(rust_2018_compatibility)], but fails to compile on edition 2018 (playground):

#![no_implicit_prelude]

fn main() {
    assert!(true);
}

error message:

error: cannot find macro `panic!` in this scope
 --> src/main.rs:4:5
  |
4 |     assert!(true);
  |     ^^^^^^^^^^^^^^
  |
  = help: have you added the `#[macro_use]` on the module/import?
@Nemo157 Nemo157 changed the title assert! in edition 2018 has a non-hygenic use of panic! assert! in edition 2018 has a non-hygienic use of panic! Nov 30, 2018
@Centril
Copy link
Contributor

Centril commented Dec 1, 2018

cc @petrochenkov

@petrochenkov
Copy link
Contributor

petrochenkov commented Dec 1, 2018

This isn't a problem with assert alone, most of std/core and built-in macros call other macros unhygienically (some were recently fixed though #55597).
I was actually going to fix this systematically, it's just not a highest priority work.

Also, using $crate::panic!() etc in std macros is technically a breaking change, because especially creative users could override standard panics and other macros with their own versions, so we'll probably need a crater run.

@Nemo157
Copy link
Member Author

Nemo157 commented Dec 1, 2018

assert isn't actually a std macro, it's a builtin (otherwise it would have to be called as std::assert! above). I was surprised to find that std::assert_eq! actually has a hygienic use of panic! while assert! doesn't. IMO it would be nice to actually remove builtins from Rust's public API and hide them all behind core macros, (e.g. macro_rules! assert { ($tt:tt) => { _builtin_assert!($tt) } }), but that's probably a breaking change at this point.

@petrochenkov
Copy link
Contributor

assert_eq also calls panic unhygienically (not via $crate::panic!()).

IMO it would be nice to actually remove builtins from Rust's public API and hide them all behind core macros

+1

but that's probably a breaking change at this point.

Probably not (modulo no_implicit_prelude which is pretty much unused).
IIRC, the main problem was documentation - it should show actual accepted inputs rather than $tt:tt, but that should be solvable somehow.

@Nemo157
Copy link
Member Author

Nemo157 commented Dec 1, 2018

assert_eq also calls panic unhygienically (not via $crate::panic!()).

That's surprising, it appears to work in edition 2018 with no_implicit_prelude still somehow...

@petrochenkov
Copy link
Contributor

Well, standard library is still built with Rust 2015, and on 2015 macros from #[macro_use] extern crate ... are still available with #[no_implicit_prelude] (#55630).
I think we can fix that, but it will also need some breakage estimation with crater.

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 22, 2019

#58702 appears to have extended the non-hygienity to other core macros, for crates using both edition 2015 and 2018:

> # d6f513ec7d1e83c8689f94fb48686dd11f1d1c80 is just before #58702
> echo '#![no_implicit_prelude] fn main() { ::std::assert_eq!(1, 1) }' | rustc +d6f513ec7d1e83c8689f94fb48686dd11f1d1c80 --edition 2018 -

> echo '#![no_implicit_prelude] fn main() { ::std::assert_eq!(1, 1) }' | rustc +d6f513ec7d1e83c8689f94fb48686dd11f1d1c80 --edition 2015 -

> # 5d20ff4d2718c820632b38c1e49d4de648a9810b is the merge of #58702
> echo '#![no_implicit_prelude] fn main() { ::std::assert_eq!(1, 1) }' | rustc +5d20ff4d2718c820632b38c1e49d4de648a9810b --edition 2015 -
error: cannot find macro `panic!` in this scope
 --> <anon>:1:37
  |
1 | #![no_implicit_prelude] fn main() { ::std::assert_eq!(1, 1) }
  |                                     ^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: have you added the `#[macro_use]` on the module/import?
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to previous error

> echo '#![no_implicit_prelude] fn main() { ::std::assert_eq!(1, 1) }' | rustc +5d20ff4d2718c820632b38c1e49d4de648a9810b --edition 2018 -
error: cannot find macro `panic!` in this scope
 --> <anon>:1:37
  |
1 | #![no_implicit_prelude] fn main() { ::std::assert_eq!(1, 1) }
  |                                     ^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: have you added the `#[macro_use]` on the module/import?
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to previous error

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 22, 2019

The non-hygienity even extends to the panic macro itself, ::std::panic!() expands to panic!("explicit panic") and gives error: cannot find macro panic! in this scope.

@petrochenkov
Copy link
Contributor

For all macros beside panic the underlying issue is fixed in #61629.
panic is special, #61629 describes the situation in more detail.

Centril added a commit to Centril/rust that referenced this issue Jun 12, 2019
Hygienize macros in the standard library

Same as rust-lang#55597, but for all macros in the standard library.
Nested macro calls will now call what they are intended to call rather than whatever is in the closest scope at call site.
Technically this is a breaking change, so crater run would probably be useful.

---

One exception that is not hygienized is calls to `panic!(...)`.
Macros defined in libcore do not want to call `core::panic`.
What they really want to call is either `std::panic` or `core::panic` depending on `no_std` settings.
EDIT: After some thought, recursive calls to `panic` from `panic` itself probably do want to use `$crate` (UPDATE: done).

Calling `std::panic` from macros defined in std and "whatever `panic` is in scope" from macros defined in libcore is probably even worse than always calling "whatever `panic` is in scope", so I kept the existing code.

The only way to do the std/core switch correctly that I'm aware of is to define a built-in panic macro that would dispatch to `std::panic` or `core::panic` using compiler magic.
Then standard library macros could delegate to this built-in macro.
The macro could be named `panic` too, that would fix rust-lang#61567.
(This PR doesn't do that.)

---
cc rust-lang#56389
cc rust-lang#61567
Fixes rust-lang#61699
r? @alexcrichton
Centril added a commit to Centril/rust that referenced this issue Jun 12, 2019
Hygienize macros in the standard library

Same as rust-lang#55597, but for all macros in the standard library.
Nested macro calls will now call what they are intended to call rather than whatever is in the closest scope at call site.
Technically this is a breaking change, so crater run would probably be useful.

---

One exception that is not hygienized is calls to `panic!(...)`.
Macros defined in libcore do not want to call `core::panic`.
What they really want to call is either `std::panic` or `core::panic` depending on `no_std` settings.
EDIT: After some thought, recursive calls to `panic` from `panic` itself probably do want to use `$crate` (UPDATE: done).

Calling `std::panic` from macros defined in std and "whatever `panic` is in scope" from macros defined in libcore is probably even worse than always calling "whatever `panic` is in scope", so I kept the existing code.

The only way to do the std/core switch correctly that I'm aware of is to define a built-in panic macro that would dispatch to `std::panic` or `core::panic` using compiler magic.
Then standard library macros could delegate to this built-in macro.
The macro could be named `panic` too, that would fix rust-lang#61567.
(This PR doesn't do that.)

---
cc rust-lang#56389
cc rust-lang#61567
Fixes rust-lang#61699
r? @alexcrichton
@petrochenkov
Copy link
Contributor

I'm going to close this in favor of #61567 (which doesn't have a discussion distracting from the primary issue).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants