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

Cleanup: Use #![feature(associated_type_bounds)] where applicable #61738

Closed
Centril opened this issue Jun 11, 2019 · 14 comments · Fixed by #63428
Closed

Cleanup: Use #![feature(associated_type_bounds)] where applicable #61738

Centril opened this issue Jun 11, 2019 · 14 comments · Fixed by #63428
Assignees
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. F-associated_type_bounds `#![feature(associated_type_bounds)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Jun 11, 2019

We can now write bounds of form:

T: MyTrait<AssociatedType: OtherTrait>,

When you find bounds of form:

T: MyTrait,
T::AssociatedType: OtherTrait,
// or, but less commonly:
<T as MyTrait>::AssociatedType: OtherTrait,

You can rewrite them to the first form (but you'll need to add the feature gate...).

One regex you can try is: ::\w+: \w+ (doesn't cover lifetimes).

This is relevant both for the standard library and throughout the compiler.
Clippy may also want to do this and possibly also add an experimental lint? cc @oli-obk

cc #52662

This issue has been assigned to @iluuu1994 via this comment.

@Centril Centril added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 11, 2019
@cuviper
Copy link
Member

cuviper commented Jun 15, 2019

Shouldn't this wait for the feature to reach beta?
(This would be an annoying thing to alternate on #[cfg(bootstrap)].)

@Centril
Copy link
Contributor Author

Centril commented Jun 15, 2019

Either works for me.

@Mark-Simulacrum Mark-Simulacrum added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. labels Jun 21, 2019
@iluuu1994
Copy link
Contributor

@rustbot claim

I'll implement this now, you can decide whether you want to merge it now or wait until it's stabilized. We could also make an issue to remove the #![feature(associated_type_bounds)] once the feature is stabilized.

@rustbot rustbot self-assigned this Jul 30, 2019
@iluuu1994
Copy link
Contributor

Do you guys prefer:

impl<T> Clone for ...<T>
    where T: IntoIterator<IntoIter: Clone>

or

impl<T: IntoIterator<IntoIter: Clone>> Clone for ...<T>

I like for former better.

@Centril
Copy link
Contributor Author

Centril commented Jul 30, 2019

@iluuu1994 The first one should be:

impl<T> Clone for ...<T>
where
    T: IntoIterator<IntoIter: Clone>,

per rustfmt.

which one you use is situational I think; I'd use the latter if it cleanly fits on one line and there are not a lot of bounds and the former otherwise.

@Centril Centril added the F-associated_type_bounds `#![feature(associated_type_bounds)]` label Jul 30, 2019
@iluuu1994
Copy link
Contributor

@Centril

Is this one expected to fail?

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=81d86cad842d74f4307d62b4e5ab1a49

pub struct Flatten<I>
where
    I: Iterator,
    I::Item: IntoIterator,
{
    inner: I::Item::IntoIter,
}
error[E0223]: ambiguous associated type
 --> src/lib.rs:6:12
  |
6 |     inner: I::Item::IntoIter,
  |            ^^^^^^^^^^^^^^^^^ help: use fully-qualified syntax: `<<I as std::iter::Iterator>::Item as Trait>::IntoIter`

Not sure how I::Item is ambiguous.

@Centril
Copy link
Contributor Author

Centril commented Jul 31, 2019

Yeah that's ambiguous.

@iluuu1994
Copy link
Contributor

iluuu1994 commented Jul 31, 2019

Additionally, switching to associated type bounds requires more as casts:

Without associated type bounds this is ok:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=a3dba78b4b8af6a3710dc3351b8e2b0a

pub struct Flatten<I>
where
    I: Iterator,
    I::Item: IntoIterator,
{
    inner: <I::Item as IntoIterator>::IntoIter,
}

This one isn't ok:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=d608b0e4b2a9b0f2bd4abfb06bc492ac

#![feature(associated_type_bounds)]

