Detect incorrect ; in Option::ok_or_else and Result::map_err

Fix rust-lang#72124.

Author: estebank

compiler/rustc_span/src/symbol.rs

@@ -988,6 +988,7 @@ symbols! {
         managed_boxes,
         manually_drop,
         map,
+        map_err,
         marker,
         marker_trait_attr,
         masked,
@@ -1152,6 +1153,7 @@ symbols! {
         offset,
         offset_of,
         offset_of_enum,
+        ok_or_else,
         omit_gdb_pretty_printer_section,
         on,
         on_unimplemented,

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

@@ -41,7 +41,7 @@ use rustc_session::config::{DumpSolverProofTree, TraitSolver};
 use rustc_session::Limit;
 use rustc_span::def_id::LOCAL_CRATE;
 use rustc_span::symbol::sym;
-use rustc_span::{BytePos, ExpnKind, Span, DUMMY_SP};
+use rustc_span::{BytePos, ExpnKind, Span, Symbol, DUMMY_SP};
 use std::borrow::Cow;
 use std::fmt;
 use std::iter;
@@ -1008,6 +1008,7 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
             return;
         }
         let self_ty = trait_ref.self_ty();
+        let found_ty = trait_ref.args.get(1).and_then(|a| a.as_type());
 
         let mut prev_ty = self.resolve_vars_if_possible(
             typeck.expr_ty_adjusted_opt(expr).unwrap_or(Ty::new_misc_error(self.tcx)),
@@ -1033,17 +1034,76 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
 
         // The following logic is simlar to `point_at_chain`, but that's focused on associated types
         let mut expr = expr;
-        while let hir::ExprKind::MethodCall(_path_segment, rcvr_expr, _args, span) = expr.kind {
+        while let hir::ExprKind::MethodCall(path_segment, rcvr_expr, args, span) = expr.kind {
             // Point at every method call in the chain with the `Result` type.
             // let foo = bar.iter().map(mapper)?;
             //               ------ -----------
             expr = rcvr_expr;
             chain.push((span, prev_ty));
 
-            prev_ty = self.resolve_vars_if_possible(
+            let next_ty = self.resolve_vars_if_possible(
                 typeck.expr_ty_adjusted_opt(expr).unwrap_or(Ty::new_misc_error(self.tcx)),
             );
 
+            let is_diagnostic_item = |symbol: Symbol, ty: Ty<'tcx>| {
+                let ty::Adt(def, _) = ty.kind() else {
+                    return false;
+                };
+                self.tcx.is_diagnostic_item(symbol, def.did())
+            };
+            // For each method in the chain, see if this is `Result::map_err` or
+            // `Option::ok_or_else` and if it is, see if the closure passed to it has an incorrect
+            // trailing `;`.
+            // Ideally we would instead use `FnCtxt::lookup_method_for_diagnostic` for 100%
+            // accurate check, but we are in the wrong stage to do that and looking for
+            // `Result::map_err` by checking the Self type and the path segment is enough.
+            // sym::ok_or_else
+            if let Some(ty) = get_e_type(prev_ty)
+                && let Some(found_ty) = found_ty
+                && (
+                    (
+                        path_segment.ident.name == sym::map_err
+                            && is_diagnostic_item(sym::Result, next_ty)
+                    ) || (
+                        path_segment.ident.name == sym::ok_or_else
+                            && is_diagnostic_item(sym::Option, next_ty)
+                    )
+                )
+                && [sym::map_err, sym::ok_or_else].contains(&path_segment.ident.name)
+                && let ty::Tuple(tys) = found_ty.kind()
+                && tys.is_empty()
+                && self.can_eq(obligation.param_env, ty, found_ty)
+                && args.len() == 1
+                && let Some(arg) = args.get(0)
+                && let hir::ExprKind::Closure(closure) = arg.kind
+                && let body = self.tcx.hir().body(closure.body)
+                && let hir::ExprKind::Block(block, _) = body.value.kind
+                && let None = block.expr
+                && let [.., stmt] = block.stmts
+                && let hir::StmtKind::Semi(expr) = stmt.kind
+                && let expr_ty = self.resolve_vars_if_possible(
+                    typeck.expr_ty_adjusted_opt(expr)
+                        .unwrap_or(Ty::new_misc_error(self.tcx)),
+                )
+                && self
+                    .infcx
+                    .type_implements_trait(
+                        self.tcx.get_diagnostic_item(sym::From).unwrap(),
+                        [self_ty, expr_ty],
+                        obligation.param_env,
+                    )
+                    .must_apply_modulo_regions()
+            {
+                err.span_suggestion_short(
+                    stmt.span.with_lo(expr.span.hi()),
+                    "remove this semicolon",
+                    String::new(),
+                    Applicability::MachineApplicable,
+                );
+            }
+
+            prev_ty = next_ty;
+
             if let hir::ExprKind::Path(hir::QPath::Resolved(None, path)) = expr.kind
                 && let hir::Path { res: hir::def::Res::Local(hir_id), .. } = path
                 && let Some(hir::Node::Pat(binding)) = self.tcx.hir().find(*hir_id)

tests/ui/traits/question-mark-result-err-mismatch.rs

@@ -8,7 +8,9 @@ fn foo() -> Result<String, String> { //~ NOTE expected `String` because of this
         });
     let one = x
         .map(|s| ())
-        .map_err(|_| ()) //~ NOTE this can't be annotated with `?` because it has type `Result<_, ()>`
+        .map_err(|e| { //~ NOTE this can't be annotated with `?` because it has type `Result<_, ()>`
+            e; //~ HELP remove this semicolon
+        })
         .map(|()| "")?; //~ ERROR `?` couldn't convert the error to `String`
     //~^ NOTE in this expansion of desugaring of operator `?`
     //~| NOTE in this expansion of desugaring of operator `?`
@@ -17,6 +19,7 @@ fn foo() -> Result<String, String> { //~ NOTE expected `String` because of this
     //~| NOTE the trait `From<()>` is not implemented for `String`
     //~| NOTE the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
     //~| NOTE required for `Result<String, String>` to implement `FromResidual<Result<Infallible, ()>>`
+    //~| HELP the following other types implement trait `From<T>`:
     Ok(one.to_string())
 }
 
@@ -33,6 +36,7 @@ fn bar() -> Result<(), String> { //~ NOTE expected `String` because of this
     //~| NOTE the trait `From<()>` is not implemented for `String`
     //~| NOTE the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
     //~| NOTE required for `Result<(), String>` to implement `FromResidual<Result<Infallible, ()>>`
+    //~| HELP the following other types implement trait `From<T>`:
     Ok(one)
 }
 
@@ -42,7 +46,7 @@ fn baz() -> Result<String, String> { //~ NOTE expected `String` because of this
         .split_whitespace()
         .next()
         .ok_or_else(|| { //~ NOTE this can't be annotated with `?` because it has type `Result<_, ()>`
-            "Couldn't split the test string";
+            "Couldn't split the test string"; //~ HELP remove this semicolon
         })?;
     //~^ ERROR `?` couldn't convert the error to `String`
     //~| NOTE in this expansion of desugaring of operator `?`
@@ -52,6 +56,7 @@ fn baz() -> Result<String, String> { //~ NOTE expected `String` because of this
     //~| NOTE the trait `From<()>` is not implemented for `String`
     //~| NOTE the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
     //~| NOTE required for `Result<String, String>` to implement `FromResidual<Result<Infallible, ()>>`
+    //~| HELP the following other types implement trait `From<T>`:
     Ok(one.to_string())
 }
 

tests/ui/traits/question-mark-result-err-mismatch.stderr

@@ -1,5 +1,5 @@
 error[E0277]: `?` couldn't convert the error to `String`
-  --> $DIR/question-mark-result-err-mismatch.rs:12:22
+  --> $DIR/question-mark-result-err-mismatch.rs:14:22
    |
 LL |   fn foo() -> Result<String, String> {
    |               ---------------------- expected `String` because of this
@@ -10,8 +10,12 @@ LL | |             "Couldn't split the test string"
 LL | |         });
    | |__________- this has type `Result<_, &str>`
 ...
-LL |           .map_err(|_| ())
-   |            --------------- this can't be annotated with `?` because it has type `Result<_, ()>`
+LL |           .map_err(|e| {
+   |  __________-
+LL | |             e;
+   | |              - help: remove this semicolon
+LL | |         })
+   | |__________- this can't be annotated with `?` because it has type `Result<_, ()>`
 LL |           .map(|()| "")?;
    |                        ^ the trait `From<()>` is not implemented for `String`
    |
@@ -26,7 +30,7 @@ LL |           .map(|()| "")?;
    = note: required for `Result<String, String>` to implement `FromResidual<Result<Infallible, ()>>`
 
 error[E0277]: `?` couldn't convert the error to `String`
-  --> $DIR/question-mark-result-err-mismatch.rs:27:25
+  --> $DIR/question-mark-result-err-mismatch.rs:30:25
    |
 LL | fn bar() -> Result<(), String> {
    |             ------------------ expected `String` because of this
@@ -49,14 +53,15 @@ LL |         .map_err(|_| ())?;
    = note: required for `Result<(), String>` to implement `FromResidual<Result<Infallible, ()>>`
 
 error[E0277]: `?` couldn't convert the error to `String`
-  --> $DIR/question-mark-result-err-mismatch.rs:46:11
+  --> $DIR/question-mark-result-err-mismatch.rs:50:11
    |
 LL |   fn baz() -> Result<String, String> {
    |               ---------------------- expected `String` because of this
 ...
 LL |           .ok_or_else(|| {
    |  __________-
 LL | |             "Couldn't split the test string";
+   | |                                             - help: remove this semicolon
 LL | |         })?;
    | |          -^ the trait `From<()>` is not implemented for `String`
    | |__________|