Auto merge of rust-lang#118247 - spastorino:type-equality-subtyping, r=lcnr

change equate for binders to not rely on subtyping

*summary by `@spastorino` and `@lcnr*`

### Context

The following code:

```rust
type One = for<'a> fn(&'a (), &'a ());
type Two = for<'a, 'b> fn(&'a (), &'b ());

mod my_api {
    use std::any::Any;
    use std::marker::PhantomData;

    pub struct Foo<T: 'static> {
        a: &'static dyn Any,
        _p: PhantomData<*mut T>, // invariant, the type of the `dyn Any`
    }

    impl<T: 'static> Foo<T> {
        pub fn deref(&self) -> &'static T {
            match self.a.downcast_ref::<T>() {
                None => unsafe { std::hint::unreachable_unchecked() },
                Some(a) => a,
            }
        }

        pub fn new(a: T) -> Foo<T> {
           Foo::<T> {
                a: Box::leak(Box::new(a)),
                _p: PhantomData,
            }
        }
    }
}

use my_api::*;

fn main() {
    let foo = Foo::<One>::new((|_, _| ()) as One);
    foo.deref();
    let foo: Foo<Two> = foo;
    foo.deref();
}
```

has UB from hitting the `unreachable_unchecked`. This happens because `TypeId::of::<One>()` is not the same as `TypeId::of::<Two>()` despite them being considered the same types by the type checker.

Currently the type checker considers binders to be equal if subtyping succeeds in both directions: `for<'a> T<'a> eq for<'b> U<'b>` holds if `for<'a> exists<'b> T<'b> <: T'<a> AND for<'b> exists<'a> T<'a> <: T<'b>` holds. This results in `for<'a> fn(&'a (), &'a ())` and `for<'a, 'b> fn(&'a (), &'b ())` being equal in the type system.

`TypeId` is computed by looking at the *structure* of a type. Even though these types are semantically equal, they have a different *structure* resulting in them having different `TypeId`. This can break invariants of unsafe code at runtime and is unsound when happening at compile time, e.g. when using const generics.

So as seen in `main`, we can assign a value of type `Foo::<One>` to a binding of type `Foo<Two>` given those are considered the same type but then when we call `deref`, it calls `downcast_ref` that relies on `TypeId` and we would hit the `None` arm as these have different `TypeId`s.

As stated in rust-lang#97156 (comment), this causes the API of existing crates to be unsound.

## What should we do about this

The same type resulting in different `TypeId`s  is a significant footgun, breaking a very reasonable assumptions by authors of unsafe code. It will also be unsound by itself once they are usable in generic contexts with const generics.

There are two options going forward here:
- change how the *structure* of a type is computed before relying on it. i.e. continue considering `for<'a> fn(&'a (), &'a ())` and `for<'a, 'b> fn(&'a (), &'b ())` to be equal, but normalize them to a common representation so that their `TypeId` are also the same.
- change how the semantic equality of binders to match the way we compute the structure of types. i.e. `for<'a> fn(&'a (), &'a ())` and `for<'a, 'b> fn(&'a (), &'b ())` still have different `TypeId`s but are now also considered to not be semantically equal.

---

Advantages of the first approach:
- with the second approach some higher ranked types stop being equal, even though they are subtypes of each other

General thoughts:
- changing the approach in the future will be breaking
    - going from first to second may break ordinary type checking, as types which were previously equal are now distinct
    - going from second to first may break coherence, because previously disjoint impls overlap as the used types are now equal
    - both of these are quite unlikely. This PR did not result in any crater failures, so this should not matter too much

Advantages of the second approach:
- the soundness of the first approach requires more non-local reasoning. We have to make sure that changes to subtyping do not cause the representative computation to diverge from semantic equality
    - e.g. we intend to consider higher ranked implied bounds when subtyping to [fix] rust-lang#25860, I don't know how this will interact and don't feel confident making any prediction here.
- computing a representative type is non-trivial and soundness critical, therefore adding complexity to the "core type system"

---

This PR goes with the second approach. A crater run did not result in any regressions. I am personally very hesitant about trying the first approach due to the above reasons. It feels like there are more unknowns when going that route.

