mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

panic_in_result_fn: Extend to also check usages of [debug_]assert* macros

Browse Source 
Also, the macro-finding logic has been moved to the util module, for
use by future lints.
This commit is contained in:
Dobe Peter 2020-10-31 20:31:34 +01:00 committed by Eduardo Broto
parent aaed9d9926
commit e58c7dd168
7 changed files with 285 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

68
clippy_lints/src/panic_in_result_fn.rs
View File

@ -1,18 +1,16 @@
use crate::utils::{is_expn_of, is_type_diagnostic_item, return_ty, span_lint_and_then};
use crate::utils::{find_macro_calls, is_type_diagnostic_item, return_ty, span_lint_and_then};
use rustc_hir as hir;
use rustc_hir::intravisit::{self, FnKind, NestedVisitorMap, Visitor};
use rustc_hir::Expr;
use rustc_hir::intravisit::FnKind;
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::hir::map::Map;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::{sym, Span};
declare_clippy_lint! {
/// **What it does:** Checks for usage of `panic!`, `unimplemented!`, `todo!` or `unreachable!` in a function of type result.
/// **What it does:** Checks for usage of `panic!`, `unimplemented!`, `todo!`, `unreachable!` or assertions in a function of type result.
///
/// **Why is this bad?** For some codebases, it is desirable for functions of type result to return an error instead of crashing. Hence unimplemented, panic and unreachable should be avoided.
/// **Why is this bad?** For some codebases, it is desirable for functions of type result to return an error instead of crashing. Hence panicking macros should be avoided.
///
/// **Known problems:** None.
/// **Known problems:** Functions called from a function returning a `Result` may invoke a panicking macro. This is not checked.
///
/// **Example:**
///
@ -22,9 +20,15 @@
/// panic!("error");
/// }
/// ```
/// Use instead:
/// ```rust
/// fn result_without_panic() -> Result<bool, String> {
/// Err(String::from("error"))
/// }
/// ```
pub PANIC_IN_RESULT_FN,
restriction,
"functions of type `Result<..>` that contain `panic!()`, `todo!()` or `unreachable()` or `unimplemented()` "
"functions of type `Result<..>` that contain `panic!()`, `todo!()`, `unreachable()`, `unimplemented()` or assertion"
}
declare_lint_pass!(PanicInResultFn => [PANIC_IN_RESULT_FN]);
@ -47,43 +51,33 @@ fn check_fn(
}
}
struct FindPanicUnimplementedUnreachable {
result: Vec<Span>,
}
impl<'tcx> Visitor<'tcx> for FindPanicUnimplementedUnreachable {
type Map = Map<'tcx>;
fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
if ["unimplemented", "unreachable", "panic", "todo"]
.iter()
.any(|fun| is_expn_of(expr.span, fun).is_some())
{
self.result.push(expr.span);
}
// and check sub-expressions
intravisit::walk_expr(self, expr);
}
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
NestedVisitorMap::None
}
}
fn lint_impl_body<'tcx>(cx: &LateContext<'tcx>, impl_span: Span, body: &'tcx hir::Body<'tcx>) {
let mut panics = FindPanicUnimplementedUnreachable { result: Vec::new() };
panics.visit_expr(&body.value);
if !panics.result.is_empty() {
let panics = find_macro_calls(
vec![
"unimplemented",
"unreachable",
"panic",
"todo",
"assert",
"assert_eq",
"assert_ne",
"debug_assert",
"debug_assert_eq",
"debug_assert_ne",
],
body,
);
if !panics.is_empty() {
span_lint_and_then(
cx,
PANIC_IN_RESULT_FN,
impl_span,
"used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`",
"used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`",
move |diag| {
diag.help(
"`unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing",
"`unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing",
);
diag.span_note(panics.result, "return Err() instead of panicking");
diag.span_note(panics, "return Err() instead of panicking");
},
);
}

33
clippy_lints/src/utils/mod.rs
View File

@ -41,7 +41,7 @@
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::{DefId, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc_hir::intravisit::{NestedVisitorMap, Visitor};
use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
use rustc_hir::Node;
use rustc_hir::{
def, Arm, Block, Body, Constness, Crate, Expr, ExprKind, FnDecl, HirId, ImplItem, ImplItemKind, Item, ItemKind,
@ -603,6 +603,37 @@ fn nested_visit_map(&mut self) -> hir::intravisit::NestedVisitorMap<Self::Map> {
visitor.found
}
struct FindMacroCalls<'a> {
names: Vec<&'a str>,
result: Vec<Span>,
}
impl<'a, 'tcx> Visitor<'tcx> for FindMacroCalls<'a> {
type Map = Map<'tcx>;
fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
if self.names.iter().any(|fun| is_expn_of(expr.span, fun).is_some()) {
self.result.push(expr.span);
}
// and check sub-expressions
intravisit::walk_expr(self, expr);
}
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
NestedVisitorMap::None
}
}
/// Finds calls of the specified macros in a function body.
pub fn find_macro_calls(names: Vec<&str>, body: &'tcx Body<'tcx>) -> Vec<Span> {
let mut fmc = FindMacroCalls {
names,
result: Vec::new(),
};
fmc.visit_expr(&body.value);
fmc.result
}
/// Converts a span to a code snippet if available, otherwise use default.
///
/// This is useful if you want to provide suggestions for your lint or more generally, if you want

24
tests/ui/panic_in_result_fn.stderr
View File

@ -1,4 +1,4 @@
error: used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`
error: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`
--> $DIR/panic_in_result_fn.rs:7:5
|
LL | / fn result_with_panic() -> Result<bool, String> // should emit lint
@ -8,7 +8,7 @@ LL | | }
| |_____^
|
= note: `-D clippy::panic-in-result-fn` implied by `-D warnings`
= help: `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
= help: `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
note: return Err() instead of panicking
--> $DIR/panic_in_result_fn.rs:9:9
|
@ -16,7 +16,7 @@ LL | panic!("error");
| ^^^^^^^^^^^^^^^^
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`
error: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`
--> $DIR/panic_in_result_fn.rs:12:5
|
LL | / fn result_with_unimplemented() -> Result<bool, String> // should emit lint
@ -25,7 +25,7 @@ LL | | unimplemented!();
LL | | }
| |_____^
|
= help: `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
= help: `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
note: return Err() instead of panicking
--> $DIR/panic_in_result_fn.rs:14:9
|
@ -33,7 +33,7 @@ LL | unimplemented!();
| ^^^^^^^^^^^^^^^^^
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`
error: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`
--> $DIR/panic_in_result_fn.rs:17:5
|
LL | / fn result_with_unreachable() -> Result<bool, String> // should emit lint
@ -42,7 +42,7 @@ LL | | unreachable!();
LL | | }
| |_____^
|
= help: `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
= help: `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
note: return Err() instead of panicking
--> $DIR/panic_in_result_fn.rs:19:9
|
@ -50,7 +50,7 @@ LL | unreachable!();
| ^^^^^^^^^^^^^^^
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`
error: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`
--> $DIR/panic_in_result_fn.rs:22:5
|
LL | / fn result_with_todo() -> Result<bool, String> // should emit lint
@ -59,7 +59,7 @@ LL | | todo!("Finish this");
LL | | }
| |_____^
|
= help: `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
= help: `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
note: return Err() instead of panicking
--> $DIR/panic_in_result_fn.rs:24:9
|
@ -67,7 +67,7 @@ LL | todo!("Finish this");
| ^^^^^^^^^^^^^^^^^^^^^
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`
error: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`
--> $DIR/panic_in_result_fn.rs:53:1
|
LL | / fn function_result_with_panic() -> Result<bool, String> // should emit lint
@ -76,7 +76,7 @@ LL | | panic!("error");
LL | | }
| |_^
|
= help: `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
= help: `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
note: return Err() instead of panicking
--> $DIR/panic_in_result_fn.rs:55:5
|
@ -84,7 +84,7 @@ LL | panic!("error");
| ^^^^^^^^^^^^^^^^
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`
error: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`
--> $DIR/panic_in_result_fn.rs:68:1
|
LL | / fn main() -> Result<(), String> {
@ -93,7 +93,7 @@ LL | | Ok(())
LL | | }
| |_^
|
= help: `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
= help: `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
note: return Err() instead of panicking
--> $DIR/panic_in_result_fn.rs:69:5
|

