Stabilize FusedIterator by bluss · Pull Request #47463 · rust-lang/rust

bluss · 2018-01-15T19:04:49Z

FusedIterator is a marker trait that promises that the implementing
iterator continues to return None from .next() once it has returned
None once (and/or .next_back(), if implemented).

The effects of FusedIterator are already widely available through
.fuse(), but with stable FusedIterator, stable Rust users can
implement this trait for their iterators when appropriate.

Closes#35602

rust-highfive · 2018-01-15T19:04:53Z

r? @shepmaster

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

sfackler · 2018-01-15T19:36:55Z

This only exists for specialization right? Are we comfortable stabilizing these kinds of things before specialization itself?

bluss · 2018-01-15T20:03:26Z

That is the right question and I think FusedIterator does not depend on any controversial or hard part of specialization, so it seems ok.

meanwhile I can't understand what tidy wants me to fix..

bluss · 2018-01-17T21:20:04Z

rebased to fix newer FusedIterator impls. That should fix tidy.

sfackler · 2018-01-18T05:46:26Z

The worst case is that this becomes a slightly weird/useless trait if we have to do something drastic like roll back specialization entirely. Seems okay to stabilize I guess.

@rfcbot fcp merge

rfcbot · 2018-01-18T05:46:26Z

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

withoutboats · 2018-01-18T08:14:46Z

That is the right question and I think FusedIterator does not depend on any controversial or hard part of specialization, so it seems ok.

Maybe I don't understand how it is used, but isn't one of the main use cases essentially something like this:

traitFuse{typeFused;fnfuse(self) -> Self::Fused;}impl<T:Iterator>FuseforT{ ... }impl<T:FusedIterator>FuseforT{ ... }

This would involve specializing the associated type, which is one of the trickiest parts of specialization (because it could result in different impl instantiations at typeck time from trans time).

bluss · 2018-01-18T11:17:42Z

The main use is in .fused and it is method specialization only, no associated types.

Your example is interesting in itself but not what std is using now.

bluss · 2018-01-18T11:18:08Z

In Iterator::fuse.

alexcrichton · 2018-01-18T15:59:11Z

Is there perhaps a "banner use case" for this trait right now? I've personally only ever seen it as a pretty niche optimization that almost never comes up in practice (I can't remember the last time I used fuse). That, coupled with the concern about specialization, gives me pause here.

I'm also pretty worried about the specialization here. If we roll it back I feel like it would be a breaking change with a trait like this. Users would use this trait only for the performance improvement (right?) and if we break that then we're effectively breaking the trait?

bluss · 2018-01-18T17:11:43Z

@alexcrichton .fuse() is something itertools uses a lot, so it's typical of that use case -- when we build upon iterators completely generically. For example, a.merge(b) from itertools uses .fuse() on both the component iterators.

The use case would be to implement the marker trait FusedIterator for itertools' own adaptors, where appropriate, which makes the .fuse() usages in itertools or other places much more "pay what you need" instead of paying for that extra flag check per iteration.

As a small nice thing, having FusedIterator stable would make the fusing behaviour of iterator adaptors visible and documented by their trait implementations and if/how those depend on the adapted iterator.

I think the specialization that is assumed here really is the basics and it was my impression that we are pretty certain to ship that part sooner rather than later.

alexcrichton · 2018-01-19T17:07:30Z

@bluss ok cool thanks for the info! To clarify as well, would you consider it a breaking change if we end up later neutering the trait to not actually do anything (if specialization is removed)?

The state of specialization AFAIK is sort of "continuously in the air" where there's always a few known soundness holes with no known fixes, but there's a big chunk that's known as highly desirable and safe but unknown how to stabilize.

bluss · 2018-01-19T17:30:15Z

No, I can't see how it is a breaking change. We would have performance setbacks all over libcore without spec, fuse is not among the most important, and I imagine we would want other mechanisms to get the same effects. There is a risk that the trait becomes obsolete then yes if the new mechanism is not trait based.