### Changing the way we equate binders

Relating bound variables from different depths already results in a universe error in equate. We therefore only need to make sure that there is 1-to-1 correspondence between bound variables when relating binders. This results in concrete types being structurally equal after anonymizing their bound variables.

We implement this by instantiating one of the binder with placeholders and the other with inference variables and then equating the instantiated types. We do so in both directions.

More formally, we change the typing rules as follows:

```
for<'r0, .., 'rn> exists<'l0, .., 'ln> LHS<'l0, .., 'ln> <: RHS<'r0, .., 'rn>
for<'l0, .., 'ln> exists<'r0, .., 'rn> RHS<'r0, .., 'rn> <: LHS<'l0, .., 'ln>
--------------------------------------------------------------------------
for<'l0, .., 'ln> LHS<'l0, .., 'ln> eq for<'r0, .., 'rn> RHS<'r0, .., 'rn>
```

to
```
for<'r0, .., 'rn> exists<'l0, .., 'ln> LHS<'l0, .., 'ln> eq RHS<'r0, .., 'rn>
for<'l0, .., 'ln> exists<'r0, .., 'rn> RHS<'r0, .., 'rn> eq LHS<'l0, .., 'ln>
--------------------------------------------------------------------------
for<'l0, .., 'ln> LHS<'l0, .., 'ln> eq for<'r0, .., 'rn> RHS<'r0, .., 'rn>
```

---

Fixes rust-lang#97156

r? `@lcnr`

Author: bors

compiler/rustc_borrowck/src/type_check/relate_tys.rs

@@ -483,61 +483,59 @@ impl<'bccx, 'tcx> TypeRelation<'tcx> for NllTypeRelating<'_, 'bccx, 'tcx> {
             return Ok(ty::Binder::dummy(a));
         }
 
