Properly handle async block and async fn in if exprs without else

[estebank]

When encountering a tail expression in the then arm of an if expression without an else arm, account for async fn and async blocks to suggest returning the value and pointing at the return type of the async fn.

We now also account for AFIT when looking for the return type to point at.

Fix #115405.

[rustbot]

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

[compiler-errors]

Why do you need to do this? If the ret_coercion has a type, then it will have already been coerced with all of these ret_exprs collected below.

[compiler-errors]

Like, the whole point of ret_coercion is to be the type that all of the return expressions in the body are coerced into

[estebank]

ret_coercion is (always?) an inference type. I could likely just remove that first type push entirely and the logic wouldn't change.

[compiler-errors]

It's not always an inference type. It may appear to be an inference type if you print it without calling resolve_vars_if_possible on it (which you don't need to do here), but it gets equated with another type as soon as the first return statement is type checked, and can_coerce will handle it properly below.

Applying this diff makes no changes to the UI tests:

diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
index afe5d0d4880..0a03429b37f 100644
--- a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
+++ b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
@@ -1,6 +1,5 @@
 use super::FnCtxt;
 
-use crate::coercion::CollectRetsVisitor;
 use crate::errors;
 use crate::fluent_generated as fluent;
 use crate::fn_ctxt::rustc_span::BytePos;
@@ -17,7 +16,6 @@
 use rustc_hir as hir;
 use rustc_hir::def::Res;
 use rustc_hir::def::{CtorKind, CtorOf, DefKind};
-use rustc_hir::intravisit::{Map, Visitor};
 use rustc_hir::lang_items::LangItem;
 use rustc_hir::{
     CoroutineDesugaring, CoroutineKind, CoroutineSource, Expr, ExprKind, GenericBound, HirId, Node,
@@ -1082,23 +1080,6 @@ pub(in super::super) fn suggest_missing_break_or_return_expr(
                     let ret_ty = ret_coercion.borrow().expected_ty();
                     rets.push(ret_ty);
                 }
-                let mut visitor = CollectRetsVisitor { ret_exprs: vec![] };
-                if let Some(item) = self.tcx.hir().find(id)
-                    && let Node::Expr(expr) = item
-                {
-                    visitor.visit_expr(expr);
-                    for expr in visitor.ret_exprs {
-                        if let Some(ty) = self.typeck_results.borrow().node_type_opt(expr.hir_id) {
-                            rets.push(ty);
-                        }
-                    }
-                    if let hir::ExprKind::Block(hir::Block { expr: Some(expr), .. }, _) = expr.kind
-                    {
-                        if let Some(ty) = self.typeck_results.borrow().node_type_opt(expr.hir_id) {
-                            rets.push(ty);
-                        }
-                    }
-                }
                 rets.into_iter().all(|ty| self.can_coerce(found, ty))
             }
             _ => false,

Unless I am mistaken, pls remove the CollectRetsVisitor usage (and the changes to it).

[compiler-errors]

For the record, that diff is to just demonstrate that it works without the usage of CollectRetsVisitor. The code should be simplified into a single can_coerce call.

[compiler-errors]

Actually, I think none of your tests aren't actually exercising this behavior of a previous return statement having constrained the ret_coercion?

Anyways, you should also be using .merged_ty() from the ret_coercion instead of .expected_ty(), but you still shouldn't need to look at any of the node_tys of the return expressions. You should write a test that exercises that the lint will not be made if the type is incompatible with other returns.
edit: actually, it's not that big of a deal which of merged-ty or expected-ty you use.

[estebank]

I wanted to avoid an incorrect suggestion in the face of

pub fn baz() -> Pin<Box<dyn Future<Output = Result<S, ()>> + 'static>> {
    Box::pin(async move {
        if true {
            Some(S) //~ ERROR mismatched types
        }
        Err(())
    })
}

Now, looking into it, not only the logic is now wrong (after doing some changes cleaning up for publication), but the type for the returning expressions isn't actually available. For some reason, I had gotten a value for them, but now with the code as is now. Given that, yes, the entire visitor thing is useless and can be removed. I was hoping to avoid nonsensical suggestions like the one above, but oh well.

[compiler-errors]

Keeping around a single can_coerce call for the ret_coercion.expected_ty() should avoid such a nonsensical suggestion, at least according to local testing.

[estebank]

I think I know why I had something that seemed to be working earlier. I tried using check_expr at some point, which "worked", but trying it again now it does cause multiple issues that make it more of a pain in the neck than we want to deal with.

[compiler-errors]

this isn't needed

[estebank]

Done.

[estebank]

Oh, shit, had in flight changes... let me clean up the mess first.

[bors]

☔ The latest upstream changes (presumably #120767) made this pull request unmergeable. Please resolve the merge conflicts.

[oli-obk]

@bor@ r+

[bors]

☔ The latest upstream changes (presumably #120951) made this pull request unmergeable. Please resolve the merge conflicts.

[oli-obk]

@bor@ r+

uhm... mobile keyboard failure

r=me after rebase

@rustbot author

[estebank]

@bors r=oli-obk

[bors]

📌 Commit d07195f has been approved by oli-obk

It is now in the queue for this repository.