Rollup merge of rust-lang#113661 - oli-obk:tait_wtf, r=lcnr

Double check that hidden types match the expected hidden type

Fixes rust-lang#113278 specifically, but I left a TODO for where we should also add some hardening.

It feels a bit like papering over the issue, but at least this way we don't get unsoundness, but just surprising errors. Errors will be improved and given spans before this PR lands.

r? `@compiler-errors` `@lcnr`

Author: matthiaskrgr

compiler/rustc_borrowck/src/region_infer/opaque_types.rs

@@ -339,8 +339,8 @@ fn check_opaque_type_well_formed<'tcx>(
     // version.
     let errors = ocx.select_all_or_error();
 
-    // This is still required for many(half of the tests in ui/type-alias-impl-trait)
-    // tests to pass
+    // This is fishy, but we check it again in `check_opaque_meets_bounds`.
+    // Remove once we can prepopulate with known hidden types.
     let _ = infcx.take_opaque_types();
 
     if errors.is_empty() {

compiler/rustc_const_eval/src/util/compare_types.rs

@@ -56,8 +56,13 @@ pub fn is_subtype<'tcx>(
     // With `Reveal::All`, opaque types get normalized away, with `Reveal::UserFacing`
     // we would get unification errors because we're unable to look into opaque types,
     // even if they're constrained in our current function.
-    //
-    // It seems very unlikely that this hides any bugs.
-    let _ = infcx.take_opaque_types();
+    for (key, ty) in infcx.take_opaque_types() {
+        span_bug!(
+            ty.hidden_type.span,
+            "{}, {}",
+            tcx.type_of(key.def_id).instantiate(tcx, key.args),
+            ty.hidden_type.ty
+        );
+    }
     errors.is_empty()
 }

compiler/rustc_hir_analysis/src/check/check.rs

@@ -19,11 +19,13 @@ use rustc_lint_defs::builtin::REPR_TRANSPARENT_EXTERNAL_PRIVATE_FIELDS;
 use rustc_middle::hir::nested_filter;
 use rustc_middle::middle::stability::EvalResult;
 use rustc_middle::traits::DefiningAnchor;
+use rustc_middle::ty::fold::BottomUpFolder;
 use rustc_middle::ty::layout::{LayoutError, MAX_SIMD_LANES};
 use rustc_middle::ty::util::{Discr, IntTypeExt};
 use rustc_middle::ty::GenericArgKind;
 use rustc_middle::ty::{
-    self, AdtDef, ParamEnv, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitableExt,
+    self, AdtDef, ParamEnv, RegionKind, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable,
+    TypeVisitableExt,
 };
 use rustc_session::lint::builtin::{UNINHABITED_STATIC, UNSUPPORTED_CALLING_CONVENTIONS};
 use rustc_span::symbol::sym;
@@ -34,6 +36,7 @@ use rustc_trait_selection::traits::error_reporting::on_unimplemented::OnUnimplem
 use rustc_trait_selection::traits::error_reporting::TypeErrCtxtExt as _;
 use rustc_trait_selection::traits::outlives_bounds::InferCtxtExt as _;
 use rustc_trait_selection::traits::{self, ObligationCtxt, TraitEngine, TraitEngineExt as _};
+use rustc_type_ir::fold::TypeFoldable;
 
 use std::ops::ControlFlow;
 
@@ -437,7 +440,7 @@ fn check_opaque_meets_bounds<'tcx>(
     // hidden type is well formed even without those bounds.
     let predicate =
         ty::Binder::dummy(ty::PredicateKind::Clause(ty::ClauseKind::WellFormed(hidden_ty.into())));
-    ocx.register_obligation(Obligation::new(tcx, misc_cause, param_env, predicate));
+    ocx.register_obligation(Obligation::new(tcx, misc_cause.clone(), param_env, predicate));
 
     // Check that all obligations are satisfied by the implementation's
     // version.
