From 8d911fe98839bab2b81291986624419c18ace64a Mon Sep 17 00:00:00 2001 From: Heinz Gies Date: Fri, 11 Oct 2019 13:58:56 +0200 Subject: [PATCH 1/4] add restirction for unreachable and panic --- clippy_lints/src/panic_unimplemented.rs | 44 +++++++++++++++++++++++-- tests/ui/panic_unimplemented.rs | 9 ++++- tests/ui/panic_unimplemented.stderr | 10 +++++- 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/panic_unimplemented.rs b/clippy_lints/src/panic_unimplemented.rs index 762003179134..bbb037ad8ebc 100644 --- a/clippy_lints/src/panic_unimplemented.rs +++ b/clippy_lints/src/panic_unimplemented.rs @@ -25,6 +25,22 @@ declare_clippy_lint! { "missing parameters in `panic!` calls" } +declare_clippy_lint! { + /// **What it does:** Checks for usage of `panic!`. + /// + /// **Why is this bad?** `panic!` will stop the execution of the executable + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```no_run + /// panic!("even with a good reason"); + /// ``` + pub PANIC, + restriction, + "missing parameters in `panic!` calls" +} + declare_clippy_lint! { /// **What it does:** Checks for usage of `unimplemented!`. /// @@ -41,7 +57,23 @@ declare_clippy_lint! { "`unimplemented!` should not be present in production code" } -declare_lint_pass!(PanicUnimplemented => [PANIC_PARAMS, UNIMPLEMENTED]); +declare_clippy_lint! { + /// **What it does:** Checks for usage of `unreachable!`. + /// + /// **Why is this bad?** This macro can cause cause code to panics + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```no_run + /// unreachable!(); + /// ``` + pub UNREACHABLE, + restriction, + "`unreachable!` should not be present in production code" +} + +declare_lint_pass!(PanicUnimplemented => [PANIC_PARAMS, UNIMPLEMENTED, UNREACHABLE]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for PanicUnimplemented { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { @@ -55,7 +87,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for PanicUnimplemented { let span = get_outer_span(expr); span_lint(cx, UNIMPLEMENTED, span, "`unimplemented` should not be present in production code"); - } else { + } else if is_expn_of(expr.span, "unreachable").is_some() { + let span = get_outer_span(expr); + span_lint(cx, UNREACHABLE, span, + "`unreachable` should not be present in production code"); + } else if is_expn_of(expr.span, "panic").is_some() { + let span = get_outer_span(expr); + span_lint(cx, PANIC, span, + "`panic` should not be present in production code"); + //} else { match_panic(params, expr, cx); } } diff --git a/tests/ui/panic_unimplemented.rs b/tests/ui/panic_unimplemented.rs index 92290da8a6ac..fed82f13515e 100644 --- a/tests/ui/panic_unimplemented.rs +++ b/tests/ui/panic_unimplemented.rs @@ -1,4 +1,4 @@ -#![warn(clippy::panic_params, clippy::unimplemented)] +#![warn(clippy::panic_params, clippy::unimplemented, clippy::unreachable)] #![allow(clippy::assertions_on_constants)] fn missing() { if true { @@ -56,6 +56,12 @@ fn unimplemented() { let b = a + 2; } +fn unreachable() { + let a = 2; + unreachable!(); + let b = a + 2; +} + fn main() { missing(); ok_single(); @@ -65,4 +71,5 @@ fn main() { ok_nomsg(); ok_escaped(); unimplemented(); + unreachable(); } diff --git a/tests/ui/panic_unimplemented.stderr b/tests/ui/panic_unimplemented.stderr index 588fa187b4ab..5f19b35fe6cf 100644 --- a/tests/ui/panic_unimplemented.stderr +++ b/tests/ui/panic_unimplemented.stderr @@ -32,5 +32,13 @@ LL | unimplemented!(); | = note: `-D clippy::unimplemented` implied by `-D warnings` -error: aborting due to 5 previous errors +error: `unreachable` should not be present in production code + --> $DIR/panic_unimplemented.rs:61:5 + | +LL | unreachable!(); + | ^^^^^^^^^^^^^^^ + | + = note: `-D clippy::unreachable` implied by `-D warnings` + +error: aborting due to 6 previous errors From 98dc3aabeaf4102669d46912fb9def5f125a05ca Mon Sep 17 00:00:00 2001 From: "Heinz N. Gies" Date: Sat, 12 Oct 2019 13:26:14 +0200 Subject: [PATCH 2/4] Add todo and tests --- CHANGELOG.md | 3 +++ README.md | 2 +- clippy_lints/src/lib.rs | 3 +++ clippy_lints/src/panic_unimplemented.rs | 23 +++++++++++++++++++++-- src/lintlist/mod.rs | 23 ++++++++++++++++++++++- tests/ui/panic.rs | 12 ++++++++++++ tests/ui/panic.stderr | 10 ++++++++++ tests/ui/panic_unimplemented.rs | 9 ++++++++- tests/ui/panic_unimplemented.stderr | 10 +++++++++- 9 files changed, 89 insertions(+), 6 deletions(-) create mode 100644 tests/ui/panic.rs create mode 100644 tests/ui/panic.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 42ab00001d97..5ce2eb4d5de2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1138,6 +1138,7 @@ Released 2018-09-13 [`or_fun_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call [`out_of_bounds_indexing`]: https://rust-lang.github.io/rust-clippy/master/index.html#out_of_bounds_indexing [`overflow_check_conditional`]: https://rust-lang.github.io/rust-clippy/master/index.html#overflow_check_conditional +[`panic`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic [`panic_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic_params [`panicking_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#panicking_unwrap [`partialeq_ne_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_ne_impl @@ -1198,6 +1199,7 @@ Released 2018-09-13 [`suspicious_unary_op_formatting`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_unary_op_formatting [`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment [`temporary_cstring_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_cstring_as_ptr +[`todo`]: https://rust-lang.github.io/rust-clippy/master/index.html#todo [`too_many_arguments`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments [`too_many_lines`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines [`toplevel_ref_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#toplevel_ref_arg @@ -1227,6 +1229,7 @@ Released 2018-09-13 [`unnecessary_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_unwrap [`unneeded_field_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_field_pattern [`unneeded_wildcard_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_wildcard_pattern +[`unreachable`]: https://rust-lang.github.io/rust-clippy/master/index.html#unreachable [`unreadable_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal [`unsafe_removed_from_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_removed_from_name [`unsafe_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_vector_initialization diff --git a/README.md b/README.md index 5023538c5ed9..7913a3eefdab 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 326 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 329 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index ccc5b74de302..9fca5a4b97a4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -631,7 +631,10 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con misc::FLOAT_CMP_CONST, missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS, missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS, + panic_unimplemented::PANIC, + panic_unimplemented::TODO, panic_unimplemented::UNIMPLEMENTED, + panic_unimplemented::UNREACHABLE, shadow::SHADOW_REUSE, shadow::SHADOW_SAME, strings::STRING_ADD, diff --git a/clippy_lints/src/panic_unimplemented.rs b/clippy_lints/src/panic_unimplemented.rs index bbb037ad8ebc..6981ecff0d01 100644 --- a/clippy_lints/src/panic_unimplemented.rs +++ b/clippy_lints/src/panic_unimplemented.rs @@ -57,6 +57,22 @@ declare_clippy_lint! { "`unimplemented!` should not be present in production code" } +declare_clippy_lint! { + /// **What it does:** Checks for usage of `todo!`. + /// + /// **Why is this bad?** This macro should not be present in production code + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```no_run + /// todo!(); + /// ``` + pub TODO, + restriction, + "`todo!` should not be present in production code" +} + declare_clippy_lint! { /// **What it does:** Checks for usage of `unreachable!`. /// @@ -73,7 +89,7 @@ declare_clippy_lint! { "`unreachable!` should not be present in production code" } -declare_lint_pass!(PanicUnimplemented => [PANIC_PARAMS, UNIMPLEMENTED, UNREACHABLE]); +declare_lint_pass!(PanicUnimplemented => [PANIC_PARAMS, UNIMPLEMENTED, UNREACHABLE, TODO, PANIC]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for PanicUnimplemented { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { @@ -87,6 +103,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for PanicUnimplemented { let span = get_outer_span(expr); span_lint(cx, UNIMPLEMENTED, span, "`unimplemented` should not be present in production code"); + } else if is_expn_of(expr.span, "todo").is_some() { + let span = get_outer_span(expr); + span_lint(cx, TODO, span, + "`todo` should not be present in production code"); } else if is_expn_of(expr.span, "unreachable").is_some() { let span = get_outer_span(expr); span_lint(cx, UNREACHABLE, span, @@ -95,7 +115,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for PanicUnimplemented { let span = get_outer_span(expr); span_lint(cx, PANIC, span, "`panic` should not be present in production code"); - //} else { match_panic(params, expr, cx); } } diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 1575f0e30278..24e01f327802 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 326] = [ +pub const ALL_LINTS: [Lint; 329] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -1456,6 +1456,13 @@ pub const ALL_LINTS: [Lint; 326] = [ deprecation: None, module: "overflow_check_conditional", }, + Lint { + name: "panic", + group: "restriction", + desc: "missing parameters in `panic!` calls", + deprecation: None, + module: "panic_unimplemented", + }, Lint { name: "panic_params", group: "style", @@ -1848,6 +1855,13 @@ pub const ALL_LINTS: [Lint; 326] = [ deprecation: None, module: "methods", }, + Lint { + name: "todo", + group: "restriction", + desc: "`todo!` should not be present in production code", + deprecation: None, + module: "panic_unimplemented", + }, Lint { name: "too_many_arguments", group: "complexity", @@ -2051,6 +2065,13 @@ pub const ALL_LINTS: [Lint; 326] = [ deprecation: None, module: "misc_early", }, + Lint { + name: "unreachable", + group: "restriction", + desc: "`unreachable!` should not be present in production code", + deprecation: None, + module: "panic_unimplemented", + }, Lint { name: "unreadable_literal", group: "style", diff --git a/tests/ui/panic.rs b/tests/ui/panic.rs new file mode 100644 index 000000000000..dee3104774cb --- /dev/null +++ b/tests/ui/panic.rs @@ -0,0 +1,12 @@ +#![warn(clippy::panic)] +#![allow(clippy::assertions_on_constants)] + +fn panic() { + let a = 2; + panic!(); + let b = a + 2; +} + +fn main() { + panic(); +} diff --git a/tests/ui/panic.stderr b/tests/ui/panic.stderr new file mode 100644 index 000000000000..cfef1a16e49e --- /dev/null +++ b/tests/ui/panic.stderr @@ -0,0 +1,10 @@ +error: `panic` should not be present in production code + --> $DIR/panic.rs:6:5 + | +LL | panic!(); + | ^^^^^^^^^ + | + = note: `-D clippy::panic` implied by `-D warnings` + +error: aborting due to previous error + diff --git a/tests/ui/panic_unimplemented.rs b/tests/ui/panic_unimplemented.rs index fed82f13515e..f3dae3bbde65 100644 --- a/tests/ui/panic_unimplemented.rs +++ b/tests/ui/panic_unimplemented.rs @@ -1,4 +1,4 @@ -#![warn(clippy::panic_params, clippy::unimplemented, clippy::unreachable)] +#![warn(clippy::panic_params, clippy::unimplemented, clippy::unreachable, clippy::todo)] #![allow(clippy::assertions_on_constants)] fn missing() { if true { @@ -62,6 +62,12 @@ fn unreachable() { let b = a + 2; } +fn todo() { + let a = 2; + todo!(); + let b = a + 2; +} + fn main() { missing(); ok_single(); @@ -72,4 +78,5 @@ fn main() { ok_escaped(); unimplemented(); unreachable(); + todo(); } diff --git a/tests/ui/panic_unimplemented.stderr b/tests/ui/panic_unimplemented.stderr index 5f19b35fe6cf..6d847e8df3eb 100644 --- a/tests/ui/panic_unimplemented.stderr +++ b/tests/ui/panic_unimplemented.stderr @@ -40,5 +40,13 @@ LL | unreachable!(); | = note: `-D clippy::unreachable` implied by `-D warnings` -error: aborting due to 6 previous errors +error: `todo` should not be present in production code + --> $DIR/panic_unimplemented.rs:67:5 + | +LL | todo!(); + | ^^^^^^^^ + | + = note: `-D clippy::todo` implied by `-D warnings` + +error: aborting due to 7 previous errors From a7ad78f3eb2886a942c077461f6e205ca72f9cb2 Mon Sep 17 00:00:00 2001 From: "Heinz N. Gies" Date: Tue, 15 Oct 2019 21:33:07 +0200 Subject: [PATCH 3/4] Add expect Co-Authored-By: Philipp Krones --- clippy_lints/src/methods/mod.rs | 87 ++++++++++++++++++++++++- clippy_lints/src/panic_unimplemented.rs | 2 +- tests/ui/methods.rs | 8 ++- 3 files changed, 94 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index efa283b823d9..ac8a1dbdac3f 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -73,7 +73,7 @@ declare_clippy_lint! { /// **Known problems:** None. /// /// **Example:** - /// Using unwrap on an `Option`: + /// Using unwrap on an `Result`: /// /// ```rust /// let res: Result = Ok(1); @@ -91,6 +91,63 @@ declare_clippy_lint! { "using `Result.unwrap()`, which might be better handled" } +declare_clippy_lint! { + /// **What it does:** Checks for `.expect()` calls on `Option`s. + /// + /// **Why is this bad?** Usually it is better to handle the `None` case. Still, + /// for a lot of quick-and-dirty code, `expect` is a good choice, which is why + /// this lint is `Allow` by default. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// Using expect on an `Option`: + /// + /// ```rust + /// let opt = Some(1); + /// opt.expect("one"); + /// ``` + /// + /// Better: + /// + /// ```rust + /// let opt = Some(1); + /// opt?; + /// ``` + pub OPTION_EXPECT_USED, + restriction, + "using `Option.expect()`, which might be better handled" +} + +declare_clippy_lint! { + /// **What it does:** Checks for `.expect()` calls on `Result`s. + /// + /// **Why is this bad?** `result.expect()` will let the thread panic on `Err` + /// values. Normally, you want to implement more sophisticated error handling, + /// and propagate errors upwards with `try!`. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// Using expect on an `Result`: + /// + /// ```rust + /// let res: Result = Ok(1); + /// res.expect("one"); + /// ``` + /// + /// Better: + /// + /// ```rust + /// let res: Result = Ok(1); + /// res?; + /// ``` + pub RESULT_EXPECT_USED, + restriction, + "using `Result.expect()`, which might be better handled" +} + declare_clippy_lint! { /// **What it does:** Checks for methods that should live in a trait /// implementation of a `std` trait (see [llogiq's blog @@ -1037,6 +1094,8 @@ declare_clippy_lint! { declare_lint_pass!(Methods => [ OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, + OPTION_EXPECT_USED, + RESULT_EXPECT_USED, SHOULD_IMPLEMENT_TRAIT, WRONG_SELF_CONVENTION, WRONG_PUB_SELF_CONVENTION, @@ -1095,6 +1154,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { ["unwrap", "get_mut"] => lint_get_unwrap(cx, expr, arg_lists[1], true), ["unwrap", ..] => lint_unwrap(cx, expr, arg_lists[0]), ["expect", "ok"] => lint_ok_expect(cx, expr, arg_lists[1]), + ["expect", ..] => lint_expect(cx, expr, arg_lists[0]), ["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0]), ["unwrap_or_else", "map"] => lint_map_unwrap_or_else(cx, expr, arg_lists[1], arg_lists[0]), ["map_or", ..] => lint_map_or_none(cx, expr, arg_lists[0]), @@ -2063,6 +2123,31 @@ fn lint_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, unwrap_args: &[hir::E } } +/// lint use of `expect()` for `Option`s and `Result`s +fn lint_expect(cx: &LateContext<'_, '_>, expr: &hir::Expr, expect_args: &[hir::Expr]) { + let obj_ty = walk_ptrs_ty(cx.tables.expr_ty(&expect_args[0])); + + let mess = if match_type(cx, obj_ty, &paths::OPTION) { + Some((OPTION_EXPECT_USED, "an Option", "None")) + } else if match_type(cx, obj_ty, &paths::RESULT) { + Some((RESULT_EXPECT_USED, "a Result", "Err")) + } else { + None + }; + + if let Some((lint, kind, none_value)) = mess { + span_lint( + cx, + lint, + expr.span, + &format!( + "used expect() on {} value. If this value is an {} it will panic", + kind, none_value + ), + ); + } +} + /// lint use of `ok().expect()` for `Result`s fn lint_ok_expect(cx: &LateContext<'_, '_>, expr: &hir::Expr, ok_args: &[hir::Expr]) { if_chain! { diff --git a/clippy_lints/src/panic_unimplemented.rs b/clippy_lints/src/panic_unimplemented.rs index 6981ecff0d01..94efb5acc000 100644 --- a/clippy_lints/src/panic_unimplemented.rs +++ b/clippy_lints/src/panic_unimplemented.rs @@ -76,7 +76,7 @@ declare_clippy_lint! { declare_clippy_lint! { /// **What it does:** Checks for usage of `unreachable!`. /// - /// **Why is this bad?** This macro can cause cause code to panics + /// **Why is this bad?** This macro can cause code to panic /// /// **Known problems:** None. /// diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 54a58e0c86a8..ca8358754ee9 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -1,7 +1,13 @@ // aux-build:option_helpers.rs // compile-flags: --edition 2018 -#![warn(clippy::all, clippy::pedantic, clippy::option_unwrap_used)] +#![warn( + clippy::all, + clippy::pedantic, + clippy::option_unwrap_used, + clippy::option_expect_used, + clippy::result_expect_used +)] #![allow( clippy::blacklisted_name, clippy::default_trait_access, From 7f454d8d06279663a330cfc4c9248056c402acd1 Mon Sep 17 00:00:00 2001 From: "Heinz N. Gies" Date: Wed, 16 Oct 2019 19:43:26 +0200 Subject: [PATCH 4/4] Split out tests --- CHANGELOG.md | 2 + README.md | 2 +- clippy_lints/src/lib.rs | 2 + clippy_lints/src/methods/mod.rs | 6 +- clippy_lints/src/panic_unimplemented.rs | 2 +- src/lintlist/mod.rs | 18 +++++- tests/ui/expect.rs | 16 +++++ tests/ui/expect.stderr | 18 ++++++ tests/ui/methods.rs | 14 ++--- tests/ui/methods.stderr | 10 +-- tests/ui/panic.rs | 61 ++++++++++++++++-- tests/ui/panic.stderr | 30 +++++++-- tests/ui/panic_unimplemented.rs | 82 ------------------------- tests/ui/panic_unimplemented.stderr | 52 ---------------- tests/ui/panicking_macros.rs | 33 ++++++++++ tests/ui/panicking_macros.stderr | 34 ++++++++++ tests/ui/unwrap.rs | 16 +++++ tests/ui/unwrap.stderr | 18 ++++++ 18 files changed, 245 insertions(+), 171 deletions(-) create mode 100644 tests/ui/expect.rs create mode 100644 tests/ui/expect.stderr delete mode 100644 tests/ui/panic_unimplemented.rs delete mode 100644 tests/ui/panic_unimplemented.stderr create mode 100644 tests/ui/panicking_macros.rs create mode 100644 tests/ui/panicking_macros.stderr create mode 100644 tests/ui/unwrap.rs create mode 100644 tests/ui/unwrap.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ce2eb4d5de2..ef21695cbab1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1129,6 +1129,7 @@ Released 2018-09-13 [`ok_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#ok_expect [`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref [`option_and_then_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_and_then_some +[`option_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_expect_used [`option_map_or_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_or_none [`option_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unit_fn [`option_map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unwrap_or @@ -1168,6 +1169,7 @@ Released 2018-09-13 [`ref_in_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_in_deref [`regex_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#regex_macro [`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts +[`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used [`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn [`result_map_unwrap_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unwrap_or_else [`result_unwrap_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_unwrap_used diff --git a/README.md b/README.md index 7913a3eefdab..41b8b4199ec5 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 329 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 331 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9fca5a4b97a4..bff1952cce77 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -625,7 +625,9 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con mem_forget::MEM_FORGET, methods::CLONE_ON_REF_PTR, methods::GET_UNWRAP, + methods::OPTION_EXPECT_USED, methods::OPTION_UNWRAP_USED, + methods::RESULT_EXPECT_USED, methods::RESULT_UNWRAP_USED, methods::WRONG_PUB_SELF_CONVENTION, misc::FLOAT_CMP_CONST, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index ac8a1dbdac3f..74538164f8e9 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -111,9 +111,10 @@ declare_clippy_lint! { /// /// Better: /// - /// ```rust + /// ```ignore /// let opt = Some(1); /// opt?; + /// # Some::<()>(()) /// ``` pub OPTION_EXPECT_USED, restriction, @@ -139,9 +140,10 @@ declare_clippy_lint! { /// /// Better: /// - /// ```rust + /// ``` /// let res: Result = Ok(1); /// res?; + /// # Ok::<(), ()>(()) /// ``` pub RESULT_EXPECT_USED, restriction, diff --git a/clippy_lints/src/panic_unimplemented.rs b/clippy_lints/src/panic_unimplemented.rs index 94efb5acc000..87ef5f9034c8 100644 --- a/clippy_lints/src/panic_unimplemented.rs +++ b/clippy_lints/src/panic_unimplemented.rs @@ -38,7 +38,7 @@ declare_clippy_lint! { /// ``` pub PANIC, restriction, - "missing parameters in `panic!` calls" + "usage of the `panic!` macro" } declare_clippy_lint! { diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 24e01f327802..f44ef226847f 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 329] = [ +pub const ALL_LINTS: [Lint; 331] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -1393,6 +1393,13 @@ pub const ALL_LINTS: [Lint; 329] = [ deprecation: None, module: "methods", }, + Lint { + name: "option_expect_used", + group: "restriction", + desc: "using `Option.expect()`, which might be better handled", + deprecation: None, + module: "methods", + }, Lint { name: "option_map_or_none", group: "style", @@ -1459,7 +1466,7 @@ pub const ALL_LINTS: [Lint; 329] = [ Lint { name: "panic", group: "restriction", - desc: "missing parameters in `panic!` calls", + desc: "usage of the `panic!` macro", deprecation: None, module: "panic_unimplemented", }, @@ -1659,6 +1666,13 @@ pub const ALL_LINTS: [Lint; 329] = [ deprecation: None, module: "replace_consts", }, + Lint { + name: "result_expect_used", + group: "restriction", + desc: "using `Result.expect()`, which might be better handled", + deprecation: None, + module: "methods", + }, Lint { name: "result_map_unit_fn", group: "complexity", diff --git a/tests/ui/expect.rs b/tests/ui/expect.rs new file mode 100644 index 000000000000..0bd4252c49aa --- /dev/null +++ b/tests/ui/expect.rs @@ -0,0 +1,16 @@ +#![warn(clippy::option_expect_used, clippy::result_expect_used)] + +fn expect_option() { + let opt = Some(0); + let _ = opt.expect(""); +} + +fn expect_result() { + let res: Result = Ok(0); + let _ = res.expect(""); +} + +fn main() { + expect_option(); + expect_result(); +} diff --git a/tests/ui/expect.stderr b/tests/ui/expect.stderr new file mode 100644 index 000000000000..4f954f611a69 --- /dev/null +++ b/tests/ui/expect.stderr @@ -0,0 +1,18 @@ +error: used expect() on an Option value. If this value is an None it will panic + --> $DIR/expect.rs:5:13 + | +LL | let _ = opt.expect(""); + | ^^^^^^^^^^^^^^ + | + = note: `-D clippy::option-expect-used` implied by `-D warnings` + +error: used expect() on a Result value. If this value is an Err it will panic + --> $DIR/expect.rs:10:13 + | +LL | let _ = res.expect(""); + | ^^^^^^^^^^^^^^ + | + = note: `-D clippy::result-expect-used` implied by `-D warnings` + +error: aborting due to 2 previous errors + diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index ca8358754ee9..847a0f0f3395 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -1,13 +1,7 @@ // aux-build:option_helpers.rs // compile-flags: --edition 2018 -#![warn( - clippy::all, - clippy::pedantic, - clippy::option_unwrap_used, - clippy::option_expect_used, - clippy::result_expect_used -)] +#![warn(clippy::all, clippy::pedantic)] #![allow( clippy::blacklisted_name, clippy::default_trait_access, @@ -307,8 +301,8 @@ fn search_is_some() { let _ = foo.rposition().is_some(); } -#[allow(clippy::similar_names)] fn main() { - let opt = Some(0); - let _ = opt.unwrap(); + option_methods(); + filter_next(); + search_is_some(); } diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index c3dc08be00b9..af7bd4a6a9d9 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -206,13 +206,5 @@ LL | | } LL | | ).is_some(); | |______________________________^ -error: used unwrap() on an Option value. If you don't want to handle the None case gracefully, consider using expect() to provide a better panic message - --> $DIR/methods.rs:307:13 - | -LL | let _ = opt.unwrap(); - | ^^^^^^^^^^^^ - | - = note: `-D clippy::option-unwrap-used` implied by `-D warnings` - -error: aborting due to 24 previous errors +error: aborting due to 23 previous errors diff --git a/tests/ui/panic.rs b/tests/ui/panic.rs index dee3104774cb..6e004aa9a924 100644 --- a/tests/ui/panic.rs +++ b/tests/ui/panic.rs @@ -1,12 +1,61 @@ -#![warn(clippy::panic)] +#![warn(clippy::panic_params)] #![allow(clippy::assertions_on_constants)] +fn missing() { + if true { + panic!("{}"); + } else if false { + panic!("{:?}"); + } else { + assert!(true, "here be missing values: {}"); + } -fn panic() { - let a = 2; - panic!(); - let b = a + 2; + panic!("{{{this}}}"); +} + +fn ok_single() { + panic!("foo bar"); +} + +fn ok_inner() { + // Test for #768 + assert!("foo bar".contains(&format!("foo {}", "bar"))); +} + +fn ok_multiple() { + panic!("{}", "This is {ok}"); +} + +fn ok_bracket() { + match 42 { + 1337 => panic!("{so is this"), + 666 => panic!("so is this}"), + _ => panic!("}so is that{"), + } +} + +const ONE: u32 = 1; + +fn ok_nomsg() { + assert!({ 1 == ONE }); + assert!(if 1 == ONE { ONE == 1 } else { false }); +} + +fn ok_escaped() { + panic!("{{ why should this not be ok? }}"); + panic!(" or {{ that ?"); + panic!(" or }} this ?"); + panic!(" {or {{ that ?"); + panic!(" }or }} this ?"); + panic!("{{ test }"); + panic!("{case }}"); } fn main() { - panic(); + missing(); + ok_single(); + ok_multiple(); + ok_bracket(); + ok_inner(); + ok_nomsg(); + ok_escaped(); } diff --git a/tests/ui/panic.stderr b/tests/ui/panic.stderr index cfef1a16e49e..1f8ff8ccf557 100644 --- a/tests/ui/panic.stderr +++ b/tests/ui/panic.stderr @@ -1,10 +1,28 @@ -error: `panic` should not be present in production code - --> $DIR/panic.rs:6:5 +error: you probably are missing some parameter in your format string + --> $DIR/panic.rs:5:16 | -LL | panic!(); - | ^^^^^^^^^ +LL | panic!("{}"); + | ^^^^ | - = note: `-D clippy::panic` implied by `-D warnings` + = note: `-D clippy::panic-params` implied by `-D warnings` -error: aborting due to previous error +error: you probably are missing some parameter in your format string + --> $DIR/panic.rs:7:16 + | +LL | panic!("{:?}"); + | ^^^^^^ + +error: you probably are missing some parameter in your format string + --> $DIR/panic.rs:9:23 + | +LL | assert!(true, "here be missing values: {}"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: you probably are missing some parameter in your format string + --> $DIR/panic.rs:12:12 + | +LL | panic!("{{{this}}}"); + | ^^^^^^^^^^^^ + +error: aborting due to 4 previous errors diff --git a/tests/ui/panic_unimplemented.rs b/tests/ui/panic_unimplemented.rs deleted file mode 100644 index f3dae3bbde65..000000000000 --- a/tests/ui/panic_unimplemented.rs +++ /dev/null @@ -1,82 +0,0 @@ -#![warn(clippy::panic_params, clippy::unimplemented, clippy::unreachable, clippy::todo)] -#![allow(clippy::assertions_on_constants)] -fn missing() { - if true { - panic!("{}"); - } else if false { - panic!("{:?}"); - } else { - assert!(true, "here be missing values: {}"); - } - - panic!("{{{this}}}"); -} - -fn ok_single() { - panic!("foo bar"); -} - -fn ok_inner() { - // Test for #768 - assert!("foo bar".contains(&format!("foo {}", "bar"))); -} - -fn ok_multiple() { - panic!("{}", "This is {ok}"); -} - -fn ok_bracket() { - match 42 { - 1337 => panic!("{so is this"), - 666 => panic!("so is this}"), - _ => panic!("}so is that{"), - } -} - -const ONE: u32 = 1; - -fn ok_nomsg() { - assert!({ 1 == ONE }); - assert!(if 1 == ONE { ONE == 1 } else { false }); -} - -fn ok_escaped() { - panic!("{{ why should this not be ok? }}"); - panic!(" or {{ that ?"); - panic!(" or }} this ?"); - panic!(" {or {{ that ?"); - panic!(" }or }} this ?"); - panic!("{{ test }"); - panic!("{case }}"); -} - -fn unimplemented() { - let a = 2; - unimplemented!(); - let b = a + 2; -} - -fn unreachable() { - let a = 2; - unreachable!(); - let b = a + 2; -} - -fn todo() { - let a = 2; - todo!(); - let b = a + 2; -} - -fn main() { - missing(); - ok_single(); - ok_multiple(); - ok_bracket(); - ok_inner(); - ok_nomsg(); - ok_escaped(); - unimplemented(); - unreachable(); - todo(); -} diff --git a/tests/ui/panic_unimplemented.stderr b/tests/ui/panic_unimplemented.stderr deleted file mode 100644 index 6d847e8df3eb..000000000000 --- a/tests/ui/panic_unimplemented.stderr +++ /dev/null @@ -1,52 +0,0 @@ -error: you probably are missing some parameter in your format string - --> $DIR/panic_unimplemented.rs:5:16 - | -LL | panic!("{}"); - | ^^^^ - | - = note: `-D clippy::panic-params` implied by `-D warnings` - -error: you probably are missing some parameter in your format string - --> $DIR/panic_unimplemented.rs:7:16 - | -LL | panic!("{:?}"); - | ^^^^^^ - -error: you probably are missing some parameter in your format string - --> $DIR/panic_unimplemented.rs:9:23 - | -LL | assert!(true, "here be missing values: {}"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: you probably are missing some parameter in your format string - --> $DIR/panic_unimplemented.rs:12:12 - | -LL | panic!("{{{this}}}"); - | ^^^^^^^^^^^^ - -error: `unimplemented` should not be present in production code - --> $DIR/panic_unimplemented.rs:55:5 - | -LL | unimplemented!(); - | ^^^^^^^^^^^^^^^^^ - | - = note: `-D clippy::unimplemented` implied by `-D warnings` - -error: `unreachable` should not be present in production code - --> $DIR/panic_unimplemented.rs:61:5 - | -LL | unreachable!(); - | ^^^^^^^^^^^^^^^ - | - = note: `-D clippy::unreachable` implied by `-D warnings` - -error: `todo` should not be present in production code - --> $DIR/panic_unimplemented.rs:67:5 - | -LL | todo!(); - | ^^^^^^^^ - | - = note: `-D clippy::todo` implied by `-D warnings` - -error: aborting due to 7 previous errors - diff --git a/tests/ui/panicking_macros.rs b/tests/ui/panicking_macros.rs new file mode 100644 index 000000000000..dabb695368db --- /dev/null +++ b/tests/ui/panicking_macros.rs @@ -0,0 +1,33 @@ +#![warn(clippy::unimplemented, clippy::unreachable, clippy::todo, clippy::panic)] +#![allow(clippy::assertions_on_constants)] + +fn panic() { + let a = 2; + panic!(); + let b = a + 2; +} + +fn todo() { + let a = 2; + todo!(); + let b = a + 2; +} + +fn unimplemented() { + let a = 2; + unimplemented!(); + let b = a + 2; +} + +fn unreachable() { + let a = 2; + unreachable!(); + let b = a + 2; +} + +fn main() { + panic(); + todo(); + unimplemented(); + unreachable(); +} diff --git a/tests/ui/panicking_macros.stderr b/tests/ui/panicking_macros.stderr new file mode 100644 index 000000000000..72319bc7e458 --- /dev/null +++ b/tests/ui/panicking_macros.stderr @@ -0,0 +1,34 @@ +error: `panic` should not be present in production code + --> $DIR/panicking_macros.rs:6:5 + | +LL | panic!(); + | ^^^^^^^^^ + | + = note: `-D clippy::panic` implied by `-D warnings` + +error: `todo` should not be present in production code + --> $DIR/panicking_macros.rs:12:5 + | +LL | todo!(); + | ^^^^^^^^ + | + = note: `-D clippy::todo` implied by `-D warnings` + +error: `unimplemented` should not be present in production code + --> $DIR/panicking_macros.rs:18:5 + | +LL | unimplemented!(); + | ^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::unimplemented` implied by `-D warnings` + +error: `unreachable` should not be present in production code + --> $DIR/panicking_macros.rs:24:5 + | +LL | unreachable!(); + | ^^^^^^^^^^^^^^^ + | + = note: `-D clippy::unreachable` implied by `-D warnings` + +error: aborting due to 4 previous errors + diff --git a/tests/ui/unwrap.rs b/tests/ui/unwrap.rs new file mode 100644 index 000000000000..fcd1fcd14d48 --- /dev/null +++ b/tests/ui/unwrap.rs @@ -0,0 +1,16 @@ +#![warn(clippy::option_unwrap_used, clippy::result_unwrap_used)] + +fn unwrap_option() { + let opt = Some(0); + let _ = opt.unwrap(); +} + +fn unwrap_result() { + let res: Result = Ok(0); + let _ = res.unwrap(); +} + +fn main() { + unwrap_option(); + unwrap_result(); +} diff --git a/tests/ui/unwrap.stderr b/tests/ui/unwrap.stderr new file mode 100644 index 000000000000..cde3ceffd9d3 --- /dev/null +++ b/tests/ui/unwrap.stderr @@ -0,0 +1,18 @@ +error: used unwrap() on an Option value. If you don't want to handle the None case gracefully, consider using expect() to provide a better panic message + --> $DIR/unwrap.rs:5:13 + | +LL | let _ = opt.unwrap(); + | ^^^^^^^^^^^^ + | + = note: `-D clippy::option-unwrap-used` implied by `-D warnings` + +error: used unwrap() on a Result value. If you don't want to handle the Err case gracefully, consider using expect() to provide a better panic message + --> $DIR/unwrap.rs:10:13 + | +LL | let _ = res.unwrap(); + | ^^^^^^^^^^^^ + | + = note: `-D clippy::result-unwrap-used` implied by `-D warnings` + +error: aborting due to 2 previous errors +