-        if self.ambient_covariance() {
-            // Covariance, so we want `for<..> A <: for<..> B` --
-            // therefore we compare any instantiation of A (i.e., A
-            // instantiated with existentials) against every
-            // instantiation of B (i.e., B instantiated with
-            // universals).
-
-            // Reset the ambient variance to covariant. This is needed
-            // to correctly handle cases like
-            //
-            //     for<'a> fn(&'a u32, &'a u32) == for<'b, 'c> fn(&'b u32, &'c u32)
-            //
-            // Somewhat surprisingly, these two types are actually
-            // **equal**, even though the one on the right looks more
-            // polymorphic. The reason is due to subtyping. To see it,
-            // consider that each function can call the other:
-            //
-            // - The left function can call the right with `'b` and
-            //   `'c` both equal to `'a`
-            //
-            // - The right function can call the left with `'a` set to
-            //   `{P}`, where P is the point in the CFG where the call
-            //   itself occurs. Note that `'b` and `'c` must both
-            //   include P. At the point, the call works because of
-            //   subtyping (i.e., `&'b u32 <: &{P} u32`).
-            let variance = std::mem::replace(&mut self.ambient_variance, ty::Variance::Covariant);
-
-            // Note: the order here is important. Create the placeholders first, otherwise
-            // we assign the wrong universe to the existential!
-            self.enter_forall(b, |this, b| {
-                let a = this.instantiate_binder_with_existentials(a);
-                this.relate(a, b)
-            })?;
-
-            self.ambient_variance = variance;
-        }
+        match self.ambient_variance {
+            ty::Variance::Covariant => {
+                // Covariance, so we want `for<..> A <: for<..> B` --
+                // therefore we compare any instantiation of A (i.e., A
+                // instantiated with existentials) against every
+                // instantiation of B (i.e., B instantiated with
+                // universals).
+
+                // Note: the order here is important. Create the placeholders first, otherwise
+                // we assign the wrong universe to the existential!
+                self.enter_forall(b, |this, b| {
+                    let a = this.instantiate_binder_with_existentials(a);
+                    this.relate(a, b)
+                })?;
+            }
 
-        if self.ambient_contravariance() {
-            // Contravariance, so we want `for<..> A :> for<..> B`
-            // -- therefore we compare every instantiation of A (i.e.,
-            // A instantiated with universals) against any
-            // instantiation of B (i.e., B instantiated with
-            // existentials). Opposite of above.
-
-            // Reset ambient variance to contravariance. See the
-            // covariant case above for an explanation.
-            let variance =
-                std::mem::replace(&mut self.ambient_variance, ty::Variance::Contravariant);
-
-            self.enter_forall(a, |this, a| {
-                let b = this.instantiate_binder_with_existentials(b);
-                this.relate(a, b)
-            })?;
-
-            self.ambient_variance = variance;
+            ty::Variance::Contravariant => {
+                // Contravariance, so we want `for<..> A :> for<..> B` --
+                // therefore we compare every instantiation of A (i.e., A
+                // instantiated with universals) against any
+                // instantiation of B (i.e., B instantiated with
+                // existentials). Opposite of above.
+
+                // Note: the order here is important. Create the placeholders first, otherwise
+                // we assign the wrong universe to the existential!
+                self.enter_forall(a, |this, a| {
+                    let b = this.instantiate_binder_with_existentials(b);
+                    this.relate(a, b)
+                })?;
+            }
+
+            ty::Variance::Invariant => {
+                // Invariant, so we want `for<..> A == for<..> B` --
+                // therefore we want `exists<..> A == for<..> B` and
+                // `exists<..> B == for<..> A`.
+                //
+                // See the comment in `fn Equate::binders` for more details.
+
+                // Note: the order here is important. Create the placeholders first, otherwise
+                // we assign the wrong universe to the existential!
+                self.enter_forall(b, |this, b| {
+                    let a = this.instantiate_binder_with_existentials(a);
+                    this.relate(a, b)
+                })?;
+                // Note: the order here is important. Create the placeholders first, otherwise
+                // we assign the wrong universe to the existential!
+                self.enter_forall(a, |this, a| {
+                    let b = this.instantiate_binder_with_existentials(b);
+                    this.relate(a, b)
+                })?;
+            }
+
+            ty::Variance::Bivariant => {}
         }
 
         Ok(a)

compiler/rustc_hir_analysis/src/check/intrinsic.rs

@@ -168,6 +168,7 @@ pub fn check_intrinsic_type(
     let name_str = intrinsic_name.as_str();
 
     let bound_vars = tcx.mk_bound_variable_kinds(&[
+        ty::BoundVariableKind::Region(ty::BrAnon),
         ty::BoundVariableKind::Region(ty::BrAnon),
         ty::BoundVariableKind::Region(ty::BrEnv),
     ]);
@@ -181,7 +182,7 @@ pub fn check_intrinsic_type(
             let env_region = ty::Region::new_bound(
                 tcx,
                 ty::INNERMOST,
-                ty::BoundRegion { var: ty::BoundVar::from_u32(1), kind: ty::BrEnv },
+                ty::BoundRegion { var: ty::BoundVar::from_u32(2), kind: ty::BrEnv },
             );
             let va_list_ty = tcx.type_of(did).instantiate(tcx, &[region.into()]);
             (Ty::new_ref(tcx, env_region, ty::TypeAndMut { ty: va_list_ty, mutbl }), va_list_ty)
@@ -493,9 +494,12 @@ pub fn check_intrinsic_type(
 
             sym::raw_eq => {
                 let br = ty::BoundRegion { var: ty::BoundVar::from_u32(0), kind: ty::BrAnon };
-                let param_ty =
+                let param_ty_lhs =
+                    Ty::new_imm_ref(tcx, ty::Region::new_bound(tcx, ty::INNERMOST, br), param(0));
+                let br = ty::BoundRegion { var: ty::BoundVar::from_u32(1), kind: ty::BrAnon };
+                let param_ty_rhs =
                     Ty::new_imm_ref(tcx, ty::Region::new_bound(tcx, ty::INNERMOST, br), param(0));
-                (1, 0, vec![param_ty; 2], tcx.types.bool)
+                (1, 0, vec![param_ty_lhs, param_ty_rhs], tcx.types.bool)
             }
 
             sym::black_box => (1, 0, vec![param(0)], param(0)),

compiler/rustc_infer/src/infer/relate/equate.rs

@@ -1,12 +1,13 @@
 use super::combine::{CombineFields, ObligationEmittingRelation};
 use super::StructurallyRelateAliases;
+use crate::infer::BoundRegionConversionTime::HigherRankedType;
 use crate::infer::{DefineOpaqueTypes, SubregionOrigin};
 use crate::traits::PredicateObligations;
 
 use rustc_middle::ty::relate::{self, Relate, RelateResult, TypeRelation};
 use rustc_middle::ty::GenericArgsRef;
 use rustc_middle::ty::TyVar;
-use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitableExt};
+use rustc_middle::ty::{self, Ty, TyCtxt};
 
 use rustc_hir::def_id::DefId;
 use rustc_span::Span;
@@ -167,12 +168,34 @@ impl<'tcx> TypeRelation<'tcx> for Equate<'_, '_, 'tcx> {
             return Ok(a);
         }
 
-        if a.skip_binder().has_escaping_bound_vars() || b.skip_binder().has_escaping_bound_vars() {
-            self.fields.higher_ranked_sub(a, b, self.a_is_expected)?;
-            self.fields.higher_ranked_sub(b, a, self.a_is_expected)?;
-        } else {
+        if let (Some(a), Some(b)) = (a.no_bound_vars(), b.no_bound_vars()) {
             // Fast path for the common case.
-            self.relate(a.skip_binder(), b.skip_binder())?;
+            self.relate(a, b)?;
+        } else {
+            // When equating binders, we check that there is a 1-to-1
+            // correspondence between the bound vars in both types.
+            //
+            // We do so by separately instantiating one of the binders with
+            // placeholders and the other with inference variables and then
+            // equating the instantiated types.
+            //
+            // We want `for<..> A == for<..> B` -- therefore we want
+            // `exists<..> A == for<..> B` and `exists<..> B == for<..> A`.
+
+            let span = self.fields.trace.cause.span;
+            let infcx = self.fields.infcx;
+
+            // Check if `exists<..> A == for<..> B`
+            infcx.enter_forall(b, |b| {
+                let a = infcx.instantiate_binder_with_fresh_vars(span, HigherRankedType, a);
+                self.relate(a, b)
+            })?;
+
+            // Check if `exists<..> B == for<..> A`.
+            infcx.enter_forall(a, |a| {
+                let b = infcx.instantiate_binder_with_fresh_vars(span, HigherRankedType, b);
+                self.relate(a, b)
+            })?;
         }
         Ok(a)
     }

tests/ui/associated-inherent-types/issue-111404-1.rs

@@ -8,7 +8,9 @@ impl<'a> Foo<fn(&'a ())> {
 }
 
 fn bar(_: fn(Foo<for<'b> fn(Foo<fn(&'b ())>::Assoc)>::Assoc)) {}
