Skip to content

Allow unsized closures in iterator adaptors #43717

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

Closed
wants to merge 1 commit into from

Conversation

bluss
Copy link
Member

@bluss bluss commented Aug 7, 2017

For example, Map<I, F> can be generalized to Map<I, F: ?Sized>,
which allows for example the type Map<I, FnMut(Foo) -> Bar> as an
iterator.

Generalize filter, filter_map, flat_map, inspect, map, skip_while, take_while
like this.

A struct can only have one unsizeable field, so we have to pick either
unsizing the iterator or closure parameter for all of these, but it
seems right to pick the closure parameter. The whole struct itself can
already coerce to the Iterator type.

Fixes #38133

Extra Remarks

  1. My first idea that specialization was required for Map::fold here was
    mistaken (and trying that finds a type inference bug). Instead the remaining
    blemish is that we have to use &mut self.f in Map::fold. The compiler
    doesn't understand that Self: Sized implies F: Sized there.
  2. I don't have a particular motivation goal with this change, just was curious
    about this and it is technically correct.

For example, `Map<I, F>` can be generalized to `Map<I, F: ?Sized>`,
which allows for example the type `Map<I, FnMut(Foo) -> Bar>` as an
iterator.

Generalize `filter, filter_map, flat_map, inspect, map, skip_while, take_while`
like this.

A struct can only have one unsizeable field, so we have to pick either
unsizing the iterator or closure parameter for all of these, but it
seems right to pick the closure parameter. The whole struct itself can
already coerce to the `Iterator` type.
@bluss bluss added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 7, 2017
@rust-highfive
Copy link
Contributor

r? @alexcrichton

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

@alexcrichton
Copy link
Member

Thanks for the PR @bluss! This looks pretty nifty to me!

You mentioned though that you don't have any particular motivational goal with this change, and that'd be my next question! This makes sense to me but I'm not really sure how we'd take advantage of it? Do you have some example use cases in mind though?

@arielb1 arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 8, 2017
@nagisa
Copy link
Member

nagisa commented Aug 8, 2017

I wanted this in something like:

struct MyAdaptor {
    ...,
    inner: Map<::std::iter::Map<::std::ops::Range<u8>, Fn(u8) -> u32>>,
}

where it was fine for MyAdaptor to be ?Sized. I do not remember anymore the exact use case I had for this though, but I do remember being annoyed by this limitation enough to consider filling an issue.

where G: FnMut(Acc, Self::Item) -> Acc,
Self: Sized,
Copy link
Contributor

Choose a reason for hiding this comment

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

couldn't you also just add an F: Sized bound?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not allowed unfortunately. Doesn't match the trait.

@alexcrichton
Copy link
Member

Hm I'm still sort of stuck about what to do for this. This is providing a guarantee in the standard library that we can't ever change. Despite that, though, there's no concrete use cases proposed yet?

@bluss
Copy link
Member Author

bluss commented Aug 10, 2017

I agree with that as a rule of maintainership; no point in merging if it doesn't have its use case clearly described.

The only thing I saw here was the factor of extending the api to the conventions of Rust and internal logic of its type system, and the expectation that we provide as generic items as we practically can.

I can't actually come up with a use case (I'm looking forward to impl Trait!), so it's fine to skip this for that reason.

Regarding @nagisa's example, Rust doesn't in practice allow such a thing verbaitm, since one can't fill the struct field.. it would have to be a type parameter and undergo the unsizing coercion, right?

// Can be cast to `MyAdaptor<Fn(u8) -> u32>`.
struct MyAdaptor<F: ?Sized> {
    ...,
    inner: ::std::iter::Map<::std::ops::Range<u8>, F>,
}

@carols10cents
Copy link
Member

ping @alexcrichton, wdyt about the last comment here?

@alexcrichton
Copy link
Member

Ah oops sorry. Ok it sounds like this is mostly opportunistic for now and doesn't have a motivating use case, so I'm going to close this. If a use case comes up though please feel free to resubmit!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants