Auto merge of #75608 - estebank:suggest-boxed-match-exprs, r=lcnr,varkor

More structured suggestions for boxed trait objects instead of impl Trait on non-coerceable tail expressions

When encountering a `match` or `if` as a tail expression where the
different arms do not have the same type *and* the return type of that
`fn` is an `impl Trait`, check whether those arms can implement `Trait`
and if so, suggest using boxed trait objects.

Use structured suggestion for `impl T` to `Box<dyn T>`.

Fix #69107

Author: bors

compiler/rustc_infer/src/infer/error_reporting/mod.rs

@@ -70,7 +70,7 @@ use rustc_middle::ty::{
     subst::{Subst, SubstsRef},
     Region, Ty, TyCtxt, TypeFoldable,
 };
-use rustc_span::{DesugaringKind, Pos, Span};
+use rustc_span::{BytePos, DesugaringKind, Pos, Span};
 use rustc_target::spec::abi;
 use std::{cmp, fmt};
 
@@ -617,11 +617,20 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                 ref prior_arms,
                 last_ty,
                 scrut_hir_id,
+                opt_suggest_box_span,
+                arm_span,
                 ..
             }) => match source {
                 hir::MatchSource::IfLetDesugar { .. } => {
                     let msg = "`if let` arms have incompatible types";
                     err.span_label(cause.span, msg);
+                    if let Some(ret_sp) = opt_suggest_box_span {
+                        self.suggest_boxing_for_return_impl_trait(
+                            err,
+                            ret_sp,
+                            prior_arms.iter().chain(std::iter::once(&arm_span)).map(|s| *s),
+                        );
+                    }
                 }
                 hir::MatchSource::TryDesugar => {
                     if let Some(ty::error::ExpectedFound { expected, .. }) = exp_found {
@@ -675,9 +684,23 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                             Applicability::MachineApplicable,
                         );
                     }
+                    if let Some(ret_sp) = opt_suggest_box_span {
+                        // Get return type span and point to it.
+                        self.suggest_boxing_for_return_impl_trait(
+                            err,
+                            ret_sp,
+                            prior_arms.iter().chain(std::iter::once(&arm_span)).map(|s| *s),
+                        );
+                    }
                 }
             },
-            ObligationCauseCode::IfExpression(box IfExpressionCause { then, outer, semicolon }) => {
+            ObligationCauseCode::IfExpression(box IfExpressionCause {
+                then,
+                else_sp,
+                outer,
+                semicolon,
+                opt_suggest_box_span,
+            }) => {
                 err.span_label(then, "expected because of this");
                 if let Some(sp) = outer {
                     err.span_label(sp, "`if` and `else` have incompatible types");
@@ -690,11 +713,48 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                         Applicability::MachineApplicable,
                     );
                 }
+                if let Some(ret_sp) = opt_suggest_box_span {
+                    self.suggest_boxing_for_return_impl_trait(
+                        err,
+                        ret_sp,
+                        vec![then, else_sp].into_iter(),
+                    );
+                }
             }
             _ => (),
         }
     }
 
+    fn suggest_boxing_for_return_impl_trait(
+        &self,
+        err: &mut DiagnosticBuilder<'tcx>,
+        return_sp: Span,
+        arm_spans: impl Iterator<Item = Span>,
+    ) {
+        err.multipart_suggestion(
+            "you could change the return type to be a boxed trait object",
+            vec![
+                (return_sp.with_hi(return_sp.lo() + BytePos(4)), "Box<dyn".to_string()),
+                (return_sp.shrink_to_hi(), ">".to_string()),
+            ],
+            Applicability::MaybeIncorrect,
+        );
+        let sugg = arm_spans
+            .flat_map(|sp| {
+                vec![
+                    (sp.shrink_to_lo(), "Box::new(".to_string()),
+                    (sp.shrink_to_hi(), ")".to_string()),
+                ]
+                .into_iter()
+            })
+            .collect::<Vec<_>>();
+        err.multipart_suggestion(
+            "if you change the return type to expect trait objects, box the returned expressions",
+            sugg,
+            Applicability::MaybeIncorrect,
+        );
+    }
+
     /// Given that `other_ty` is the same as a type argument for `name` in `sub`, populate `value`
     /// highlighting `name` and every type argument that isn't at `pos` (which is `other_ty`), and
     /// populate `other_value` with `other_ty`.

compiler/rustc_middle/src/traits/mod.rs

@@ -348,13 +348,16 @@ pub struct MatchExpressionArmCause<'tcx> {
     pub prior_arms: Vec<Span>,
     pub last_ty: Ty<'tcx>,
     pub scrut_hir_id: hir::HirId,
+    pub opt_suggest_box_span: Option<Span>,
 }
 
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub struct IfExpressionCause {
     pub then: Span,
+    pub else_sp: Span,
     pub outer: Option<Span>,
     pub semicolon: Option<Span>,
+    pub opt_suggest_box_span: Option<Span>,
 }
 
 #[derive(Clone, Debug, PartialEq, Eq, Hash, Lift)]

