Add match-based manual try to clippy::question_mark

Author: GnomedDev

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: <https://github.com/rust-lang/rust-clippy/issues/8628>
 try_block_depth_stack:Vec<u32>,
+/// 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)]
+enumTryMode{
+Result,
+Option,
+}
+
+fnfind_try_mode<'tcx>(cx:&LateContext<'tcx>,scrutinee:&Expr<'tcx>) -> Option<TryMode>{
+let scrutinee_ty = cx.typeck_results().expr_ty_adjusted(scrutinee);
+let ty::Adt(scrutinee_adt_def, _) = scrutinee_ty.kind()else{
+returnNone;
+};
+
+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`.
+fnextract_ctor_call<'a,'tcx>(
+cx:&LateContext<'tcx>,
+expected_ctor:LangItem,
+pat:&'aPat<'tcx>,
+) -> Option<&'aPat<'tcx>>{
+ifletPatKind::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.
+fnextract_binding_pat(pat:&Pat<'_>) -> Option<HirId>{
+ifletPatKind::Binding(BindingMode::NONE, binding, _,None) = pat.kind{
+Some(binding)
+}else{
+None
+}
+}
+
+fncheck_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()
+ && letSome(val_binding) = extract_ctor_call(cx, happy_ctor, arm.pat)
+// Extract out `val`
+ && letSome(binding) = extract_binding_pat(val_binding)
+// Check body is just `=> val`
+ && path_to_local_id(peel_blocks(arm.body), binding)
+{
+true
+}else{
+false
+}
+}
+
+fncheck_arm_is_none_or_err<'tcx>(cx:&LateContext<'tcx>,mode:TryMode,arm:&Arm<'tcx>) -> bool{
+if arm.guard.is_some(){
+returnfalse;
+}
+
+let arm_body = peel_blocks(arm.body);
+match mode {
+TryMode::Result => {
+// Check that pat is Err(val)
+ifletSome(ok_pat) = extract_ctor_call(cx,ResultErr, arm.pat)
+ && letSome(ok_val) = extract_binding_pat(ok_pat)
+// check `=> return Err(...)`
+ && letExprKind::Ret(Some(wrapped_ret_expr)) = arm_body.kind
+ && letExprKind::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`
+ifis_res_lang_ctor(cx,path_res(cx, arm.pat),OptionNone)
+// Check `=> return None`
+ && letExprKind::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
+}
+},
+}
+}
+
+fncheck_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))
+}
+
+fncheck_if_try_match<'tcx>(cx:&LateContext<'tcx>,expr:&Expr<'tcx>){
+ifletExprKind::Match(scrutinee,[arm1, arm2],MatchSource::Normal | MatchSource::Postfix) = expr.kind
+ && !expr.span.from_expansion()
+ && letSome(mode) = find_try_mode(cx, scrutinee)
+ && !span_contains_cfg(cx, expr.span)
+ && check_arms_are_try(cx, mode, arm1, arm2)
+{
+letmut 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,
+);
+}
+}
+
 fncheck_if_let_some_or_err_and_early_return<'tcx>(cx:&LateContext<'tcx>,expr:&Expr<'tcx>){
 ifletSome(higher::IfLet{
  let_pat,
@@ -339,6 +472,17 @@ fn is_try_block(cx: &LateContext<'_>, bl: &Block<'_>) -> bool {
 }
 }
 
+fnis_inferred_ret_closure(expr:&Expr<'_>) -> bool{
+letExprKind::Closure(closure) = expr.kindelse{
+returnfalse;
+};
+
+match closure.fn_decl.output{
+FnRetTy::Return(ret_ty) => matches!(ret_ty.kind,TyKind::Infer),
+FnRetTy::DefaultReturn(_) => true,
+}
+}
+
 impl<'tcx>LateLintPass<'tcx>forQuestionMark{
 fncheck_stmt(&mutself,cx:&LateContext<'tcx>,stmt:&'tcxStmt<'_>){
 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);
 }
+
 fncheck_expr(&mutself,cx:&LateContext<'tcx>,expr:&'tcxExpr<'_>){
+ifis_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);
+
+ifself.inferred_ret_closure_stack == 0{
+check_if_try_match(cx, expr);
+}
+}
+}
+
+fncheck_expr_post(&mutself, _:&LateContext<'tcx>,expr:&'tcxExpr<'tcx>){
+ifis_inferred_ret_closure(expr){
+self.inferred_ret_closure_stack -= 1;
 }
 }
 

tests/ui/question_mark.fixed

@@ -96,12 +96,42 @@ impl MoveStruct {
 }
 
 fn func() -> Option<i32> {
+ macro_rules! opt_none {
+ () => {
+ None
+ };
+ }
+
  fn f() -> Option<String> {
  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<i32, i32>) -> Result<i32, i32> {
 
  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<i32, i32>) -> Result<i32, i32> {
  Ok(y)
 }
 
+fn infer_check() {
+ let closure = |x: Result<u8, ()>| {
+ // `?` 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,

tests/ui/question_mark.rs

@@ -124,6 +124,12 @@ impl MoveStruct {
 }
 
 fnfunc() -> Option<i32>{
+macro_rules! opt_none {
+() => {
+None
+};
+}
+
 fnf() -> Option<String>{
 Some(String::new())
 }
@@ -132,6 +138,39 @@ fn func() -> Option<i32> {
 returnNone;
 }
 
+let _val = matchf(){
+Some(val) => val,
+None => returnNone,
+};
+
+let s:&str = match&Some(String::new()){
+Some(v) => v,
+None => returnNone,
+};
+
+matchf(){
+Some(val) => val,
+None => returnNone,
+};
+
+matchopt_none!(){
+Some(x) => x,
+None => returnNone,
+};
+
+matchf(){
+Some(x) => x,
+None => returnopt_none!(),
+};
+
+matchf(){
+Some(val) => {
+println!("{val}");
+ val
+},
+None => returnNone,
+};
+
 Some(0)
 }
 
@@ -146,6 +185,16 @@ fn result_func(x: Result<i32, i32>) -> Result<i32, i32> {
 return x;
 }
 
+let _val = matchfunc_returning_result(){
+Ok(val) => val,
+Err(err) => returnErr(err),
+};
+
+matchfunc_returning_result(){
+Ok(val) => val,
+Err(err) => returnErr(err),
+};
+
 // No warning
 let y = ifletOk(x) = x {
  x
@@ -189,6 +238,18 @@ fn result_func(x: Result<i32, i32>) -> Result<i32, i32> {
 Ok(y)
 }
 
+fninfer_check(){
+let closure = |x:Result<u8,()>| {
+// `?` would fail here, as it expands to `Err(val.into())` which is not constrained.
+let _val = match x {
+Ok(val) => val,
+Err(val) => returnErr(val),
+};
+
+Ok(())
+};
+}
+
 // see issue #8019
 pubenumNotOption{
 None,