Skip to content

Commit

Permalink
Rollup merge of #87200 - oli-obk:fixup_fixup_opaque_types, r=nikomats…
Browse files Browse the repository at this point in the history 
…akis

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`
  • Loading branch information
@GuillaumeGomez
GuillaumeGomez authored Jul 16, 2021
2 parents e596aa2 + ebe21ac commit 7d36d69
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 152 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/ty/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down
14 changes: 6 additions & 8 deletions compiler/rustc_trait_selection/src/opaque_types.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand All @@ -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();

Expand All @@ -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
}
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand Down
117 changes: 1 addition & 116 deletions compiler/rustc_typeck/src/check/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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 {
Expand Down
52 changes: 28 additions & 24 deletions compiler/rustc_typeck/src/check/writeback.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>`
Expand All @@ -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,
Expand All @@ -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");
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/type-alias-impl-trait/issue-60371.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 5 additions & 2 deletions src/test/ui/type-alias-impl-trait/issue-60371.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit 7d36d69

Please sign in to comment.