Rollup merge of #87200 - oli-obk:fixup_fixup_opaque_types, r=nikomatsakis

TAIT: Infer all inference variables in opaque type substitutions via InferCx

The previous algorithm was correct for the example given in its
documentation, but when the TAIT was declared as a free item
instead of an associated item, the generic parameters were the
wrong ones.

cc `@spastorino`

r? `@nikomatsakis`

Author: GuillaumeGomez

compiler/rustc_middle/src/ty/mod.rs

@@ -857,7 +857,7 @@ impl<'tcx> InstantiatedPredicates<'tcx> {
     }
 }
 
-#[derive(Copy, Clone, Debug, PartialEq, Eq, HashStable, TyEncodable, TyDecodable)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, HashStable, TyEncodable, TyDecodable, TypeFoldable)]
 pub struct OpaqueTypeKey<'tcx> {
     pub def_id: DefId,
     pub substs: SubstsRef<'tcx>,

compiler/rustc_trait_selection/src/opaque_types.rs

@@ -568,6 +568,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
     /// - `substs`, the substs  used to instantiate this opaque type
     /// - `instantiated_ty`, the inferred type C1 -- fully resolved, lifted version of
     ///   `opaque_defn.concrete_ty`
+    #[instrument(skip(self))]
     fn infer_opaque_definition_from_instantiation(
         &self,
         opaque_type_key: OpaqueTypeKey<'tcx>,
@@ -576,18 +577,14 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
     ) -> Ty<'tcx> {
         let OpaqueTypeKey { def_id, substs } = opaque_type_key;
 
-        debug!(
-            "infer_opaque_definition_from_instantiation(def_id={:?}, instantiated_ty={:?})",
-            def_id, instantiated_ty
-        );
-
         // Use substs to build up a reverse map from regions to their
         // identity mappings. This is necessary because of `impl
         // Trait` lifetimes are computed by replacing existing
         // lifetimes with 'static and remapping only those used in the
         // `impl Trait` return type, resulting in the parameters
         // shifting.
         let id_substs = InternalSubsts::identity_for_item(self.tcx, def_id);
+        debug!(?id_substs);
         let map: FxHashMap<GenericArg<'tcx>, GenericArg<'tcx>> =
             substs.iter().enumerate().map(|(index, subst)| (subst, id_substs[index])).collect();
 
@@ -602,7 +599,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             instantiated_ty,
             span,
         ));
-        debug!("infer_opaque_definition_from_instantiation: definition_ty={:?}", definition_ty);
+        debug!(?definition_ty);
 
         definition_ty
     }
@@ -857,14 +854,15 @@ impl TypeFolder<'tcx> for ReverseMapper<'tcx> {
                 self.tcx.mk_generator(def_id, substs, movability)
             }
 
