Qualify panic! as core::panic! in non-built-in core macros

[camelid]

Fixes #78333.

---

Otherwise code like this

#![no_implicit_prelude]

fn main() {
    ::std::todo!();
    ::std::unimplemented!();
}

will fail to compile, which is unfortunate and presumably unintended.

This changes many invocations of panic! in a macro_rules! definition
to invocations of $crate::panic!, which makes the invocations hygienic.

Note that this does not make the built-in macro assert! hygienic.

[rust-highfive]

r? @Mark-Simulacrum

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

[camelid]

I would like to write a test for this, but I'm not sure where to put it. Where are library compiletests put?

[camelid]

Tested locally and it seems to work.

stderr (compiling)

warning: unreachable statement
 --> issue-78333.rs:5:5
  |
4 |     ::std::todo!();
  |     --------------- any code following this expression is unreachable
5 |     ::std::unimplemented!();
  |     ^^^^^^^^^^^^^^^^^^^^^^^^ unreachable statement
  |
  = note: `#[warn(unreachable_code)]` on by default
  = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

warning: 1 warning emitted

stderr (running)

thread 'main' panicked at 'not yet implemented', issue-78333.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[jyn514]

I would like to write a test for this, but I'm not sure where to put it. Where are library compiletests put?

src/test/ui/macros/ seems fine.

[petrochenkov]

This PR reroutes majority of code that previously called std::panic to core::panic.
We can only do this if core::panic and std::panic have identical interface and behavior in all case affected by the PR.
cc @RalfJung @m-ou-se

cc #61629 #63613

[RalfJung]

Seems fine to me, and actually with @m-ou-se's recent changes the two macros are now more similar than before.

[m-ou-se]

For all cases of the form panic!(format, one or more args ..), they do exactly the same, which covers almost all cases in this PR.

The other cases in this PR are of the form panic!("just a literal"), which used to work differently, but that was fixed just a few hours ago by #78119.

However, two important notes:

1. You used to be able to 'intercept' panic!() from these macros by adding your own custom panic! macro. This is not a guarantee made anywhere in the documentation, so I think it's fine to break that.

2. This PR changes assert_eq! and assert_ne!, but doesn't change assert!. (assert! is not a macro_rules, but it's implemented in compiler/rustc_builtin_macros/src/assert.rs.) assert!(expr, args ..) has the same problem this PR is fixing: it also expands to just panic!(args ..) without $crate::. However, this macro does expose the forms of {std,core}::panic! that are different:

assert!(1 + 1 == 3, 123); // ok with std, error with core
assert!(1 + 1 == 3, &String::from("hello")); // ok with core, error with std

I think it's fine to merge this PR as is, and fix assert!() later if possible. I already have a PR open that fixes this problem for assert!(expr), but not for assert!(expr, args ..): 049f2f2. The second form probably has to wait for std::panic!() and core::panic!() to become completely identical in behaviour, which will probably only happen in Rust 2021 to avoid breakage.

[RalfJung]

assert!(1 + 1 == 3, 123); // ok with std, error with core

I doubt this was ever intended to work, and even for people using panic! with a custom payload, this seems like a stretch. It would be worth a crater run to see how often this actually occurs in practice.

[m-ou-se]

assert!(1 + 1 == 3, 123); // ok with std, error with core

I doubt this was ever intended to work, and even for people using panic! with a custom payload, this seems like a stretch. It would be worth a crater run to see how often this actually occurs in practice.

I'm all for just breaking assert!(.., 123) (and panic!(123)). But most discussions about this came to the conclusion that this should probably be gated on the next edition.

I'll turn my notes about this into an RFC today (and also add something about assert!()), so we can make a final decision about how to go forward with panic!() (and assert!() (and debug_assert!())).

[RalfJung]

I'm all for just breaking assert!(.., 123) (and panic!(123)). But most discussions about this came to the conclusion that this should probably be gated on the next edition.

For panic! I agree the change is too invasive, but for assert! I am less sure.

I'll turn my notes about this into an RFC today (and also add something about assert!()), so we can make a final decision about how to go forward with panic!() (and assert!() (and debug_assert!())).

Okay, great. :) (But some of this will likely require further data, in particular for assert! I think it would be really good to know how wide-spread such non-string uses are.)

[m-ou-se]

I'll turn my notes about this into an RFC today

Here it is

The proposed steps include this PR and the problem it solves. See the second to last step under 'Proposed solution'.

[Mark-Simulacrum]

I'm going to r? @m-ou-se here, I'm not sure what the right path is -- would need to think about it -- but you seem to have a better handle on it than I :)

[camelid]

Yeah, this is much more complicated than I thought!

[m-ou-se]

Now that you're also updating assert!, this PR is a good candidate for a crater run to see if anyone relies on being able to intercept panic!()s from those macros, or uses non-string messages for assert! (e.g. assert!(false, 123)).

[camelid]

Yeah, I think I'll have to adjust the code if we're going to go forward with updating assert!, because it no longer allows you to use String as the assert message :/

error[E0308]: mismatched types
  --> /Users/nlroscoemeow/rust/src/test/ui/macros/assert-macro-owned.rs:6:20
   |
LL |     assert!(false, "test-assert-owned".to_string());
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |                    |
   |                    expected `&str`, found struct `String`
   |                    help: consider borrowing here: `&"test-assert-owned".to_string()`

[m-ou-se]

Yup. But we can still use this to do a crater run. Curious to see if anything other than this unit test fails.

@bors try

[bors]

⌛ Trying commit 3fd3d0e53589340759a75a5ea28c858c21860484 with merge d17e02cd82564103b338f152142ccfe1a9b9fbc0...

[rust-log-analyzer]

The job dist-x86_64-linux of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

error[E0308]: mismatched types
    | 
   ::: /cargo/registry/src/cf-workers-proxy-9e9.pages.dev-1ecc6299db9ec823/clap-2.33.3/src/app/parser.rs:183:13
    |
183 |               format!("Non-unique argument name: {} is already in use", a.b.name)

   Compiling bitmaps v2.1.0
   Compiling cc v1.0.60
   Compiling regex v1.3.9
   Compiling regex v1.3.9
error[E0308]: mismatched types
    | 
   ::: /cargo/registry/src/cf-workers-proxy-9e9.pages.dev-1ecc6299db9ec823/clap-2.33.3/src/usage_parser.rs:64:13
    |
64  | /             format!(
65  | |                 "No name found for Arg when parsing usage string: {}",
66  | |                 self.usage
    | |_____________- in this macro invocation

error: aborting due to 2 previous errors

---
== clock drift check ==
  local time: Wed Oct 28 10:29:02 UTC 2020
  network time: Tue, 27 Oct 2020 15:46:01 GMT
== end clock drift check ==
##[error]Process completed with exit code 1.
Terminate orphan process: pid (6055) (python)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

[camelid]

Rebased.

[camelid]

CI should pass now.

[camelid]

Squashed.

[m-ou-se]

@bors r+ 🎉

[bors]

📌 Commit d8b1d51 has been approved by m-ou-se

[bors]

⌛ Testing commit d8b1d51 with merge f32a0cc...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing f32a0cc to master...

[m-ou-se]

🎉 Thanks for making this happen, @camelid!

[camelid]

Thanks for helping me with this, @m-ou-se and @flip1995!