@@ -464,11 +467,179 @@ fn check_opaque_meets_bounds<'tcx>(
             ocx.resolve_regions_and_report_errors(defining_use_anchor, &outlives_env)?;
         }
     }
-    // Clean up after ourselves
-    let _ = infcx.take_opaque_types();
+    // Check that any hidden types found during wf checking match the hidden types that `type_of` sees.
+    for (key, mut ty) in infcx.take_opaque_types() {
+        ty.hidden_type.ty = infcx.resolve_vars_if_possible(ty.hidden_type.ty);
+        sanity_check_found_hidden_type(tcx, key, ty.hidden_type, defining_use_anchor, origin)?;
+    }
     Ok(())
 }
 
+fn sanity_check_found_hidden_type<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    key: ty::OpaqueTypeKey<'tcx>,
+    mut ty: ty::OpaqueHiddenType<'tcx>,
+    defining_use_anchor: LocalDefId,
+    origin: &hir::OpaqueTyOrigin,
+) -> Result<(), ErrorGuaranteed> {
+    if ty.ty.is_ty_var() {
+        // Nothing was actually constrained.
+        return Ok(());
+    }
+    if let ty::Alias(ty::Opaque, alias) = ty.ty.kind() {
+        if alias.def_id == key.def_id.to_def_id() && alias.args == key.args {
+            // Nothing was actually constrained, this is an opaque usage that was
+            // only discovered to be opaque after inference vars resolved.
+            return Ok(());
+        }
+    }
+    // Closures frequently end up containing erased lifetimes in their final representation.
+    // These correspond to lifetime variables that never got resolved, so we patch this up here.
+    ty.ty = ty.ty.fold_with(&mut BottomUpFolder {
+        tcx,
+        ty_op: |t| t,
+        ct_op: |c| c,
+        lt_op: |l| match l.kind() {
+            RegionKind::ReVar(_) => tcx.lifetimes.re_erased,
+            _ => l,
+        },
+    });
+    // Get the hidden type.
+    let mut hidden_ty = tcx.type_of(key.def_id).instantiate(tcx, key.args);
+    if let hir::OpaqueTyOrigin::FnReturn(..) | hir::OpaqueTyOrigin::AsyncFn(..) = origin {
+        if hidden_ty != ty.ty {
+            hidden_ty = find_and_apply_rpit_args(
+                tcx,
+                hidden_ty,
+                defining_use_anchor.to_def_id(),
+                key.def_id.to_def_id(),
+            )?;
+        }
+    }
+
+    // If the hidden types differ, emit a type mismatch diagnostic.
+    if hidden_ty == ty.ty {
+        Ok(())
+    } else {
+        let span = tcx.def_span(key.def_id);
+        let other = ty::OpaqueHiddenType { ty: hidden_ty, span };
+        Err(ty.report_mismatch(&other, key.def_id, tcx).emit())
+    }
+}
+
+/// In case it is in a nested opaque type, find that opaque type's
+/// usage in the function signature and use the generic arguments from the usage site.
+/// We need to do because RPITs ignore the lifetimes of the function,
+/// as they have their own copies of all the lifetimes they capture.
+/// So the only way to get the lifetimes represented in terms of the function,
+/// is to look how they are used in the function signature (or do some other fancy
+/// recording of this mapping at ast -> hir lowering time).
+///
+/// As an example:
+/// ```text
+/// trait Id {
+///     type Assoc;
+/// }
+/// impl<'a> Id for &'a () {
+///     type Assoc = &'a ();
+/// }
+/// fn func<'a>(x: &'a ()) -> impl Id<Assoc = impl Sized + 'a> { x }
+/// // desugared to
+/// fn func<'a>(x: &'a () -> Outer<'a> where <Outer<'a> as Id>::Assoc = Inner<'a> {
+///     // Note that in contrast to other nested items, RPIT type aliases can
+///     // access their parents' generics.
+///
+///     // hidden type is `&'aDupOuter ()`
+///     // During wfcheck the hidden type of `Inner<'aDupOuter>` is `&'a ()`, but
+///     // `typeof(Inner<'aDupOuter>) = &'aDupOuter ()`.
+///     // So we walk the signature of `func` to find the use of `Inner<'a>`
+///     // and then use that to replace the lifetimes in the hidden type, obtaining
+///     // `&'a ()`.
+///     type Outer<'aDupOuter> = impl Id<Assoc = Inner<'aDupOuter>>;
+///
+///     // hidden type is `&'aDupInner ()`
+///     type Inner<'aDupInner> = impl Sized + 'aDupInner;
+///
+///     x
+/// }
+/// ```
+fn find_and_apply_rpit_args<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    mut hidden_ty: Ty<'tcx>,
+    function: DefId,
+    opaque: DefId,
+) -> Result<Ty<'tcx>, ErrorGuaranteed> {
+    // Find use of the RPIT in the function signature and thus find the right args to
+    // convert it into the parameter space of the function signature. This is needed,
+    // because that's what `type_of` returns, against which we compare later.
+    let ret = tcx.fn_sig(function).instantiate_identity().output();
+    struct Visitor<'tcx> {
+        tcx: TyCtxt<'tcx>,
+        opaque: DefId,
+        function: DefId,
+        seen: FxHashSet<DefId>,
+    }
+    impl<'tcx> ty::TypeVisitor<TyCtxt<'tcx>> for Visitor<'tcx> {
+        type BreakTy = GenericArgsRef<'tcx>;
+
+        #[instrument(level = "trace", skip(self), ret)]
+        fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
+            trace!("{:#?}", t.kind());
+            match t.kind() {
+                ty::Alias(ty::Opaque, alias) => {
+                    trace!(?alias.def_id);
+                    if alias.def_id == self.opaque {
+                        return ControlFlow::Break(alias.args);
+                    } else if self.seen.insert(alias.def_id) {
+                        for clause in self
+                            .tcx
+                            .explicit_item_bounds(alias.def_id)
+                            .iter_instantiated_copied(self.tcx, alias.args)
+                        {
+                            trace!(?clause);
+                            clause.visit_with(self)?;
+                        }
+                    }
+                }
+                ty::Alias(ty::Projection, alias) => {
+                    if self.tcx.is_impl_trait_in_trait(alias.def_id)
+                        && self.tcx.impl_trait_in_trait_parent_fn(alias.def_id) == self.function
+                    {
+                        // If we're lowering to associated item, install the opaque type which is just
+                        // the `type_of` of the trait's associated item. If we're using the old lowering
+                        // strategy, then just reinterpret the associated type like an opaque :^)
+                        self.tcx
+                            .type_of(alias.def_id)
+                            .instantiate(self.tcx, alias.args)
+                            .visit_with(self)?;
+                    }
+                }
+                ty::Alias(ty::Weak, alias) => {
+                    self.tcx
+                        .type_of(alias.def_id)
+                        .instantiate(self.tcx, alias.args)
+                        .visit_with(self)?;
+                }
+                _ => (),
+            }
+
+            t.super_visit_with(self)
+        }
+    }
+    if let ControlFlow::Break(args) =
+        ret.visit_with(&mut Visitor { tcx, function, opaque, seen: Default::default() })
+    {
+        trace!(?args);
+        trace!("expected: {hidden_ty:#?}");
+        hidden_ty = ty::EarlyBinder::bind(hidden_ty).instantiate(tcx, args);
+        trace!("expected: {hidden_ty:#?}");
+    } else {
+        tcx.sess
+            .delay_span_bug(tcx.def_span(function), format!("{ret:?} does not contain {opaque:?}"));
+    }
+    Ok(hidden_ty)
+}
+
 fn is_enum_of_nonnullable_ptr<'tcx>(
     tcx: TyCtxt<'tcx>,
     adt_def: AdtDef<'tcx>,

