Skip to content

Rewrite associated types to use projection rather than dummy type parameters #20307

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

Merged

Conversation

nikomatsakis
Copy link
Contributor

Rewrite associated types to use projection rather than dummy type parameters. This closes almost every (major) open issue, but I'm holding off on that until the code has landed and baked a bit. Probably it should have more tests, as well, but I wanted to get this landed as fast as possible so that we can collaborate on improving it.

The commit history is a little messy, particularly the merge commit at the end. If I get some time, I might just "reset" to the beginning and try to carve up the final state into logical pieces. Let me know if it seems hard to follow. By far the most crucial commit is "Implement associated type projection and normalization."

r? @nick29581

@nikomatsakis
Copy link
Contributor Author

Oh, and this incorporates PR #20159 so I will close that.

@jroesch
Copy link
Member

jroesch commented Dec 29, 2014

Awesome, I'm excited to see this land!

ProjectionTyCandidate::Impl(data));
}
super::VtableParam(..) => {
// This case tell us nothing about the value of an
Copy link
Member

Choose a reason for hiding this comment

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

This makes me happy, it seems like a really clean way to handle this extra constraints.

pub fn normalize_associated_type<'a>(&mut self,
infcx: &InferCtxt<'a,'tcx>,
trait_ref: Rc<ty::TraitRef<'tcx>>,
item_name: ast::Name,
Copy link
Member

Choose a reason for hiding this comment

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

item_name should have a more precise name or a comment (which item is it - the associated item, the trait, the concrete item?) It would be good to have a comment on the fn too stating what normalisation actually is

@nikomatsakis nikomatsakis force-pushed the assoc-types-normalization-extend-bound branch from 6bdf21e to 3e0b041 Compare December 30, 2014 00:53
NoCandidate,
TooManyCandidates,

///
Copy link
Member

Choose a reason for hiding this comment

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

missing comment?

@kinghajj
Copy link

The following example still fails to compile with this branch merged.

#![feature(associated_types)]

trait Foo {
  fn foo(&self) -> ();
}

trait Bar {
  type F: Foo;
}

struct Wat<B: Bar> {
  fs: Vec<B::F>,
}

impl<B: Bar> Wat<B> {
  fn wat(&mut self) -> () {
    self.fs.pop().unwrap().foo();
  }
}

fn main() {
}

I get the following error:

17:33 error: type `<B as Bar>::F` does not implement any method in scope named `foo`

enum CreateTypeParametersForAssociatedTypesFlag {
DontCreateTypeParametersForAssociatedTypes,
CreateTypeParametersForAssociatedTypes,
}
Copy link
Member

Choose a reason for hiding this comment

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

\o/ I'm so glad this is gone

@nrc
Copy link
Member

nrc commented Dec 30, 2014

r=me with the changes (all minor)

@nikomatsakis
Copy link
Contributor Author

@kinghajj the reason is that the method dispatch is not yet integrated with assoc types; should open an issue on that. If you convert to UFCS form, your test passes:

#![feature(associated_types)]

trait Foo {
  fn foo(&self) -> ();
}

trait Bar {
  type F: Foo;
}

struct Wat<B: Bar> {
  fs: Vec<B::F>,
}

impl<B: Bar> Wat<B> {
  fn wat(&mut self) -> () {
      Foo::foo(&self.fs.pop().unwrap()); // <-- changed to UFCS here
  }
}

fn main() {
}

@nikomatsakis nikomatsakis force-pushed the assoc-types-normalization-extend-bound branch 2 times, most recently from 81f8feb to 3fed71b Compare December 30, 2014 12:32
types are always known and hence the ParameterEnvironment is not
necessary. For other `Sized` queries, use the trait infrastructure
just like `Copy`.
…r several reasons:

1. Produced more unique types than is necessary. This increases memory consumption.
2. Linking the type parameter to its definition *seems* like a good idea, but it
   encourages reliance on the bounds listing.
3. It made pretty-printing harder and in particular was causing bad error messages
   when errors occurred before the `TypeParameterDef` entries were fully stored.
…e instantiate

them. Also fix some assertions and handling of builtin bounds.
…ow. Seems better to err on the side of being more correct rather than less. Fix a bug in typing index expressions that was exposed as a result, and add one type annotation that is not required. Delete some random tests that were relying on old behavior and don't seem to add anything anymore.
…k to their respective

commits but oh dear what a pain.
for lack of impl-trait-for-trait just a bit more targeted (don't
substitute err, just drop the troublesome bound for now) -- otherwise
substituting false types leads us into trouble when we normalize etc.
@nikomatsakis nikomatsakis force-pushed the assoc-types-normalization-extend-bound branch from 5938adc to 6f6944e Compare December 30, 2014 15:05
@nikomatsakis nikomatsakis force-pushed the assoc-types-normalization-extend-bound branch from 6f6944e to e186acc Compare December 30, 2014 17:09
bors added a commit that referenced this pull request Dec 30, 2014
…tend-bound, r=nrc

Rewrite associated types to use projection rather than dummy type parameters. This closes almost every (major) open issue, but I'm holding off on that until the code has landed and baked a bit. Probably it should have more tests, as well, but I wanted to get this landed as fast as possible so that we can collaborate on improving it.

The commit history is a little messy, particularly the merge commit at the end. If I get some time, I might just "reset" to the beginning and try to carve up the final state into logical pieces. Let me know if it seems hard to follow. By far the most crucial commit is "Implement associated type projection and normalization."

r? @nick29581
@bors bors merged commit e186acc into rust-lang:master Dec 30, 2014
@nikomatsakis nikomatsakis deleted the assoc-types-normalization-extend-bound branch March 30, 2016 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants