Skip to content

Commit

Permalink
Auto merge of #38920 - petrochenkov:selfimpl, r=eddyb
Browse files Browse the repository at this point in the history
Partially implement RFC 1647 (`Self` in impl headers)

The name resolution part is easy, but the typeck part contains an unexpected problem.

It turns out that `Self` type *depends* on bounds and `where` clauses, so we need to convert them first to determine what the `Self` type is! If bounds/`where` clauses can refer to `Self` then we have a cyclic dependency.
This is required to support impls like this:
```
// Found in libcollections
impl<I: IntoIterator> SpecExtend<I> for LinkedList<I::Item> { .... }
                                                      ^^^^^ associated type `Item` is found using information from bounds

```
I'm not yet sure how to resolve this issue.
One possible solution (that feels hacky) is to make two passes over generics - first collect predicates ignoring everything involving `Self`, then determine `Self`, then collect predicates again without ignoring anything. (Some kind of lazy on-demand checking or something looks like a proper solution.)

This patch in its current state doesn't solve the problem with `Self` in bounds, so the only observable things it does is improving error messages and supporting `impl Trait<Self> for Type {}`.

There's also a question about feature gating. It's non-trivial to *detect* "newly resolved" `Self`s to feature gate them, but it's simple to *enable* the new resolution behavior when the feature gate is already specified. Alternatively this can be considered a bug fix and merged without a feature gate.

cc #38864
r? @nikomatsakis
cc @eddyb
Whitespace ignoring diff https://github.com/rust-lang/rust/pull/38920/files?w=1
  • Loading branch information
bors committed Jan 25, 2017
2 parents 94d4589 + f9bdf34 commit df8debf
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 67 deletions.
114 changes: 61 additions & 53 deletions src/librustc_resolve/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1548,7 +1548,10 @@ impl<'a> Resolver<'a> {
}

