From 6205bcf0f15038a74b9c156a541ea29479c50ffd Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Fri, 1 Nov 2024 19:30:37 +0000 Subject: [PATCH 1/2] Add match-based manual try to clippy::question_mark --- clippy_lints/src/question_mark.rs | 170 +++++++++++++++++++++++++++++- tests/ui/question_mark.fixed | 46 ++++++++ tests/ui/question_mark.rs | 61 +++++++++++ tests/ui/question_mark.stderr | 63 +++++++++-- 4 files changed, 327 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 28968866e..ce6297176 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -8,18 +8,18 @@ use clippy_utils::source::snippet_with_applicability; 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_to_local, path_to_local_id, peel_blocks, peel_blocks_with_stmt, - span_contains_comment, + 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, }; use rustc_errors::Applicability; use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk}; use rustc_hir::def::Res; use rustc_hir::{ - BindingMode, Block, Body, ByRef, Expr, ExprKind, LetStmt, Mutability, Node, PatKind, PathSegment, QPath, Stmt, - StmtKind, + Arm, BindingMode, Block, Body, ByRef, Expr, ExprKind, FnRetTy, HirId, LetStmt, MatchSource, Mutability, Node, Pat, + PatKind, PathSegment, QPath, Stmt, StmtKind, TyKind, }; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::Ty; +use rustc_middle::ty::{self, Ty}; use rustc_session::impl_lint_pass; use rustc_span::sym; use rustc_span::symbol::Symbol; @@ -58,6 +58,9 @@ pub struct QuestionMark { /// if it is greater than zero. /// As for why we need this in the first place: try_block_depth_stack: Vec, + /// Keeps track of the number of inferred return type closures we are inside, to avoid problems + /// with the `Err(x.into())` expansion being ambiguious. + inferred_ret_closure_stack: u16, } impl_lint_pass!(QuestionMark => [QUESTION_MARK, MANUAL_LET_ELSE]); @@ -68,6 +71,7 @@ impl QuestionMark { msrv: conf.msrv.clone(), matches_behaviour: conf.matches_for_let_else, try_block_depth_stack: Vec::new(), + inferred_ret_closure_stack: 0, } } } @@ -271,6 +275,135 @@ fn check_is_none_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr: &Ex } } +#[derive(Clone, Copy, Debug)] +enum TryMode { + Result, + Option, +} + +fn find_try_mode<'tcx>(cx: &LateContext<'tcx>, scrutinee: &Expr<'tcx>) -> Option { + let scrutinee_ty = cx.typeck_results().expr_ty_adjusted(scrutinee); + let ty::Adt(scrutinee_adt_def, _) = scrutinee_ty.kind() else { + return None; + }; + + match cx.tcx.get_diagnostic_name(scrutinee_adt_def.did())? { + sym::Result => Some(TryMode::Result), + sym::Option => Some(TryMode::Option), + _ => None, + } +} + +// Check that `pat` is `{ctor_lang_item}(val)`, returning `val`. +fn extract_ctor_call<'a, 'tcx>( + cx: &LateContext<'tcx>, + expected_ctor: LangItem, + pat: &'a Pat<'tcx>, +) -> Option<&'a Pat<'tcx>> { + if let PatKind::TupleStruct(variant_path, [val_binding], _) = &pat.kind + && is_res_lang_ctor(cx, cx.qpath_res(variant_path, pat.hir_id), expected_ctor) + { + Some(val_binding) + } else { + None + } +} + +// Extracts the local ID of a plain `val` pattern. +fn extract_binding_pat(pat: &Pat<'_>) -> Option { + if let PatKind::Binding(BindingMode::NONE, binding, _, None) = pat.kind { + Some(binding) + } else { + None + } +} + +fn check_arm_is_some_or_ok<'tcx>(cx: &LateContext<'tcx>, mode: TryMode, arm: &Arm<'tcx>) -> bool { + let happy_ctor = match mode { + TryMode::Result => ResultOk, + TryMode::Option => OptionSome, + }; + + // Check for `Ok(val)` or `Some(val)` + if arm.guard.is_none() + && let Some(val_binding) = extract_ctor_call(cx, happy_ctor, arm.pat) + // Extract out `val` + && let Some(binding) = extract_binding_pat(val_binding) + // Check body is just `=> val` + && path_to_local_id(peel_blocks(arm.body), binding) + { + true + } else { + false + } +} + +fn check_arm_is_none_or_err<'tcx>(cx: &LateContext<'tcx>, mode: TryMode, arm: &Arm<'tcx>) -> bool { + if arm.guard.is_some() { + return false; + } + + let arm_body = peel_blocks(arm.body); + match mode { + TryMode::Result => { + // Check that pat is Err(val) + if let Some(ok_pat) = extract_ctor_call(cx, ResultErr, arm.pat) + && let Some(ok_val) = extract_binding_pat(ok_pat) + // check `=> return Err(...)` + && 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) + { + true + } else { + false + } + }, + TryMode::Option => { + // Check the pat is `None` + if is_res_lang_ctor(cx, path_res(cx, arm.pat), OptionNone) + // Check `=> return None` + && let ExprKind::Ret(Some(ret_expr)) = arm_body.kind + && is_res_lang_ctor(cx, path_res(cx, ret_expr), OptionNone) + && !ret_expr.span.from_expansion() + { + true + } else { + false + } + }, + } +} + +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)) +} + +fn check_if_try_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + if let ExprKind::Match(scrutinee, [arm1, arm2], MatchSource::Normal | MatchSource::Postfix) = expr.kind + && !expr.span.from_expansion() + && let Some(mode) = find_try_mode(cx, scrutinee) + && !span_contains_cfg(cx, expr.span) + && check_arms_are_try(cx, mode, arm1, arm2) + { + let mut applicability = Applicability::MachineApplicable; + let snippet = snippet_with_applicability(cx, scrutinee.span.source_callsite(), "..", &mut applicability); + + span_lint_and_sugg( + cx, + QUESTION_MARK, + expr.span, + "this `match` expression can be replaced with `?`", + "try instead", + snippet.into_owned() + "?", + 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, @@ -339,6 +472,17 @@ fn is_try_block(cx: &LateContext<'_>, bl: &Block<'_>) -> bool { } } +fn is_inferred_ret_closure(expr: &Expr<'_>) -> bool { + let ExprKind::Closure(closure) = expr.kind else { + return false; + }; + + match closure.fn_decl.output { + FnRetTy::Return(ret_ty) => matches!(ret_ty.kind, TyKind::Infer), + FnRetTy::DefaultReturn(_) => true, + } +} + impl<'tcx> LateLintPass<'tcx> for QuestionMark { fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { if !is_lint_allowed(cx, QUESTION_MARK_USED, stmt.hir_id) { @@ -350,11 +494,27 @@ impl<'tcx> LateLintPass<'tcx> for QuestionMark { } self.check_manual_let_else(cx, stmt); } + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if is_inferred_ret_closure(expr) { + self.inferred_ret_closure_stack += 1; + return; + } + if !self.inside_try_block() && !is_in_const_context(cx) && 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); + + if self.inferred_ret_closure_stack == 0 { + check_if_try_match(cx, expr); + } + } + } + + fn check_expr_post(&mut self, _: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if is_inferred_ret_closure(expr) { + self.inferred_ret_closure_stack -= 1; } } diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed index 679388372..fb5bb953b 100644 --- a/tests/ui/question_mark.fixed +++ b/tests/ui/question_mark.fixed @@ -96,12 +96,42 @@ impl MoveStruct { } fn func() -> Option { + macro_rules! opt_none { + () => { + None + }; + } + fn f() -> Option { Some(String::new()) } f()?; + let _val = f()?; + + let s: &str = match &Some(String::new()) { + Some(v) => v, + None => return None, + }; + + f()?; + + opt_none!()?; + + match f() { + Some(x) => x, + None => return opt_none!(), + }; + + match f() { + Some(val) => { + println!("{val}"); + val + }, + None => return None, + }; + Some(0) } @@ -114,6 +144,10 @@ fn result_func(x: Result) -> Result { x?; + let _val = func_returning_result()?; + + func_returning_result()?; + // No warning let y = if let Ok(x) = x { x @@ -157,6 +191,18 @@ fn result_func(x: Result) -> Result { Ok(y) } +fn infer_check() { + let closure = |x: Result| { + // `?` would fail here, as it expands to `Err(val.into())` which is not constrained. + let _val = match x { + Ok(val) => val, + Err(val) => return Err(val), + }; + + Ok(()) + }; +} + // see issue #8019 pub enum NotOption { None, diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index 601ab78bf..f266bd9d5 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -124,6 +124,12 @@ impl MoveStruct { } fn func() -> Option { + macro_rules! opt_none { + () => { + None + }; + } + fn f() -> Option { Some(String::new()) } @@ -132,6 +138,39 @@ fn func() -> Option { return None; } + let _val = match f() { + Some(val) => val, + None => return None, + }; + + let s: &str = match &Some(String::new()) { + Some(v) => v, + None => return None, + }; + + match f() { + Some(val) => val, + None => return None, + }; + + match opt_none!() { + Some(x) => x, + None => return None, + }; + + match f() { + Some(x) => x, + None => return opt_none!(), + }; + + match f() { + Some(val) => { + println!("{val}"); + val + }, + None => return None, + }; + Some(0) } @@ -146,6 +185,16 @@ fn result_func(x: Result) -> Result { return x; } + let _val = match func_returning_result() { + Ok(val) => val, + Err(err) => return Err(err), + }; + + match func_returning_result() { + Ok(val) => val, + Err(err) => return Err(err), + }; + // No warning let y = if let Ok(x) = x { x @@ -189,6 +238,18 @@ fn result_func(x: Result) -> Result { Ok(y) } +fn infer_check() { + let closure = |x: Result| { + // `?` would fail here, as it expands to `Err(val.into())` which is not constrained. + let _val = match x { + Ok(val) => val, + Err(val) => return Err(val), + }; + + Ok(()) + }; +} + // see issue #8019 pub enum NotOption { None, diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index 5f26a7ea2..8ba5f5906 100644 --- a/tests/ui/question_mark.stderr +++ b/tests/ui/question_mark.stderr @@ -94,29 +94,76 @@ LL | | }; | |_________^ help: replace it with: `self.opt?` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:131:5 + --> tests/ui/question_mark.rs:137:5 | LL | / if f().is_none() { LL | | return None; LL | | } | |_____^ help: replace it with: `f()?;` +error: this `match` expression can be replaced with `?` + --> tests/ui/question_mark.rs:141:16 + | +LL | let _val = match f() { + | ________________^ +LL | | Some(val) => val, +LL | | None => return None, +LL | | }; + | |_____^ help: try instead: `f()?` + +error: this `match` expression can be replaced with `?` + --> tests/ui/question_mark.rs:151:5 + | +LL | / match f() { +LL | | Some(val) => val, +LL | | None => return None, +LL | | }; + | |_____^ help: try instead: `f()?` + +error: this `match` expression can be replaced with `?` + --> tests/ui/question_mark.rs:156:5 + | +LL | / match opt_none!() { +LL | | Some(x) => x, +LL | | None => return None, +LL | | }; + | |_____^ help: try instead: `opt_none!()?` + error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:143:13 + --> tests/ui/question_mark.rs:182: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:145:5 + --> tests/ui/question_mark.rs:184:5 | LL | / if x.is_err() { LL | | return x; LL | | } | |_____^ help: replace it with: `x?;` +error: this `match` expression can be replaced with `?` + --> tests/ui/question_mark.rs:188:16 + | +LL | let _val = match func_returning_result() { + | ________________^ +LL | | Ok(val) => val, +LL | | Err(err) => return Err(err), +LL | | }; + | |_____^ help: try instead: `func_returning_result()?` + +error: this `match` expression can be replaced with `?` + --> tests/ui/question_mark.rs:193:5 + | +LL | / match func_returning_result() { +LL | | Ok(val) => val, +LL | | Err(err) => return Err(err), +LL | | }; + | |_____^ help: try instead: `func_returning_result()?` + error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:213:5 + --> tests/ui/question_mark.rs:274:5 | LL | / if let Err(err) = func_returning_result() { LL | | return Err(err); @@ -124,7 +171,7 @@ LL | | } | |_____^ help: replace it with: `func_returning_result()?;` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:220:5 + --> tests/ui/question_mark.rs:281:5 | LL | / if let Err(err) = func_returning_result() { LL | | return Err(err); @@ -132,7 +179,7 @@ LL | | } | |_____^ help: replace it with: `func_returning_result()?;` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:297:13 + --> tests/ui/question_mark.rs:358:13 | LL | / if a.is_none() { LL | | return None; @@ -142,12 +189,12 @@ LL | | } | |_____________^ help: replace it with: `a?;` error: this `let...else` may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:357:5 + --> tests/ui/question_mark.rs:418:5 | LL | / let Some(v) = bar.foo.owned.clone() else { LL | | return None; LL | | }; | |______^ help: replace it with: `let v = bar.foo.owned.clone()?;` -error: aborting due to 17 previous errors +error: aborting due to 22 previous errors From 139bb25927f75c53e05d0c36dc9cfbc8081750c5 Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Sat, 2 Nov 2024 13:41:16 +0000 Subject: [PATCH 2/2] Avoid linting for closures with inferred return types --- clippy_lints/src/question_mark.rs | 4 ++-- tests/ui/question_mark.fixed | 10 ++++++++++ tests/ui/question_mark.rs | 10 ++++++++++ tests/ui/question_mark.stderr | 8 ++++---- 4 files changed, 26 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index ce6297176..7d1ba45c3 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -16,7 +16,7 @@ use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk}; use rustc_hir::def::Res; use rustc_hir::{ Arm, BindingMode, Block, Body, ByRef, Expr, ExprKind, FnRetTy, HirId, LetStmt, MatchSource, Mutability, Node, Pat, - PatKind, PathSegment, QPath, Stmt, StmtKind, TyKind, + PatKind, PathSegment, QPath, Stmt, StmtKind, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self, Ty}; @@ -478,7 +478,7 @@ fn is_inferred_ret_closure(expr: &Expr<'_>) -> bool { }; match closure.fn_decl.output { - FnRetTy::Return(ret_ty) => matches!(ret_ty.kind, TyKind::Infer), + FnRetTy::Return(ret_ty) => ret_ty.is_suggestable_infer_ty(), FnRetTy::DefaultReturn(_) => true, } } diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed index fb5bb953b..b6e148e9f 100644 --- a/tests/ui/question_mark.fixed +++ b/tests/ui/question_mark.fixed @@ -201,6 +201,16 @@ fn infer_check() { Ok(()) }; + + let closure = |x: Result| -> Result<(), _> { + // `?` would fail here, as it expands to `Err(val.into())` which is not constrained. + let _val = match x { + Ok(val) => val, + Err(val) => return Err(val), + }; + + Ok(()) + }; } // see issue #8019 diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index f266bd9d5..48dc9eb0a 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -248,6 +248,16 @@ fn infer_check() { Ok(()) }; + + let closure = |x: Result| -> Result<(), _> { + // `?` would fail here, as it expands to `Err(val.into())` which is not constrained. + let _val = match x { + Ok(val) => val, + Err(val) => return Err(val), + }; + + Ok(()) + }; } // see issue #8019 diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index 8ba5f5906..0a48c4e80 100644 --- a/tests/ui/question_mark.stderr +++ b/tests/ui/question_mark.stderr @@ -163,7 +163,7 @@ LL | | }; | |_____^ help: try instead: `func_returning_result()?` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:274:5 + --> tests/ui/question_mark.rs:284:5 | LL | / if let Err(err) = func_returning_result() { LL | | return Err(err); @@ -171,7 +171,7 @@ LL | | } | |_____^ help: replace it with: `func_returning_result()?;` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:281:5 + --> tests/ui/question_mark.rs:291:5 | LL | / if let Err(err) = func_returning_result() { LL | | return Err(err); @@ -179,7 +179,7 @@ LL | | } | |_____^ help: replace it with: `func_returning_result()?;` error: this block may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:358:13 + --> tests/ui/question_mark.rs:368:13 | LL | / if a.is_none() { LL | | return None; @@ -189,7 +189,7 @@ LL | | } | |_____________^ help: replace it with: `a?;` error: this `let...else` may be rewritten with the `?` operator - --> tests/ui/question_mark.rs:418:5 + --> tests/ui/question_mark.rs:428:5 | LL | / let Some(v) = bar.foo.owned.clone() else { LL | | return None;