Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix opaque types resulting from projections in function signature #66178

Merged
merged 7 commits into from
Nov 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 108 additions & 1 deletion src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ use rustc::ty::{
use rustc::ty::adjustment::{
Adjust, Adjustment, AllowTwoPhase, AutoBorrow, AutoBorrowMutability, PointerCast
};
use rustc::ty::fold::TypeFoldable;
use rustc::ty::fold::{TypeFoldable, TypeFolder};
use rustc::ty::query::Providers;
use rustc::ty::subst::{
GenericArgKind, Subst, InternalSubsts, SubstsRef, UserSelfTy, UserSubsts,
Expand Down Expand Up @@ -872,6 +872,111 @@ fn used_trait_imports(tcx: TyCtxt<'_>, def_id: DefId) -> &DefIdSet {
&*tcx.typeck_tables_of(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 occuring
// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the wrong indentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is deliberate - the line wrapping needed to keep each line below 100 characters at thornlq indentation made this almost impossible to read

// '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_tables_of(tcx: TyCtxt<'_>, def_id: DefId) -> &ty::TypeckTables<'_> {
// Closures' tables come from their outermost function,
// as they are part of the same "inference environment".
Expand Down Expand Up @@ -911,6 +1016,8 @@ fn typeck_tables_of(tcx: TyCtxt<'_>, def_id: DefId) -> &ty::TypeckTables<'_> {
param_env,
&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
20 changes: 15 additions & 5 deletions src/librustc_typeck/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1617,11 +1617,18 @@ fn find_opaque_ty_constraints(tcx: TyCtxt<'_>, def_id: DefId) -> Ty<'_> {
ty::Param(_) => true,
_ => false,
};
if !substs.types().all(is_param) {
self.tcx.sess.span_err(
span,
"defining opaque type use does not fully define opaque type",
);
let bad_substs: Vec<_> = substs.types().enumerate()
.filter(|(_, ty)| !is_param(ty)).collect();
if !bad_substs.is_empty() {
let identity_substs = InternalSubsts::identity_for_item(self.tcx, self.def_id);
for (i, bad_subst) in bad_substs {
self.tcx.sess.span_err(
span,
&format!("defining opaque type use does not fully define opaque type: \
generic parameter `{}` is specified as concrete type `{}`",
identity_substs.type_at(i), bad_subst)
);
}
} else if let Some((prev_span, prev_ty, ref prev_indices)) = self.found {
let mut ty = concrete_type.walk().fuse();
let mut p_ty = prev_ty.walk().fuse();
Expand Down Expand Up @@ -2059,6 +2066,9 @@ fn explicit_predicates_of(
ty::print::with_no_queries(|| {
let substs = InternalSubsts::identity_for_item(tcx, def_id);
let opaque_ty = tcx.mk_opaque(def_id, substs);
debug!("explicit_predicates_of({:?}): created opaque type {:?}",
def_id, opaque_ty);


// Collect the bounds, i.e., the `A + B + 'c` in `impl A + B + 'c`.
let bounds = AstConv::compute_bounds(
Expand Down
22 changes: 22 additions & 0 deletions src/test/ui/impl-trait/type-alias-generic-param.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Regression test for issue #59342
// Checks that we properly detect defining uses of opaque
// types in 'item' position when generic parameters are involved
//
// run-pass
#![feature(type_alias_impl_trait)]

trait Meow {
type MeowType;
fn meow(self) -> Self::MeowType;
}

impl<T, I> Meow for I
where I: Iterator<Item = T>
{
type MeowType = impl Iterator<Item = T>;
fn meow(self) -> Self::MeowType {
self
}
}

fn main() {}
36 changes: 36 additions & 0 deletions src/test/ui/type-alias-impl-trait/assoc-type-const.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Tests that we properly detect defining usages when using
// const generics in an associated opaque type
// check-pass

#![feature(type_alias_impl_trait)]
#![feature(const_generics)]
//~^ WARN the feature `const_generics` is incomplete and may cause the compiler to crash

trait UnwrapItemsExt<const C: usize> {
type Iter;
fn unwrap_items(self) -> Self::Iter;
}

struct MyStruct<const C: usize> {}

trait MyTrait<'a, const C: usize> {
type MyItem;
const MY_CONST: usize;
}

impl<'a, const C: usize> MyTrait<'a, {C}> for MyStruct<{C}> {
type MyItem = u8;
const MY_CONST: usize = C;
}

impl<'a, I, const C: usize> UnwrapItemsExt<{C}> for I
where
{
type Iter = impl MyTrait<'a, {C}>;

fn unwrap_items(self) -> Self::Iter {
MyStruct::<{C}> {}
}
}

fn main() {}
8 changes: 8 additions & 0 deletions src/test/ui/type-alias-impl-trait/assoc-type-const.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
warning: the feature `const_generics` is incomplete and may cause the compiler to crash
--> $DIR/assoc-type-const.rs:6:12
|
LL | #![feature(const_generics)]
| ^^^^^^^^^^^^^^
|
= note: `#[warn(incomplete_features)]` on by default

28 changes: 28 additions & 0 deletions src/test/ui/type-alias-impl-trait/assoc-type-lifetime.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Tests that we still detect defining usages when
// lifetimes are used in an associated opaque type
// check-pass

#![feature(type_alias_impl_trait)]

trait UnwrapItemsExt {
type Iter;
fn unwrap_items(self) -> Self::Iter;
}

struct MyStruct {}

trait MyTrait<'a> {}

impl<'a> MyTrait<'a> for MyStruct {}

impl<'a, I> UnwrapItemsExt for I
where
{
type Iter = impl MyTrait<'a>;

fn unwrap_items(self) -> Self::Iter {
MyStruct {}
}
}

fn main() {}
2 changes: 1 addition & 1 deletion src/test/ui/type-alias-impl-trait/bound_reduction2.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: defining opaque type use does not fully define opaque type
error: defining opaque type use does not fully define opaque type: generic parameter `V` is specified as concrete type `<T as TraitWithAssoc>::Assoc`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could improve this error. (Also, I think define should be determine.) Maybe with a help message like:

uses of an opaque type must fully determine it, but here `Foo` is only determined for `V = <T as TraitWithAssoc>::Assoc`

It took me a couple of tries to work out what the error message was saying before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@varkor: Your proposed error message is backward - V is the parameter that is not determined (because it's a concrete type instead of a generic parameter).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think there's room to improve this error, this modification provides strictly more information. I think further improvements should go in another PR, since this PR is really about changing when error messages occur, not the actual contente.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but let's open an issue for improving this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #66723.

--> $DIR/bound_reduction2.rs:17:1
|
LL | / fn foo_desugared<T: TraitWithAssoc>(_: T) -> Foo<T::Assoc> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ error: at least one trait must be specified
LL | type Cmp<T> = impl 'static;
| ^^^^^^^^^^^^

error: defining opaque type use does not fully define opaque type
error: defining opaque type use does not fully define opaque type: generic parameter `T` is specified as concrete type `u32`
--> $DIR/generic_nondefining_use.rs:11:1
|
LL | / fn cmp() -> Cmp<u32> {
Expand Down
5 changes: 2 additions & 3 deletions src/test/ui/type-alias-impl-trait/issue-58887.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// run-pass

#![feature(type_alias_impl_trait)]

trait UnwrapItemsExt {
Expand All @@ -11,11 +13,8 @@ where
E: std::fmt::Debug,
{
type Iter = impl Iterator<Item = T>;
//~^ ERROR: could not find defining uses

fn unwrap_items(self) -> Self::Iter {
//~^ ERROR: type parameter `T` is part of concrete type
//~| ERROR: type parameter `E` is part of concrete type
self.map(|x| x.unwrap())
}
}
Expand Down
30 changes: 0 additions & 30 deletions src/test/ui/type-alias-impl-trait/issue-58887.stderr

This file was deleted.

2 changes: 1 addition & 1 deletion src/test/ui/type-alias-impl-trait/issue-60564.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ where
{
type BitsIter = IterBitsIter<T, E, u8>;
fn iter_bits(self, n: u8) -> Self::BitsIter {
//~^ ERROR type parameter `E` is part of concrete type but not used
//~^ ERROR defining opaque type use does not fully define opaque type
(0u8..n)
.rev()
.map(move |shift| ((self >> T::from(shift)) & T::from(1)).try_into().unwrap())
Expand Down
7 changes: 3 additions & 4 deletions src/test/ui/type-alias-impl-trait/issue-60564.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
error: type parameter `E` is part of concrete type but not used in parameter list for the `impl Trait` type alias
--> $DIR/issue-60564.rs:20:49
error: defining opaque type use does not fully define opaque type: generic parameter `I` is specified as concrete type `u8`
--> $DIR/issue-60564.rs:20:5
|
LL | fn iter_bits(self, n: u8) -> Self::BitsIter {
| _________________________________________________^
LL | / fn iter_bits(self, n: u8) -> Self::BitsIter {
LL | |
LL | | (0u8..n)
LL | | .rev()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: defining opaque type use does not fully define opaque type
error: defining opaque type use does not fully define opaque type: generic parameter `U` is specified as concrete type `u32`
--> $DIR/not_a_defining_use.rs:9:1
|
LL | / fn two<T: Debug>(t: T) -> Two<T, u32> {
Expand Down