Rollup merge of rust-lang#102863 - compiler-errors:call-suggestion-on-unimplemented, r=nagisa

Standardize "use parentheses to call" suggestions between typeck and trait selection

1. Suggest calling constructors, since they're basically `FnDef`s but they have a different def kind and hir representation, so we were leaving them out.
2. Standardize the call suggestions between trait fulfillment errors and type mismatch. In the type mismatch suggestion, we suggest `/* Ty */` as the placeholder for an arg, and not the parameter's name, which is less helpful.
3. Use `predicate_must_hold_modulo_regions` instead of matching on `EvaluationResult` -- this might cause some suggestions to be filtered out, but we really shouldn't be suggesting a call if it "may" hold, only when it "must" hold.
4. Borrow some logic from `extract_callable_info` to generalize this suggestion to fn pointers, type parameters, and opaque types.

Fixes rust-lang#102852

Author: Dylan-DPC

compiler/rustc_hir_analysis/src/check/callee.rs

@@ -1,6 +1,6 @@
 use super::method::probe::{IsSuggestion, Mode, ProbeScope};
 use super::method::MethodCallee;
-use super::{DefIdOrName, Expectation, FnCtxt, TupleArgumentsFlag};
+use super::{Expectation, FnCtxt, TupleArgumentsFlag};
 use crate::type_error_struct;
 
 use rustc_ast::util::parser::PREC_POSTFIX;
@@ -27,6 +27,7 @@ use rustc_span::Span;
 use rustc_target::spec::abi;
 use rustc_trait_selection::autoderef::Autoderef;
 use rustc_trait_selection::infer::InferCtxtExt as _;
+use rustc_trait_selection::traits::error_reporting::DefIdOrName;
 use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt as _;
 
 use std::iter;

compiler/rustc_hir_analysis/src/check/fn_ctxt/suggestions.rs

@@ -2,7 +2,6 @@ use super::FnCtxt;
 use crate::astconv::AstConv;
 use crate::errors::{AddReturnTypeSuggestion, ExpectedReturnTypeLabel};
 
-use hir::def_id::DefId;
 use rustc_ast::util::parser::{ExprPrecedence, PREC_POSTFIX};
 use rustc_errors::{Applicability, Diagnostic, MultiSpan};
 use rustc_hir as hir;
@@ -19,6 +18,7 @@ use rustc_session::errors::ExprParenthesesNeeded;
 use rustc_span::symbol::sym;
 use rustc_span::Span;
 use rustc_trait_selection::infer::InferCtxtExt;
+use rustc_trait_selection::traits::error_reporting::DefIdOrName;
 use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt as _;
 
 impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
@@ -90,7 +90,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                             if ty.is_suggestable(self.tcx, false) {
                                 format!("/* {ty} */")
                             } else {
-                                "".to_string()
+                                "/* value */".to_string()
                             }
                         })
                         .collect::<Vec<_>>()
@@ -102,10 +102,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
             let msg = match def_id_or_name {
                 DefIdOrName::DefId(def_id) => match self.tcx.def_kind(def_id) {
-                    DefKind::Ctor(CtorOf::Struct, _) => "instantiate this tuple struct".to_string(),
-                    DefKind::Ctor(CtorOf::Variant, _) => {
-                        "instantiate this tuple variant".to_string()
-                    }
+                    DefKind::Ctor(CtorOf::Struct, _) => "construct this tuple struct".to_string(),
+                    DefKind::Ctor(CtorOf::Variant, _) => "construct this tuple variant".to_string(),
                     kind => format!("call this {}", kind.descr(def_id)),
                 },
                 DefIdOrName::Name(name) => format!("call this {name}"),
@@ -1209,8 +1207,3 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         }
     }
 }
-
-pub enum DefIdOrName {
-    DefId(DefId),
-    Name(&'static str),
-}

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

@@ -2,10 +2,10 @@ pub mod on_unimplemented;
 pub mod suggestions;
 
 use super::{
-    EvaluationResult, FulfillmentContext, FulfillmentError, FulfillmentErrorCode,
-    MismatchedProjectionTypes, Obligation, ObligationCause, ObligationCauseCode,
-    OnUnimplementedDirective, OnUnimplementedNote, OutputTypeParameterMismatch, Overflow,
-    PredicateObligation, SelectionContext, SelectionError, TraitNotObjectSafe,
+    FulfillmentContext, FulfillmentError, FulfillmentErrorCode, MismatchedProjectionTypes,
+    Obligation, ObligationCause, ObligationCauseCode, OnUnimplementedDirective,
+    OnUnimplementedNote, OutputTypeParameterMismatch, Overflow, PredicateObligation,
+    SelectionContext, SelectionError, TraitNotObjectSafe,
 };
 
 use crate::infer::error_reporting::{TyCategory, TypeAnnotationNeeded as ErrorCode};
@@ -2796,3 +2796,8 @@ impl<'tcx> ty::TypeVisitor<'tcx> for HasNumericInferVisitor {
         }
     }
 }