-            ty::Param(..) => {
+            ty::Param(param) => {
                 // Look it up in the substitution list.
                 match self.map.get(&ty.into()).map(|k| k.unpack()) {
                     // Found it in the substitution list; replace with the parameter from the
                     // opaque type.
                     Some(GenericArgKind::Type(t1)) => t1,
                     Some(u) => panic!("type mapped to unexpected kind: {:?}", u),
                     None => {
+                        debug!(?param, ?self.map);
                         self.tcx
                             .sess
                             .struct_span_err(
@@ -931,8 +929,8 @@ struct Instantiator<'a, 'tcx> {
 }
 
 impl<'a, 'tcx> Instantiator<'a, 'tcx> {
+    #[instrument(skip(self))]
     fn instantiate_opaque_types_in_map<T: TypeFoldable<'tcx>>(&mut self, value: T) -> T {
-        debug!("instantiate_opaque_types_in_map(value={:?})", value);
         let tcx = self.infcx.tcx;
         value.fold_with(&mut BottomUpFolder {
             tcx,

compiler/rustc_typeck/src/check/mod.rs

@@ -112,11 +112,9 @@ use rustc_hir::{HirIdMap, ImplicitSelfKind, Node};
 use rustc_index::bit_set::BitSet;
 use rustc_index::vec::Idx;
 use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
-use rustc_middle::ty::fold::{TypeFoldable, TypeFolder};
 use rustc_middle::ty::query::Providers;
-use rustc_middle::ty::subst::GenericArgKind;
 use rustc_middle::ty::subst::{InternalSubsts, Subst, SubstsRef};
-use rustc_middle::ty::{self, RegionKind, Ty, TyCtxt, UserType};
+use rustc_middle::ty::{self, Ty, TyCtxt, UserType};
 use rustc_session::config;
 use rustc_session::parse::feature_err;
 use rustc_session::Session;
@@ -321,117 +319,6 @@ fn used_trait_imports(tcx: TyCtxt<'_>, def_id: LocalDefId) -> &FxHashSet<LocalDe
     &*tcx.typeck(def_id).used_trait_imports
 }
 
-/// Inspects the substs of opaque types, replacing any inference variables
-/// with proper generic parameter from the identity substs.
-///
-/// This is run after we normalize the function signature, to fix any inference
-/// variables introduced by the projection of associated types. This ensures that
-/// any opaque types used in the signature continue to refer to generic parameters,
-/// allowing them to be considered for defining uses in the function body
-///
-/// For example, consider this code.
-///
-/// ```rust
-/// trait MyTrait {
-///     type MyItem;
-///     fn use_it(self) -> Self::MyItem
-/// }
-/// impl<T, I> MyTrait for T where T: Iterator<Item = I> {
-///     type MyItem = impl Iterator<Item = I>;
-///     fn use_it(self) -> Self::MyItem {
-///         self
-///     }
-/// }
-/// ```
-///
-/// When we normalize the signature of `use_it` from the impl block,
-/// we will normalize `Self::MyItem` to the opaque type `impl Iterator<Item = I>`
-/// However, this projection result may contain inference variables, due
-/// to the way that projection works. We didn't have any inference variables
-/// in the signature to begin with - leaving them in will cause us to incorrectly
-/// conclude that we don't have a defining use of `MyItem`. By mapping inference
-/// variables back to the actual generic parameters, we will correctly see that
-/// we have a defining use of `MyItem`
-fn fixup_opaque_types<'tcx, T>(tcx: TyCtxt<'tcx>, val: T) -> T
-where
-    T: TypeFoldable<'tcx>,
-{
-    struct FixupFolder<'tcx> {
-        tcx: TyCtxt<'tcx>,
-    }
-
-    impl<'tcx> TypeFolder<'tcx> for FixupFolder<'tcx> {
-        fn tcx<'a>(&'a self) -> TyCtxt<'tcx> {
-            self.tcx
-        }
-
-        fn fold_ty(&mut self, ty: Ty<'tcx>) -> Ty<'tcx> {
-            match *ty.kind() {
-                ty::Opaque(def_id, substs) => {
-                    debug!("fixup_opaque_types: found type {:?}", ty);
-                    // Here, we replace any inference variables that occur within
-                    // the substs of an opaque type. By definition, any type occurring
-                    // in the substs has a corresponding generic parameter, which is what
-                    // we replace it with.
-                    // This replacement is only run on the function signature, so any
-                    // inference variables that we come across must be the rust of projection
-                    // (there's no other way for a user to get inference variables into
-                    // a function signature).
-                    if ty.needs_infer() {
-                        let new_substs = InternalSubsts::for_item(self.tcx, def_id, |param, _| {
-                            let old_param = substs[param.index as usize];
-                            match old_param.unpack() {
-                                GenericArgKind::Type(old_ty) => {
-                                    if let ty::Infer(_) = old_ty.kind() {
-                                        // Replace inference type with a generic parameter
-                                        self.tcx.mk_param_from_def(param)
-                                    } else {
-                                        old_param.fold_with(self)
-                                    }
-                                }
-                                GenericArgKind::Const(old_const) => {
-                                    if let ty::ConstKind::Infer(_) = old_const.val {
-                                        // This should never happen - we currently do not support
-                                        // 'const projections', e.g.:
-                                        // `impl<T: SomeTrait> MyTrait for T where <T as SomeTrait>::MyConst == 25`
-                                        // which should be the only way for us to end up with a const inference
-                                        // variable after projection. If Rust ever gains support for this kind
-                                        // of projection, this should *probably* be changed to
-                                        // `self.tcx.mk_param_from_def(param)`
-                                        bug!(
-                                            "Found infer const: `{:?}` in opaque type: {:?}",
-                                            old_const,
-                                            ty
-                                        );
-                                    } else {
-                                        old_param.fold_with(self)
-                                    }
-                                }
-                                GenericArgKind::Lifetime(old_region) => {
-                                    if let RegionKind::ReVar(_) = old_region {
-                                        self.tcx.mk_param_from_def(param)
-                                    } else {
-                                        old_param.fold_with(self)
-                                    }
-                                }
-                            }
-                        });
-                        let new_ty = self.tcx.mk_opaque(def_id, new_substs);
-                        debug!("fixup_opaque_types: new type: {:?}", new_ty);
-                        new_ty
-                    } else {
-                        ty
-                    }
-                }
-                _ => ty.super_fold_with(self),
-            }
-        }
-    }
-
-    debug!("fixup_opaque_types({:?})", val);
-    val.fold_with(&mut FixupFolder { tcx })
-}
-
 fn typeck_const_arg<'tcx>(
     tcx: TyCtxt<'tcx>,
     (did, param_did): (LocalDefId, DefId),
@@ -510,8 +397,6 @@ fn typeck_with_fallback<'tcx>(
                 fn_sig,
             );
 
-            let fn_sig = fixup_opaque_types(tcx, fn_sig);
-
             let fcx = check_fn(&inh, param_env, fn_sig, decl, id, body, None).0;
             fcx
         } else {

compiler/rustc_typeck/src/check/writeback.rs

@@ -496,6 +496,8 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> {
 
             debug_assert!(!instantiated_ty.has_escaping_bound_vars());
 
+            let opaque_type_key = self.fcx.fully_resolve(opaque_type_key).unwrap();
+
             // Prevent:
             // * `fn foo<T>() -> Foo<T>`
             // * `fn foo<T: Bound + Other>() -> Foo<T>`
@@ -508,6 +510,8 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> {
             // fn foo<U>() -> Foo<U> { .. }
             // ```
             // figures out the concrete type with `U`, but the stored type is with `T`.
+
+            // FIXME: why are we calling this here? This seems too early, and duplicated.
             let definition_ty = self.fcx.infer_opaque_definition_from_instantiation(
                 opaque_type_key,
                 instantiated_ty,
@@ -529,33 +533,33 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> {
                 }
             }
 
-            if !opaque_type_key.substs.needs_infer() {
-                // We only want to add an entry into `concrete_opaque_types`
-                // if we actually found a defining usage of this opaque type.
-                // Otherwise, we do nothing - we'll either find a defining usage
-                // in some other location, or we'll end up emitting an error due
-                // to the lack of defining usage
-                if !skip_add {
-                    let old_concrete_ty = self
-                        .typeck_results
-                        .concrete_opaque_types
-                        .insert(opaque_type_key, definition_ty);
-                    if let Some(old_concrete_ty) = old_concrete_ty {
-                        if old_concrete_ty != definition_ty {
-                            span_bug!(
-                                span,
-                                "`visit_opaque_types` tried to write different types for the same \
+            if opaque_type_key.substs.needs_infer() {
+                span_bug!(span, "{:#?} has inference variables", opaque_type_key.substs)
+            }
+
+            // We only want to add an entry into `concrete_opaque_types`
+            // if we actually found a defining usage of this opaque type.
+            // Otherwise, we do nothing - we'll either find a defining usage
+            // in some other location, or we'll end up emitting an error due
+            // to the lack of defining usage
+            if !skip_add {
+                let old_concrete_ty = self
+                    .typeck_results
+                    .concrete_opaque_types
+                    .insert(opaque_type_key, definition_ty);
+                if let Some(old_concrete_ty) = old_concrete_ty {
+                    if old_concrete_ty != definition_ty {
+                        span_bug!(
+                            span,
+                            "`visit_opaque_types` tried to write different types for the same \
                                  opaque type: {:?}, {:?}, {:?}, {:?}",
-                                opaque_type_key.def_id,
-                                definition_ty,
-                                opaque_defn,
-                                old_concrete_ty,
-                            );
-                        }
+                            opaque_type_key.def_id,
+                            definition_ty,
+                            opaque_defn,
+                            old_concrete_ty,
+                        );
                     }
                 }
-            } else {
-                self.tcx().sess.delay_span_bug(span, "`opaque_defn` has inference variables");
             }
         }
     }

src/test/ui/type-alias-impl-trait/issue-60371.rs

@@ -9,7 +9,7 @@ trait Bug {
 impl Bug for &() {
     type Item = impl Bug; //~ ERROR `impl Trait` in type aliases is unstable
     //~^ ERROR the trait bound `(): Bug` is not satisfied
-    //~^^ ERROR could not find defining uses
+    //~^^ ERROR the trait bound `(): Bug` is not satisfied
 
     const FUN: fn() -> Self::Item = || ();
     //~^ ERROR type alias impl trait is not permitted here

src/test/ui/type-alias-impl-trait/issue-60371.stderr

@@ -25,11 +25,14 @@ LL |     type Item = impl Bug;
    = help: the following implementations were found:
              <&() as Bug>
 
-error: could not find defining uses
+error[E0277]: the trait bound `(): Bug` is not satisfied
   --> $DIR/issue-60371.rs:10:17
    |
 LL |     type Item = impl Bug;
-   |                 ^^^^^^^^
+   |                 ^^^^^^^^ the trait `Bug` is not implemented for `()`
+   |
+   = help: the following implementations were found:
+             <&() as Bug>
 
 error: aborting due to 4 previous errors