Skip to content

Add unnecessary_literal_unwrap lint #10358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Jun 12, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
95 changes: 71 additions & 24 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
95 changes: 95 additions & 0 deletions clippy_lints/src/methods/unnecessary_literal_unwrap.rs
Original file line number Diff line number Diff line change
@@ -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);
},
}
});
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//@run-rustfix
#![warn(clippy::uninlined_format_args)]
#![allow(clippy::unnecessary_literal_unwrap)]

fn main() {
let local_i32 = 1;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//@run-rustfix
#![warn(clippy::uninlined_format_args)]
#![allow(clippy::unnecessary_literal_unwrap)]

fn main() {
let local_i32 = 1;
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -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);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -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);
| ^^^
Expand All @@ -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);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -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);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -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());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![warn(clippy::arithmetic_side_effects)]
#![allow(clippy::unnecessary_literal_unwrap)]

use core::ops::{Add, Neg};

Expand Down
Original file line number Diff line number Diff line change
@@ -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;
| ^^^^
Expand Down
1 change: 1 addition & 0 deletions tests/ui-toml/expect_used/expect_used.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//@compile-flags: --test
#![warn(clippy::expect_used)]
#![allow(clippy::unnecessary_literal_unwrap)]

fn expect_option() {
let opt = Some(0);
Expand Down
Loading