-//~^ ERROR higher-ranked subtype error
+//~^ ERROR mismatched types [E0308]
+//~| ERROR mismatched types [E0308]
+//~| ERROR higher-ranked subtype error
 //~| ERROR higher-ranked subtype error
 
 fn main() {}

tests/ui/associated-inherent-types/issue-111404-1.stderr

@@ -1,3 +1,22 @@
+error[E0308]: mismatched types
+  --> $DIR/issue-111404-1.rs:10:11
+   |
+LL | fn bar(_: fn(Foo<for<'b> fn(Foo<fn(&'b ())>::Assoc)>::Assoc)) {}
+   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ one type is more general than the other
+   |
+   = note: expected struct `Foo<fn(&())>`
+              found struct `Foo<for<'b> fn(&'b ())>`
+
+error[E0308]: mismatched types
+  --> $DIR/issue-111404-1.rs:10:11
+   |
+LL | fn bar(_: fn(Foo<for<'b> fn(Foo<fn(&'b ())>::Assoc)>::Assoc)) {}
+   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ one type is more general than the other
+   |
+   = note: expected struct `Foo<fn(&())>`
+              found struct `Foo<for<'b> fn(&'b ())>`
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+
 error: higher-ranked subtype error
   --> $DIR/issue-111404-1.rs:10:1
    |