48
tests/ui/panic_in_result_fn_assertions.rs Normal file
View File

@ -0,0 +1,48 @@
#![warn(clippy::panic_in_result_fn)]
#![allow(clippy::unnecessary_wraps)]
struct A;
impl A {
fn result_with_assert_with_message(x: i32) -> Result<bool, String> // should emit lint
{
assert!(x == 5, "wrong argument");
Ok(true)
}
fn result_with_assert_eq(x: i32) -> Result<bool, String> // should emit lint
{
assert_eq!(x, 5);
Ok(true)
}
fn result_with_assert_ne(x: i32) -> Result<bool, String> // should emit lint
{
assert_ne!(x, 1);
Ok(true)
}
fn other_with_assert_with_message(x: i32) // should not emit lint
{
assert!(x == 5, "wrong argument");
}
fn other_with_assert_eq(x: i32) // should not emit lint
{
assert_eq!(x, 5);
}
fn other_with_assert_ne(x: i32) // should not emit lint
{
assert_ne!(x, 1);
}
fn result_without_banned_functions() -> Result<bool, String> // should not emit lint
{
let assert = "assert!";
println!("No {}", assert);
Ok(true)
}
}
fn main() {}

57
tests/ui/panic_in_result_fn_assertions.stderr Normal file
View File