alexcrichton · 2018-01-19T22:10:24Z

Hm ok, if you're ok with that outcome so am I!

kennytm · 2018-01-31T12:28:55Z

Triage ping, ticky boxes for you @BurntSushi!

emilyalbini · 2018-02-05T10:56:52Z

@BurntSushi there is a nice checkbox in #47463 (comment) waiting for you!

ollie27 · 2018-02-05T14:03:26Z

 }

-#[unstable(feature = "fused", issue = "35602")]
+#[stable(feature = "fused", since = "1.25.0")]


DecodeUtf8 is still unstable so this should be #[unstable(feature = "decode_utf8", issue = "33906")].

ollie27 · 2018-02-05T14:05:10Z

 }

-#[unstable(feature = "fused", issue = "35602")]
+#[stable(feature = "fused", since = "1.25.0")]


RangeInclusive is still unstable so this should be #[unstable(feature = "inclusive_range", reason = "recently added, follows RFC", issue = "28237")].

ollie27 · 2018-02-05T14:07:00Z

 }

-#[unstable(feature = "fused", issue = "35602")]
+#[stable(feature = "fused", since = "1.25.0")]


ExactChunks is still unstable so this should be #[unstable(feature = "exact_chunks", issue = "47115")].

ollie27 · 2018-02-05T14:07:09Z

 }

-#[unstable(feature = "fused", issue = "35602")]
+#[stable(feature = "fused", since = "1.25.0")]


ExactChunksMut is still unstable so this should be #[unstable(feature = "exact_chunks", issue = "47115")].

ollie27 · 2018-02-05T14:11:10Z

 }

-#[unstable(feature = "fused", issue = "35602")]
+#[stable(feature = "fused", since = "1.25.0")]


Utf16Encoder is still unstable so this line can be removed.

ollie27 · 2018-02-05T14:14:47Z

 }

-//#[unstable(feature = "fused", issue = "35602")]
+//#[stable(feature = "fused", since = "1.25.0")]


RSplit is still unstable so this line can just be removed.

ollie27 · 2018-02-05T14:14:56Z

 }

-//#[unstable(feature = "fused", issue = "35602")]
+//#[stable(feature = "fused", since = "1.25.0")]


RSplitMut is still unstable so this line can just be removed.

rfcbot · 2018-02-06T21:22:43Z

🔔 This is now entering its final comment period, as per the review above. 🔔

alexcrichton · 2018-02-06T21:29:24Z

Ok great! @bluss do you wanna address @ollie27's comments and I'll r+?

bors · 2018-03-05T21:43:39Z

⌛ Testing commit c7c23fe with merge d9455e8a7f272c0fec238dbba7af39b560dbf2f0...

bors · 2018-03-06T00:44:16Z

💔 Test failed - status-travis

alexcrichton · 2018-03-06T02:47:01Z

@bors: retry