pub struct Flatten<I>
where
    I: Iterator<Item: IntoIterator>,
{
    inner: <I::Item as IntoIterator>::IntoIter,
}
error[E0221]: ambiguous associated type `Item` in bounds of `I`
 --> src/lib.rs:7:13
  |
7 |     inner: <I::Item as IntoIterator>::IntoIter,
  |             ^^^^^^^ ambiguous associated type `Item`
  |
note: associated type `I` could derive from `std::iter::IntoIterator`
 --> src/lib.rs:7:13
  |
7 |     inner: <I::Item as IntoIterator>::IntoIter,
  |             ^^^^^^^
note: associated type `I` could derive from `std::iter::Iterator`
 --> src/lib.rs:7:13
  |
7 |     inner: <I::Item as IntoIterator>::IntoIter,
  |             ^^^^^^^

This one is ok:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=aea5544d3d7e4163af7d28402434701b

#![feature(associated_type_bounds)]

pub struct Flatten<I>
where
    I: Iterator<Item: IntoIterator>,
{
    inner: <<I as Iterator>::Item as IntoIterator>::IntoIter,
}

Note the <I as Iterator> .

@iluuu1994
Copy link
Contributor

Yeah that's ambiguous.

How so? Can't I just be an Iterator which only has one associated type Item? Please note I'm not questioning your answer, I just don't understand it.

@Centril
Copy link
Contributor Author

Centril commented Jul 31, 2019

This one isn't ok:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=d608b0e4b2a9b0f2bd4abfb06bc492ac

#![feature(associated_type_bounds)]

pub struct Flatten<I>
where
    I: Iterator<Item: IntoIterator>,
{
    inner: <I::Item as IntoIterator>::IntoIter,
}

This seems similar to the bug in #61752 :(
cc @alexreg

How so? Can't I just be an Iterator which only has one associated type Item? Please note I'm not questioning your answer, I just don't understand it.

Honestly I don't really sure why that is the case but it has been ambiguous since forever. :)

@iluuu1994
Copy link
Contributor

Honestly I don't really sure why that is the case but it has been ambiguous since forever. :)

Haha ok that works for me 🙂

@alexreg
Copy link
Contributor

alexreg commented Aug 1, 2019

@Centril Sorry, I need to get the PR that fixes that bug merged soon. Niko left some feedback which I should address. Pester me tomorrow or the day after if I still haven't done it.

@alexreg
Copy link
Contributor

alexreg commented Aug 5, 2019

Okay, #61919 is done, just waiting on review & merge. :-)

@iluuu1994
Copy link
Contributor

Great! That was the only issue I found with the associated_type_bounds feature. I'll rebase and remove the then unneeded workarounds after this has been merged. Thanks @Centril and @alexreg!

Centril added a commit to Centril/rust that referenced this issue Aug 9, 2019
…s, r=Centril

Use associated_type_bounds where applicable - closes rust-lang#61738
Centril added a commit to Centril/rust that referenced this issue Aug 9, 2019
…s, r=Centril

Use associated_type_bounds where applicable - closes rust-lang#61738
Centril added a commit to Centril/rust that referenced this issue Aug 10, 2019
…s, r=Centril

Use associated_type_bounds where applicable - closes rust-lang#61738
Centril added a commit to Centril/rust that referenced this issue Aug 10, 2019
…s, r=Centril

Use associated_type_bounds where applicable - closes rust-lang#61738
bors added a commit that referenced this issue Aug 10, 2019
Rollup of 7 pull requests

Successful merges:

 - #63056 (Give built-in macros stable addresses in the standard library)
 - #63337 (Tweak mismatched types error)
 - #63350 (Use associated_type_bounds where applicable - closes #61738)
 - #63394 (Add test for issue 36804)
 - #63399 (More explicit diagnostic when using a `vec![]` in a pattern)
 - #63419 (check against more collisions for TypeId of fn pointer)
 - #63423 (Mention that tuple structs are private if any of their fields are)

Failed merges:

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. F-associated_type_bounds `#![feature(associated_type_bounds)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants