From 0671d782836665600666223f3c9566a1d36559b4 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Fri, 23 Feb 2024 19:16:41 +0100 Subject: [PATCH 1/2] check for try blocks in `LintPass` methods --- clippy_lints/src/question_mark.rs | 10 ++++------ tests/ui/question_mark.fixed | 10 ++++++++++ tests/ui/question_mark.rs | 10 ++++++++++ 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 1df2d5e4602..6adbc4a23af 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -224,8 +224,7 @@ fn inside_try_block(&self) -> bool { /// /// If it matches, it will suggest to use the question mark operator instead fn check_is_none_or_err_and_early_return<'tcx>(&self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { - if !self.inside_try_block() - && let Some(higher::If { cond, then, r#else }) = higher::If::hir(expr) + if let Some(higher::If { cond, then, r#else }) = higher::If::hir(expr) && !is_else_clause(cx.tcx, expr) && let ExprKind::MethodCall(segment, caller, ..) = &cond.kind && let caller_ty = cx.typeck_results().expr_ty(caller) @@ -259,8 +258,7 @@ fn check_is_none_or_err_and_early_return<'tcx>(&self, cx: &LateContext<'tcx>, ex } fn check_if_let_some_or_err_and_early_return<'tcx>(&self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { - if !self.inside_try_block() - && let Some(higher::IfLet { + if let Some(higher::IfLet { let_pat, let_expr, if_then, @@ -324,13 +322,13 @@ fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { return; } - if !in_constant(cx, stmt.hir_id) { + if !self.inside_try_block() && !in_constant(cx, stmt.hir_id) { check_let_some_else_return_none(cx, stmt); } self.check_manual_let_else(cx, stmt); } fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if !in_constant(cx, expr.hir_id) && is_lint_allowed(cx, QUESTION_MARK_USED, expr.hir_id) { + if !self.inside_try_block() && !in_constant(cx, expr.hir_id) && is_lint_allowed(cx, QUESTION_MARK_USED, expr.hir_id) { self.check_is_none_or_err_and_early_return(cx, expr); self.check_if_let_some_or_err_and_early_return(cx, expr); } diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed index 2ef006c1419..567472a8af2 100644 --- a/tests/ui/question_mark.fixed +++ b/tests/ui/question_mark.fixed @@ -273,3 +273,13 @@ const fn issue9175(option: Option<()>) -> Option<()> { //stuff Some(()) } + +fn issue12337() -> Option { + let _: Option = try { + let Some(_) = Some(42) else { + return None; + }; + 123 + }; + Some(42) +} diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index c170669823f..abf8c270de8 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -313,3 +313,13 @@ const fn issue9175(option: Option<()>) -> Option<()> { //stuff Some(()) } + +fn issue12337() -> Option { + let _: Option = try { + let Some(_) = Some(42) else { + return None; + }; + 123 + }; + Some(42) +} From 1430623e0470fa90bf6415f18225ecac88e0cba8 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Fri, 23 Feb 2024 19:21:54 +0100 Subject: [PATCH 2/2] move methods out of impl and remove unused `&self` param --- clippy_lints/src/question_mark.rs | 203 +++++++++++++++--------------- tests/ui/manual_let_else.rs | 10 ++ tests/ui/manual_let_else.stderr | 68 +++++----- 3 files changed, 150 insertions(+), 131 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 6adbc4a23af..cf7f730140c 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -203,107 +203,107 @@ fn expr_return_none_or_err( } } +/// Checks if the given expression on the given context matches the following structure: +/// +/// ```ignore +/// if option.is_none() { +/// return None; +/// } +/// ``` +/// +/// ```ignore +/// if result.is_err() { +/// return result; +/// } +/// ``` +/// +/// If it matches, it will suggest to use the question mark operator instead +fn check_is_none_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + if let Some(higher::If { cond, then, r#else }) = higher::If::hir(expr) + && !is_else_clause(cx.tcx, expr) + && let ExprKind::MethodCall(segment, caller, ..) = &cond.kind + && let caller_ty = cx.typeck_results().expr_ty(caller) + && let if_block = IfBlockType::IfIs(caller, caller_ty, segment.ident.name, then) + && (is_early_return(sym::Option, cx, &if_block) || is_early_return(sym::Result, cx, &if_block)) + { + let mut applicability = Applicability::MachineApplicable; + let receiver_str = snippet_with_applicability(cx, caller.span, "..", &mut applicability); + let by_ref = !caller_ty.is_copy_modulo_regions(cx.tcx, cx.param_env) + && !matches!(caller.kind, ExprKind::Call(..) | ExprKind::MethodCall(..)); + let sugg = if let Some(else_inner) = r#else { + if eq_expr_value(cx, caller, peel_blocks(else_inner)) { + format!("Some({receiver_str}?)") + } else { + return; + } + } else { + format!("{receiver_str}{}?;", if by_ref { ".as_ref()" } else { "" }) + }; + + span_lint_and_sugg( + cx, + QUESTION_MARK, + expr.span, + "this block may be rewritten with the `?` operator", + "replace it with", + sugg, + applicability, + ); + } +} + +fn check_if_let_some_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + if let Some(higher::IfLet { + let_pat, + let_expr, + if_then, + if_else, + .. + }) = higher::IfLet::hir(cx, expr) + && !is_else_clause(cx.tcx, expr) + && let PatKind::TupleStruct(ref path1, [field], ddpos) = let_pat.kind + && ddpos.as_opt_usize().is_none() + && let PatKind::Binding(BindingAnnotation(by_ref, _), bind_id, ident, None) = field.kind + && let caller_ty = cx.typeck_results().expr_ty(let_expr) + && let if_block = IfBlockType::IfLet( + cx.qpath_res(path1, let_pat.hir_id), + caller_ty, + ident.name, + let_expr, + if_then, + if_else, + ) + && ((is_early_return(sym::Option, cx, &if_block) && path_to_local_id(peel_blocks(if_then), bind_id)) + || is_early_return(sym::Result, cx, &if_block)) + && if_else + .map(|e| eq_expr_value(cx, let_expr, peel_blocks(e))) + .filter(|e| *e) + .is_none() + { + let mut applicability = Applicability::MachineApplicable; + let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability); + let requires_semi = matches!(cx.tcx.parent_hir_node(expr.hir_id), Node::Stmt(_)); + let sugg = format!( + "{receiver_str}{}?{}", + if by_ref == ByRef::Yes { ".as_ref()" } else { "" }, + if requires_semi { ";" } else { "" } + ); + span_lint_and_sugg( + cx, + QUESTION_MARK, + expr.span, + "this block may be rewritten with the `?` operator", + "replace it with", + sugg, + applicability, + ); + } +} + impl QuestionMark { fn inside_try_block(&self) -> bool { self.try_block_depth_stack.last() > Some(&0) } - - /// Checks if the given expression on the given context matches the following structure: - /// - /// ```ignore - /// if option.is_none() { - /// return None; - /// } - /// ``` - /// - /// ```ignore - /// if result.is_err() { - /// return result; - /// } - /// ``` - /// - /// If it matches, it will suggest to use the question mark operator instead - fn check_is_none_or_err_and_early_return<'tcx>(&self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { - if let Some(higher::If { cond, then, r#else }) = higher::If::hir(expr) - && !is_else_clause(cx.tcx, expr) - && let ExprKind::MethodCall(segment, caller, ..) = &cond.kind - && let caller_ty = cx.typeck_results().expr_ty(caller) - && let if_block = IfBlockType::IfIs(caller, caller_ty, segment.ident.name, then) - && (is_early_return(sym::Option, cx, &if_block) || is_early_return(sym::Result, cx, &if_block)) - { - let mut applicability = Applicability::MachineApplicable; - let receiver_str = snippet_with_applicability(cx, caller.span, "..", &mut applicability); - let by_ref = !caller_ty.is_copy_modulo_regions(cx.tcx, cx.param_env) - && !matches!(caller.kind, ExprKind::Call(..) | ExprKind::MethodCall(..)); - let sugg = if let Some(else_inner) = r#else { - if eq_expr_value(cx, caller, peel_blocks(else_inner)) { - format!("Some({receiver_str}?)") - } else { - return; - } - } else { - format!("{receiver_str}{}?;", if by_ref { ".as_ref()" } else { "" }) - }; - - span_lint_and_sugg( - cx, - QUESTION_MARK, - expr.span, - "this block may be rewritten with the `?` operator", - "replace it with", - sugg, - applicability, - ); - } - } - - fn check_if_let_some_or_err_and_early_return<'tcx>(&self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { - if let Some(higher::IfLet { - let_pat, - let_expr, - if_then, - if_else, - .. - }) = higher::IfLet::hir(cx, expr) - && !is_else_clause(cx.tcx, expr) - && let PatKind::TupleStruct(ref path1, [field], ddpos) = let_pat.kind - && ddpos.as_opt_usize().is_none() - && let PatKind::Binding(BindingAnnotation(by_ref, _), bind_id, ident, None) = field.kind - && let caller_ty = cx.typeck_results().expr_ty(let_expr) - && let if_block = IfBlockType::IfLet( - cx.qpath_res(path1, let_pat.hir_id), - caller_ty, - ident.name, - let_expr, - if_then, - if_else, - ) - && ((is_early_return(sym::Option, cx, &if_block) && path_to_local_id(peel_blocks(if_then), bind_id)) - || is_early_return(sym::Result, cx, &if_block)) - && if_else - .map(|e| eq_expr_value(cx, let_expr, peel_blocks(e))) - .filter(|e| *e) - .is_none() - { - let mut applicability = Applicability::MachineApplicable; - let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability); - let requires_semi = matches!(cx.tcx.parent_hir_node(expr.hir_id), Node::Stmt(_)); - let sugg = format!( - "{receiver_str}{}?{}", - if by_ref == ByRef::Yes { ".as_ref()" } else { "" }, - if requires_semi { ";" } else { "" } - ); - span_lint_and_sugg( - cx, - QUESTION_MARK, - expr.span, - "this block may be rewritten with the `?` operator", - "replace it with", - sugg, - applicability, - ); - } - } } fn is_try_block(cx: &LateContext<'_>, bl: &rustc_hir::Block<'_>) -> bool { @@ -328,9 +328,12 @@ fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { self.check_manual_let_else(cx, stmt); } fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if !self.inside_try_block() && !in_constant(cx, expr.hir_id) && is_lint_allowed(cx, QUESTION_MARK_USED, expr.hir_id) { - self.check_is_none_or_err_and_early_return(cx, expr); - self.check_if_let_some_or_err_and_early_return(cx, expr); + if !self.inside_try_block() + && !in_constant(cx, expr.hir_id) + && is_lint_allowed(cx, QUESTION_MARK_USED, expr.hir_id) + { + check_is_none_or_err_and_early_return(cx, expr); + check_if_let_some_or_err_and_early_return(cx, expr); } } diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs index 5d94660ec89..1fb252e3f97 100644 --- a/tests/ui/manual_let_else.rs +++ b/tests/ui/manual_let_else.rs @@ -1,3 +1,4 @@ +#![feature(try_blocks)] #![allow(unused_braces, unused_variables, dead_code)] #![allow( clippy::collapsible_else_if, @@ -446,3 +447,12 @@ struct U { w: T, x: T, } + +fn issue12337() { + // We want to generally silence question_mark lints within try blocks, since `?` has different + // behavior to `return`, and question_mark calls into manual_let_else logic, so make sure that + // we still emit a lint for manual_let_else + let _: Option<()> = try { + let v = if let Some(v_some) = g() { v_some } else { return }; + }; +} diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr index b6433fc460c..7012c6b8891 100644 --- a/tests/ui/manual_let_else.stderr +++ b/tests/ui/manual_let_else.stderr @@ -1,5 +1,5 @@ error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:27:5 + --> tests/ui/manual_let_else.rs:28:5 | LL | let v = if let Some(v_some) = g() { v_some } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };` @@ -8,7 +8,7 @@ LL | let v = if let Some(v_some) = g() { v_some } else { return }; = help: to override `-D warnings` add `#[allow(clippy::manual_let_else)]` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:30:5 + --> tests/ui/manual_let_else.rs:31:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -26,7 +26,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:37:5 + --> tests/ui/manual_let_else.rs:38:5 | LL | / let v = if let Some(v) = g() { LL | | @@ -47,25 +47,25 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:49:9 + --> tests/ui/manual_let_else.rs:50:9 | LL | let v = if let Some(v_some) = g() { v_some } else { continue }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { continue };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:51:9 + --> tests/ui/manual_let_else.rs:52:9 | LL | let v = if let Some(v_some) = g() { v_some } else { break }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { break };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:56:5 + --> tests/ui/manual_let_else.rs:57:5 | LL | let v = if let Some(v_some) = g() { v_some } else { panic!() }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { panic!() };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:60:5 + --> tests/ui/manual_let_else.rs:61:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -83,7 +83,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:68:5 + --> tests/ui/manual_let_else.rs:69:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -101,7 +101,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:76:5 + --> tests/ui/manual_let_else.rs:77:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -121,7 +121,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:85:5 + --> tests/ui/manual_let_else.rs:86:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -141,7 +141,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:94:5 + --> tests/ui/manual_let_else.rs:95:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -168,7 +168,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:110:5 + --> tests/ui/manual_let_else.rs:111:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -190,7 +190,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:121:5 + --> tests/ui/manual_let_else.rs:122:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -217,7 +217,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:137:5 + --> tests/ui/manual_let_else.rs:138:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -239,7 +239,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:148:5 + --> tests/ui/manual_let_else.rs:149:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -257,7 +257,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:156:5 + --> tests/ui/manual_let_else.rs:157:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -278,7 +278,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:166:5 + --> tests/ui/manual_let_else.rs:167:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -299,7 +299,7 @@ LL + } }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:176:5 + --> tests/ui/manual_let_else.rs:177:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -328,7 +328,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:194:5 + --> tests/ui/manual_let_else.rs:195:5 | LL | / let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) { LL | | @@ -346,7 +346,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:202:5 + --> tests/ui/manual_let_else.rs:203:5 | LL | / let (w, S { v }) = if let (Some(v_some), w_some) = (g().map(|_| S { v: 0 }), 0) { LL | | @@ -364,7 +364,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:212:13 + --> tests/ui/manual_let_else.rs:213:13 | LL | let $n = if let Some(v) = $e { v } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some($n) = g() else { return };` @@ -375,19 +375,19 @@ LL | create_binding_if_some!(w, g()); = note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info) error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:221:5 + --> tests/ui/manual_let_else.rs:222:5 | LL | let v = if let Variant::A(a, 0) = e() { a } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(v, 0) = e() else { return };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:225:5 + --> tests/ui/manual_let_else.rs:226:5 | LL | let mut v = if let Variant::B(b) = e() { b } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::B(mut v) = e() else { return };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:230:5 + --> tests/ui/manual_let_else.rs:231:5 | LL | / let v = if let Ok(Some(Variant::B(b))) | Err(Some(Variant::A(b, _))) = nested { LL | | @@ -405,19 +405,19 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:237:5 + --> tests/ui/manual_let_else.rs:238:5 | LL | let v = if let Variant::A(.., a) = e() { a } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(.., v) = e() else { return };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:241:5 + --> tests/ui/manual_let_else.rs:242:5 | LL | let w = if let (Some(v), ()) = (g(), ()) { v } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let (Some(w), ()) = (g(), ()) else { return };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:245:5 + --> tests/ui/manual_let_else.rs:246:5 | LL | / let w = if let Some(S { v: x }) = Some(S { v: 0 }) { LL | | @@ -435,7 +435,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:253:5 + --> tests/ui/manual_let_else.rs:254:5 | LL | / let v = if let Some(S { v: x }) = Some(S { v: 0 }) { LL | | @@ -453,7 +453,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:261:5 + --> tests/ui/manual_let_else.rs:262:5 | LL | / let (x, S { v }, w) = if let Some(U { v, w, x }) = None::>> { LL | | @@ -471,7 +471,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:378:5 + --> tests/ui/manual_let_else.rs:379:5 | LL | / let _ = match ff { LL | | @@ -480,5 +480,11 @@ LL | | _ => macro_call!(), LL | | }; | |______^ help: consider writing: `let Some(_) = ff else { macro_call!() };` -error: aborting due to 30 previous errors +error: this could be rewritten as `let...else` + --> tests/ui/manual_let_else.rs:456:9 + | +LL | let v = if let Some(v_some) = g() { v_some } else { return }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };` + +error: aborting due to 31 previous errors