Rollup merge of rust-lang#97206 - jackh726:issue-73154, r=nikomatsakis

Do leak check after function pointer coercion

cc rust-lang#73154

I still need to clean diagnostics just a tad, but figured I would put this up anyways.

This change is made in order to make match arm coercion order-independent.

Basically, any time we do function pointer coercion, we follow it by doing a leak check. This is necessary because the LUB code doesn't handler higher-ranked things correctly, leading us to "coerce", but use the wrong type. A proper fix is to actually fix that code (so the type returned by `unify_and` is a supertype of both `a` and `b` if `Ok`). However, that requires a more in-depth fix, likely heavily overlapping with the new subtyping changes.

Here, I've been conservative and error early if we generate unsatisfiable constraints. Note, this should *mostly* only affect NLL, since migrate mode falls back to the LUB implementation (followed by leak check), whereas NLL only does sub.

There could be other coercion code that has an order-dependence where a leak check in the coercion code might be useful. However, this is more of a spot-fix for rust-lang#73154 than a "permanent" fix, since we likely want to go the other way long-term, and allow this pattern without error.

r? `@nikomatsakis`

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

@@ -1442,6 +1442,10 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
     /// the message in `secondary_span` as the primary label, and apply the message that would
     /// otherwise be used for the primary label on the `secondary_span` `Span`. This applies on
     /// E0271, like `src/test/ui/issues/issue-39970.stderr`.
+    #[tracing::instrument(
+        level = "debug",
+        skip(self, diag, secondary_span, swap_secondary_and_primary, force_label)
+    )]
     pub fn note_type_err(
         &self,
         diag: &mut Diagnostic,
@@ -1453,7 +1457,6 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
         force_label: bool,
     ) {
         let span = cause.span(self.tcx);
-        debug!("note_type_err cause={:?} values={:?}, terr={:?}", cause, values, terr);
 
         // For some types of errors, expected-found does not make
         // sense, so just ignore the values we were given.
@@ -1621,9 +1624,9 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
             }
         };
 
