Rollup merge of rust-lang#137303 - compiler-errors:maybe-forgor, r=cjgillot

Remove `MaybeForgetReturn` suggestion

rust-lang#115196 implemented a suggestion to add a missing `return` when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function.

I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on `StashKey::MaybeForgetReturn`, which is only stashed when we have *Sized* obligation ambiguity errors. Let's remove it for now.

I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for:

```rust
struct W<T>(T);

fn bar<T: Default>() -> W<T> { todo!() }

fn foo() -> W<i32> {
    if true {
        bar();
    }
    W(0)
}
```

Nor is it triggered for:

```
fn foo() -> i32 {
    if true {
        Default::default();
    }
    0
}
```

It's basically only triggered iff there's only one ambiguity error on the type, which is `Sized`.

Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high.

One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.

Author: workingjubilee

compiler/rustc_errors/src/lib.rs

@@ -626,7 +626,6 @@ pub enum StashKey {
     MaybeFruTypo,
     CallAssocMethod,
     AssociatedTypeSuggestion,
-    MaybeForgetReturn,
     /// Query cycle detected, stashing in favor of a better error.
     Cycle,
     UndeterminedMacroResolution,

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

@@ -669,12 +669,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
         if !errors.is_empty() {
             self.adjust_fulfillment_errors_for_expr_obligation(&mut errors);
-            let errors_causecode = errors
-                .iter()
-                .map(|e| (e.obligation.cause.span, e.root_obligation.cause.code().clone()))
-                .collect::<Vec<_>>();
             self.err_ctxt().report_fulfillment_errors(errors);
-            self.collect_unused_stmts_for_coerce_return_ty(errors_causecode);
         }
     }
 

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

@@ -3,9 +3,7 @@ use std::{fmt, iter, mem};
 use itertools::Itertools;
 use rustc_data_structures::fx::FxIndexSet;
 use rustc_errors::codes::*;
-use rustc_errors::{
-    Applicability, Diag, ErrorGuaranteed, MultiSpan, StashKey, a_or_an, listify, pluralize,
-};
+use rustc_errors::{Applicability, Diag, ErrorGuaranteed, MultiSpan, a_or_an, listify, pluralize};
 use rustc_hir::def::{CtorOf, DefKind, Res};
 use rustc_hir::def_id::DefId;
 use rustc_hir::intravisit::Visitor;
@@ -2193,62 +2191,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         }
     }
 
-    pub(super) fn collect_unused_stmts_for_coerce_return_ty(
-        &self,
-        errors_causecode: Vec<(Span, ObligationCauseCode<'tcx>)>,
-    ) {
-        for (span, code) in errors_causecode {
-            self.dcx().try_steal_modify_and_emit_err(span, StashKey::MaybeForgetReturn, |err| {
-                if let Some(fn_sig) = self.body_fn_sig()
-                    && let ObligationCauseCode::WhereClauseInExpr(_, _, binding_hir_id, ..) = code
-                    && !fn_sig.output().is_unit()
-                {
-                    let mut block_num = 0;
-                    let mut found_semi = false;
-                    for (hir_id, node) in self.tcx.hir_parent_iter(binding_hir_id) {
-                        // Don't proceed into parent bodies
-                        if hir_id.owner != binding_hir_id.owner {
-                            break;
-                        }
-                        match node {
-                            hir::Node::Stmt(stmt) => {
-                                if let hir::StmtKind::Semi(expr) = stmt.kind {
-                                    let expr_ty = self.typeck_results.borrow().expr_ty(expr);
-                                    let return_ty = fn_sig.output();
-                                    if !matches!(expr.kind, hir::ExprKind::Ret(..))
-                                        && self.may_coerce(expr_ty, return_ty)
-                                    {
-                                        found_semi = true;
-                                    }
-                                }
-                            }
-                            hir::Node::Block(_block) => {
-                                if found_semi {
-                                    block_num += 1;
-                                }
-                            }
-                            hir::Node::Item(item) => {
-                                if let hir::ItemKind::Fn { .. } = item.kind {
-                                    break;
-                                }
-                            }
-                            _ => {}
-                        }
-                    }
-                    if block_num > 1 && found_semi {
-                        err.span_suggestion_verbose(
-                            // use the span of the *whole* expr
-                            self.tcx.hir().span(binding_hir_id).shrink_to_lo(),
-                            "you might have meant to return this to infer its type parameters",
-                            "return ",
-                            Applicability::MaybeIncorrect,
-                        );
-                    }
-                }
-            });
-        }
-    }
-
     /// Given a vector of fulfillment errors, try to adjust the spans of the
     /// errors to more accurately point at the cause of the failure.
     ///

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

@@ -1,8 +1,6 @@
 use std::ops::ControlFlow;
 
-use rustc_errors::{
-    Applicability, Diag, E0283, E0284, E0790, MultiSpan, StashKey, struct_span_code_err,
-};
+use rustc_errors::{Applicability, Diag, E0283, E0284, E0790, MultiSpan, struct_span_code_err};
 use rustc_hir as hir;
 use rustc_hir::LangItem;
 use rustc_hir::def::{DefKind, Res};
@@ -197,7 +195,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
                 // be ignoring the fact that we don't KNOW the type works
                 // out. Though even that would probably be harmless, given that
                 // we're only talking about builtin traits, which are known to be
-                // inhabited. We used to check for `self.tcx.sess.has_errors()` to
+                // inhabited. We used to check for `self.tainted_by_errors()` to
                 // avoid inundating the user with unnecessary errors, but we now
                 // check upstream for type errors and don't add the obligations to
                 // begin with in those cases.
@@ -211,7 +209,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
                                 TypeAnnotationNeeded::E0282,
                                 false,
                             );
-                            return err.stash(span, StashKey::MaybeForgetReturn).unwrap();
+                            return err.emit();
                         }
                         Some(e) => return e,
                     }

tests/ui/inference/issue-86094-suggest-add-return-to-coerce-ret-ty.stderr

@@ -8,10 +8,6 @@ help: consider specifying the generic arguments
    |
 LL |         Err::<T, MyError>(MyError);
    |            ++++++++++++++
-help: you might have meant to return this to infer its type parameters
-   |
-LL |         return Err(MyError);
-   |         ++++++
 
 error[E0282]: type annotations needed
   --> $DIR/issue-86094-suggest-add-return-to-coerce-ret-ty.rs:14:9
@@ -23,10 +19,6 @@ help: consider specifying the generic arguments
    |
 LL |         Ok::<(), E>(());
    |           +++++++++
-help: you might have meant to return this to infer its type parameters
-   |
-LL |         return Ok(());
-   |         ++++++
 
 error[E0308]: mismatched types
   --> $DIR/issue-86094-suggest-add-return-to-coerce-ret-ty.rs:21:20

tests/ui/return/tail-expr-as-potential-return.rs

@@ -60,7 +60,6 @@ fn method() -> Option<i32> {
         Receiver.generic();
         //~^ ERROR type annotations needed
         //~| HELP consider specifying the generic argument
-        //~| HELP you might have meant to return this to infer its type parameters
     }
 
     None

tests/ui/return/tail-expr-as-potential-return.stderr

@@ -57,10 +57,6 @@ help: consider specifying the generic argument
    |
 LL |         Receiver.generic::<T>();
    |                         +++++
-help: you might have meant to return this to infer its type parameters
-   |
-LL |         return Receiver.generic();
-   |         ++++++
 
 error: aborting due to 4 previous errors