compiler/rustc_typeck/src/check/_match.rs

@@ -1,28 +1,31 @@
 use crate::check::coercion::CoerceMany;
 use crate::check::{Diverges, Expectation, FnCtxt, Needs};
-use rustc_hir as hir;
-use rustc_hir::ExprKind;
+use rustc_hir::{self as hir, ExprKind};
 use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
-use rustc_middle::ty::Ty;
+use rustc_infer::traits::Obligation;
+use rustc_middle::ty::{self, ToPredicate, Ty};
 use rustc_span::Span;
-use rustc_trait_selection::traits::ObligationCauseCode;
-use rustc_trait_selection::traits::{IfExpressionCause, MatchExpressionArmCause, ObligationCause};
+use rustc_trait_selection::opaque_types::InferCtxtExt as _;
+use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt;
+use rustc_trait_selection::traits::{
+    IfExpressionCause, MatchExpressionArmCause, ObligationCause, ObligationCauseCode,
+};
 
 impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     pub fn check_match(
         &self,
         expr: &'tcx hir::Expr<'tcx>,
         scrut: &'tcx hir::Expr<'tcx>,
         arms: &'tcx [hir::Arm<'tcx>],
-        expected: Expectation<'tcx>,
+        orig_expected: Expectation<'tcx>,
         match_src: hir::MatchSource,
     ) -> Ty<'tcx> {
         let tcx = self.tcx;
 
         use hir::MatchSource::*;
         let (source_if, if_no_else, force_scrutinee_bool) = match match_src {
             IfDesugar { contains_else_clause } => (true, !contains_else_clause, true),
-            IfLetDesugar { contains_else_clause } => (true, !contains_else_clause, false),
+            IfLetDesugar { contains_else_clause, .. } => (true, !contains_else_clause, false),
             WhileDesugar => (false, false, true),
             _ => (false, false, false),
         };
@@ -69,7 +72,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         // type in that case)
         let mut all_arms_diverge = Diverges::WarnedAlways;
 
-        let expected = expected.adjust_for_branches(self);
+        let expected = orig_expected.adjust_for_branches(self);
 
         let mut coercion = {
             let coerce_first = match expected {
@@ -112,14 +115,75 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 self.check_expr_with_expectation(&arm.body, expected)
             };
             all_arms_diverge &= self.diverges.get();
+
+            // When we have a `match` as a tail expression in a `fn` with a returned `impl Trait`
+            // we check if the different arms would work with boxed trait objects instead and
+            // provide a structured suggestion in that case.
+            let opt_suggest_box_span = match (
+                orig_expected,
+                self.ret_coercion_impl_trait.map(|ty| (self.body_id.owner, ty)),
+            ) {
+                (Expectation::ExpectHasType(expected), Some((id, ty)))
+                    if self.in_tail_expr && self.can_coerce(arm_ty, expected) =>
+                {
+                    let impl_trait_ret_ty = self.infcx.instantiate_opaque_types(
+                        id,
+                        self.body_id,
+                        self.param_env,
+                        &ty,
+                        arm.body.span,
+                    );
+                    let mut suggest_box = !impl_trait_ret_ty.obligations.is_empty();
+                    for o in impl_trait_ret_ty.obligations {
+                        match o.predicate.skip_binders_unchecked() {
+                            ty::PredicateAtom::Trait(t, constness) => {
+                                let pred = ty::PredicateAtom::Trait(
+                                    ty::TraitPredicate {
+                                        trait_ref: ty::TraitRef {
+                                            def_id: t.def_id(),
+                                            substs: self.infcx.tcx.mk_substs_trait(arm_ty, &[]),
+                                        },
+                                    },
+                                    constness,
+                                );
+                                let obl = Obligation::new(
+                                    o.cause.clone(),
+                                    self.param_env,
+                                    pred.to_predicate(self.infcx.tcx),
+                                );
+                                suggest_box &= self.infcx.predicate_must_hold_modulo_regions(&obl);
+                                if !suggest_box {
+                                    // We've encountered some obligation that didn't hold, so the
+                                    // return expression can't just be boxed. We don't need to
+                                    // evaluate the rest of the obligations.
+                                    break;
+                                }
+                            }
+                            _ => {}
+                        }
+                    }
+                    // If all the obligations hold (or there are no obligations) the tail expression
+                    // we can suggest to return a boxed trait object instead of an opaque type.
+                    if suggest_box { self.ret_type_span.clone() } else { None }
+                }
+                _ => None,
+            };
+
             if source_if {
                 let then_expr = &arms[0].body;
                 match (i, if_no_else) {
                     (0, _) => coercion.coerce(self, &self.misc(expr.span), &arm.body, arm_ty),
                     (_, true) => {} // Handled above to avoid duplicated type errors (#60254).
                     (_, _) => {
                         let then_ty = prior_arm_ty.unwrap();
-                        let cause = self.if_cause(expr.span, then_expr, &arm.body, then_ty, arm_ty);
+                        let cause = self.if_cause(
+                            expr.span,
+                            then_expr,
+                            &arm.body,
+                            then_ty,
+                            arm_ty,
+                            opt_suggest_box_span,
+                        );
                         coercion.coerce(self, &cause, &arm.body, arm_ty);
                     }
                 }
@@ -142,6 +206,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                             prior_arms: other_arms.clone(),
                             last_ty: prior_arm_ty.unwrap(),
                             scrut_hir_id: scrut.hir_id,
+                            opt_suggest_box_span,
                         }),
                     ),
                 };
@@ -266,6 +331,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         else_expr: &'tcx hir::Expr<'tcx>,
         then_ty: Ty<'tcx>,
         else_ty: Ty<'tcx>,
+        opt_suggest_box_span: Option<Span>,
     ) -> ObligationCause<'tcx> {
         let mut outer_sp = if self.tcx.sess.source_map().is_multiline(span) {
             // The `if`/`else` isn't in one line in the output, include some context to make it
@@ -353,8 +419,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             error_sp,
             ObligationCauseCode::IfExpression(box IfExpressionCause {
                 then: then_sp,
+                else_sp: error_sp,
                 outer: outer_sp,
                 semicolon: remove_semicolon,
+                opt_suggest_box_span,
             }),
         )
     }

compiler/rustc_typeck/src/check/coercion.rs

@@ -37,7 +37,7 @@
 
 use crate::astconv::AstConv;
 use crate::check::FnCtxt;
-use rustc_errors::{struct_span_err, DiagnosticBuilder};
+use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder};
 use rustc_hir as hir;
 use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
 use rustc_infer::infer::{Coercion, InferOk, InferResult};
@@ -51,7 +51,7 @@ use rustc_middle::ty::subst::SubstsRef;
 use rustc_middle::ty::{self, Ty, TypeAndMut};
 use rustc_session::parse::feature_err;
 use rustc_span::symbol::sym;
-use rustc_span::{self, Span};
+use rustc_span::{self, BytePos, Span};
 use rustc_target::spec::abi::Abi;
 use rustc_trait_selection::traits::error_reporting::InferCtxtExt;
 use rustc_trait_selection::traits::{self, ObligationCause, ObligationCauseCode};
@@ -1459,14 +1459,15 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
             }
         }
         if let (Some(sp), Some(fn_output)) = (fcx.ret_coercion_span.borrow().as_ref(), fn_output) {
-            self.add_impl_trait_explanation(&mut err, fcx, expected, *sp, fn_output);
+            self.add_impl_trait_explanation(&mut err, cause, fcx, expected, *sp, fn_output);
         }
         err
     }
 
     fn add_impl_trait_explanation<'a>(
         &self,
         err: &mut DiagnosticBuilder<'a>,
+        cause: &ObligationCause<'tcx>,
         fcx: &FnCtxt<'a, 'tcx>,
         expected: Ty<'tcx>,
         sp: Span,
@@ -1523,10 +1524,30 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
         };
         if has_impl {
             if is_object_safe {
-                err.help(&format!(
-                    "you can instead return a boxed trait object using `Box<dyn {}>`",
-                    &snippet[5..]
-                ));
+                err.multipart_suggestion(
+                    "you could change the return type to be a boxed trait object",
+                    vec![
+                        (return_sp.with_hi(return_sp.lo() + BytePos(4)), "Box<dyn".to_string()),
+                        (return_sp.shrink_to_hi(), ">".to_string()),
+                    ],
+                    Applicability::MachineApplicable,
+                );
+                let sugg = vec![sp, cause.span]
+                    .into_iter()
+                    .flat_map(|sp| {
+                        vec![
+                            (sp.shrink_to_lo(), "Box::new(".to_string()),
+                            (sp.shrink_to_hi(), ")".to_string()),
+                        ]
+                        .into_iter()
+                    })
+                    .collect::<Vec<_>>();
+                err.multipart_suggestion(
+                    "if you change the return type to expect trait objects, box the returned \
+                     expressions",
+                    sugg,
+                    Applicability::MaybeIncorrect,
+                );
             } else {
                 err.help(&format!(
                     "if the trait `{}` were object safe, you could return a boxed trait object",
@@ -1535,7 +1556,7 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
             }
             err.note(trait_obj_msg);
         }
-        err.help("alternatively, create a new `enum` with a variant for each returned type");
+        err.help("you could instead create a new `enum` with a variant for each returned type");
     }
 
     fn is_return_ty_unsized(&self, fcx: &FnCtxt<'a, 'tcx>, blk_id: hir::HirId) -> bool {