Auto merge of #9338 - sgued:9331-unwrap-err-used, r=giraffate

unwrap_used and expect_used: trigger on uses of their _err variants

changelog: [`unwrap_used`]: lint uses of `unwrap_err`
changelog: [`expect_used`]: lint uses of `expect_err`

fixes #9331
This commit is contained in:
bors 2022-08-17 23:35:44 +00:00
commit 849c1c0465
9 changed files with 101 additions and 24 deletions

View file

@ -7,18 +7,26 @@ use rustc_span::sym;
use super::EXPECT_USED;
/// lint use of `expect()` for `Option`s and `Result`s
pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, allow_expect_in_tests: bool) {
/// lint use of `expect()` or `expect_err` for `Result` and `expect()` for `Option`.
pub(super) fn check(
cx: &LateContext<'_>,
expr: &hir::Expr<'_>,
recv: &hir::Expr<'_>,
is_err: bool,
allow_expect_in_tests: bool,
) {
let obj_ty = cx.typeck_results().expr_ty(recv).peel_refs();
let mess = if is_type_diagnostic_item(cx, obj_ty, sym::Option) {
let mess = if is_type_diagnostic_item(cx, obj_ty, sym::Option) && !is_err {
Some((EXPECT_USED, "an Option", "None", ""))
} else if is_type_diagnostic_item(cx, obj_ty, sym::Result) {
Some((EXPECT_USED, "a Result", "Err", "an "))
Some((EXPECT_USED, "a Result", if is_err { "Ok" } else { "Err" }, "an "))
} else {
None
};
let method = if is_err { "expect_err" } else { "expect" };
if allow_expect_in_tests && is_in_test_function(cx.tcx, expr.hir_id) {
return;
}
@ -28,7 +36,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr
cx,
lint,
expr.span,
&format!("used `expect()` on `{kind}` value"),
&format!("used `{method}()` on `{kind}` value"),
None,
&format!("if this value is {none_prefix}`{none_value}`, it will panic"),
);

View file

@ -174,7 +174,7 @@ declare_clippy_lint! {
declare_clippy_lint! {
/// ### What it does
/// Checks for `.unwrap()` calls on `Option`s and on `Result`s.
/// Checks for `.unwrap()` or `.unwrap_err()` calls on `Result`s and `.unwrap()` call on `Option`s.
///
/// ### Why is this bad?
/// It is better to handle the `None` or `Err` case,
@ -224,7 +224,7 @@ declare_clippy_lint! {
declare_clippy_lint! {
/// ### What it does
/// Checks for `.expect()` calls on `Option`s and `Result`s.
/// Checks for `.expect()` or `.expect_err()` calls on `Result`s and `.expect()` call on `Option`s.
///
/// ### Why is this bad?
/// Usually it is better to handle the `None` or `Err` case.
@ -2740,8 +2740,9 @@ impl Methods {
("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, self.msrv, span, err_span),
_ => expect_used::check(cx, expr, recv, self.allow_expect_in_tests),
_ => expect_used::check(cx, expr, recv, false, 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);
@ -2874,8 +2875,9 @@ impl Methods {
},
_ => {},
}
unwrap_used::check(cx, expr, recv, self.allow_unwrap_in_tests);
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()..]);

View file

@ -7,18 +7,26 @@ use rustc_span::sym;
use super::{EXPECT_USED, UNWRAP_USED};
/// lint use of `unwrap()` for `Option`s and `Result`s
pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, allow_unwrap_in_tests: bool) {
/// lint use of `unwrap()` or `unwrap_err` for `Result` and `unwrap()` for `Option`.
pub(super) fn check(
cx: &LateContext<'_>,
expr: &hir::Expr<'_>,
recv: &hir::Expr<'_>,
is_err: bool,
allow_unwrap_in_tests: bool,
) {
let obj_ty = cx.typeck_results().expr_ty(recv).peel_refs();
let mess = if is_type_diagnostic_item(cx, obj_ty, sym::Option) {
let mess = if is_type_diagnostic_item(cx, obj_ty, sym::Option) && !is_err {
Some((UNWRAP_USED, "an Option", "None", ""))
} else if is_type_diagnostic_item(cx, obj_ty, sym::Result) {
Some((UNWRAP_USED, "a Result", "Err", "an "))
Some((UNWRAP_USED, "a Result", if is_err { "Ok" } else { "Err" }, "an "))
} else {
None
};
let method_suffix = if is_err { "_err" } else { "" };
if allow_unwrap_in_tests && is_in_test_function(cx.tcx, expr.hir_id) {
return;
}
@ -27,7 +35,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr
let help = if is_lint_allowed(cx, EXPECT_USED, expr.hir_id) {
format!(
"if you don't want to handle the `{none_value}` case gracefully, consider \
using `expect()` to provide a better panic message"
using `expect{method_suffix}()` to provide a better panic message"
)
} else {
format!("if this value is {none_prefix}`{none_value}`, it will panic")
@ -37,7 +45,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr
cx,
lint,
expr.span,
&format!("used `unwrap()` on `{kind}` value"),
&format!("used `unwrap{method_suffix}()` on `{kind}` value"),
None,
&help,
);

View file

@ -6,8 +6,9 @@ fn expect_option() {
}
fn expect_result() {
let res: Result<u8, ()> = Ok(0);
let res: Result<u8, u8> = Ok(0);
let _ = res.expect("");
let _ = res.expect_err("");
}
fn main() {

View file

@ -15,5 +15,13 @@ LL | let _ = res.expect("");
|
= help: if this value is an `Err`, it will panic
error: aborting due to 2 previous errors
error: used `expect_err()` on `a Result` value
--> $DIR/expect.rs:11:13
|
LL | let _ = res.expect_err("");
| ^^^^^^^^^^^^^^^^^^
|
= help: if this value is an `Ok`, it will panic
error: aborting due to 3 previous errors

View file

@ -6,8 +6,9 @@ fn unwrap_option() {
}
fn unwrap_result() {
let res: Result<u8, ()> = Ok(0);
let res: Result<u8, u8> = Ok(0);
let _ = res.unwrap();
let _ = res.unwrap_err();
}
fn main() {

View file

@ -15,5 +15,13 @@ LL | let _ = res.unwrap();
|
= help: if you don't want to handle the `Err` case gracefully, consider using `expect()` to provide a better panic message
error: aborting due to 2 previous errors
error: used `unwrap_err()` on `a Result` value
--> $DIR/unwrap.rs:11:13
|
LL | let _ = res.unwrap_err();
| ^^^^^^^^^^^^^^^^
|
= help: if you don't want to handle the `Ok` case gracefully, consider using `expect_err()` to provide a better panic message
error: aborting due to 3 previous errors

View file

@ -1,10 +1,35 @@
#![warn(clippy::unwrap_used, clippy::expect_used)]
trait OptionExt {
type Item;
fn unwrap_err(self) -> Self::Item;
fn expect_err(self, msg: &str) -> Self::Item;
}
impl<T> OptionExt for Option<T> {
type Item = T;
fn unwrap_err(self) -> T {
panic!();
}
fn expect_err(self, msg: &str) -> T {
panic!();
}
}
fn main() {
Some(3).unwrap();
Some(3).expect("Hello world!");
// Don't trigger on unwrap_err on an option
Some(3).unwrap_err();
Some(3).expect_err("Hellow none!");
let a: Result<i32, i32> = Ok(3);
a.unwrap();
a.expect("Hello world!");
a.unwrap_err();
a.expect_err("Hello error!");
}

View file

@ -1,5 +1,5 @@
error: used `unwrap()` on `an Option` value
--> $DIR/unwrap_expect_used.rs:4:5
--> $DIR/unwrap_expect_used.rs:23:5
|
LL | Some(3).unwrap();
| ^^^^^^^^^^^^^^^^
@ -8,7 +8,7 @@ LL | Some(3).unwrap();
= help: if this value is `None`, it will panic
error: used `expect()` on `an Option` value
--> $DIR/unwrap_expect_used.rs:5:5
--> $DIR/unwrap_expect_used.rs:24:5
|
LL | Some(3).expect("Hello world!");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@ -17,7 +17,7 @@ LL | Some(3).expect("Hello world!");
= help: if this value is `None`, it will panic
error: used `unwrap()` on `a Result` value
--> $DIR/unwrap_expect_used.rs:8:5
--> $DIR/unwrap_expect_used.rs:31:5
|
LL | a.unwrap();
| ^^^^^^^^^^
@ -25,12 +25,28 @@ LL | a.unwrap();
= help: if this value is an `Err`, it will panic
error: used `expect()` on `a Result` value
--> $DIR/unwrap_expect_used.rs:9:5
--> $DIR/unwrap_expect_used.rs:32:5
|
LL | a.expect("Hello world!");
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: if this value is an `Err`, it will panic
error: aborting due to 4 previous errors
error: used `unwrap_err()` on `a Result` value
--> $DIR/unwrap_expect_used.rs:33:5
|
LL | a.unwrap_err();
| ^^^^^^^^^^^^^^
|
= help: if this value is an `Ok`, it will panic
error: used `expect_err()` on `a Result` value
--> $DIR/unwrap_expect_used.rs:34:5
|
LL | a.expect_err("Hello error!");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: if this value is an `Ok`, it will panic
error: aborting due to 6 previous errors