Qualify `panic!` as `core::panic!` in non-built-in `core` macros by camelid · Pull Request #78343 · rust-lang/rust

camelid · 2020-10-25T01:40:16Z

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 · 2020-10-25T01:40:20Z

r? @Mark-Simulacrum

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

camelid · 2020-10-25T01:42:20Z

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

camelid · 2020-10-25T02:03:30Z

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 · 2020-10-25T05:33:17Z

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 · 2020-10-25T08:38:51Z

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 · 2020-10-25T09:08:46Z

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 · 2020-10-25T09:28:56Z

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:

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.
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 · 2020-10-25T10:31:24Z

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 · 2020-10-25T10:40:41Z

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 · 2020-10-25T10:45:00Z

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 · 2020-10-25T15:36:05Z

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 · 2020-10-26T01:28:40Z

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 · 2020-10-26T01:31:08Z

Yeah, this is much more complicated than I thought!

compiler/rustc_builtin_macros/src/assert.rs

m-ou-se · 2020-10-26T08:20:04Z

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 · 2020-10-28T02:41:22Z

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 · 2020-10-28T09:53:16Z

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 · 2020-10-28T09:53:31Z

⌛ Trying commit 3fd3d0e53589340759a75a5ea28c858c21860484 with merge d17e02cd82564103b338f152142ccfe1a9b9fbc0...

rust-log-analyzer · 2020-10-28T10:29:17Z

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/github.com-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/github.com-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)

bors · 2020-11-19T21:39:32Z

⌛ Testing commit 0510fd3 with merge 82ed431104bc6c77c7a15da62fd70d557634db25...

camelid · 2020-11-19T22:23:36Z

Note to whoever writes the relnotes for this: You can no longer 'intercept' panic!; instead you should probably use #[panic_handler].

bors · 2020-11-19T22:26:26Z

💔 Test failed - checks-actions

camelid · 2020-11-19T22:30:11Z

It seems like Clippy tests need to be blessed?

m-ou-se · 2020-11-19T23:50:10Z

It seems like Clippy tests need to be blessed?

How come the output changed? Shouldn't it stay the same as before?

camelid · 2020-11-19T23:53:12Z

I'm not fully sure; it seems like Clippy is picking up on more things now?

m-ou-se · 2020-11-19T23:55:23Z

@flip1995 ^

camelid · 2020-11-19T23:57:24Z

@m-ou-se Also see the discussion on Zulip.

flip1995 · 2020-11-20T07:45:06Z

Don't bless the tests, just allow the lint in the affected test files. Those files aren't meant for testing this lint anyway.

The patch to apply:

diff --git a/tests/ui/logic_bug.rs b/tests/ui/logic_bug.rs
index b4163d776..a01c6ef99 100644
--- a/src/tools/clippy/tests/ui/logic_bug.rs
+++ b/src/tools/clippy/tests/ui/logic_bug.rs
@@ -1,4 +1,4 @@
-#![allow(unused, clippy::many_single_char_names)]
+#![allow(unused, clippy::many_single_char_names, clippy::diverging_sub_expression)]
 #![warn(clippy::logic_bug)]
 
 fn main() {
diff --git a/tests/ui/nonminimal_bool.rs b/tests/ui/nonminimal_bool.rs
index 7ea154cb9..971be2627 100644
--- a/src/tools/clippy/tests/ui/nonminimal_bool.rs
+++ b/src/tools/clippy/tests/ui/nonminimal_bool.rs
@@ -1,4 +1,4 @@
-#![allow(unused, clippy::many_single_char_names)]
+#![allow(unused, clippy::many_single_char_names, clippy::diverging_sub_expression)]
 #![warn(clippy::nonminimal_bool)]
 
 fn main() {
diff --git a/tests/ui/nonminimal_bool_methods.rs b/tests/ui/nonminimal_bool_methods.rs
index 4de48cd08..907587402 100644
--- a/src/tools/clippy/tests/ui/nonminimal_bool_methods.rs
+++ b/src/tools/clippy/tests/ui/nonminimal_bool_methods.rs
@@ -1,4 +1,4 @@
-#![allow(unused, clippy::many_single_char_names)]
+#![allow(unused, clippy::many_single_char_names, clippy::diverging_sub_expression)]
 #![warn(clippy::nonminimal_bool)]
 
 fn methods_with_negation() {
diff --git a/tests/ui/wildcard_enum_match_arm.fixed b/tests/ui/wildcard_enum_match_arm.fixed
index 4f8754a93..b1e5742b7 100644
--- a/src/tools/clippy/tests/ui/wildcard_enum_match_arm.fixed
+++ b/src/tools/clippy/tests/ui/wildcard_enum_match_arm.fixed
@@ -7,7 +7,7 @@
     dead_code,
     clippy::single_match,
     clippy::wildcard_in_or_patterns,
-    clippy::unnested_or_patterns
+    clippy::unnested_or_patterns, clippy::diverging_sub_expression
 )]
 
 use std::io::ErrorKind;
diff --git a/tests/ui/wildcard_enum_match_arm.rs b/tests/ui/wildcard_enum_match_arm.rs
index 5e66644ce..cd3ec3ea8 100644
--- a/src/tools/clippy/tests/ui/wildcard_enum_match_arm.rs
+++ b/src/tools/clippy/tests/ui/wildcard_enum_match_arm.rs
@@ -7,7 +7,7 @@
     dead_code,
     clippy::single_match,
     clippy::wildcard_in_or_patterns,
-    clippy::unnested_or_patterns
+    clippy::unnested_or_patterns, clippy::diverging_sub_expression
 )]
 
 use std::io::ErrorKind;

camelid · 2020-11-22T20:24:50Z