@@ -12,5 +31,6 @@ LL | fn bar(_: fn(Foo<for<'b> fn(Foo<fn(&'b ())>::Assoc)>::Assoc)) {}
    |
    = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
 
-error: aborting due to 2 previous errors
+error: aborting due to 4 previous errors
 
+For more information about this error, try `rustc --explain E0308`.

tests/ui/closure-expected-type/expect-fn-supply-fn.rs

@@ -30,14 +30,16 @@ fn expect_free_supply_bound() {
     // Here, we are given a function whose region is bound at closure level,
     // but we expect one bound in the argument. Error results.
     with_closure_expecting_fn_with_free_region(|x: fn(&u32), y| {});
-    //~^ ERROR mismatched types
+    //~^ ERROR mismatched types [E0308]
+    //~| ERROR lifetime may not live long enough
 }
 
 fn expect_bound_supply_free_from_fn<'x>(x: &'x u32) {
     // Here, we are given a `fn(&u32)` but we expect a `fn(&'x
     // u32)`. In principle, this could be ok, but we demand equality.
     with_closure_expecting_fn_with_bound_region(|x: fn(&'x u32), y| {});
-    //~^ ERROR mismatched types
+    //~^ ERROR mismatched types [E0308]
+    //~| ERROR lifetime may not live long enough
 }
 
 fn expect_bound_supply_free_from_closure() {

tests/ui/closure-expected-type/expect-fn-supply-fn.stderr

@@ -19,6 +19,15 @@ LL | fn expect_free_supply_free_from_fn<'x>(x: &'x u32) {
 LL |     with_closure_expecting_fn_with_free_region(|x: fn(&'x u32), y| {});
    |                                                 ^ requires that `'x` must outlive `'static`
 
+error: lifetime may not live long enough
+  --> $DIR/expect-fn-supply-fn.rs:32:49
+   |
+LL |     with_closure_expecting_fn_with_free_region(|x: fn(&u32), y| {});
+   |                                                 ^
+   |                                                 |
+   |                                                 has type `fn(&'1 u32)`
+   |                                                 requires that `'1` must outlive `'static`
+
 error[E0308]: mismatched types
   --> $DIR/expect-fn-supply-fn.rs:32:49
    |
@@ -29,23 +38,32 @@ LL |     with_closure_expecting_fn_with_free_region(|x: fn(&u32), y| {});
               found fn pointer `for<'a> fn(&'a _)`
 
 error[E0308]: mismatched types
-  --> $DIR/expect-fn-supply-fn.rs:39:50
+  --> $DIR/expect-fn-supply-fn.rs:40:50
    |
 LL |     with_closure_expecting_fn_with_bound_region(|x: fn(&'x u32), y| {});
    |                                                  ^ one type is more general than the other
    |
    = note: expected fn pointer `for<'a> fn(&'a _)`
               found fn pointer `fn(&_)`
 
+error: lifetime may not live long enough
+  --> $DIR/expect-fn-supply-fn.rs:40:50
+   |
+LL | fn expect_bound_supply_free_from_fn<'x>(x: &'x u32) {
+   |                                     -- lifetime `'x` defined here
+...
+LL |     with_closure_expecting_fn_with_bound_region(|x: fn(&'x u32), y| {});
+   |                                                  ^ requires that `'x` must outlive `'static`
+
 error[E0308]: mismatched types
-  --> $DIR/expect-fn-supply-fn.rs:48:50
+  --> $DIR/expect-fn-supply-fn.rs:50:50
    |
 LL |     with_closure_expecting_fn_with_bound_region(|x: Foo<'_>, y| {
    |                                                  ^ one type is more general than the other
    |
    = note: expected fn pointer `for<'a> fn(&'a _)`
               found fn pointer `fn(&_)`
 
-error: aborting due to 5 previous errors
+error: aborting due to 7 previous errors
 
 For more information about this error, try `rustc --explain E0308`.

tests/ui/coherence/coherence-fn-covariant-bound-vs-static.rs

@@ -1,10 +1,15 @@
-// Test that impls for these two types are considered ovelapping:
+//@ check-pass
+
+// These types were previously considered equal as they are subtypes of each other.
+// This has been changed in #118247 and we now consider them to be disjoint.
+//
+// In our test:
 //
 // * `for<'r> fn(fn(&'r u32))`
 // * `fn(fn(&'a u32)` where `'a` is free
 //
-// This is because, for `'a = 'static`, the two types overlap.
-// Effectively for them to be equal to you get:
+// These were considered equal as for `'a = 'static` subtyping succeeds in both
+// directions:
 //
 // * `for<'r> fn(fn(&'r u32)) <: fn(fn(&'static u32))`
 //   * true if `exists<'r> { 'r: 'static }` (obviously true)
@@ -15,12 +20,7 @@ trait Trait {}
 
 impl Trait for for<'r> fn(fn(&'r ())) {}
 impl<'a> Trait for fn(fn(&'a ())) {}
-//~^ ERROR conflicting implementations
-//
-// Note in particular that we do NOT get a future-compatibility warning
-// here. This is because the new leak-check proposed in [MCP 295] does not
-// "error" when these two types are equated.
-//
-// [MCP 295]: https://github.com/rust-lang/compiler-team/issues/295
+//~^ WARN conflicting implementations of trait `Trait` for type `for<'r> fn(fn(&'r ()))` [coherence_leak_check]
+//~| WARN the behavior may change in a future release
 
 fn main() {}

tests/ui/coherence/coherence-fn-covariant-bound-vs-static.stderr

@@ -1,13 +1,15 @@
-error[E0119]: conflicting implementations of trait `Trait` for type `for<'r> fn(fn(&'r ()))`
-  --> $DIR/coherence-fn-covariant-bound-vs-static.rs:17:1
+warning: conflicting implementations of trait `Trait` for type `for<'r> fn(fn(&'r ()))`
+  --> $DIR/coherence-fn-covariant-bound-vs-static.rs:22:1
    |
 LL | impl Trait for for<'r> fn(fn(&'r ())) {}
    | ------------------------------------- first implementation here
 LL | impl<'a> Trait for fn(fn(&'a ())) {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `for<'r> fn(fn(&'r ()))`
    |
+   = warning: the behavior may change in a future release
+   = note: for more information, see issue #56105 <https://github.com/rust-lang/rust/issues/56105>
    = note: this behavior recently changed as a result of a bug fix; see rust-lang/rust#56105 for details
+   = note: `#[warn(coherence_leak_check)]` on by default
 
-error: aborting due to 1 previous error
+warning: 1 warning emitted
 
-For more information about this error, try `rustc --explain E0119`.

tests/ui/coherence/coherence-fn-inputs.rs

@@ -1,19 +1,21 @@
-// Test that we consider these two types completely equal:
+//@ check-pass
+
+// These types were previously considered equal as they are subtypes of each other.
+// This has been changed in #118247 and we now consider them to be disjoint.
 //
 // * `for<'a, 'b> fn(&'a u32, &'b u32)`
 // * `for<'c> fn(&'c u32, &'c u32)`
 //
-// For a long time we considered these to be distinct types. But in fact they
-// are equivalent, if you work through the implications of subtyping -- this is
-// because:
+// These types are subtypes of each other as:
 //
 // * `'c` can be the intersection of `'a` and `'b` (and there is always an intersection)
 // * `'a` and `'b` can both be equal to `'c`
 
 trait Trait {}
 impl Trait for for<'a, 'b> fn(&'a u32, &'b u32) {}
 impl Trait for for<'c> fn(&'c u32, &'c u32) {
-    //~^ ERROR conflicting implementations
+    //~^ WARN conflicting implementations of trait `Trait` for type `for<'a, 'b> fn(&'a u32, &'b u32)` [coherence_leak_check]
+    //~| WARN the behavior may change in a future release
     //
     // Note in particular that we do NOT get a future-compatibility warning
     // here. This is because the new leak-check proposed in [MCP 295] does not