@ -0,0 +1,57 @@
error: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`
--> $DIR/panic_in_result_fn_assertions.rs:7:5
|
LL | / fn result_with_assert_with_message(x: i32) -> Result<bool, String> // should emit lint
LL | | {
LL | | assert!(x == 5, "wrong argument");
LL | | Ok(true)
LL | | }
| |_____^
|
= note: `-D clippy::panic-in-result-fn` implied by `-D warnings`
= help: `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
note: return Err() instead of panicking
--> $DIR/panic_in_result_fn_assertions.rs:9:9
|
LL | assert!(x == 5, "wrong argument");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`
--> $DIR/panic_in_result_fn_assertions.rs:13:5
|
LL | / fn result_with_assert_eq(x: i32) -> Result<bool, String> // should emit lint
LL | | {
LL | | assert_eq!(x, 5);
LL | | Ok(true)
LL | | }
| |_____^
|
= help: `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
note: return Err() instead of panicking
--> $DIR/panic_in_result_fn_assertions.rs:15:9
|
LL | assert_eq!(x, 5);
| ^^^^^^^^^^^^^^^^^
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`
--> $DIR/panic_in_result_fn_assertions.rs:19:5
|
LL | / fn result_with_assert_ne(x: i32) -> Result<bool, String> // should emit lint
LL | | {
LL | | assert_ne!(x, 1);
LL | | Ok(true)
LL | | }
| |_____^
|
= help: `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
note: return Err() instead of panicking
--> $DIR/panic_in_result_fn_assertions.rs:21:9
|
LL | assert_ne!(x, 1);
| ^^^^^^^^^^^^^^^^^
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: aborting due to 3 previous errors

48
tests/ui/panic_in_result_fn_debug_assertions.rs Normal file
View File

@ -0,0 +1,48 @@
#![warn(clippy::panic_in_result_fn)]
#![allow(clippy::unnecessary_wraps)]
struct A;
impl A {
fn result_with_debug_assert_with_message(x: i32) -> Result<bool, String> // should emit lint
{
debug_assert!(x == 5, "wrong argument");
Ok(true)
}
fn result_with_debug_assert_eq(x: i32) -> Result<bool, String> // should emit lint
{
debug_assert_eq!(x, 5);
Ok(true)
}
fn result_with_debug_assert_ne(x: i32) -> Result<bool, String> // should emit lint
{
debug_assert_ne!(x, 1);
Ok(true)
}
fn other_with_debug_assert_with_message(x: i32) // should not emit lint
{
debug_assert!(x == 5, "wrong argument");
}
fn other_with_debug_assert_eq(x: i32) // should not emit lint
{
debug_assert_eq!(x, 5);
}
fn other_with_debug_assert_ne(x: i32) // should not emit lint
{
debug_assert_ne!(x, 1);
}
fn result_without_banned_functions() -> Result<bool, String> // should not emit lint
{
let debug_assert = "debug_assert!";
println!("No {}", debug_assert);
Ok(true)
}
}
fn main() {}

57
tests/ui/panic_in_result_fn_debug_assertions.stderr Normal file
View File

@ -0,0 +1,57 @@
error: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`
--> $DIR/panic_in_result_fn_debug_assertions.rs:7:5
|
LL | / fn result_with_debug_assert_with_message(x: i32) -> Result<bool, String> // should emit lint
LL | | {
LL | | debug_assert!(x == 5, "wrong argument");
LL | | Ok(true)
LL | | }
| |_____^
|
= note: `-D clippy::panic-in-result-fn` implied by `-D warnings`
= help: `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
note: return Err() instead of panicking
--> $DIR/panic_in_result_fn_debug_assertions.rs:9:9
|
LL | debug_assert!(x == 5, "wrong argument");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`
--> $DIR/panic_in_result_fn_debug_assertions.rs:13:5
|
LL | / fn result_with_debug_assert_eq(x: i32) -> Result<bool, String> // should emit lint
LL | | {
LL | | debug_assert_eq!(x, 5);
LL | | Ok(true)
LL | | }
| |_____^
|
= help: `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
note: return Err() instead of panicking
--> $DIR/panic_in_result_fn_debug_assertions.rs:15:9
|
LL | debug_assert_eq!(x, 5);
| ^^^^^^^^^^^^^^^^^^^^^^^
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`
--> $DIR/panic_in_result_fn_debug_assertions.rs:19:5
|
LL | / fn result_with_debug_assert_ne(x: i32) -> Result<bool, String> // should emit lint
LL | | {
LL | | debug_assert_ne!(x, 1);
LL | | Ok(true)
LL | | }
| |_____^
|
= help: `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
note: return Err() instead of panicking
--> $DIR/panic_in_result_fn_debug_assertions.rs:21:9
|
LL | debug_assert_ne!(x, 1);
| ^^^^^^^^^^^^^^^^^^^^^^^
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: aborting due to 3 previous errors