Auto merge of #10358 - pksunkara:unnecessary-unwrap, r=llogiq

Add `unnecessary_literal_unwrap` lint Add lint for more unnecessary unwraps and suggest fixes for them. Fixes #10352 - [x] Followed [lint naming conventions][lint_naming] - [x] Added passing UI tests (including committed `.stderr` file) - [x] `cargo test` passes locally - [x] Executed `cargo dev update_lints` - [x] Added lint documentation - [x] Run `cargo dev fmt` r? `@llogiq` --- changelog: New lint [`unnecessary_literal_unwrap`] [#10358](#10358)
rust-lang · Jun 12, 2023 · 7c2bf28 · 7c2bf28
2 parents da56c35 + bfd5aba
commit 7c2bf28
Show file tree

Hide file tree

Showing 72 changed files with 1,883 additions and 379 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5248,6 +5248,7 @@ Released 2018-09-13
 [`unnecessary_fold`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fold
 [`unnecessary_join`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_join
 [`unnecessary_lazy_evaluations`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations
+[`unnecessary_literal_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_literal_unwrap
 [`unnecessary_mut_passed`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed
 [`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation
 [`unnecessary_owned_empty_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_owned_empty_strings

diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -408,6 +408,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::methods::UNNECESSARY_FOLD_INFO,
     crate::methods::UNNECESSARY_JOIN_INFO,
     crate::methods::UNNECESSARY_LAZY_EVALUATIONS_INFO,
+    crate::methods::UNNECESSARY_LITERAL_UNWRAP_INFO,
     crate::methods::UNNECESSARY_SORT_BY_INFO,
     crate::methods::UNNECESSARY_TO_OWNED_INFO,
     crate::methods::UNWRAP_OR_ELSE_DEFAULT_INFO,

diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -93,6 +93,7 @@ mod unnecessary_fold;
 mod unnecessary_iter_cloned;
 mod unnecessary_join;
 mod unnecessary_lazy_eval;
+mod unnecessary_literal_unwrap;
 mod unnecessary_sort_by;
 mod unnecessary_to_owned;
 mod unwrap_or_else_default;
@@ -273,6 +274,32 @@ declare_clippy_lint! {
     "using `.unwrap()` on `Result` or `Option`, which should at least get a better message using `expect()`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `.unwrap()` related calls on `Result`s and `Option`s that are constructed.
+    ///
+    /// ### Why is this bad?
+    /// It is better to write the value directly without the indirection.
+    ///
+    /// ### Examples
+    /// ```rust
+    /// let val1 = Some(1).unwrap();
+    /// let val2 = Ok::<_, ()>(1).unwrap();
+    /// let val3 = Err::<(), _>(1).unwrap_err();
+    /// ```
+    ///
+    /// Use instead:
+    /// ```rust
+    /// let val1 = 1;
+    /// let val2 = 1;
+    /// let val3 = 1;
+    /// ```
+    #[clippy::version = "1.69.0"]
+    pub UNNECESSARY_LITERAL_UNWRAP,
+    complexity,
+    "using `unwrap()` related calls on `Result` and `Option` constructors"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for `.expect()` or `.expect_err()` calls on `Result`s and `.expect()` call on `Option`s.
@@ -3349,6 +3376,7 @@ impl_lint_pass!(Methods => [
     SUSPICIOUS_COMMAND_ARG_SPACE,
     CLEAR_WITH_DRAIN,
     MANUAL_NEXT_BACK,
+    UNNECESSARY_LITERAL_UNWRAP,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -3606,12 +3634,18 @@ impl Methods {
                         case_sensitive_file_extension_comparisons::check(cx, expr, span, recv, arg);
                     }
                 },
-                ("expect", [_]) => match method_call(recv) {
-                    Some(("ok", recv, [], _, _)) => ok_expect::check(cx, expr, recv),
-                    Some(("err", recv, [], err_span, _)) => err_expect::check(cx, expr, recv, span, err_span, &self.msrv),
-                    _ => expect_used::check(cx, expr, recv, false, self.allow_expect_in_tests),
+                ("expect", [_]) => {
+                    match method_call(recv) {
+                        Some(("ok", recv, [], _, _)) => ok_expect::check(cx, expr, recv),
+                        Some(("err", recv, [], err_span, _)) => err_expect::check(cx, expr, recv, span, err_span, &self.msrv),
+                        _ => expect_used::check(cx, expr, recv, false, self.allow_expect_in_tests),
+                    }
+                    unnecessary_literal_unwrap::check(cx, expr, recv, name, args);
+                },
+                ("expect_err", [_]) => {
+                    unnecessary_literal_unwrap::check(cx, expr, recv, name, args);
+                    expect_used::check(cx, expr, recv, true, self.allow_expect_in_tests);
                 },
-                ("expect_err", [_]) => expect_used::check(cx, expr, recv, true, self.allow_expect_in_tests),
                 ("extend", [arg]) => {
                     string_extend_chars::check(cx, expr, recv, arg);
                     extend_with_drain::check(cx, expr, recv, arg);
@@ -3816,28 +3850,41 @@ impl Methods {
                         },
                         _ => {},
                     }
+                    unnecessary_literal_unwrap::check(cx, expr, recv, name, args);
                     unwrap_used::check(cx, expr, recv, false, self.allow_unwrap_in_tests);
                 },
-                ("unwrap_err", []) => unwrap_used::check(cx, expr, recv, true, self.allow_unwrap_in_tests),
-                ("unwrap_or", [u_arg]) => match method_call(recv) {
-                    Some((arith @ ("checked_add" | "checked_sub" | "checked_mul"), lhs, [rhs], _, _)) => {
-                        manual_saturating_arithmetic::check(cx, expr, lhs, rhs, u_arg, &arith["checked_".len()..]);
-                    },
-                    Some(("map", m_recv, [m_arg], span, _)) => {
-                        option_map_unwrap_or::check(cx, expr, m_recv, m_arg, recv, u_arg, span);
-                    },
-                    Some(("then_some", t_recv, [t_arg], _, _)) => {
-                        obfuscated_if_else::check(cx, expr, t_recv, t_arg, u_arg);
-                    },
-                    _ => {},
+                ("unwrap_err", []) => {
+                    unnecessary_literal_unwrap::check(cx, expr, recv, name, args);
+                    unwrap_used::check(cx, expr, recv, true, self.allow_unwrap_in_tests);
                 },
-                ("unwrap_or_else", [u_arg]) => match method_call(recv) {
-                    Some(("map", recv, [map_arg], _, _))
-                        if map_unwrap_or::check(cx, expr, recv, map_arg, u_arg, &self.msrv) => {},
-                    _ => {
-                        unwrap_or_else_default::check(cx, expr, recv, u_arg);
-                        unnecessary_lazy_eval::check(cx, expr, recv, u_arg, "unwrap_or");
-                    },
+                ("unwrap_or", [u_arg]) => {
+                    match method_call(recv) {
+                        Some((arith @ ("checked_add" | "checked_sub" | "checked_mul"), lhs, [rhs], _, _)) => {
+                            manual_saturating_arithmetic::check(cx, expr, lhs, rhs, u_arg, &arith["checked_".len()..]);
+                        },
+                        Some(("map", m_recv, [m_arg], span, _)) => {
+                            option_map_unwrap_or::check(cx, expr, m_recv, m_arg, recv, u_arg, span);
+                        },
+                        Some(("then_some", t_recv, [t_arg], _, _)) => {
+                            obfuscated_if_else::check(cx, expr, t_recv, t_arg, u_arg);
+                        },
+                        _ => {},
+                    }
+                    unnecessary_literal_unwrap::check(cx, expr, recv, name, args);
+                },
+                ("unwrap_or_default", []) => {
+                    unnecessary_literal_unwrap::check(cx, expr, recv, name, args);
+                }
+                ("unwrap_or_else", [u_arg]) => {
+                    match method_call(recv) {
+                        Some(("map", recv, [map_arg], _, _))
+                            if map_unwrap_or::check(cx, expr, recv, map_arg, u_arg, &self.msrv) => {},
+                        _ => {
+                            unwrap_or_else_default::check(cx, expr, recv, u_arg);
+                            unnecessary_lazy_eval::check(cx, expr, recv, u_arg, "unwrap_or");
+                        },
+                    }
+                    unnecessary_literal_unwrap::check(cx, expr, recv, name, args);
                 },
                 ("zip", [arg]) => {
                     if let ExprKind::MethodCall(name, iter_recv, [], _) = recv.kind

diff --git a/clippy_lints/src/methods/unnecessary_literal_unwrap.rs b/clippy_lints/src/methods/unnecessary_literal_unwrap.rs
@@ -0,0 +1,95 @@
+use clippy_utils::{diagnostics::span_lint_and_then, is_res_lang_ctor, last_path_segment, path_res, MaybePath};
+use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_lint::LateContext;
+
+use super::UNNECESSARY_LITERAL_UNWRAP;
+
+fn get_ty_from_args<'a>(args: Option<&'a [hir::GenericArg<'a>]>, index: usize) -> Option<&'a hir::Ty<'a>> {
+    let args = args?;
+
+    if args.len() <= index {
+        return None;
+    }
+
+    match args[index] {
+        hir::GenericArg::Type(ty) => match ty.kind {
+            hir::TyKind::Infer => None,
+            _ => Some(ty),
+        },
+        _ => None,
+    }
+}
+
+pub(super) fn check(
+    cx: &LateContext<'_>,
+    expr: &hir::Expr<'_>,
+    recv: &hir::Expr<'_>,
+    method: &str,
+    args: &[hir::Expr<'_>],
+) {
+    let init = clippy_utils::expr_or_init(cx, recv);
+
+    let (constructor, call_args, ty) = if let hir::ExprKind::Call(call, call_args) = init.kind {
+        let Some(qpath) = call.qpath_opt() else { return };
+
+        let args = last_path_segment(qpath).args.map(|args| args.args);
+        let res = cx.qpath_res(qpath, call.hir_id());
+
+        if is_res_lang_ctor(cx, res, hir::LangItem::OptionSome) {
+            ("Some", call_args, get_ty_from_args(args, 0))
+        } else if is_res_lang_ctor(cx, res, hir::LangItem::ResultOk) {
+            ("Ok", call_args, get_ty_from_args(args, 0))
+        } else if is_res_lang_ctor(cx, res, hir::LangItem::ResultErr) {
+            ("Err", call_args, get_ty_from_args(args, 1))
+        } else {
+            return;
+        }
+    } else if is_res_lang_ctor(cx, path_res(cx, init), hir::LangItem::OptionNone) {
+        let call_args: &[hir::Expr<'_>] = &[];
+        ("None", call_args, None)
+    } else {
+        return;
+    };
+
+    let help_message = format!("used `{method}()` on `{constructor}` value");
+    let suggestion_message = format!("remove the `{constructor}` and `{method}()`");
+
+    span_lint_and_then(cx, UNNECESSARY_LITERAL_UNWRAP, expr.span, &help_message, |diag| {
+        let suggestions = match (constructor, method, ty) {
+            ("None", "unwrap", _) => Some(vec![(expr.span, "panic!()".to_string())]),
+            ("None", "expect", _) => Some(vec![
+                (expr.span.with_hi(args[0].span.lo()), "panic!(".to_string()),
+                (expr.span.with_lo(args[0].span.hi()), ")".to_string()),
+            ]),
+            (_, _, Some(_)) => None,
+            ("Ok", "unwrap_err", None) | ("Err", "unwrap", None) => Some(vec![
+                (
+                    recv.span.with_hi(call_args[0].span.lo()),
+                    "panic!(\"{:?}\", ".to_string(),
+                ),
+                (expr.span.with_lo(call_args[0].span.hi()), ")".to_string()),
+            ]),
+            ("Ok", "expect_err", None) | ("Err", "expect", None) => Some(vec![
+                (
+                    recv.span.with_hi(call_args[0].span.lo()),
+                    "panic!(\"{1}: {:?}\", ".to_string(),
+                ),
+                (call_args[0].span.with_lo(args[0].span.lo()), ", ".to_string()),
+            ]),
+            (_, _, None) => Some(vec![
+                (recv.span.with_hi(call_args[0].span.lo()), String::new()),
+                (expr.span.with_lo(call_args[0].span.hi()), String::new()),
+            ]),
+        };
+
+        match (init.span == recv.span, suggestions) {
+            (true, Some(suggestions)) => {
+                diag.multipart_suggestion(suggestion_message, suggestions, Applicability::MachineApplicable);
+            },
+            _ => {
+                diag.span_help(init.span, suggestion_message);
+            },
+        }
+    });
+}
diff --git a/tests/ui-toml/allow_mixed_uninlined_format_args/uninlined_format_args.fixed b/tests/ui-toml/allow_mixed_uninlined_format_args/uninlined_format_args.fixed
@@ -1,5 +1,6 @@
 //@run-rustfix
 #![warn(clippy::uninlined_format_args)]
+#![allow(clippy::unnecessary_literal_unwrap)]
 
 fn main() {
     let local_i32 = 1;

diff --git a/tests/ui-toml/allow_mixed_uninlined_format_args/uninlined_format_args.rs b/tests/ui-toml/allow_mixed_uninlined_format_args/uninlined_format_args.rs
@@ -1,5 +1,6 @@
 //@run-rustfix
 #![warn(clippy::uninlined_format_args)]
+#![allow(clippy::unnecessary_literal_unwrap)]
 
 fn main() {
     let local_i32 = 1;

diff --git a/tests/ui-toml/allow_mixed_uninlined_format_args/uninlined_format_args.stderr b/tests/ui-toml/allow_mixed_uninlined_format_args/uninlined_format_args.stderr
@@ -1,5 +1,5 @@
 error: variables can be used directly in the `format!` string
-  --> $DIR/uninlined_format_args.rs:9:5
+  --> $DIR/uninlined_format_args.rs:10:5
    |
 LL |     println!("val='{}'", local_i32);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -12,7 +12,7 @@ LL +     println!("val='{local_i32}'");
    |
 
 error: variables can be used directly in the `format!` string
-  --> $DIR/uninlined_format_args.rs:10:5
+  --> $DIR/uninlined_format_args.rs:11:5
    |
 LL |     println!("Hello {} is {:.*}", "x", local_i32, local_f64);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -24,7 +24,7 @@ LL +     println!("Hello {} is {local_f64:.local_i32$}", "x");
    |
 
 error: literal with an empty format string
-  --> $DIR/uninlined_format_args.rs:10:35
+  --> $DIR/uninlined_format_args.rs:11:35
    |
 LL |     println!("Hello {} is {:.*}", "x", local_i32, local_f64);
    |                                   ^^^
@@ -37,7 +37,7 @@ LL +     println!("Hello x is {:.*}", local_i32, local_f64);
    |
 
 error: variables can be used directly in the `format!` string
-  --> $DIR/uninlined_format_args.rs:11:5
+  --> $DIR/uninlined_format_args.rs:12:5
    |
 LL |     println!("Hello {} is {:.*}", local_i32, 5, local_f64);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -49,7 +49,7 @@ LL +     println!("Hello {local_i32} is {local_f64:.*}", 5);
    |
 
 error: variables can be used directly in the `format!` string
-  --> $DIR/uninlined_format_args.rs:12:5
+  --> $DIR/uninlined_format_args.rs:13:5
    |
 LL |     println!("Hello {} is {2:.*}", local_i32, 5, local_f64);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -61,7 +61,7 @@ LL +     println!("Hello {local_i32} is {local_f64:.*}", 5);
    |
 
 error: variables can be used directly in the `format!` string
-  --> $DIR/uninlined_format_args.rs:13:5
+  --> $DIR/uninlined_format_args.rs:14:5
    |
 LL |     println!("{}, {}", local_i32, local_opt.unwrap());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

diff --git a/tests/ui-toml/arithmetic_side_effects_allowed/arithmetic_side_effects_allowed.rs b/tests/ui-toml/arithmetic_side_effects_allowed/arithmetic_side_effects_allowed.rs
@@ -1,4 +1,5 @@
 #![warn(clippy::arithmetic_side_effects)]
+#![allow(clippy::unnecessary_literal_unwrap)]
 
 use core::ops::{Add, Neg};
 

diff --git a/tests/ui-toml/arithmetic_side_effects_allowed/arithmetic_side_effects_allowed.stderr b/tests/ui-toml/arithmetic_side_effects_allowed/arithmetic_side_effects_allowed.stderr
@@ -1,55 +1,55 @@
 error: arithmetic operation that can potentially result in unexpected side-effects
-  --> $DIR/arithmetic_side_effects_allowed.rs:68:13
+  --> $DIR/arithmetic_side_effects_allowed.rs:69:13
    |
 LL |     let _ = Baz + Baz;
    |             ^^^^^^^^^
    |
    = note: `-D clippy::arithmetic-side-effects` implied by `-D warnings`
 
 error: arithmetic operation that can potentially result in unexpected side-effects
-  --> $DIR/arithmetic_side_effects_allowed.rs:79:13
+  --> $DIR/arithmetic_side_effects_allowed.rs:80:13
    |
 LL |     let _ = 1i32 + Baz;
    |             ^^^^^^^^^^
 
 error: arithmetic operation that can potentially result in unexpected side-effects
-  --> $DIR/arithmetic_side_effects_allowed.rs:82:13
+  --> $DIR/arithmetic_side_effects_allowed.rs:83:13
    |
 LL |     let _ = 1i64 + Foo;
    |             ^^^^^^^^^^
 
 error: arithmetic operation that can potentially result in unexpected side-effects
-  --> $DIR/arithmetic_side_effects_allowed.rs:86:13
+  --> $DIR/arithmetic_side_effects_allowed.rs:87:13
    |
 LL |     let _ = 1i64 + Baz;
    |             ^^^^^^^^^^
 
 error: arithmetic operation that can potentially result in unexpected side-effects
-  --> $DIR/arithmetic_side_effects_allowed.rs:97:13
+  --> $DIR/arithmetic_side_effects_allowed.rs:98:13
    |
 LL |     let _ = Baz + 1i32;
    |             ^^^^^^^^^^
 
 error: arithmetic operation that can potentially result in unexpected side-effects
-  --> $DIR/arithmetic_side_effects_allowed.rs:100:13
+  --> $DIR/arithmetic_side_effects_allowed.rs:101:13
    |
 LL |     let _ = Foo + 1i64;
    |             ^^^^^^^^^^
 
 error: arithmetic operation that can potentially result in unexpected side-effects
-  --> $DIR/arithmetic_side_effects_allowed.rs:104:13
+  --> $DIR/arithmetic_side_effects_allowed.rs:105:13
    |
 LL |     let _ = Baz + 1i64;
    |             ^^^^^^^^^^
 
 error: arithmetic operation that can potentially result in unexpected side-effects
-  --> $DIR/arithmetic_side_effects_allowed.rs:113:13
+  --> $DIR/arithmetic_side_effects_allowed.rs:114:13
    |
 LL |     let _ = -Bar;
    |             ^^^^
 
 error: arithmetic operation that can potentially result in unexpected side-effects
-  --> $DIR/arithmetic_side_effects_allowed.rs:115:13
+  --> $DIR/arithmetic_side_effects_allowed.rs:116:13
    |
 LL |     let _ = -Baz;
    |             ^^^^

diff --git a/tests/ui-toml/expect_used/expect_used.rs b/tests/ui-toml/expect_used/expect_used.rs
@@ -1,5 +1,6 @@
 //@compile-flags: --test
 #![warn(clippy::expect_used)]
+#![allow(clippy::unnecessary_literal_unwrap)]
 
 fn expect_option() {
     let opt = Some(0);