Skip to content

Implementation of RFC 2289 (associated_type_bounds)#57428

Merged
bors merged 24 commits intorust-lang:masterfrom
alexreg:associated_type_bounds
Jun 6, 2019
Merged

Implementation of RFC 2289 (associated_type_bounds)#57428
bors merged 24 commits intorust-lang:masterfrom
alexreg:associated_type_bounds

Conversation

@alexreg
Copy link
Contributor

@alexreg alexreg commented Jan 7, 2019

This PR implements the asociated_type_bounds feature.

Associated type bounds are implemented in:

  • function/method arguments and return types
  • structs, enums, unions
  • associated items in traits
  • type aliases
  • type parameter defaults
  • trait objects
  • let bindings

CC @nikomatsakis @Centril

@rust-highfive
Copy link
Contributor

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 7, 2019
@michaelwoerister
Copy link
Member

r? @nikomatsakis for re-assignment.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 19, 2019

A few things:

  • Would you be so kind and remove the unrelated commits from this PR?
  • A test would be great
  • we might have to create additional inference variables in the inference code for existential types. I would start by looking at what the difference in inference is between impl trait and existential types

@oli-obk oli-obk closed this Jan 19, 2019
@oli-obk

This comment has been minimized.

@oli-obk oli-obk reopened this Jan 19, 2019
@alexreg
Copy link
Contributor Author

alexreg commented Jan 20, 2019

@oli-obk Oh absolutely – I'll clean up the commits in due time. You mean a test for your own debugging purposes? I can add one, though I was planning on writing actual regression tests later.

"what the difference in inference is between impl trait and existential types"

Adding an inference variable sounds like a sensible idea. What do you mean by this however?

@oli-obk
Copy link
Contributor

oli-obk commented Jan 20, 2019

I am not actually sure how to address the issue, so I'd suggest comparing what happens when you use explicit existential types instead of impl Trait (on discord you said that works), an then see how to replicate that behaviour with impl trait. Basically look for the point where the behaviour diverges. You can use RUST_LOG to dump the debug messages into separate files and compare the output.

@alexreg
Copy link
Contributor Author

alexreg commented Jan 20, 2019

@oli-obk Yeah, I've tried that, but it's too difficult for me to spot.

@alexreg
Copy link
Contributor Author

alexreg commented Jan 20, 2019

@oli-obk Here are the debug outputs produced by rustc_typeck in the working case and failing case. https://gist.github.com/alexreg/b39523cf61c25d5229fa0316ef372b4e

Here's the code.

#![feature(existential_type)]

use std::fmt::{Debug, Display};

trait TraitB {
    type AssocB;
}

trait TraitC {
}

// Failing case:
existential type Bar: Debug + TraitB<AssocB = impl Send>;
// Working case:
existential type Bar: Debug + TraitB<AssocB = _0>;
existential type _0: Send;

impl TraitB for i32 {
    type AssocB = u32;
}

impl TraitC for i32 {
}

fn a() -> Bar {
    42
}

fn main() {}

@oli-obk
Copy link
Contributor

oli-obk commented Jan 21, 2019

The major difference seems to be this part. Tracking down why that additional code is run only in the successful case should shed some light on what we need to do in the failing case

You will get a cleaner diff if you add existential type _0: Send; in the failing case, too, but don't use it. You might need some reordering of the items to get the compiler to run everything in the same order.

@alexreg
Copy link
Contributor Author

alexreg commented Jan 21, 2019

@oli-obk I'm pretty sure that's not the issue. The extra code for the working case is just because type resolution has to go through the indirection of resolving the path before it gets to the opaque types. If you look at the code that follows, it's basically identical. Hmm.

@nikomatsakis
Copy link
Contributor

@alexreg I've pushed a commit that gets things building and adds some debug printouts.

I compared the logs from the working/not-working example.

The problem seems to be that, in the not working case, the "parent" of the impl trait is the wrong thing -- I see it as NodeId(23). I didn't dig hard enough to be sure what that is, but I would presume it's the existential type declaration itself.