compiler/rustc_trait_selection/src/solve/eval_ctxt.rs

@@ -272,6 +272,7 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
         // assertions against dropping an `InferCtxt` without taking opaques.
         // FIXME: Once we remove support for the old impl we can remove this.
         if input.anchor != DefiningAnchor::Error {
+            // This seems ok, but fragile.
             let _ = infcx.take_opaque_types();
         }
 

src/tools/tidy/src/ui_tests.rs

@@ -10,7 +10,7 @@ use std::path::{Path, PathBuf};
 
 const ENTRY_LIMIT: usize = 900;
 // FIXME: The following limits should be reduced eventually.
-const ISSUES_ENTRY_LIMIT: usize = 1894;
+const ISSUES_ENTRY_LIMIT: usize = 1893;
 const ROOT_ENTRY_LIMIT: usize = 870;
 
 const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[

tests/ui/type-alias-impl-trait/hidden_type_mismatch.rs

@@ -0,0 +1,57 @@
+//! This test checks that we don't lose hidden types
+//! for *other* opaque types that we register and use
+//! to prove bounds while checking that a hidden type
+//! satisfies its opaque type's bounds.
+
+#![feature(trivial_bounds, type_alias_impl_trait)]
+#![allow(trivial_bounds)]
+
+mod sus {
+    use super::*;
+    pub type Sep = impl Sized + std::fmt::Display;
+    //~^ ERROR: concrete type differs from previous defining opaque type use
+    pub fn mk_sep() -> Sep {
+        String::from("hello")
+    }
+
+    pub trait Proj {
+        type Assoc;
+    }
+    impl Proj for () {
+        type Assoc = sus::Sep;
+    }
+
+    pub struct Bar<T: Proj> {
+        pub inner: <T as Proj>::Assoc,
+        pub _marker: T,
+    }
+    impl<T: Proj> Clone for Bar<T> {
+        fn clone(&self) -> Self {
+            todo!()
+        }
+    }
+    impl<T: Proj<Assoc = i32> + Copy> Copy for Bar<T> {}
+    // This allows producing `Tait`s via `From`, even though
+    // `define_tait` is not actually callable, and thus assumed
+    // `Bar<()>: Copy` even though it isn't.
+    pub type Tait = impl Copy + From<Bar<()>> + Into<Bar<()>>;
+    pub fn define_tait() -> Tait
+    where
+        // this proves `Bar<()>: Copy`, but `define_tait` is
+        // now uncallable
+        (): Proj<Assoc = i32>,
+    {
+        Bar { inner: 1i32, _marker: () }
+    }
+}
+
+fn copy_tait(x: sus::Tait) -> (sus::Tait, sus::Tait) {
+    (x, x)
+}
+
+fn main() {
+    let bar = sus::Bar { inner: sus::mk_sep(), _marker: () };
+    let (y, z) = copy_tait(bar.into()); // copy a string
+    drop(y.into()); // drop one instance
+    println!("{}", z.into().inner); // print the other
+}

tests/ui/type-alias-impl-trait/hidden_type_mismatch.stderr

@@ -0,0 +1,14 @@
+error: concrete type differs from previous defining opaque type use
+  --> $DIR/hidden_type_mismatch.rs:11:20
+   |
+LL |     pub type Sep = impl Sized + std::fmt::Display;
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `i32`, got `String`
+   |
+note: previous use here
+  --> $DIR/hidden_type_mismatch.rs:37:21
+   |
+LL |     pub type Tait = impl Copy + From<Bar<()>> + Into<Bar<()>>;
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to previous error
+

tests/ui/issues/issue-83190.rs renamed to tests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs

@@ -1,7 +1,7 @@
-// check-pass
-
 // Regression test for issue #83190, triggering an ICE in borrowck.
 
+// check-pass
+
 pub trait Any {}
 impl<T> Any for T {}