From 95eff664269adc18daca51b0de969b699a4ddb01 Mon Sep 17 00:00:00 2001 From: Camelid Date: Tue, 17 Nov 2020 12:16:15 -0800 Subject: [PATCH] Fix handling of panic calls This should make Clippy more resilient and will unblock #78343. This PR is made against rust-lang/rust to avoid the need for a subtree sync at @flip1995's suggestion in rust-lang/rust-clippy#6310. --- .../src/assertions_on_constants.rs | 5 +- src/tools/clippy/clippy_lints/src/attrs.rs | 4 +- .../clippy_lints/src/fallible_impl_from.rs | 9 ++-- .../clippy_lints/src/implicit_return.rs | 9 +--- .../clippy_lints/src/panic_unimplemented.rs | 44 ++++++++-------- .../clippy/clippy_lints/src/utils/mod.rs | 20 +++++++- .../clippy/clippy_lints/src/utils/paths.rs | 8 ++- src/tools/clippy/tests/ui/panicking_macros.rs | 11 ++++ .../clippy/tests/ui/panicking_macros.stderr | 50 ++++++++++++++----- 9 files changed, 106 insertions(+), 54 deletions(-) diff --git a/src/tools/clippy/clippy_lints/src/assertions_on_constants.rs b/src/tools/clippy/clippy_lints/src/assertions_on_constants.rs index 982d5ecf8d02f..a2ccb0369c4a4 100644 --- a/src/tools/clippy/clippy_lints/src/assertions_on_constants.rs +++ b/src/tools/clippy/clippy_lints/src/assertions_on_constants.rs @@ -1,6 +1,5 @@ use crate::consts::{constant, Constant}; -use crate::utils::paths; -use crate::utils::{is_direct_expn_of, is_expn_of, match_function_call, snippet_opt, span_lint_and_help}; +use crate::utils::{is_direct_expn_of, is_expn_of, match_panic_call, snippet_opt, span_lint_and_help}; use if_chain::if_chain; use rustc_ast::ast::LitKind; use rustc_hir::{Expr, ExprKind, PatKind, UnOp}; @@ -133,7 +132,7 @@ fn match_assert_with_message<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) if let ExprKind::Block(ref inner_block, _) = block_expr.kind; if let Some(begin_panic_call) = &inner_block.expr; // function call - if let Some(args) = match_function_call(cx, begin_panic_call, &paths::BEGIN_PANIC); + if let Some(args) = match_panic_call(cx, begin_panic_call); if args.len() == 1; // bind the second argument of the `assert!` macro if it exists if let panic_message = snippet_opt(cx, args[0].span); diff --git a/src/tools/clippy/clippy_lints/src/attrs.rs b/src/tools/clippy/clippy_lints/src/attrs.rs index 55904a0ec0a84..57702dafa6a0c 100644 --- a/src/tools/clippy/clippy_lints/src/attrs.rs +++ b/src/tools/clippy/clippy_lints/src/attrs.rs @@ -1,7 +1,7 @@ //! checks for attributes use crate::utils::{ - first_line_of_span, is_present_in_source, match_def_path, paths, snippet_opt, span_lint, span_lint_and_help, + first_line_of_span, is_present_in_source, match_panic_def_id, snippet_opt, span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, without_block_comments, }; use if_chain::if_chain; @@ -513,7 +513,7 @@ fn is_relevant_expr(cx: &LateContext<'_>, typeck_results: &ty::TypeckResults<'_> typeck_results .qpath_res(qpath, path_expr.hir_id) .opt_def_id() - .map_or(true, |fun_id| !match_def_path(cx, fun_id, &paths::BEGIN_PANIC)) + .map_or(true, |fun_id| !match_panic_def_id(cx, fun_id)) } else { true } diff --git a/src/tools/clippy/clippy_lints/src/fallible_impl_from.rs b/src/tools/clippy/clippy_lints/src/fallible_impl_from.rs index fe817fe94f2e4..509a4a4e15f62 100644 --- a/src/tools/clippy/clippy_lints/src/fallible_impl_from.rs +++ b/src/tools/clippy/clippy_lints/src/fallible_impl_from.rs @@ -1,5 +1,7 @@ -use crate::utils::paths::{BEGIN_PANIC, BEGIN_PANIC_FMT, FROM_TRAIT}; -use crate::utils::{is_expn_of, is_type_diagnostic_item, match_def_path, method_chain_args, span_lint_and_then}; +use crate::utils::paths::FROM_TRAIT; +use crate::utils::{ + is_expn_of, is_type_diagnostic_item, match_def_path, match_panic_def_id, method_chain_args, span_lint_and_then, +}; use if_chain::if_chain; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; @@ -84,8 +86,7 @@ fn lint_impl_body<'tcx>(cx: &LateContext<'tcx>, impl_span: Span, impl_items: &[h if let ExprKind::Call(ref func_expr, _) = expr.kind; if let ExprKind::Path(QPath::Resolved(_, ref path)) = func_expr.kind; if let Some(path_def_id) = path.res.opt_def_id(); - if match_def_path(self.lcx, path_def_id, &BEGIN_PANIC) || - match_def_path(self.lcx, path_def_id, &BEGIN_PANIC_FMT); + if match_panic_def_id(self.lcx, path_def_id); if is_expn_of(expr.span, "unreachable").is_none(); then { self.result.push(expr.span); diff --git a/src/tools/clippy/clippy_lints/src/implicit_return.rs b/src/tools/clippy/clippy_lints/src/implicit_return.rs index 22c4fef32a32b..ed7f3b9293dbf 100644 --- a/src/tools/clippy/clippy_lints/src/implicit_return.rs +++ b/src/tools/clippy/clippy_lints/src/implicit_return.rs @@ -1,8 +1,4 @@ -use crate::utils::{ - fn_has_unsatisfiable_preds, match_def_path, - paths::{BEGIN_PANIC, BEGIN_PANIC_FMT}, - snippet_opt, span_lint_and_then, -}; +use crate::utils::{fn_has_unsatisfiable_preds, match_panic_def_id, snippet_opt, span_lint_and_then}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::intravisit::FnKind; @@ -109,8 +105,7 @@ fn expr_match(cx: &LateContext<'_>, expr: &Expr<'_>) { if_chain! { if let ExprKind::Path(qpath) = &expr.kind; if let Some(path_def_id) = cx.qpath_res(qpath, expr.hir_id).opt_def_id(); - if match_def_path(cx, path_def_id, &BEGIN_PANIC) || - match_def_path(cx, path_def_id, &BEGIN_PANIC_FMT); + if match_panic_def_id(cx, path_def_id); then { } else { lint(cx, expr.span, expr.span, LINT_RETURN) diff --git a/src/tools/clippy/clippy_lints/src/panic_unimplemented.rs b/src/tools/clippy/clippy_lints/src/panic_unimplemented.rs index 6379dffd22e37..3d888fe732573 100644 --- a/src/tools/clippy/clippy_lints/src/panic_unimplemented.rs +++ b/src/tools/clippy/clippy_lints/src/panic_unimplemented.rs @@ -1,4 +1,4 @@ -use crate::utils::{is_direct_expn_of, is_expn_of, match_function_call, paths, span_lint}; +use crate::utils::{is_direct_expn_of, is_expn_of, match_panic_call, span_lint}; use if_chain::if_chain; use rustc_ast::ast::LitKind; use rustc_hir::{Expr, ExprKind}; @@ -93,27 +93,27 @@ declare_lint_pass!(PanicUnimplemented => [PANIC_PARAMS, UNIMPLEMENTED, UNREACHAB impl<'tcx> LateLintPass<'tcx> for PanicUnimplemented { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if_chain! { - if let ExprKind::Block(ref block, _) = expr.kind; - if let Some(ref ex) = block.expr; - if let Some(params) = match_function_call(cx, ex, &paths::BEGIN_PANIC) - .or_else(|| match_function_call(cx, ex, &paths::BEGIN_PANIC_FMT)); - then { - let span = get_outer_span(expr); - if is_expn_of(expr.span, "unimplemented").is_some() { - span_lint(cx, UNIMPLEMENTED, span, - "`unimplemented` should not be present in production code"); - } else if is_expn_of(expr.span, "todo").is_some() { - span_lint(cx, TODO, span, - "`todo` should not be present in production code"); - } else if is_expn_of(expr.span, "unreachable").is_some() { - span_lint(cx, UNREACHABLE, span, - "`unreachable` should not be present in production code"); - } else if is_expn_of(expr.span, "panic").is_some() { - span_lint(cx, PANIC, span, - "`panic` should not be present in production code"); - match_panic(params, expr, cx); - } + if let Some(params) = match_panic_call(cx, expr) { + let span = get_outer_span(expr); + if is_expn_of(expr.span, "unimplemented").is_some() { + span_lint( + cx, + UNIMPLEMENTED, + span, + "`unimplemented` should not be present in production code", + ); + } else if is_expn_of(expr.span, "todo").is_some() { + span_lint(cx, TODO, span, "`todo` should not be present in production code"); + } else if is_expn_of(expr.span, "unreachable").is_some() { + span_lint( + cx, + UNREACHABLE, + span, + "`unreachable` should not be present in production code", + ); + } else if is_expn_of(expr.span, "panic").is_some() { + span_lint(cx, PANIC, span, "`panic` should not be present in production code"); + match_panic(params, expr, cx); } } } diff --git a/src/tools/clippy/clippy_lints/src/utils/mod.rs b/src/tools/clippy/clippy_lints/src/utils/mod.rs index 0d43fd0392eb2..a68262d56c64d 100644 --- a/src/tools/clippy/clippy_lints/src/utils/mod.rs +++ b/src/tools/clippy/clippy_lints/src/utils/mod.rs @@ -1196,7 +1196,7 @@ pub fn has_iter_method(cx: &LateContext<'_>, probably_ref_ty: Ty<'_>) -> Option< /// Usage: /// /// ```rust,ignore -/// if let Some(args) = match_function_call(cx, begin_panic_call, &paths::BEGIN_PANIC); +/// if let Some(args) = match_function_call(cx, cmp_max_call, &paths::CMP_MAX); /// ``` pub fn match_function_call<'tcx>( cx: &LateContext<'tcx>, @@ -1231,6 +1231,24 @@ pub fn match_def_path<'tcx>(cx: &LateContext<'tcx>, did: DefId, syms: &[&str]) - cx.match_def_path(did, &syms) } +pub fn match_panic_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx [Expr<'tcx>]> { + match_function_call(cx, expr, &paths::BEGIN_PANIC) + .or_else(|| match_function_call(cx, expr, &paths::BEGIN_PANIC_FMT)) + .or_else(|| match_function_call(cx, expr, &paths::PANIC_ANY)) + .or_else(|| match_function_call(cx, expr, &paths::PANICKING_PANIC)) + .or_else(|| match_function_call(cx, expr, &paths::PANICKING_PANIC_FMT)) + .or_else(|| match_function_call(cx, expr, &paths::PANICKING_PANIC_STR)) +} + +pub fn match_panic_def_id(cx: &LateContext<'_>, did: DefId) -> bool { + match_def_path(cx, did, &paths::BEGIN_PANIC) + || match_def_path(cx, did, &paths::BEGIN_PANIC_FMT) + || match_def_path(cx, did, &paths::PANIC_ANY) + || match_def_path(cx, did, &paths::PANICKING_PANIC) + || match_def_path(cx, did, &paths::PANICKING_PANIC_FMT) + || match_def_path(cx, did, &paths::PANICKING_PANIC_STR) +} + /// Returns the list of condition expressions and the list of blocks in a /// sequence of `if/else`. /// E.g., this returns `([a, b], [c, d, e])` for the expression diff --git a/src/tools/clippy/clippy_lints/src/utils/paths.rs b/src/tools/clippy/clippy_lints/src/utils/paths.rs index 1ad8c6029860b..8f5fbfd9f846a 100644 --- a/src/tools/clippy/clippy_lints/src/utils/paths.rs +++ b/src/tools/clippy/clippy_lints/src/utils/paths.rs @@ -8,8 +8,8 @@ pub const ANY_TRAIT: [&str; 3] = ["std", "any", "Any"]; pub const ARC_PTR_EQ: [&str; 4] = ["alloc", "sync", "Arc", "ptr_eq"]; pub const ASMUT_TRAIT: [&str; 3] = ["core", "convert", "AsMut"]; pub const ASREF_TRAIT: [&str; 3] = ["core", "convert", "AsRef"]; -pub const BEGIN_PANIC: [&str; 3] = ["std", "panicking", "begin_panic"]; -pub const BEGIN_PANIC_FMT: [&str; 3] = ["std", "panicking", "begin_panic_fmt"]; +pub(super) const BEGIN_PANIC: [&str; 3] = ["std", "panicking", "begin_panic"]; +pub(super) const BEGIN_PANIC_FMT: [&str; 3] = ["std", "panicking", "begin_panic_fmt"]; pub const BINARY_HEAP: [&str; 4] = ["alloc", "collections", "binary_heap", "BinaryHeap"]; pub const BORROW_TRAIT: [&str; 3] = ["core", "borrow", "Borrow"]; pub const BOX: [&str; 3] = ["alloc", "boxed", "Box"]; @@ -78,6 +78,10 @@ pub const ORD: [&str; 3] = ["core", "cmp", "Ord"]; pub const OS_STRING: [&str; 4] = ["std", "ffi", "os_str", "OsString"]; pub const OS_STRING_AS_OS_STR: [&str; 5] = ["std", "ffi", "os_str", "OsString", "as_os_str"]; pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"]; +pub(super) const PANICKING_PANIC: [&str; 3] = ["core", "panicking", "panic"]; +pub(super) const PANICKING_PANIC_FMT: [&str; 3] = ["core", "panicking", "panic_fmt"]; +pub(super) const PANICKING_PANIC_STR: [&str; 3] = ["core", "panicking", "panic_str"]; +pub(super) const PANIC_ANY: [&str; 3] = ["std", "panic", "panic_any"]; pub const PARKING_LOT_MUTEX_GUARD: [&str; 2] = ["parking_lot", "MutexGuard"]; pub const PARKING_LOT_RWLOCK_READ_GUARD: [&str; 2] = ["parking_lot", "RwLockReadGuard"]; pub const PARKING_LOT_RWLOCK_WRITE_GUARD: [&str; 2] = ["parking_lot", "RwLockWriteGuard"]; diff --git a/src/tools/clippy/tests/ui/panicking_macros.rs b/src/tools/clippy/tests/ui/panicking_macros.rs index f91ccfaed743d..77fcb8dfd02fd 100644 --- a/src/tools/clippy/tests/ui/panicking_macros.rs +++ b/src/tools/clippy/tests/ui/panicking_macros.rs @@ -1,6 +1,8 @@ #![warn(clippy::unimplemented, clippy::unreachable, clippy::todo, clippy::panic)] #![allow(clippy::assertions_on_constants)] +extern crate core; + fn panic() { let a = 2; panic!(); @@ -33,9 +35,18 @@ fn unreachable() { let b = a + 2; } +fn core_versions() { + use core::{panic, todo, unimplemented, unreachable}; + panic!(); + todo!(); + unimplemented!(); + unreachable!(); +} + fn main() { panic(); todo(); unimplemented(); unreachable(); + core_versions(); } diff --git a/src/tools/clippy/tests/ui/panicking_macros.stderr b/src/tools/clippy/tests/ui/panicking_macros.stderr index 37c11d72a574a..83234c0ed92cc 100644 --- a/src/tools/clippy/tests/ui/panicking_macros.stderr +++ b/src/tools/clippy/tests/ui/panicking_macros.stderr @@ -1,5 +1,5 @@ error: `panic` should not be present in production code - --> $DIR/panicking_macros.rs:6:5 + --> $DIR/panicking_macros.rs:8:5 | LL | panic!(); | ^^^^^^^^^ @@ -7,7 +7,7 @@ LL | panic!(); = note: `-D clippy::panic` implied by `-D warnings` error: `panic` should not be present in production code - --> $DIR/panicking_macros.rs:7:5 + --> $DIR/panicking_macros.rs:9:5 | LL | panic!("message"); | ^^^^^^^^^^^^^^^^^^ @@ -15,7 +15,7 @@ LL | panic!("message"); = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: `panic` should not be present in production code - --> $DIR/panicking_macros.rs:8:5 + --> $DIR/panicking_macros.rs:10:5 | LL | panic!("{} {}", "panic with", "multiple arguments"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -23,7 +23,7 @@ LL | panic!("{} {}", "panic with", "multiple arguments"); = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: `todo` should not be present in production code - --> $DIR/panicking_macros.rs:14:5 + --> $DIR/panicking_macros.rs:16:5 | LL | todo!(); | ^^^^^^^^ @@ -31,19 +31,19 @@ LL | todo!(); = note: `-D clippy::todo` implied by `-D warnings` error: `todo` should not be present in production code - --> $DIR/panicking_macros.rs:15:5 + --> $DIR/panicking_macros.rs:17:5 | LL | todo!("message"); | ^^^^^^^^^^^^^^^^^ error: `todo` should not be present in production code - --> $DIR/panicking_macros.rs:16:5 + --> $DIR/panicking_macros.rs:18:5 | LL | todo!("{} {}", "panic with", "multiple arguments"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: `unimplemented` should not be present in production code - --> $DIR/panicking_macros.rs:22:5 + --> $DIR/panicking_macros.rs:24:5 | LL | unimplemented!(); | ^^^^^^^^^^^^^^^^^ @@ -51,19 +51,19 @@ LL | unimplemented!(); = note: `-D clippy::unimplemented` implied by `-D warnings` error: `unimplemented` should not be present in production code - --> $DIR/panicking_macros.rs:23:5 + --> $DIR/panicking_macros.rs:25:5 | LL | unimplemented!("message"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: `unimplemented` should not be present in production code - --> $DIR/panicking_macros.rs:24:5 + --> $DIR/panicking_macros.rs:26:5 | LL | unimplemented!("{} {}", "panic with", "multiple arguments"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: `unreachable` should not be present in production code - --> $DIR/panicking_macros.rs:30:5 + --> $DIR/panicking_macros.rs:32:5 | LL | unreachable!(); | ^^^^^^^^^^^^^^^ @@ -71,7 +71,7 @@ LL | unreachable!(); = note: `-D clippy::unreachable` implied by `-D warnings` error: `unreachable` should not be present in production code - --> $DIR/panicking_macros.rs:31:5 + --> $DIR/panicking_macros.rs:33:5 | LL | unreachable!("message"); | ^^^^^^^^^^^^^^^^^^^^^^^^ @@ -79,10 +79,34 @@ LL | unreachable!("message"); = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: `unreachable` should not be present in production code - --> $DIR/panicking_macros.rs:32:5 + --> $DIR/panicking_macros.rs:34:5 | LL | unreachable!("{} {}", "panic with", "multiple arguments"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 12 previous errors +error: `panic` should not be present in production code + --> $DIR/panicking_macros.rs:40:5 + | +LL | panic!(); + | ^^^^^^^^^ + +error: `todo` should not be present in production code + --> $DIR/panicking_macros.rs:41:5 + | +LL | todo!(); + | ^^^^^^^^ + +error: `unimplemented` should not be present in production code + --> $DIR/panicking_macros.rs:42:5 + | +LL | unimplemented!(); + | ^^^^^^^^^^^^^^^^^ + +error: `unreachable` should not be present in production code + --> $DIR/panicking_macros.rs:43:5 + | +LL | unreachable!(); + | ^^^^^^^^^^^^^^^ + +error: aborting due to 16 previous errors