This means that we only search the children of that existential type declaration to find the "defining uses" for the impl trait -- and naturally we don't find any, because the defining use is elsewhere in the module.

In this case of an impl Trait embedded in the bounds of an existential type, presumably the parent should be the "grandparent" -- the module that encloses the existential type -- so that the defining uses can be found at the same time.

@nikomatsakis
Copy link
Contributor

If you'd like to see what I'm talking about, try running your example with RUST_LOG=rustc_typeck::collect and grepping for find_existential_constraints.

@alexreg
Copy link
Contributor Author

alexreg commented Jan 25, 2019

@nikomatsakis So, I thought a pretty similar thing at first, but it turns out the error is coming before find_existential_constraints is called, as highlighted by this log:

error[E0271]: type mismatch resolving `<i32 as TraitB>::AssocB == impl std::marker::Send`
  --> atb.rs:37:1
   |
37 | existential type Bar: Debug + TraitB<AssocB = impl Send>;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected u32, found opaque type
   |
   = note: expected type `u32`
              found type `impl std::marker::Send`
   = note: the return type of a function must have a statically known size

DEBUG 2019-01-25T02:19:53Z: rustc_typeck::collect: find_existential_constraints(DefId(0/1:9 ~ atb[317d]::Bar[0]::{{impl-Trait}}[0]))
TRACE 2019-01-25T02:19:53Z: rustc_typeck::collect: find_existential_constraints: parent_id=NodeId(0)

I've fixed it so parent_id=NodeId(0) is correct now (i.e. the grandparent not parent), but that's not enough, because the type mismatch has already occurred. Do we need to delay type-checking of the associated type in such cases? Perhaps by creating an inference var? Advice appreciated. :-)

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

I find it very hard to review this PR with all the unrelated formatting changes intermingled with the actual changes. Could you use git add -p from now on to do separate commits for the different kinds of changes?

@Dylan-DPC-zz

This comment has been minimized.

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2019
@alexreg

This comment has been minimized.

@Dylan-DPC-zz

This comment has been minimized.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 5, 2019
…ikomatsakis,Centril

Implementation of RFC 2289 (associated_type_bounds)

This PR implements the [`asociated_type_bounds` feature](https://github.com/rust-lang/rfcs/blob/master/text/2289-associated-type-bounds.md).

Associated type bounds are implemented in:
   - function/method arguments and return types
   - structs, enums, unions
   - associated items in traits
   - type aliases
   - type parameter defaults
   - trait objects
   - let bindings

CC @nikomatsakis @Centril
@bors

This comment has been minimized.

@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 6, 2019
@alexreg
Copy link
Contributor Author

alexreg commented Jun 6, 2019

@bors r=nikomatsakis,Centril

@bors
Copy link
Collaborator

bors commented Jun 6, 2019

📌 Commit ee89033 has been approved by nikomatsakis,Centril

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2019
@bors
Copy link
Collaborator

bors commented Jun 6, 2019

⌛ Testing commit ee89033 with merge 740668d...

bors added a commit that referenced this pull request Jun 6, 2019
…,Centril

Implementation of RFC 2289 (associated_type_bounds)

This PR implements the [`asociated_type_bounds` feature](https://github.com/rust-lang/rfcs/blob/master/text/2289-associated-type-bounds.md).

Associated type bounds are implemented in:
   - function/method arguments and return types
   - structs, enums, unions
   - associated items in traits
   - type aliases
   - type parameter defaults
   - trait objects
   - let bindings

CC @nikomatsakis @Centril
@bors
Copy link
Collaborator

bors commented Jun 6, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: nikomatsakis,Centril
Pushing 740668d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 6, 2019
@bors bors closed this Jun 6, 2019
@rust-highfive

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

F-associated_type_bounds `#![feature(associated_type_bounds)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Development

Successfully merging this pull request may close these issues.