+
+pub enum DefIdOrName {
+    DefId(DefId),
+    Name(&'static str),
+}

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

@@ -1,12 +1,13 @@
 use super::{
-    EvaluationResult, Obligation, ObligationCause, ObligationCauseCode, PredicateObligation,
+    DefIdOrName, Obligation, ObligationCause, ObligationCauseCode, PredicateObligation,
     SelectionContext,
 };
 
 use crate::autoderef::Autoderef;
 use crate::infer::InferCtxt;
 use crate::traits::normalize_to;
 
+use hir::def::CtorOf;
 use hir::HirId;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_data_structures::stack::ensure_sufficient_stack;
@@ -22,14 +23,15 @@ use rustc_hir::lang_items::LangItem;
 use rustc_hir::{AsyncGeneratorKind, GeneratorKind, Node};
 use rustc_infer::infer::error_reporting::TypeErrCtxt;
 use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
+use rustc_infer::infer::LateBoundRegionConversionTime;
 use rustc_middle::hir::map;
 use rustc_middle::ty::{
     self, suggest_arbitrary_trait_bound, suggest_constraining_type_param, AdtKind, DefIdTree,
     GeneratorDiagnosticData, GeneratorInteriorTypeCause, Infer, InferTy, IsSuggestable,
     ToPredicate, Ty, TyCtxt, TypeFoldable, TypeFolder, TypeSuperFoldable, TypeVisitable,
 };
 use rustc_middle::ty::{TypeAndMut, TypeckResults};
-use rustc_span::symbol::{kw, sym, Ident, Symbol};
+use rustc_span::symbol::{sym, Ident, Symbol};
 use rustc_span::{BytePos, DesugaringKind, ExpnKind, Span, DUMMY_SP};
 use rustc_target::spec::abi;
 use std::fmt;
@@ -812,74 +814,136 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
         err: &mut Diagnostic,
         trait_pred: ty::PolyTraitPredicate<'tcx>,
     ) -> bool {
-        // Skipping binder here, remapping below
-        let self_ty = trait_pred.self_ty().skip_binder();
-
-        let (def_id, output_ty, callable) = match *self_ty.kind() {
-            ty::Closure(def_id, substs) => (def_id, substs.as_closure().sig().output(), "closure"),
-            ty::FnDef(def_id, _) => (def_id, self_ty.fn_sig(self.tcx).output(), "function"),
-            _ => return false,
-        };
-        let msg = format!("use parentheses to call the {}", callable);
+        if let ty::PredicateKind::Trait(trait_pred) = obligation.predicate.kind().skip_binder()
+            && Some(trait_pred.def_id()) == self.tcx.lang_items().sized_trait()
+        {
+            // Don't suggest calling to turn an unsized type into a sized type
+            return false;
+        }
 
-        // "We should really create a single list of bound vars from the combined vars
-        // from the predicate and function, but instead we just liberate the function bound vars"
-        let output_ty = self.tcx.liberate_late_bound_regions(def_id, output_ty);
+        // This is duplicated from `extract_callable_info` in typeck, which
+        // relies on autoderef, so we can't use it here.
+        let found = trait_pred.self_ty().skip_binder().peel_refs();
+        let Some((def_id_or_name, output, inputs)) = (match *found.kind()
+        {
+            ty::FnPtr(fn_sig) => {
+                Some((DefIdOrName::Name("function pointer"), fn_sig.output(), fn_sig.inputs()))
+            }
+            ty::FnDef(def_id, _) => {
+                let fn_sig = found.fn_sig(self.tcx);
+                Some((DefIdOrName::DefId(def_id), fn_sig.output(), fn_sig.inputs()))
+            }
+            ty::Closure(def_id, substs) => {
+                let fn_sig = substs.as_closure().sig();
+                Some((
+                    DefIdOrName::DefId(def_id),
+                    fn_sig.output(),
+                    fn_sig.inputs().map_bound(|inputs| &inputs[1..]),
+                ))
+            }
+            ty::Opaque(def_id, substs) => {
+                self.tcx.bound_item_bounds(def_id).subst(self.tcx, substs).iter().find_map(|pred| {
+                    if let ty::PredicateKind::Projection(proj) = pred.kind().skip_binder()
+                    && Some(proj.projection_ty.item_def_id) == self.tcx.lang_items().fn_once_output()
+                    // args tuple will always be substs[1]
+                    && let ty::Tuple(args) = proj.projection_ty.substs.type_at(1).kind()
+                    {
+                        Some((
+                            DefIdOrName::DefId(def_id),
+                            pred.kind().rebind(proj.term.ty().unwrap()),
+                            pred.kind().rebind(args.as_slice()),
+                        ))
+                    } else {
+                        None
+                    }
+                })
+            }
+            ty::Dynamic(data, _, ty::Dyn) => {
+                data.iter().find_map(|pred| {
+                    if let ty::ExistentialPredicate::Projection(proj) = pred.skip_binder()
+                    && Some(proj.item_def_id) == self.tcx.lang_items().fn_once_output()
+                    // for existential projection, substs are shifted over by 1
+                    && let ty::Tuple(args) = proj.substs.type_at(0).kind()
+                    {
+                        Some((
+                            DefIdOrName::Name("trait object"),
+                            pred.rebind(proj.term.ty().unwrap()),
+                            pred.rebind(args.as_slice()),
+                        ))
+                    } else {
+                        None
+                    }
+                })
+            }
+            ty::Param(_) => {
+                obligation.param_env.caller_bounds().iter().find_map(|pred| {
+                    if let ty::PredicateKind::Projection(proj) = pred.kind().skip_binder()
+                    && Some(proj.projection_ty.item_def_id) == self.tcx.lang_items().fn_once_output()
+                    && proj.projection_ty.self_ty() == found
+                    // args tuple will always be substs[1]
+                    && let ty::Tuple(args) = proj.projection_ty.substs.type_at(1).kind()
+                    {
+                        Some((
+                            DefIdOrName::Name("type parameter"),
+                            pred.kind().rebind(proj.term.ty().unwrap()),
+                            pred.kind().rebind(args.as_slice()),
+                        ))
+                    } else {
+                        None
+                    }
+                })
+            }
+            _ => None,
+        }) else { return false; };
+        let output = self.replace_bound_vars_with_fresh_vars(
+            obligation.cause.span,
+            LateBoundRegionConversionTime::FnCall,
+            output,
+        );
+        let inputs = inputs.skip_binder().iter().map(|ty| {
+            self.replace_bound_vars_with_fresh_vars(
+                obligation.cause.span,
+                LateBoundRegionConversionTime::FnCall,
+                inputs.rebind(*ty),
+            )
+        });
 
         // Remapping bound vars here
-        let trait_pred_and_self = trait_pred.map_bound(|trait_pred| (trait_pred, output_ty));
+        let trait_pred_and_self = trait_pred.map_bound(|trait_pred| (trait_pred, output));
 
         let new_obligation =
             self.mk_trait_obligation_with_new_self_ty(obligation.param_env, trait_pred_and_self);
-
-        match self.evaluate_obligation(&new_obligation) {
-            Ok(
-                EvaluationResult::EvaluatedToOk
-                | EvaluationResult::EvaluatedToOkModuloRegions
-                | EvaluationResult::EvaluatedToOkModuloOpaqueTypes
-                | EvaluationResult::EvaluatedToAmbig,
-            ) => {}
-            _ => return false,
+        if !self.predicate_must_hold_modulo_regions(&new_obligation) {
+            return false;
         }
-        let hir = self.tcx.hir();
+
         // Get the name of the callable and the arguments to be used in the suggestion.
-        let (snippet, sugg) = match hir.get_if_local(def_id) {
-            Some(hir::Node::Expr(hir::Expr {
-                kind: hir::ExprKind::Closure(hir::Closure { fn_decl, fn_decl_span, .. }),
-                ..
-            })) => {
-                err.span_label(*fn_decl_span, "consider calling this closure");
-                let Some(name) = self.get_closure_name(def_id, err, &msg) else {
-                    return false;
-                };
-                let args = fn_decl.inputs.iter().map(|_| "_").collect::<Vec<_>>().join(", ");
-                let sugg = format!("({})", args);
-                (format!("{}{}", name, sugg), sugg)
-            }
-            Some(hir::Node::Item(hir::Item {
-                ident,
-                kind: hir::ItemKind::Fn(.., body_id),
-                ..
-            })) => {
-                err.span_label(ident.span, "consider calling this function");
-                let body = hir.body(*body_id);
-                let args = body
-                    .params
-                    .iter()
-                    .map(|arg| match &arg.pat.kind {
-                        hir::PatKind::Binding(_, _, ident, None)
-                        // FIXME: provide a better suggestion when encountering `SelfLower`, it
-                        // should suggest a method call.
-                        if ident.name != kw::SelfLower => ident.to_string(),
-                        _ => "_".to_string(),
-                    })
-                    .collect::<Vec<_>>()
-                    .join(", ");
-                let sugg = format!("({})", args);
-                (format!("{}{}", ident, sugg), sugg)
-            }
-            _ => return false,
+        let hir = self.tcx.hir();
+
+        let msg = match def_id_or_name {
+            DefIdOrName::DefId(def_id) => match self.tcx.def_kind(def_id) {
+                DefKind::Ctor(CtorOf::Struct, _) => {
+                    "use parentheses to construct this tuple struct".to_string()
+                }
+                DefKind::Ctor(CtorOf::Variant, _) => {
+                    "use parentheses to construct this tuple variant".to_string()
+                }
+                kind => format!("use parentheses to call this {}", kind.descr(def_id)),
+            },
+            DefIdOrName::Name(name) => format!("use parentheses to call this {name}"),
         };
+
+        let args = inputs
+            .map(|ty| {
+                if ty.is_suggestable(self.tcx, false) {
+                    format!("/* {ty} */")
+                } else {
+                    "/* value */".to_string()
+                }
+            })
+            .collect::<Vec<_>>()
+            .join(", ");
+
         if matches!(obligation.cause.code(), ObligationCauseCode::FunctionArgumentObligation { .. })
             && obligation.cause.span.can_be_used_for_suggestions()
         {
@@ -890,11 +954,36 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
             err.span_suggestion_verbose(
                 obligation.cause.span.shrink_to_hi(),
                 &msg,
-                sugg,
+                format!("({args})"),
                 Applicability::HasPlaceholders,
             );
-        } else {
-            err.help(&format!("{}: `{}`", msg, snippet));
+        } else if let DefIdOrName::DefId(def_id) = def_id_or_name {
+            let name = match hir.get_if_local(def_id) {
+                Some(hir::Node::Expr(hir::Expr {
+                    kind: hir::ExprKind::Closure(hir::Closure { fn_decl_span, .. }),
+                    ..
+                })) => {
+                    err.span_label(*fn_decl_span, "consider calling this closure");
+                    let Some(name) = self.get_closure_name(def_id, err, &msg) else {
+                        return false;
+                    };
+                    name.to_string()
+                }
+                Some(hir::Node::Item(hir::Item { ident, kind: hir::ItemKind::Fn(..), .. })) => {
+                    err.span_label(ident.span, "consider calling this function");
+                    ident.to_string()
+                }
+                Some(hir::Node::Ctor(..)) => {
+                    let name = self.tcx.def_path_str(def_id);
+                    err.span_label(
+                        self.tcx.def_span(def_id),
+                        format!("consider calling the constructor for `{}`", name),
+                    );
+                    name
+                }
+                _ => return false,
+            };
+            err.help(&format!("{msg}: `{name}({args})`"));
         }
         true
     }

src/test/ui/binop/issue-77910-1.stderr

@@ -19,7 +19,7 @@ LL |     assert_eq!(foo, y);
    |     ^^^^^^^^^^^^^^^^^^ `for<'a> fn(&'a i32) -> &'a i32 {foo}` cannot be formatted using `{:?}` because it doesn't implement `Debug`
    |
    = help: the trait `Debug` is not implemented for fn item `for<'a> fn(&'a i32) -> &'a i32 {foo}`
-   = help: use parentheses to call the function: `foo(s)`
+   = help: use parentheses to call this function: `foo(/* &i32 */)`
    = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error: aborting due to 2 previous errors

src/test/ui/closures/closure-bounds-subtype.stderr

@@ -11,6 +11,10 @@ note: required by a bound in `take_const_owned`
    |
 LL | fn take_const_owned<F>(_: F) where F: FnOnce() + Sync + Send {
    |                                                  ^^^^ required by this bound in `take_const_owned`
+help: use parentheses to call this type parameter
+   |
+LL |     take_const_owned(f());
+   |                       ++
 help: consider further restricting this bound
    |
 LL | fn give_owned<F>(f: F) where F: FnOnce() + Send + std::marker::Sync {

src/test/ui/issues/issue-35241.stderr

@@ -11,7 +11,7 @@ LL | fn test() -> Foo { Foo }
    |
    = note: expected struct `Foo`
              found fn item `fn(u32) -> Foo {Foo}`
-help: use parentheses to instantiate this tuple struct
+help: use parentheses to construct this tuple struct
    |
 LL | fn test() -> Foo { Foo(/* u32 */) }
    |                       +++++++++++

src/test/ui/issues/issue-70724-add_type_neq_err_label-unwrap.stderr

@@ -29,7 +29,7 @@ LL |     assert_eq!(a, 0);
    |     ^^^^^^^^^^^^^^^^ `fn() -> i32 {a}` cannot be formatted using `{:?}` because it doesn't implement `Debug`
    |
    = help: the trait `Debug` is not implemented for fn item `fn() -> i32 {a}`
-   = help: use parentheses to call the function: `a()`
+   = help: use parentheses to call this function: `a()`
    = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error: aborting due to 3 previous errors