diff options
| -rw-r--r-- | clippy_lints/src/question_mark.rs | 21 | ||||
| -rw-r--r-- | tests/ui/question_mark.fixed | 14 | ||||
| -rw-r--r-- | tests/ui/question_mark.rs | 26 | ||||
| -rw-r--r-- | tests/ui/question_mark.stderr | 92 |
4 files changed, 113 insertions, 40 deletions
diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index de12a25b03d..4aa100a50e0 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -8,9 +8,9 @@ use clippy_utils::source::snippet_with_applicability; use clippy_utils::sugg::Sugg; use clippy_utils::ty::{implements_trait, is_type_diagnostic_item}; use clippy_utils::{ - eq_expr_value, higher, is_else_clause, is_in_const_context, is_lint_allowed, is_path_lang_item, is_res_lang_ctor, - pat_and_expr_can_be_question_mark, path_res, path_to_local, path_to_local_id, peel_blocks, peel_blocks_with_stmt, - span_contains_cfg, span_contains_comment, sym, + eq_expr_value, fn_def_id_with_node_args, higher, is_else_clause, is_in_const_context, is_lint_allowed, + is_path_lang_item, is_res_lang_ctor, pat_and_expr_can_be_question_mark, path_res, path_to_local, path_to_local_id, + peel_blocks, peel_blocks_with_stmt, span_contains_cfg, span_contains_comment, sym, }; use rustc_errors::Applicability; use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk}; @@ -393,8 +393,8 @@ fn check_arm_is_none_or_err<'tcx>(cx: &LateContext<'tcx>, mode: TryMode, arm: &A && let ExprKind::Ret(Some(wrapped_ret_expr)) = arm_body.kind && let ExprKind::Call(ok_ctor, [ret_expr]) = wrapped_ret_expr.kind && is_res_lang_ctor(cx, path_res(cx, ok_ctor), ResultErr) - // check `...` is `val` from binding - && path_to_local_id(ret_expr, ok_val) + // check if `...` is `val` from binding or `val.into()` + && is_local_or_local_into(cx, ret_expr, ok_val) { true } else { @@ -417,6 +417,17 @@ fn check_arm_is_none_or_err<'tcx>(cx: &LateContext<'tcx>, mode: TryMode, arm: &A } } +/// Check if `expr` is `val` or `val.into()` +fn is_local_or_local_into(cx: &LateContext<'_>, expr: &Expr<'_>, val: HirId) -> bool { + let is_into_call = fn_def_id_with_node_args(cx, expr) + .and_then(|(fn_def_id, _)| cx.tcx.trait_of_assoc(fn_def_id)) + .is_some_and(|trait_def_id| cx.tcx.is_diagnostic_item(sym::Into, trait_def_id)); + match expr.kind { + ExprKind::MethodCall(_, recv, [], _) | ExprKind::Call(_, [recv]) => is_into_call && path_to_local_id(recv, val), + _ => path_to_local_id(expr, val), + } +} + fn check_arms_are_try<'tcx>(cx: &LateContext<'tcx>, mode: TryMode, arm1: &Arm<'tcx>, arm2: &Arm<'tcx>) -> bool { (check_arm_is_some_or_ok(cx, mode, arm1) && check_arm_is_none_or_err(cx, mode, arm2)) || (check_arm_is_some_or_ok(cx, mode, arm2) && check_arm_is_none_or_err(cx, mode, arm1)) diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed index 8d6f5fbadca..ac81b324c20 100644 --- a/tests/ui/question_mark.fixed +++ b/tests/ui/question_mark.fixed @@ -1,6 +1,4 @@ #![feature(try_blocks)] -#![allow(unreachable_code)] -#![allow(dead_code)] #![allow(clippy::unnecessary_wraps)] use std::sync::MutexGuard; @@ -465,3 +463,15 @@ fn issue_13642(x: Option<i32>) -> Option<()> { None } + +fn issue_15679() -> Result<i32, String> { + let some_result: Result<i32, &'static str> = todo!(); + + some_result?; + + some_result?; + + some_result?; + + Ok(0) +} diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index f13eee29c11..b5866dac6b8 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -1,6 +1,4 @@ #![feature(try_blocks)] -#![allow(unreachable_code)] -#![allow(dead_code)] #![allow(clippy::unnecessary_wraps)] use std::sync::MutexGuard; @@ -561,3 +559,27 @@ fn issue_13642(x: Option<i32>) -> Option<()> { None } + +fn issue_15679() -> Result<i32, String> { + let some_result: Result<i32, &'static str> = todo!(); + + match some_result { + //~^ question_mark + Ok(val) => val, + Err(err) => return Err(err.into()), + }; + + match some_result { + //~^ question_mark + Ok(val) => val, + Err(err) => return Err(Into::into(err)), + }; + + match some_result { + //~^ question_mark + Ok(val) => val, + Err(err) => return Err(<&str as Into<String>>::into(err)), + }; + + Ok(0) +} diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index d8ce4420aee..1ecd936292e 100644 --- a/tests/ui/question_mark.stderr +++ b/tests/ui/question_mark.stderr @@ -1,5 +1,5 @@ error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:9:5 + --> tests/ui/question_mark.rs:7:5 | LL | / if a.is_none() { LL | | @@ -11,7 +11,7 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::question_mark)]` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:55:9 + --> tests/ui/question_mark.rs:53:9 | LL | / if (self.opt).is_none() { LL | | @@ -20,7 +20,7 @@ LL | | } | |_________^ help: replace it with: `(self.opt)?;` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:60:9 + --> tests/ui/question_mark.rs:58:9 | LL | / if self.opt.is_none() { LL | | @@ -29,7 +29,7 @@ LL | | } | |_________^ help: replace it with: `self.opt?;` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:65:17 + --> tests/ui/question_mark.rs:63:17 | LL | let _ = if self.opt.is_none() { | _________________^ @@ -41,7 +41,7 @@ LL | | }; | |_________^ help: replace it with: `Some(self.opt?)` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:72:17 + --> tests/ui/question_mark.rs:70:17 | LL | let _ = if let Some(x) = self.opt { | _________________^ @@ -53,7 +53,7 @@ LL | | }; | |_________^ help: replace it with: `self.opt?` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:90:9 + --> tests/ui/question_mark.rs:88:9 | LL | / if self.opt.is_none() { LL | | @@ -62,7 +62,7 @@ LL | | } | |_________^ help: replace it with: `self.opt.as_ref()?;` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:99:9 + --> tests/ui/question_mark.rs:97:9 | LL | / if self.opt.is_none() { LL | | @@ -71,7 +71,7 @@ LL | | } | |_________^ help: replace it with: `self.opt.as_ref()?;` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:108:9 + --> tests/ui/question_mark.rs:106:9 | LL | / if self.opt.is_none() { LL | | @@ -80,7 +80,7 @@ LL | | } | |_________^ help: replace it with: `self.opt.as_ref()?;` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:116:26 + --> tests/ui/question_mark.rs:114:26 | LL | let v: &Vec<_> = if let Some(ref v) = self.opt { | __________________________^ @@ -92,7 +92,7 @@ LL | | }; | |_________^ help: replace it with: `self.opt.as_ref()?` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:127:17 + --> tests/ui/question_mark.rs:125:17 | LL | let v = if let Some(v) = self.opt { | _________________^ @@ -104,7 +104,7 @@ LL | | }; | |_________^ help: replace it with: `self.opt?` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:149:5 + --> tests/ui/question_mark.rs:147:5 | LL | / if f().is_none() { LL | | @@ -113,7 +113,7 @@ LL | | } | |_____^ help: replace it with: `f()?;` error: this `match` expression can be replaced with `?` - --> tests/ui/question_mark.rs:154:16 + --> tests/ui/question_mark.rs:152:16 | LL | let _val = match f() { | ________________^ @@ -124,7 +124,7 @@ LL | | }; | |_____^ help: try instead: `f()?` error: this `match` expression can be replaced with `?` - --> tests/ui/question_mark.rs:165:5 + --> tests/ui/question_mark.rs:163:5 | LL | / match f() { LL | | @@ -134,7 +134,7 @@ LL | | }; | |_____^ help: try instead: `f()?` error: this `match` expression can be replaced with `?` - --> tests/ui/question_mark.rs:171:5 + --> tests/ui/question_mark.rs:169:5 | LL | / match opt_none!() { LL | | @@ -144,13 +144,13 @@ LL | | }; | |_____^ help: try instead: `opt_none!()?` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:198:13 + --> tests/ui/question_mark.rs:196:13 | LL | let _ = if let Ok(x) = x { x } else { return x }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x?` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:201:5 + --> tests/ui/question_mark.rs:199:5 | LL | / if x.is_err() { LL | | @@ -159,7 +159,7 @@ LL | | } | |_____^ help: replace it with: `x?;` error: this `match` expression can be replaced with `?` - --> tests/ui/question_mark.rs:206:16 + --> tests/ui/question_mark.rs:204:16 | LL | let _val = match func_returning_result() { | ________________^ @@ -170,7 +170,7 @@ LL | | }; | |_____^ help: try instead: `func_returning_result()?` error: this `match` expression can be replaced with `?` - --> tests/ui/question_mark.rs:212:5 + --> tests/ui/question_mark.rs:210:5 | LL | / match func_returning_result() { LL | | @@ -180,7 +180,7 @@ LL | | }; | |_____^ help: try instead: `func_returning_result()?` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:304:5 + --> tests/ui/question_mark.rs:302:5 | LL | / if let Err(err) = func_returning_result() { LL | | @@ -189,7 +189,7 @@ LL | | } | |_____^ help: replace it with: `func_returning_result()?;` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:312:5 + --> tests/ui/question_mark.rs:310:5 | LL | / if let Err(err) = func_returning_result() { LL | | @@ -198,7 +198,7 @@ LL | | } | |_____^ help: replace it with: `func_returning_result()?;` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:395:13 + --> tests/ui/question_mark.rs:393:13 | LL | / if a.is_none() { LL | | @@ -208,7 +208,7 @@ LL | | } | |_____________^ help: replace it with: `a?;` error: this `let...else` may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:456:5 + --> tests/ui/question_mark.rs:454:5 | LL | / let Some(v) = bar.foo.owned.clone() else { LL | | return None; @@ -216,7 +216,7 @@ LL | | }; | |______^ help: replace it with: `let v = bar.foo.owned.clone()?;` error: this `let...else` may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:471:5 + --> tests/ui/question_mark.rs:469:5 | LL | / let Some(ref x) = foo.opt_x else { LL | | return None; @@ -224,7 +224,7 @@ LL | | }; | |______^ help: replace it with: `let x = foo.opt_x.as_ref()?;` error: this `let...else` may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:481:5 + --> tests/ui/question_mark.rs:479:5 | LL | / let Some(ref mut x) = foo.opt_x else { LL | | return None; @@ -232,7 +232,7 @@ LL | | }; | |______^ help: replace it with: `let x = foo.opt_x.as_mut()?;` error: this `let...else` may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:492:5 + --> tests/ui/question_mark.rs:490:5 | LL | / let Some(ref x @ ref y) = foo.opt_x else { LL | | return None; @@ -240,7 +240,7 @@ LL | | }; | |______^ help: replace it with: `let x @ y = foo.opt_x.as_ref()?;` error: this `let...else` may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:496:5 + --> tests/ui/question_mark.rs:494:5 | LL | / let Some(ref x @ WrapperStructWithString(_)) = bar else { LL | | return None; @@ -248,7 +248,7 @@ LL | | }; | |______^ help: replace it with: `let x @ &WrapperStructWithString(_) = bar.as_ref()?;` error: this `let...else` may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:500:5 + --> tests/ui/question_mark.rs:498:5 | LL | / let Some(ref mut x @ WrapperStructWithString(_)) = bar else { LL | | return None; @@ -256,7 +256,7 @@ LL | | }; | |______^ help: replace it with: `let x @ &mut WrapperStructWithString(_) = bar.as_mut()?;` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:522:5 + --> tests/ui/question_mark.rs:520:5 | LL | / if arg.is_none() { LL | | @@ -265,7 +265,7 @@ LL | | } | |_____^ help: replace it with: `arg?;` error: this `match` expression can be replaced with `?` - --> tests/ui/question_mark.rs:526:15 + --> tests/ui/question_mark.rs:524:15 | LL | let val = match arg { | _______________^ @@ -276,12 +276,42 @@ LL | | }; | |_____^ help: try instead: `arg?` error: this `let...else` may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:536:5 + --> tests/ui/question_mark.rs:534:5 | LL | / let Some(a) = *a else { LL | | return None; LL | | }; | |______^ help: replace it with: `let a = (*a)?;` -error: aborting due to 30 previous errors +error: this `match` expression can be replaced with `?` + --> tests/ui/question_mark.rs:566:5 + | +LL | / match some_result { +LL | | +LL | | Ok(val) => val, +LL | | Err(err) => return Err(err.into()), +LL | | }; + | |_____^ help: try instead: `some_result?` + +error: this `match` expression can be replaced with `?` + --> tests/ui/question_mark.rs:572:5 + | +LL | / match some_result { +LL | | +LL | | Ok(val) => val, +LL | | Err(err) => return Err(Into::into(err)), +LL | | }; + | |_____^ help: try instead: `some_result?` + +error: this `match` expression can be replaced with `?` + --> tests/ui/question_mark.rs:578:5 + | +LL | / match some_result { +LL | | +LL | | Ok(val) => val, +LL | | Err(err) => return Err(<&str as Into<String>>::into(err)), +LL | | }; + | |_____^ help: try instead: `some_result?` + +error: aborting due to 33 previous errors |