Do not suggest removing param & when expr also has &

Author: fru1tworld

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

@@ -243,6 +243,15 @@ pub fn suggest_restriction<'tcx, G: EmissionGuarantee>(
     }
 }
 
+/// A single layer of `&` peeled from an expression, used by
+/// [`TypeErrCtxt::peel_expr_refs`].
+struct PeeledRef<'tcx> {
+    /// The span covering the `&` (and any whitespace/mutability keyword) to remove.
+    span: Span,
+    /// The type after peeling this layer (and all prior layers).
+    peeled_ty: Ty<'tcx>,
+}
+
 impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
     pub fn note_field_shadowed_by_private_candidate_in_cause(
         &self,
@@ -1818,6 +1827,85 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
         );
     }
 
+    /// Peel `&`-borrows from an expression, following through untyped let-bindings.
+    /// Returns a list of removable `&` layers (each with the span to remove and the
+    /// resulting type), plus an optional terminal [`hir::Param`] when the chain ends
+    /// at a function parameter (including async-fn desugared parameters).
+    fn peel_expr_refs(
+        &self,
+        mut expr: &'tcx hir::Expr<'tcx>,
+        mut ty: Ty<'tcx>,
+    ) -> (Vec<PeeledRef<'tcx>>, Option<&'tcx hir::Param<'tcx>>) {
+        let mut refs = Vec::new();
+        'outer: loop {
+            while let hir::ExprKind::AddrOf(_, _, borrowed) = expr.kind {
+                let span =
+                    if let Some(borrowed_span) = borrowed.span.find_ancestor_inside(expr.span) {
+                        expr.span.until(borrowed_span)
+                    } else {
+                        break 'outer;
+                    };
+
+                // Double check that the span actually corresponds to a borrow,
+                // rather than some macro garbage.
+                // The span may include leading parens from parenthesized expressions
+                // (e.g., `(&expr)` where HIR removes the Paren but keeps the span).
+                // In that case, trim the span to start at the `&`.
+                let span = match self.tcx.sess.source_map().span_to_snippet(span) {
+                    Ok(ref snippet) if snippet.starts_with("&") => span,
+                    Ok(ref snippet) if let Some(amp) = snippet.find('&') => {
+                        span.with_lo(span.lo() + BytePos(amp as u32))
+                    }
+                    _ => break 'outer,
+                };
+
+                let ty::Ref(_, inner_ty, _) = ty.kind() else {
+                    break 'outer;
+                };
+                ty = *inner_ty;
+                refs.push(PeeledRef { span, peeled_ty: ty });
+                expr = borrowed;
+            }
+            if let hir::ExprKind::Path(hir::QPath::Resolved(None, path)) = expr.kind
+                && let Res::Local(hir_id) = path.res
+                && let hir::Node::Pat(binding) = self.tcx.hir_node(hir_id)
+            {
+                match self.tcx.parent_hir_node(binding.hir_id) {
+                    // Untyped let-binding: follow to its initializer.
+                    hir::Node::LetStmt(local)
+                        if local.ty.is_none()
+                            && let Some(init) = local.init =>
+                    {
+                        expr = init;
+                        continue;
+                    }
+                    // Async fn desugared parameter: `let x = __arg0;` with AsyncFn source.
+                    // Follow to the original parameter.
+                    hir::Node::LetStmt(local)
+                        if matches!(local.source, hir::LocalSource::AsyncFn)
+                            && let Some(init) = local.init
+                            && let hir::ExprKind::Path(hir::QPath::Resolved(None, arg_path)) =
+                                init.kind
+                            && let Res::Local(arg_hir_id) = arg_path.res
+                            && let hir::Node::Pat(arg_binding) = self.tcx.hir_node(arg_hir_id)
+                            && let hir::Node::Param(param) =
+                                self.tcx.parent_hir_node(arg_binding.hir_id) =>
+                    {
+                        return (refs, Some(param));
+                    }
+                    // Direct parameter reference.
+                    hir::Node::Param(param) => {
+                        return (refs, Some(param));
+                    }
+                    _ => break 'outer,
+                }
+            } else {
+                break 'outer;
+            }
+        }
+        (refs, None)
+    }
+
     /// Whenever references are used by mistake, like `for (i, e) in &vec.iter().enumerate()`,
     /// suggest removing these references until we reach a type that implements the trait.
     pub(super) fn suggest_remove_reference(
@@ -1894,53 +1982,40 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
         }
 
         // Maybe suggest removal of borrows from expressions, like in `for i in &&&foo {}`.
