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

Implementation of RFC 2289 (associated_type_bounds) #57428

Merged
merged 24 commits into from
Jun 6, 2019

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
Collaborator

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.

src/librustc/hir/lowering.rs Outdated Show resolved Hide resolved
src/librustc/hir/lowering.rs Outdated Show resolved Hide resolved
src/librustc/hir/lowering.rs Outdated Show resolved Hide resolved
@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?

src/librustc/hir/map/mod.rs Outdated Show resolved Hide resolved
src/librustc/mir/interpret/error.rs Outdated Show resolved Hide resolved
src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved
@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
@bors
Copy link
Contributor

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
Contributor

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
Contributor

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 merged commit ee89033 into rust-lang:master Jun 6, 2019
@rust-highfive

This comment has been minimized.

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jun 6, 2019
Tested on commit rust-lang/rust@740668d.
Direct link to PR: <rust-lang/rust#57428>

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 rls on windows: test-pass → build-fail (cc @Xanewok, @rust-lang/infra).
💔 rls on linux: test-pass → build-fail (cc @Xanewok, @rust-lang/infra).
matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this pull request Jun 6, 2019
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jun 6, 2019
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 6, 2019
Changes:
````
rustup rust-lang#57428
Remove `to_string()`s from CompilerLintFunctions
Fix comment grammar
Fix .map(..).unwrap_or_else(..) bad suggestion
add suggestions for print/write with newline lint
````
Centril added a commit to Centril/rust that referenced this pull request Jun 6, 2019
submodules: update clippy from 20da8f4 to 71be6f6

Changes:
````
rustup rust-lang#57428
Remove `to_string()`s from CompilerLintFunctions
Fix comment grammar
Fix .map(..).unwrap_or_else(..) bad suggestion
add suggestions for print/write with newline lint
````
Fixes rust-lang#61578
r? @oli-obk
Centril added a commit to Centril/rust that referenced this pull request Jul 9, 2019
…nkfelix

Fix ICEs when `Self` is used in type aliases

I think it is right just to disallow this at resolution stage rather than let typeck produce a cyclic error. This is in line with previous behaviour. There was probably no need at all for the change that introduced this bug in rust-lang#57428, so I've simply reversed it.

Fixes rust-lang#62263, rust-lang#62364, rust-lang#62305.

r? @eddyb
Centril added a commit to Centril/rust that referenced this pull request Jul 9, 2019
…nkfelix

Fix ICEs when `Self` is used in type aliases

I think it is right just to disallow this at resolution stage rather than let typeck produce a cyclic error. This is in line with previous behaviour. There was probably no need at all for the change that introduced this bug in rust-lang#57428, so I've simply reversed it.

Fixes rust-lang#62263, rust-lang#62364, rust-lang#62305.

r? @eddyb
@Centril Centril added the F-associated_type_bounds `#![feature(associated_type_bounds)]` label Aug 10, 2019
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 5, 2020
Changes:
````
rustup rust-lang/rust#57428
Remove `to_string()`s from CompilerLintFunctions
Fix comment grammar
Fix .map(..).unwrap_or_else(..) bad suggestion
add suggestions for print/write with newline lint
````
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 18, 2024
…li-obk

Stabilize associated type bounds (RFC 2289)

This PR stabilizes associated type bounds, which were laid out in [RFC 2289]. This gives us a shorthand to express nested type bounds that would otherwise need to be expressed with nested `impl Trait` or broken into several `where` clauses.

### What are we stabilizing?

We're stabilizing the associated item bounds syntax, which allows us to put bounds in associated type position within other bounds, i.e. `T: Trait<Assoc: Bounds...>`. See [RFC 2289] for motivation.

In all position, the associated type bound syntax expands into a set of two (or more) bounds, and never anything else (see "How does this differ[...]" section for more info).

Associated type bounds are stabilized in four positions:
* **`where` clauses (and APIT)** - This is equivalent to breaking up the bound into two (or more) `where` clauses. For example, `where T: Trait<Assoc: Bound>` is equivalent to `where T: Trait, <T as Trait>::Assoc: Bound`.
* **Supertraits** - Similar to above, `trait CopyIterator: Iterator<Item: Copy> {}`. This is almost equivalent to breaking up the bound into two (or more) `where` clauses; however, the bound on the associated item is implied whenever the trait is used. See rust-lang#112573/rust-lang#112629.
* **Associated type item bounds** - This allows constraining the *nested* rigid projections that are associated with a trait's associated types. e.g. `trait Trait { type Assoc: Trait2<Assoc2: Copy>; }`.
* **opaque item bounds (RPIT, TAIT)** - This allows constraining associated types that are associated with the opaque without having to *name* the opaque. For example, `impl Iterator<Item: Copy>` defines an iterator whose item is `Copy` without having to actually name that item bound.