Thanks for providing that patch :)

src/test/ui/hygiene/no_implicit_prelude.stderr

camelid · 2020-11-23T05:08:03Z

Rebased.

camelid · 2020-11-23T05:10:42Z

CI should pass now.

src/tools/clippy/tests/ui/wildcard_enum_match_arm.fixed

src/tools/clippy/tests/ui/wildcard_enum_match_arm.rs

camelid · 2020-11-23T18:41:23Z

Squashed.

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.

* Switch a couple links over to intra-doc links * Clean up some formatting/typography

m-ou-se · 2020-11-23T21:20:56Z

@bors r+ 🎉

bors · 2020-11-23T21:20:58Z

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

bors · 2020-11-23T22:05:36Z

⌛ Testing commit d8b1d51 with merge f32a0cc...

bors · 2020-11-24T00:30:17Z

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

m-ou-se · 2020-11-24T00:32:22Z

🎉 Thanks for making this happen, @camelid!

camelid · 2020-11-24T00:35:22Z

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

camelid added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 25, 2020

rust-highfive assigned Mark-Simulacrum Oct 25, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2020

m-ou-se mentioned this pull request Oct 25, 2020

regression: assert_eq cannot find panic with no_implicit_prelude #61567

Closed

m-ou-se added the A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows label Oct 25, 2020

rust-highfive assigned m-ou-se and unassigned Mark-Simulacrum Oct 26, 2020

camelid changed the title ~~Qualify panic! with $crate:: in core macros~~ Qualify panic! as core::panic! Oct 26, 2020

camelid commented Oct 26, 2020

View reviewed changes

compiler/rustc_builtin_macros/src/assert.rs Outdated Show resolved Hide resolved

camelid marked this pull request as draft October 28, 2020 02:41

m-ou-se reviewed Nov 22, 2020

View reviewed changes

src/test/ui/hygiene/no_implicit_prelude.stderr Outdated Show resolved Hide resolved

flip1995 reviewed Nov 23, 2020

View reviewed changes

src/tools/clippy/tests/ui/wildcard_enum_match_arm.fixed Outdated Show resolved Hide resolved

src/tools/clippy/tests/ui/wildcard_enum_match_arm.rs Outdated Show resolved Hide resolved

camelid added 2 commits November 23, 2020 11:28

Clean up core macros documentation

d8b1d51

* Switch a couple links over to intra-doc links * Clean up some formatting/typography

bors mentioned this pull request Nov 24, 2020

path trimming: ignore type aliases #78052

Merged

m-ou-se mentioned this pull request Dec 18, 2020

Tracking Issue for RFC 3007: Making core and std's panic identical in Rust 2021 #80162

Closed

16 tasks

syvb mentioned this pull request Apr 23, 2021

::core::assert! in #![no_implicit_prelude] fails to compile because it calls panic! instead of ::core::panic! #84357

Closed

birkenfeld mentioned this pull request Aug 29, 2021

more macro hygiene cleanup: test #[pymethods] and more arg parsing and protos PyO3/pyo3#1843

Merged

m-ou-se mentioned this pull request Sep 3, 2021

🎉 Rust 2021 celebration and thanks #88623

Closed

m-ou-se mentioned this pull request Oct 19, 2021

Clippy breaks when we change anything about the std macros rust-lang/rust-clippy#7843

Closed

davidhewitt mentioned this pull request Dec 18, 2021

#[pyclass] for fieldless enums PyO3/pyo3#2034

Merged

Uh oh!

Conversation

camelid commented Oct 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Oct 25, 2020

Uh oh!

camelid commented Oct 25, 2020

Uh oh!

camelid commented Oct 25, 2020

stderr (compiling)

stderr (running)

Uh oh!

jyn514 commented Oct 25, 2020

Uh oh!

petrochenkov commented Oct 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Oct 25, 2020

Uh oh!

m-ou-se commented Oct 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Oct 25, 2020

Uh oh!

m-ou-se commented Oct 25, 2020

Uh oh!

RalfJung commented Oct 25, 2020

Uh oh!

m-ou-se commented Oct 25, 2020

Uh oh!

Mark-Simulacrum commented Oct 26, 2020

Uh oh!

camelid commented Oct 26, 2020

Uh oh!

Uh oh!

m-ou-se commented Oct 26, 2020

Uh oh!

camelid commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-ou-se commented Oct 28, 2020

Uh oh!

bors commented Oct 28, 2020

Uh oh!

rust-log-analyzer commented Oct 28, 2020

Uh oh!

bors commented Nov 19, 2020

Uh oh!

camelid commented Nov 19, 2020

Uh oh!

bors commented Nov 19, 2020

Uh oh!

camelid commented Nov 19, 2020

Uh oh!

m-ou-se commented Nov 19, 2020

Uh oh!

camelid commented Nov 19, 2020

Uh oh!

m-ou-se commented Nov 19, 2020

Uh oh!

camelid commented Nov 19, 2020

Uh oh!

flip1995 commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camelid commented Nov 22, 2020

Uh oh!

Uh oh!

camelid commented Nov 23, 2020

Uh oh!

camelid commented Nov 23, 2020

Uh oh!

Uh oh!

Uh oh!

camelid commented Nov 23, 2020

Uh oh!

m-ou-se commented Nov 23, 2020

Uh oh!

bors commented Nov 23, 2020

camelid commented Oct 25, 2020 •

edited

Loading

petrochenkov commented Oct 25, 2020 •

edited

Loading

m-ou-se commented Oct 25, 2020 •

edited

Loading

camelid commented Oct 28, 2020 •

edited

Loading

flip1995 commented Nov 20, 2020 •

edited

Loading