Double check that hidden types match the expected hidden type

Author: oli-obk

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,143 @@ 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, and 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.
+    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_substs(
+                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())
+    }
+}
+
+fn find_and_apply_rpit_substs<'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 substs 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

@@ -271,6 +271,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 {}