ItemKind::DefaultImpl(_, ref trait_ref) => {
self.with_optional_trait_ref(Some(trait_ref), |_, _| {});
self.with_optional_trait_ref(Some(trait_ref), |this, _| {
// Resolve type arguments in trait path
visit::walk_trait_ref(this, trait_ref);
});
}
ItemKind::Impl(.., ref generics, ref opt_trait_ref, ref self_type, ref impl_items) =>
self.resolve_implementation(generics,
Expand Down Expand Up @@ -1712,7 +1715,6 @@ impl<'a> Resolver<'a> {
new_val = Some((def.def_id(), trait_ref.clone()));
new_id = Some(def.def_id());
}
visit::walk_trait_ref(self, trait_ref);
}
let original_trait_ref = replace(&mut self.current_trait_ref, new_val);
let result = f(self, new_id);
Expand Down Expand Up @@ -1740,60 +1742,66 @@ impl<'a> Resolver<'a> {
impl_items: &[ImplItem]) {
// If applicable, create a rib for the type parameters.
self.with_type_parameter_rib(HasTypeParameters(generics, ItemRibKind), |this| {
// Resolve the type parameters.
this.visit_generics(generics);

// Resolve the trait reference, if necessary.
this.with_optional_trait_ref(opt_trait_reference.as_ref(), |this, trait_id| {
// Resolve the self type.
this.visit_ty(self_type);

let item_def_id = this.definitions.local_def_id(item_id);
this.with_self_rib(Def::SelfTy(trait_id, Some(item_def_id)), |this| {
this.with_current_self_type(self_type, |this| {
for impl_item in impl_items {
this.check_proc_macro_attrs(&impl_item.attrs);
this.resolve_visibility(&impl_item.vis);
match impl_item.node {
ImplItemKind::Const(..) => {
// If this is a trait impl, ensure the const
// exists in trait
this.check_trait_item(impl_item.ident.name,
ValueNS,
impl_item.span,
|n, s| ResolutionError::ConstNotMemberOfTrait(n, s));
visit::walk_impl_item(this, impl_item);
}
ImplItemKind::Method(ref sig, _) => {
// If this is a trait impl, ensure the method
// exists in trait
this.check_trait_item(impl_item.ident.name,
ValueNS,
impl_item.span,
|n, s| ResolutionError::MethodNotMemberOfTrait(n, s));

// We also need a new scope for the method-
// specific type parameters.
let type_parameters =
HasTypeParameters(&sig.generics,
MethodRibKind(!sig.decl.has_self()));
this.with_type_parameter_rib(type_parameters, |this| {
// Dummy self type for better errors if `Self` is used in the trait path.
this.with_self_rib(Def::SelfTy(None, None), |this| {
// Resolve the trait reference, if necessary.
this.with_optional_trait_ref(opt_trait_reference.as_ref(), |this, trait_id| {
let item_def_id = this.definitions.local_def_id(item_id);
this.with_self_rib(Def::SelfTy(trait_id, Some(item_def_id)), |this| {
if let Some(trait_ref) = opt_trait_reference.as_ref() {
// Resolve type arguments in trait path
visit::walk_trait_ref(this, trait_ref);
}
// Resolve the self type.
this.visit_ty(self_type);
// Resolve the type parameters.
this.visit_generics(generics);
this.with_current_self_type(self_type, |this| {
for impl_item in impl_items {
this.check_proc_macro_attrs(&impl_item.attrs);
this.resolve_visibility(&impl_item.vis);
match impl_item.node {
ImplItemKind::Const(..) => {
// If this is a trait impl, ensure the const
// exists in trait
this.check_trait_item(impl_item.ident.name,
ValueNS,
impl_item.span,
|n, s| ResolutionError::ConstNotMemberOfTrait(n, s));
visit::walk_impl_item(this, impl_item);
});
}
ImplItemKind::Type(ref ty) => {
// If this is a trait impl, ensure the type
// exists in trait
this.check_trait_item(impl_item.ident.name,
TypeNS,
impl_item.span,
|n, s| ResolutionError::TypeNotMemberOfTrait(n, s));

this.visit_ty(ty);
}
ImplItemKind::Method(ref sig, _) => {
// If this is a trait impl, ensure the method
// exists in trait
this.check_trait_item(impl_item.ident.name,
ValueNS,
impl_item.span,
|n, s| ResolutionError::MethodNotMemberOfTrait(n, s));

// We also need a new scope for the method-
// specific type parameters.
let type_parameters =
HasTypeParameters(&sig.generics,
MethodRibKind(!sig.decl.has_self()));
this.with_type_parameter_rib(type_parameters, |this| {
visit::walk_impl_item(this, impl_item);
});
}
ImplItemKind::Type(ref ty) => {
// If this is a trait impl, ensure the type
// exists in trait
this.check_trait_item(impl_item.ident.name,
TypeNS,
impl_item.span,
|n, s| ResolutionError::TypeNotMemberOfTrait(n, s));

this.visit_ty(ty);
}
ImplItemKind::Macro(_) =>
panic!("unexpanded macro in resolve!"),
}
ImplItemKind::Macro(_) => panic!("unexpanded macro in resolve!"),
}
}
});
});
});
});
Expand Down
20 changes: 16 additions & 4 deletions src/librustc_typeck/astconv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1401,11 +1401,23 @@ impl<'o, 'gcx: 'tcx, 'tcx> AstConv<'gcx, 'tcx>+'o {

assert_eq!(opt_self_ty, None);
tcx.prohibit_type_params(&path.segments);
let ty = tcx.item_type(def_id);
if let Some(free_substs) = self.get_free_substs() {
ty.subst(tcx, free_substs)

// FIXME: Self type is not always computed when we are here because type parameter
// bounds may affect Self type and have to be converted before it.
let ty = if def_id.is_local() {
tcx.item_types.borrow().get(&def_id).cloned()
} else {
ty
Some(tcx.item_type(def_id))
};
if let Some(ty) = ty {
if let Some(free_substs) = self.get_free_substs() {
ty.subst(tcx, free_substs)
} else {
ty
}
} else {
tcx.sess.span_err(span, "`Self` type is used before it's determined");
tcx.types.err
}
}
Def::SelfTy(Some(_), None) => {
Expand Down
17 changes: 17 additions & 0 deletions src/test/compile-fail/resolve-self-in-impl-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

struct S<T = u8>(T);
trait Tr<T = u8> {}

impl Self for S {} //~ ERROR expected trait, found self type `Self`
impl Self::N for S {} //~ ERROR cannot find trait `N` in `Self`

fn main() {}
26 changes: 26 additions & 0 deletions src/test/compile-fail/resolve-self-in-impl.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

struct S<T = u8>(T);
trait Tr<T = u8> {}

impl Tr<Self> for S {} // OK

// FIXME: `Self` cannot be used in bounds because it depends on bounds itself.
impl<T: Tr<Self>> Tr<T> for S {} //~ ERROR `Self` type is used before it's determined
impl<T = Self> Tr<T> for S {} //~ ERROR `Self` type is used before it's determined
impl Tr for S where Self: Copy {} //~ ERROR `Self` type is used before it's determined
impl Tr for S where S<Self>: Copy {} //~ ERROR `Self` type is used before it's determined
impl Tr for Self {} //~ ERROR `Self` type is used before it's determined
impl Tr for S<Self> {} //~ ERROR `Self` type is used before it's determined
impl Self {} //~ ERROR `Self` type is used before it's determined
impl S<Self> {} //~ ERROR `Self` type is used before it's determined

fn main() {}
12 changes: 2 additions & 10 deletions src/test/ui/resolve/issue-23305.stderr
Original file line number Diff line number Diff line change
@@ -1,16 +1,8 @@
error[E0411]: cannot find type `Self` in this scope
error: `Self` type is used before it's determined
--> $DIR/issue-23305.rs:15:12
|
15 | impl ToNbt<Self> {}
| ^^^^ `Self` is only available in traits and impls

error[E0038]: the trait `ToNbt` cannot be made into an object
--> $DIR/issue-23305.rs:15:6
|
15 | impl ToNbt<Self> {}
| ^^^^^^^^^^^ the trait `ToNbt` cannot be made into an object
|
= note: method `new` has no receiver
| ^^^^

error: aborting due to previous error

0 comments on commit df8debf

Please sign in to comment.