Fix handling of panic calls

[camelid]

Fix handling of panic calls. This should make Clippy more resilient and will unblock rust-lang/rust#78343.

changelog: none

[rust-highfive]

r? @Manishearth

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

[camelid]

r? @flip1995

[camelid]

I'm getting this error:

error: invalid path
  --> clippy_lints/src/utils/paths.rs:84:1
   |
84 | pub const PANIC_ANY: [&str; 3] = ["std", "panicking", "panic_any"];
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D clippy::clippy-lints-internal` implied by `-D clippy::internal`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clippy_lints_internal

Is that because panic_any hasn't landed yet?

[camelid]

Hmm, panic_any is definitely on nightly.

[camelid]

Cc @m-ou-se

[camelid]

Oops, clippy is correct: the path is std::panic::panic_any, not std::panicking::panic_any ;)

[m-ou-se]

It might be good to add a test to check that clippy now reacts to core::panic too.

[camelid]

It's a good thing you had me add that test, because the output looks wonky :)

[flip1995]

Ultimately this PR should be opened in rust-lang/rust, to avoid the sync round trip. But we can review and test it here and just apply the patches in rust-lang/rust.

[m-ou-se]

Is there anything we can do in std/core to make it easier for Clippy to recognise panic/unimplemented/assert/etc.? Its detection right now seems very fragile, as it depends on the exact expansion of these macros. Those macros are going to change soon, and some will expand differently in Rust 2021. Also, assert!() might get a less predictable expansion once RFC 2011 gets implemented.

Maybe some #[rustc_diagnostic_item = ..] Clippy could use, or something in that direction?

[flip1995]

The problem is, that Clippy only sees the expanded version of macros and has no access to the pre-expansion code. So I don't think a diag item will help here. We can check if a certain span is from a specific macro expansion with is_expn_of (well normally it works, but for some reason not in the above case).

Macros are tricky, and for example, if we need macro arguments it leads to hacks like this:

rust-clippy/clippy_lints/src/utils/higher.rs

Lines 246 to 253 in d212c38

	/// Extract args from an assert-like macro.
	/// Currently working with:
	/// - `assert!`, `assert_eq!` and `assert_ne!`
	/// - `debug_assert!`, `debug_assert_eq!` and `debug_assert_ne!`
	/// For example:
	/// `assert!(expr)` will return Some([expr])
	/// `debug_assert_eq!(a, b)` will return Some([a, b])
	pub fn extract_assert_macro_args<'tcx>(e: &'tcx Expr<'tcx>) -> Option<Vec<&'tcx Expr<'tcx>>> {

[camelid]

Ugh, now only core::unreachable is warning. Ideas?

[m-ou-se]

All the other tests are wrapped in let a = 2; and let b = a + 2;. I guess that might be related?

[camelid]

I don't know; I tried adding that, and it didn't change the output at all.

[m-ou-se]

Ugh, now only core::unreachable is warning. Ideas?

std::panic expands to { $crate::begin_panic_fmt(..) } (note the {}), but core::panic expands to $crate::panicking::panic(..) (without {}). The only core macro that works is core::unreachable!(), which is the only macro that does include {} in its expansion for some reason.

Clippy seems to check specifically for that {} block.

[m-ou-se]

This makes it work:

-            if let ExprKind::Block(ref block, _) = expr.kind;
-            if let Some(ref ex) = block.expr;
-            if let Some(params) = match_panic_call(cx, ex);
+            if let Some(params) = match_panic_call(cx, expr);

(clippy_lints/src/panic_unimplemented.rs:100)

[camelid]

@m-ou-se It worked! Wonder why the code was expecting a block... Thanks so much for helping me figure this stuff out :)

[flip1995]

Wonder why the code was expecting a block...

Probably some legacy code, no one knows the reason anymore. 😄

---

This LGTM. It would be great, if you could do this in rustc directly, so we don't have to sync this back, before your rust PR can get merged.

# in Clippy repo
git diff master --patch > clippy.patch
sed -ie "s/--- a/--- a\/src\/tools\/clippy/g" clippy.patch
sed -ie "s/+++ b/+++ b\/src\/tools\/clippy/g" clippy.patch

# in Rust repo
git apply /path/to/clippy.patch

[camelid]

Closing in favor of rust-lang/rust#79145 to avoid the sync.