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

Remove `MaybeForgetReturn` suggestion

#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.

7 files changed, 4 insertions, 83 deletions

diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs
index f2b133f5677..f69e756a3e1 100644
--- a/compiler/rustc_errors/src/lib.rs
+++ b/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,
diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs
index 054e3bcb67c..c46a42c5de1 100644
--- a/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs
+++ b/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);
         }
     }
 
diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs
index 82e99ca7afa..db947b6744d 100644
--- a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs
+++ b/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.
     ///
diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/ambiguity.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/ambiguity.rs
index f15f1b78b52..d673e5672a0 100644
--- a/compiler/rustc_trait_selection/src/error_reporting/traits/ambiguity.rs
+++ b/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,
                     }
diff --git a/tests/ui/inference/issue-86094-suggest-add-return-to-coerce-ret-ty.stderr b/tests/ui/inference/issue-86094-suggest-add-return-to-coerce-ret-ty.stderr
index 1fea73529a8..c61ca699b0d 100644
--- a/tests/ui/inference/issue-86094-suggest-add-return-to-coerce-ret-ty.stderr
+++ b/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
diff --git a/tests/ui/return/tail-expr-as-potential-return.rs b/tests/ui/return/tail-expr-as-potential-return.rs
index 11ecddb049b..2e638f1897c 100644
--- a/tests/ui/return/tail-expr-as-potential-return.rs
+++ b/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
diff --git a/tests/ui/return/tail-expr-as-potential-return.stderr b/tests/ui/return/tail-expr-as-potential-return.stderr
index 756de2b5a16..8105b2df3fe 100644
--- a/tests/ui/return/tail-expr-as-potential-return.stderr
+++ b/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