-        // Ignore msg for object safe coercion
-        // since E0038 message will be printed
         match terr {
+            // Ignore msg for object safe coercion
+            // since E0038 message will be printed
             TypeError::ObjectUnsafeCoercion(_) => {}
             _ => {
                 let mut label_or_note = |span: Span, msg: &str| {
@@ -1774,6 +1777,8 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
         // It reads better to have the error origin as the final
         // thing.
         self.note_error_origin(diag, cause, exp_found, terr);
+
+        debug!(?diag);
     }
 
     fn suggest_tuple_pattern(

compiler/rustc_middle/src/ty/error.rs

@@ -135,11 +135,10 @@ impl<'tcx> fmt::Display for TypeError<'tcx> {
             ArgCount => write!(f, "incorrect number of function parameters"),
             FieldMisMatch(adt, field) => write!(f, "field type mismatch: {}.{}", adt, field),
             RegionsDoesNotOutlive(..) => write!(f, "lifetime mismatch"),
-            RegionsInsufficientlyPolymorphic(br, _) => write!(
-                f,
-                "expected bound lifetime parameter{}, found concrete lifetime",
-                br_string(br)
-            ),
+            // Actually naming the region here is a bit confusing because context is lacking
+            RegionsInsufficientlyPolymorphic(..) => {
+                write!(f, "one type is more general than the other")
+            }
             RegionsOverlyPolymorphic(br, _) => write!(
                 f,
                 "expected concrete lifetime, found bound lifetime parameter{}",

compiler/rustc_mir_build/src/build/matches/mod.rs

@@ -151,6 +151,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     ///
     /// * From each pre-binding block to the next pre-binding block.
     /// * From each otherwise block to the next pre-binding block.
+    #[tracing::instrument(level = "debug", skip(self, arms))]
     crate fn match_expr(
         &mut self,
         destination: Place<'tcx>,

compiler/rustc_mir_build/src/thir/cx/block.rs

@@ -74,6 +74,7 @@ impl<'tcx> Cx<'tcx> {
                         };
 
                         let mut pattern = self.pattern_from_hir(local.pat);
+                        debug!(?pattern);
 
                         if let Some(ty) = &local.ty {
                             if let Some(&user_ty) =

compiler/rustc_mir_build/src/thir/cx/mod.rs

@@ -96,6 +96,7 @@ impl<'tcx> Cx<'tcx> {
         }
     }
 
+    #[tracing::instrument(level = "debug", skip(self))]
     crate fn pattern_from_hir(&mut self, p: &hir::Pat<'_>) -> Pat<'tcx> {
         let p = match self.tcx.hir().get(p.hir_id) {
             Node::Pat(p) | Node::Binding(p) => p,

compiler/rustc_typeck/src/check/_match.rs

@@ -56,6 +56,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         let mut all_arms_diverge = Diverges::WarnedAlways;
 
         let expected = orig_expected.adjust_for_branches(self);
+        debug!(?expected);
 
         let mut coercion = {
             let coerce_first = match expected {
@@ -127,6 +128,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 Some(&arm.body),
                 arm_ty,
                 Some(&mut |err: &mut Diagnostic| {
+                    let Some(ret) = self.ret_type_span else {
+                        return;
+                    };
+                    let Expectation::IsLast(stmt) = orig_expected else {
+                        return
+                    };
                     let can_coerce_to_return_ty = match self.ret_coercion.as_ref() {
                         Some(ret_coercion) if self.in_tail_expr => {
                             let ret_ty = ret_coercion.borrow().expected_ty();
@@ -138,38 +145,38 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                         }
                         _ => false,
                     };
-                    if let (Expectation::IsLast(stmt), Some(ret), true) =
-                        (orig_expected, self.ret_type_span, can_coerce_to_return_ty)
-                    {
-                        let semi_span = expr.span.shrink_to_hi().with_hi(stmt.hi());
-                        let mut ret_span: MultiSpan = semi_span.into();
-                        ret_span.push_span_label(
-                            expr.span,
-                            "this could be implicitly returned but it is a statement, not a \
-                                tail expression"
-                                .to_owned(),
-                        );
-                        ret_span.push_span_label(
-                            ret,
-                            "the `match` arms can conform to this return type".to_owned(),
-                        );
-                        ret_span.push_span_label(
-                            semi_span,
-                            "the `match` is a statement because of this semicolon, consider \
-                                removing it"
-                                .to_owned(),
-                        );
-                        err.span_note(
-                            ret_span,
-                            "you might have meant to return the `match` expression",
-                        );
-                        err.tool_only_span_suggestion(
-                            semi_span,
-                            "remove this semicolon",
-                            String::new(),
-                            Applicability::MaybeIncorrect,
-                        );
+                    if !can_coerce_to_return_ty {
+                        return;
                     }
+
+                    let semi_span = expr.span.shrink_to_hi().with_hi(stmt.hi());
+                    let mut ret_span: MultiSpan = semi_span.into();
+                    ret_span.push_span_label(
+                        expr.span,
+                        "this could be implicitly returned but it is a statement, not a \
+                            tail expression"
+                            .to_owned(),
+                    );
+                    ret_span.push_span_label(
+                        ret,
+                        "the `match` arms can conform to this return type".to_owned(),
+                    );
+                    ret_span.push_span_label(
+                        semi_span,
+                        "the `match` is a statement because of this semicolon, consider \
+                            removing it"
+                            .to_owned(),
+                    );
+                    err.span_note(
+                        ret_span,
+                        "you might have meant to return the `match` expression",
+                    );
+                    err.tool_only_span_suggestion(
+                        semi_span,
+                        "remove this semicolon",
+                        String::new(),
+                        Applicability::MaybeIncorrect,
+                    );
                 }),
                 false,
             );
@@ -199,7 +206,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         // We won't diverge unless the scrutinee or all arms diverge.
         self.diverges.set(scrut_diverges | all_arms_diverge);
 
-        coercion.complete(self)
+        let match_ty = coercion.complete(self);
+        debug!(?match_ty);
+        match_ty
     }
 
     fn get_appropriate_arm_semicolon_removal_span(

compiler/rustc_typeck/src/check/coercion.rs

@@ -737,14 +737,27 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> {
         F: FnOnce(Ty<'tcx>) -> Vec<Adjustment<'tcx>>,
         G: FnOnce(Ty<'tcx>) -> Vec<Adjustment<'tcx>>,
     {
-        if let ty::FnPtr(fn_ty_b) = b.kind()
-            && let (hir::Unsafety::Normal, hir::Unsafety::Unsafe) =
-                (fn_ty_a.unsafety(), fn_ty_b.unsafety())
-        {
-            let unsafe_a = self.tcx.safe_to_unsafe_fn_ty(fn_ty_a);
-            return self.unify_and(unsafe_a, b, to_unsafe);
-        }
-        self.unify_and(a, b, normal)
+        self.commit_unconditionally(|snapshot| {
+            let result = if let ty::FnPtr(fn_ty_b) = b.kind()
+                && let (hir::Unsafety::Normal, hir::Unsafety::Unsafe) =
+                    (fn_ty_a.unsafety(), fn_ty_b.unsafety())
+            {
+                let unsafe_a = self.tcx.safe_to_unsafe_fn_ty(fn_ty_a);
+                self.unify_and(unsafe_a, b, to_unsafe)
+            } else {
+                self.unify_and(a, b, normal)
+            };
+
+            // FIXME(#73154): This is a hack. Currently LUB can generate
+            // unsolvable constraints. Additionally, it returns `a`
+            // unconditionally, even when the "LUB" is `b`. In the future, we
+            // want the coerced type to be the actual supertype of these two,
+            // but for now, we want to just error to ensure we don't lock
+            // ourselves into a specific behavior with NLL.
+            self.leak_check(false, snapshot)?;
+
+            result
+        })
     }
 
     fn coerce_from_fn_pointer(
@@ -1133,8 +1146,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                     let (adjustments, target) = self.register_infer_ok_obligations(ok);
                     self.apply_adjustments(new, adjustments);
                     debug!(
-                        "coercion::try_find_coercion_lub: was able to coerce from previous type {:?} to new type {:?}",
-                        prev_ty, new_ty,
+                        "coercion::try_find_coercion_lub: was able to coerce from new type {:?} to previous type {:?} ({:?})",
+                        new_ty, prev_ty, target
                     );
                     return Ok(target);
                 }
@@ -1190,15 +1203,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 }
             }
             Ok(ok) => {
-                debug!(
-                    "coercion::try_find_coercion_lub: was able to coerce previous type {:?} to new type {:?}",
-                    prev_ty, new_ty,
-                );
                 let (adjustments, target) = self.register_infer_ok_obligations(ok);
                 for expr in exprs {
                     let expr = expr.as_coercion_site();
                     self.apply_adjustments(expr, adjustments.clone());
                 }
+                debug!(
+                    "coercion::try_find_coercion_lub: was able to coerce previous type {:?} to new type {:?} ({:?})",
+                    prev_ty, new_ty, target
+                );
                 Ok(target)
             }
         }
@@ -1430,6 +1443,7 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
                 })
         };
 
+        debug!(?result);
         match result {
             Ok(v) => {
                 self.final_ty = Some(v);
@@ -1520,7 +1534,10 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
                     augment_error(&mut err);
                 }
 
-                if let Some(expr) = expression {
+                let is_insufficiently_polymorphic =
+                    matches!(coercion_error, TypeError::RegionsInsufficientlyPolymorphic(..));
+
+                if !is_insufficiently_polymorphic && let Some(expr) = expression {
                     fcx.emit_coerce_suggestions(
                         &mut err,
                         expr,

compiler/rustc_typeck/src/check/demand.rs

@@ -129,6 +129,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     ///
     /// N.B., this code relies on `self.diverges` to be accurate. In particular, assignments to `!`
     /// will be permitted if the diverges flag is currently "always".
+    #[tracing::instrument(level = "debug", skip(self, expr, expected_ty_expr, allow_two_phase))]
     pub fn demand_coerce_diag(
         &self,
         expr: &hir::Expr<'tcx>,
@@ -150,7 +151,22 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         let expr_ty = self.resolve_vars_with_obligations(checked_ty);
         let mut err = self.report_mismatched_types(&cause, expected, expr_ty, e.clone());
 
-        self.emit_coerce_suggestions(&mut err, expr, expr_ty, expected, expected_ty_expr, Some(e));
+        let is_insufficiently_polymorphic =
+            matches!(e, TypeError::RegionsInsufficientlyPolymorphic(..));
+
+        // FIXME(#73154): For now, we do leak check when coercing function
+        // pointers in typeck, instead of only during borrowck. This can lead
+        // to these `RegionsInsufficientlyPolymorphic` errors that aren't helpful.
+        if !is_insufficiently_polymorphic {
+            self.emit_coerce_suggestions(
+                &mut err,
+                expr,
+                expr_ty,
+                expected,
+                expected_ty_expr,
+                Some(e),
+            );
+        }
 
         (expected, Some(err))
     }

src/test/ui/hrtb/hrtb-exists-forall-fn.nll.stderr

@@ -1,8 +1,8 @@
 error[E0308]: mismatched types
-  --> $DIR/hrtb-exists-forall-fn.rs:17:12
+  --> $DIR/hrtb-exists-forall-fn.rs:17:34
    |
 LL |     let _: for<'b> fn(&'b u32) = foo();
-   |            ^^^^^^^^^^^^^^^^^^^ one type is more general than the other
+   |                                  ^^^^^ one type is more general than the other
    |
    = note: expected fn pointer `for<'b> fn(&'b u32)`
               found fn pointer `fn(&u32)`

src/test/ui/lub-glb/old-lub-glb-hr-noteq1.stderr → src/test/ui/lub-glb/old-lub-glb-hr-noteq1.baseleak.stderr

@@ -1,12 +1,15 @@
 error[E0308]: `match` arms have incompatible types
-  --> $DIR/old-lub-glb-hr-noteq1.rs:11:14
+  --> $DIR/old-lub-glb-hr-noteq1.rs:17:14
    |
 LL |       let z = match 22 {
    |  _____________-
 LL | |         0 => x,
    | |              - this is found to be of type `for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8`
 LL | |         _ => y,
    | |              ^ one type is more general than the other
+LL | |
+...  |
+LL | |
 LL | |     };
    | |_____- `match` arms have incompatible types
    |

src/test/ui/lub-glb/old-lub-glb-hr-noteq1.basenoleak.stderr

@@ -0,0 +1,21 @@
+error[E0308]: `match` arms have incompatible types
+  --> $DIR/old-lub-glb-hr-noteq1.rs:17:14
+   |
+LL |       let z = match 22 {
+   |  _____________-
+LL | |         0 => x,
+   | |              - this is found to be of type `for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8`
+LL | |         _ => y,
+   | |              ^ one type is more general than the other
+LL | |
+...  |
+LL | |
+LL | |     };
+   | |_____- `match` arms have incompatible types
+   |
+   = note: expected fn pointer `for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8`
+              found fn pointer `for<'a> fn(&'a u8, &'a u8) -> &'a u8`
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0308`.

src/test/ui/lub-glb/old-lub-glb-hr-noteq1.nllleak.stderr

@@ -0,0 +1,21 @@
+error[E0308]: `match` arms have incompatible types
+  --> $DIR/old-lub-glb-hr-noteq1.rs:17:14
+   |
+LL |       let z = match 22 {
+   |  _____________-
+LL | |         0 => x,
+   | |              - this is found to be of type `for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8`
+LL | |         _ => y,
+   | |              ^ one type is more general than the other
+LL | |
+...  |
+LL | |
+LL | |     };
+   | |_____- `match` arms have incompatible types
+   |
+   = note: expected fn pointer `for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8`
+              found fn pointer `for<'a> fn(&'a u8, &'a u8) -> &'a u8`
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0308`.

src/test/ui/lub-glb/old-lub-glb-hr-noteq1.nll.stderr → src/test/ui/lub-glb/old-lub-glb-hr-noteq1.nllnoleak.stderr

@@ -1,5 +1,5 @@
 error[E0308]: mismatched types
-  --> $DIR/old-lub-glb-hr-noteq1.rs:11:14
+  --> $DIR/old-lub-glb-hr-noteq1.rs:17:14
    |
 LL |         _ => y,
    |              ^ one type is more general than the other