The latter three are not expressible in surface Rust (though for associated type item bounds, this will change in rust-lang#120752, which I don't believe should block this PR), so this does represent a slight expansion of what can be expressed in trait bounds.

### How does this differ from the RFC?

Compared to the RFC, the current implementation *always* desugars associated type bounds to sets of `ty::Clause`s internally. Specifically, it does *not* introduce a position-dependent desugaring as laid out in [RFC 2289], and in particular:
* It does *not* desugar to anonymous associated items in associated type item bounds.
* It does *not* desugar to nested RPITs in RPIT bounds, nor nested TAITs in TAIT bounds.

This position-dependent desugaring laid out in the RFC existed simply to side-step limitations of the trait solver, which have mostly been fixed in rust-lang#120584. The desugaring laid out in the RFC also added unnecessary complication to the design of the feature, and introduces its own limitations to, for example:
* Conditionally lowering to nested `impl Trait` in certain positions such as RPIT and TAIT means that we inherit the limitations of RPIT/TAIT, namely lack of support for higher-ranked opaque inference. See this code example: rust-lang#120752 (comment).
* Introducing anonymous associated types makes traits no longer object safe, since anonymous associated types are not nameable, and all associated types must be named in `dyn` types.

This last point motivates why this PR is *not* stabilizing support for associated type bounds in `dyn` types, e.g, `dyn Assoc<Item: Bound>`. Why? Because `dyn` types need to have *concrete* types for all associated items, this would necessitate a distinct lowering for associated type bounds, which seems both complicated and unnecessary compared to just requiring the user to write `impl Trait` themselves. See rust-lang#120719.

### Implementation history:

Limited to the significant behavioral changes and fixes and relevant PRs, ping me if I left something out--
* rust-lang#57428
* rust-lang#108063
* rust-lang#110512
* rust-lang#112629
* rust-lang#120719
* rust-lang#120584

Closes rust-lang#52662

[RFC 2289]: https://rust-lang.github.io/rfcs/2289-associated-type-bounds.html
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2024
…-obk

Stabilize associated type bounds (RFC 2289)

This PR stabilizes associated type bounds, which were laid out in [RFC 2289]. This gives us a shorthand to express nested type bounds that would otherwise need to be expressed with nested `impl Trait` or broken into several `where` clauses.

### What are we stabilizing?

We're stabilizing the associated item bounds syntax, which allows us to put bounds in associated type position within other bounds, i.e. `T: Trait<Assoc: Bounds...>`. See [RFC 2289] for motivation.

In all position, the associated type bound syntax expands into a set of two (or more) bounds, and never anything else (see "How does this differ[...]" section for more info).

Associated type bounds are stabilized in four positions:
* **`where` clauses (and APIT)** - This is equivalent to breaking up the bound into two (or more) `where` clauses. For example, `where T: Trait<Assoc: Bound>` is equivalent to `where T: Trait, <T as Trait>::Assoc: Bound`.
* **Supertraits** - Similar to above, `trait CopyIterator: Iterator<Item: Copy> {}`. This is almost equivalent to breaking up the bound into two (or more) `where` clauses; however, the bound on the associated item is implied whenever the trait is used. See rust-lang#112573/rust-lang#112629.
* **Associated type item bounds** - This allows constraining the *nested* rigid projections that are associated with a trait's associated types. e.g. `trait Trait { type Assoc: Trait2<Assoc2: Copy>; }`.
* **opaque item bounds (RPIT, TAIT)** - This allows constraining associated types that are associated with the opaque without having to *name* the opaque. For example, `impl Iterator<Item: Copy>` defines an iterator whose item is `Copy` without having to actually name that item bound.

The latter three are not expressible in surface Rust (though for associated type item bounds, this will change in rust-lang#120752, which I don't believe should block this PR), so this does represent a slight expansion of what can be expressed in trait bounds.

### How does this differ from the RFC?

Compared to the RFC, the current implementation *always* desugars associated type bounds to sets of `ty::Clause`s internally. Specifically, it does *not* introduce a position-dependent desugaring as laid out in [RFC 2289], and in particular:
* It does *not* desugar to anonymous associated items in associated type item bounds.
* It does *not* desugar to nested RPITs in RPIT bounds, nor nested TAITs in TAIT bounds.

This position-dependent desugaring laid out in the RFC existed simply to side-step limitations of the trait solver, which have mostly been fixed in rust-lang#120584. The desugaring laid out in the RFC also added unnecessary complication to the design of the feature, and introduces its own limitations to, for example:
* Conditionally lowering to nested `impl Trait` in certain positions such as RPIT and TAIT means that we inherit the limitations of RPIT/TAIT, namely lack of support for higher-ranked opaque inference. See this code example: rust-lang#120752 (comment).
* Introducing anonymous associated types makes traits no longer object safe, since anonymous associated types are not nameable, and all associated types must be named in `dyn` types.

This last point motivates why this PR is *not* stabilizing support for associated type bounds in `dyn` types, e.g, `dyn Assoc<Item: Bound>`. Why? Because `dyn` types need to have *concrete* types for all associated items, this would necessitate a distinct lowering for associated type bounds, which seems both complicated and unnecessary compared to just requiring the user to write `impl Trait` themselves. See rust-lang#120719.

### Implementation history:

Limited to the significant behavioral changes and fixes and relevant PRs, ping me if I left something out--
* rust-lang#57428
* rust-lang#108063
* rust-lang#110512
* rust-lang#112629
* rust-lang#120719
* rust-lang#120584

Closes rust-lang#52662

[RFC 2289]: https://rust-lang.github.io/rfcs/2289-associated-type-bounds.html
GuillaumeGomez pushed a commit to GuillaumeGomez/rust that referenced this pull request Jul 10, 2024
…-obk

Stabilize associated type bounds (RFC 2289)

This PR stabilizes associated type bounds, which were laid out in [RFC 2289]. This gives us a shorthand to express nested type bounds that would otherwise need to be expressed with nested `impl Trait` or broken into several `where` clauses.

### What are we stabilizing?

We're stabilizing the associated item bounds syntax, which allows us to put bounds in associated type position within other bounds, i.e. `T: Trait<Assoc: Bounds...>`. See [RFC 2289] for motivation.

In all position, the associated type bound syntax expands into a set of two (or more) bounds, and never anything else (see "How does this differ[...]" section for more info).

Associated type bounds are stabilized in four positions:
* **`where` clauses (and APIT)** - This is equivalent to breaking up the bound into two (or more) `where` clauses. For example, `where T: Trait<Assoc: Bound>` is equivalent to `where T: Trait, <T as Trait>::Assoc: Bound`.
* **Supertraits** - Similar to above, `trait CopyIterator: Iterator<Item: Copy> {}`. This is almost equivalent to breaking up the bound into two (or more) `where` clauses; however, the bound on the associated item is implied whenever the trait is used. See rust-lang#112573/rust-lang#112629.
* **Associated type item bounds** - This allows constraining the *nested* rigid projections that are associated with a trait's associated types. e.g. `trait Trait { type Assoc: Trait2<Assoc2: Copy>; }`.
* **opaque item bounds (RPIT, TAIT)** - This allows constraining associated types that are associated with the opaque without having to *name* the opaque. For example, `impl Iterator<Item: Copy>` defines an iterator whose item is `Copy` without having to actually name that item bound.

The latter three are not expressible in surface Rust (though for associated type item bounds, this will change in rust-lang#120752, which I don't believe should block this PR), so this does represent a slight expansion of what can be expressed in trait bounds.

### How does this differ from the RFC?

Compared to the RFC, the current implementation *always* desugars associated type bounds to sets of `ty::Clause`s internally. Specifically, it does *not* introduce a position-dependent desugaring as laid out in [RFC 2289], and in particular:
* It does *not* desugar to anonymous associated items in associated type item bounds.
* It does *not* desugar to nested RPITs in RPIT bounds, nor nested TAITs in TAIT bounds.

This position-dependent desugaring laid out in the RFC existed simply to side-step limitations of the trait solver, which have mostly been fixed in rust-lang#120584. The desugaring laid out in the RFC also added unnecessary complication to the design of the feature, and introduces its own limitations to, for example:
* Conditionally lowering to nested `impl Trait` in certain positions such as RPIT and TAIT means that we inherit the limitations of RPIT/TAIT, namely lack of support for higher-ranked opaque inference. See this code example: rust-lang#120752 (comment).
* Introducing anonymous associated types makes traits no longer object safe, since anonymous associated types are not nameable, and all associated types must be named in `dyn` types.

This last point motivates why this PR is *not* stabilizing support for associated type bounds in `dyn` types, e.g, `dyn Assoc<Item: Bound>`. Why? Because `dyn` types need to have *concrete* types for all associated items, this would necessitate a distinct lowering for associated type bounds, which seems both complicated and unnecessary compared to just requiring the user to write `impl Trait` themselves. See rust-lang#120719.

### Implementation history:

Limited to the significant behavioral changes and fixes and relevant PRs, ping me if I left something out--
* rust-lang#57428
* rust-lang#108063
* rust-lang#110512
* rust-lang#112629
* rust-lang#120719
* rust-lang#120584

Closes rust-lang#52662

[RFC 2289]: https://rust-lang.github.io/rfcs/2289-associated-type-bounds.html
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.