-        let Some(mut expr) = expr_finder.result else {
+        let Some(expr) = expr_finder.result else {
             return false;
         };
-        let mut count = 0;
-        let mut suggestions = vec![];
         // Skipping binder here, remapping below
-        let mut suggested_ty = trait_pred.self_ty().skip_binder();
-        'outer: loop {
-            while let hir::ExprKind::AddrOf(_, _, borrowed) = expr.kind {
-                count += 1;
-                let span =
-                    if let Some(borrowed_span) = borrowed.span.find_ancestor_inside(expr.span) {
-                        expr.span.until(borrowed_span)
-                    } else {
-                        break 'outer;
-                    };
-
-                // Double check that the span we extracted actually corresponds to a borrow,
-                // rather than some macro garbage.
-                match self.tcx.sess.source_map().span_to_snippet(span) {
-                    Ok(snippet) if snippet.starts_with("&") => {}
-                    _ => break 'outer,
-                }
-
-                suggestions.push((span, String::new()));
-
-                let ty::Ref(_, inner_ty, _) = suggested_ty.kind() else {
-                    break 'outer;
-                };
-                suggested_ty = *inner_ty;
-
-                expr = borrowed;
+        let suggested_ty = trait_pred.self_ty().skip_binder();
+        let (peeled_refs, _) = self.peel_expr_refs(expr, suggested_ty);
+        for (i, peeled) in peeled_refs.iter().enumerate() {
+            let suggestions: Vec<_> =
+                peeled_refs[..=i].iter().map(|r| (r.span, String::new())).collect();
+            if maybe_suggest(peeled.peeled_ty, i + 1, suggestions) {
+                return true;
+            }
+        }
+        false
+    }
 
-                if maybe_suggest(suggested_ty, count, suggestions.clone()) {
+    /// Suggest removing `&` from a function parameter type like `&impl Future`.
+    fn suggest_remove_ref_from_param(&self, param: &hir::Param<'_>, err: &mut Diag<'_>) -> bool {
+        if let Some(decl) = self.tcx.parent_hir_node(param.hir_id).fn_decl()
+            && let Some(input_ty) = decl.inputs.iter().find(|t| param.ty_span.contains(t.span))
+            && let hir::TyKind::Ref(_, mut_ty) = input_ty.kind
+        {
+            let ref_span = input_ty.span.until(mut_ty.ty.span);
+            match self.tcx.sess.source_map().span_to_snippet(ref_span) {
+                Ok(snippet) if snippet.starts_with("&") => {
+                    err.span_suggestion_verbose(
+                        ref_span,
+                        "consider removing the `&` from the parameter type",
+                        "",
+                        Applicability::MaybeIncorrect,
+                    );
                     return true;
                 }
-            }
-            if let hir::ExprKind::Path(hir::QPath::Resolved(None, path)) = expr.kind
-                && let Res::Local(hir_id) = path.res
-                && let hir::Node::Pat(binding) = self.tcx.hir_node(hir_id)
-                && let hir::Node::LetStmt(local) = self.tcx.parent_hir_node(binding.hir_id)
-                && let None = local.ty
-                && let Some(binding_expr) = local.init
-            {
-                expr = binding_expr;
-            } else {
-                break 'outer;
+                _ => {}
             }
         }
         false
@@ -1959,6 +2034,89 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
             // could also check if it is an fn call (very likely) and suggest changing *that*, if
             // it is from the local crate.
 
+            // If the type is `&..&T` where `T: Future`, suggest removing `&`
+            // instead of removing `.await`.
+            if let ty::PredicateKind::Clause(ty::ClauseKind::Trait(pred)) =
+                obligation.predicate.kind().skip_binder()
+            {
+                let self_ty = pred.self_ty();
+                let future_trait =
+                    self.tcx.require_lang_item(LangItem::Future, obligation.cause.span);
+
+                // Peel through references to check if there's a Future underneath.
+                let has_future = {
+                    let mut ty = self_ty;
+                    loop {
+                        match *ty.kind() {
+                            ty::Ref(_, inner_ty, _)
+                                if !matches!(inner_ty.kind(), ty::Dynamic(..)) =>
+                            {
+                                if self
+                                    .type_implements_trait(
+                                        future_trait,
+                                        [inner_ty],
+                                        obligation.param_env,
+                                    )
+                                    .must_apply_modulo_regions()
+                                {
+                                    break true;
+                                }
+                                ty = inner_ty;
+                            }
+                            _ => break false,
+                        }
+                    }
+                };
+
+                if has_future {
+                    let (peeled_refs, terminal_param) = self.peel_expr_refs(expr, self_ty);
+
+                    // Try removing `&`s from the expression.
+                    for (i, peeled) in peeled_refs.iter().enumerate() {
+                        if self
+                            .type_implements_trait(
+                                future_trait,
+                                [peeled.peeled_ty],
+                                obligation.param_env,
+                            )
+                            .must_apply_modulo_regions()
+                        {
+                            let count = i + 1;
+                            let msg = if count == 1 {
+                                "consider removing the leading `&`-reference".to_string()
+                            } else {
+                                format!("consider removing {count} leading `&`-references")
+                            };
+                            let suggestions: Vec<_> =
+                                peeled_refs[..=i].iter().map(|r| (r.span, String::new())).collect();
+                            err.multipart_suggestion(
+                                msg,
+                                suggestions,
+                                Applicability::MachineApplicable,
+                            );
+                            return;
+                        }
+                    }
+
+                    // Try removing `&` from the parameter type, but only when there's
+                    // no `&` in the expression itself (otherwise removing from the param
+                    // alone wouldn't fix the error).
+                    if peeled_refs.is_empty()
+                        && let Some(param) = terminal_param
+                        && self.suggest_remove_ref_from_param(param, err)
+                    {
+                        return;
+                    }
+
+                    // Fallback: emit a help message when we can't provide a specific span.
+                    err.help(
+                        "a reference to a future is not a future; \
+                     consider removing the leading `&`-reference",
+                    );
+                    return;
+                }
+            }
+
             // use nth(1) to skip one layer of desugaring from `IntoIter::into_iter`
             if let Some((_, hir::Node::Expr(await_expr))) = self.tcx.hir_parent_iter(*hir_id).nth(1)
                 && let Some(expr_span) = expr.span.find_ancestor_inside_same_ctxt(await_expr.span)

tests/ui/async-await/await-ref-future.rs

@@ -0,0 +1,43 @@
+//@ edition:2021
+// Regression test for #87211.
+// Test that we suggest removing `&` from references to futures,
+// including let-bindings and parameter types, but not `&dyn Future`.
+
+async fn my_async_fn() {}
+
+async fn foo() {
+    let fut = &my_async_fn();
+    fut.await; //~ ERROR `&impl Future<Output = ()>` is not a future
+}
+
+async fn direct_ref_await() {
+    (&my_async_fn()).await; //~ ERROR `&impl Future<Output = ()>` is not a future
+}
+
+async fn bar(fut: &impl std::future::Future<Output = ()>) {
+    fut.await; //~ ERROR is not a future
+}
+
+async fn dyn_ref_param(fut: &dyn std::future::Future<Output = ()>) {
+    fut.await; //~ ERROR is not a future
+}
+
+async fn typed_let_binding() {
+    let fut: &_ = &my_async_fn();
+    fut.await; //~ ERROR `&impl Future<Output = ()>` is not a future
+}
+
+async fn ref_param_borrowed_expr(fut: &impl std::future::Future<Output = ()>) {
+    (&fut).await; //~ ERROR is not a future
+}
+
+async fn double_ref_direct() {
+    (&&my_async_fn()).await; //~ ERROR is not a future
+}
+
+async fn double_ref_let() {
+    let fut = &&my_async_fn();
+    fut.await; //~ ERROR is not a future
+}
+
+fn main() {}