dist timeout :(

bors · 2018-03-06T03:47:35Z

⌛ Testing commit c7c23fe with merge 386cd00ba6fa150f64cdae31223f5a648fcf08c4...

bors · 2018-03-06T05:25:53Z

💔 Test failed - status-appveyor

kennytm · 2018-03-06T10:06:56Z

@bors retry #48775

Stabilize FusedIterator FusedIterator is a marker trait that promises that the implementing iterator continues to return `None` from `.next()` once it has returned `None` once (and/or `.next_back()`, if implemented). The effects of FusedIterator are already widely available through `.fuse()`, but with stable `FusedIterator`, stable Rust users can implement this trait for their iterators when appropriate. Closesrust-lang#35602

Rollup of 14 pull requests - Successful merges: #48403, #48432, #48546, #48573, #48590, #48657, #48727, #48732, #48753, #48754, #48761, #48474, #48507, #47463 - Failed merges:

bluss · 2018-03-07T20:26:23Z

Aw, that's quite the fight with bors. You pushed it over the finish line @alexcrichton, thank you!

…r=bluss Unstabilize FusedIterator for Flatten since Flatten is unstable PR rust-lang#47463 made `impl<I, U> FusedIterator for Flatten<I>` stable but shouldn't have since `Flatten` is still unstable. This PR makes the impl unstable again.

rust-highfive assigned shepmaster Jan 15, 2018

bluss added the T-libs-api [DEPRECATED; DO NOT USE] label Jan 15, 2018

bluss force-pushed the fused-iterator branch from 76113a2 to 9273422Compare January 15, 2018 20:02

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 16, 2018

bluss force-pushed the fused-iterator branch 2 times, most recently from 644f111 to 96e6103Compare January 17, 2018 21:19

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 18, 2018

kennytm added S-waiting-on-team and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2018

ollie27 reviewed Feb 5, 2018
View reviewed changes

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 6, 2018

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 5, 2018

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 Mar 6, 2018

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 Mar 6, 2018

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 Mar 6, 2018

kennytm mentioned this pull request Mar 6, 2018
Spurious PermissionDenied error with cargo's rename_with_link_search_path test in AppVeyor #48775
Closed

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 Mar 6, 2018

kennytm mentioned this pull request Mar 6, 2018
Rollup of 14 pull requests #48768
Merged

bors added a commit that referenced this pull request Mar 6, 2018
Auto merge of#48768- kennytm:rollup, r=kennytm
2789b06
Rollup of 14 pull requests - Successful merges: #48403, #48432, #48546, #48573, #48590, #48657, #48727, #48732, #48753, #48754, #48761, #48474, #48507, #47463 - Failed merges:

alexcrichton merged commit c7c23fe into rust-lang:masterMar 6, 2018

bluss deleted the fused-iterator branch March 7, 2018 20:16

Centril mentioned this pull request Mar 15, 2018
Unstabilize FusedIterator for Flatten since Flatten is unstable #49049
Merged

orium mentioned this pull request Apr 1, 2018
Implement FusedIterator for our iterators (when it stabilizes) orium/rpds#16
Open

bluss mentioned this pull request Nov 24, 2018
Document optimizations enabled by FusedIterator #55896
Merged

Uh oh!

Uh oh!

Conversation

bluss commented Jan 15, 2018

Uh oh!

rust-highfive commented Jan 15, 2018

Uh oh!

sfackler commented Jan 15, 2018

Uh oh!

bluss commented Jan 15, 2018

Uh oh!

bluss commented Jan 17, 2018

Uh oh!

sfackler commented Jan 18, 2018

Uh oh!

rfcbot commented Jan 18, 2018• edited by BurntSushi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

withoutboats commented Jan 18, 2018

Uh oh!

bluss commented Jan 18, 2018

Uh oh!

bluss commented Jan 18, 2018

Uh oh!

alexcrichton commented Jan 18, 2018

Uh oh!

bluss commented Jan 18, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented Jan 19, 2018

Uh oh!

bluss commented Jan 19, 2018

Uh oh!

alexcrichton commented Jan 19, 2018

Uh oh!

kennytm commented Jan 31, 2018

Uh oh!

emilyalbini commented Feb 5, 2018

Uh oh!

ollie27Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

ollie27Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

ollie27Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

ollie27Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

ollie27Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

ollie27Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

ollie27Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

rfcbot commented Feb 6, 2018

Uh oh!

alexcrichton commented Feb 6, 2018

Uh oh!

bors commented Mar 5, 2018

Uh oh!

bors commented Mar 6, 2018

Uh oh!

alexcrichton commented Mar 6, 2018

Uh oh!

bors commented Mar 6, 2018

Uh oh!

bors commented Mar 6, 2018

Uh oh!

kennytm commented Mar 6, 2018

Uh oh!

bluss commented Mar 7, 2018

Uh oh!

Reviewers

Assignees

rfcbot commented Jan 18, 2018•
edited by BurntSushi
Loading

bluss commented Jan 18, 2018•
edited
Loading