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

FusedIterator marker trait and iter::Fuse specialization #1581

Merged
merged 5 commits into from
Aug 11, 2016

Conversation

Stebalien
Copy link
Contributor

@Stebalien Stebalien commented Apr 15, 2016

This RFC adds a FusedIterator marker trait and specializes iter::Fuse to do
nothing when the underlying iterator already provides the Fuse guarantee.

See this discussion for context.

rendered

WIP PR

/cc @bluss, @gankro

@apasel422
Copy link
Contributor

apasel422 commented Apr 15, 2016

Should this discuss the consequences of an iterator implementing this trait without meeting its requirements? I don't know if it's necessary to make this an unsafe trait, but similar discussion has occurred around ExactSizeIterator.

See also #1051.

@Stebalien
Copy link
Contributor Author

@apasel422, are you worried that someone might be relying on the behavior of Fused for safety? I can't see how this would happen in practice.

@apasel422
Copy link
Contributor

@Stebalien I'm not sure how it would happen in practice either, just pointing out some previous discussions around this matter.

This RFC adds a `FusedIterator` marker trait and specializes `iter::Fuse` to do
nothing when the underlying iterator already provides the `Fuse` guarantee.
@Stebalien
Copy link
Contributor Author

@apasel422 I've added it to the unresolved questions (it's a very good question and, unfortunately, one I can't answer). Maybe if you eagerly freed or truncated something immediately after the iterator returned None and then tried to reference it later? Even if we can't come up with a good example, it might be a good idea to make this unsafe (a) to be safe and (b) to be consistent with other potential specialization traits.

noticeable overhead. Furthermore, many iterators (most if not all iterators in
std) already act as if they were fused (this is considered to be the "polite"
behavior). Therefore, it would be nice to be able to pay the `Fused` overhead
iff necessary.
Copy link
Member

Choose a reason for hiding this comment

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

s/iff necessary/if possible/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I meant "if and only if necessary". Did you read an extra "not" into that sentence?

Copy link
Member

@nagisa nagisa Apr 15, 2016

Choose a reason for hiding this comment

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

Did you read an extra "not" into that sentence?

Oh, I indeed did.

That being said, iff is equivalent to logic-↔ and

“nice to be able to pay the Fused overhead” → “necessary” ∧
“necessary” → “nice to be able to pay the Fused overhead”

doesn’t really look right to me. /me shrugs.

EDIT: I propose “only when necessary” as replacement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. FYI, when writing that sentence, I didn't mean to include the "nice to be able to" in the iff. That is, I meant "pay the overhead" → "necessary to pay the overhead" ∧ "necessary to pay the overhead" -> "pay the overhead". However, I guess the second part is implied.

@nagisa
Copy link
Member

nagisa commented Apr 15, 2016

I feel like we could just specialise exact cases we already know to be fused (that wouldn’t even need an RFC!). The only benefit of FusedIterator I see is a reduction in code duplication at the much larger drawback of increased API surface.

@Stebalien
Copy link
Contributor Author

The RFC hides quite a bit of code. You'd have to repeat all this each time you wanted to specialize. This also prevents programmers from specializing Fuse in arbitrary ways.

@bluss
Copy link
Member

bluss commented Apr 15, 2016

I think we will have some iterator traits with much stronger "regularity" guarantees than this, maybe something like TrustedIterator. It doesn't seem needed for FusedIterator to be unsafe. Isn't it a good thing if it's a simple safe trait?

@Stebalien
Copy link
Contributor Author

@nagisa a more general approach is to make it possible to specialize all iterator adapters (added to the RFC). Unfortunately, this has some drawbacks (also added to the RFC).

@nrc nrc added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Apr 17, 2016
@aturon aturon self-assigned this May 2, 2016
@rkjnsn
Copy link
Contributor

rkjnsn commented May 3, 2016

Does specialization as it exists today provide any way to omit the flag on Fuse (or make it a zero-sized type) when the inner iterator implements FusedIterator?

@Stebalien
Copy link
Contributor Author

Yes. See rust-lang/rust#32999 (comment). Unfortunately, it's significantly more complicated and requires specializing associated types.

@alexcrichton
Copy link
Member

🔔 This RFC is now entering its week-long final comment period 🔔

The libs team is leaning towards merging this as it provides a clear with for already-fused iterators and also allows the same benefits externally. Seems like a win win!

@alexcrichton alexcrichton added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed I-nominated labels Jul 26, 2016
@Stebalien
Copy link
Contributor Author

@alexcrichton would you like me to squash?

@alexcrichton
Copy link
Member

@Stebalien nah leaving all the commits as-is is fine, we actually prefer on RFCs to not squash!

@ruuda
Copy link

ruuda commented Aug 10, 2016

I just came here via TWiR, sorry to be late to the party. +1 for the idea, but I think the name is a bit unfortunate. It might be confusing to people who know about stream fusion, because “fusion” is used here in a similar context but with a totally different meaning. (Paper that uses the term too.) I’ve seen the term ”subject to fusion” used in Haskell documentation, but the FusedIterator trait would have a completely different meaning in Rust. That said, I haven’t seen the term “stream fustion” used in Rust documentation, so perhaps it is not an issue.

It is probably too late to change this now, as Iterator::fuse exists already, and if you think about it for a while then stream fusion doesn’t make sense in the context of Rust iterators. But still, it would be nice if we could avoid the ambiguity.

@rkjnsn
Copy link
Contributor

rkjnsn commented Aug 11, 2016

@Stebalien Is the interface the same for both approaches? That is could we start with the simpler implementation and transition to the more complex but slightly more efficient one later?

@Stebalien
Copy link
Contributor Author

@Stebalien Is the interface the same for both approaches? That is could we start with the simpler implementation and transition to the more complex but slightly more efficient one later?

Yes.

@alexcrichton
Copy link
Member

Discussed at libs triage the other day, the decision was to merge

Thanks again for the RFC @Stebalien!

Tracking issue

@alexcrichton alexcrichton merged commit c697b6a into rust-lang:master Aug 11, 2016
@Stebalien
Copy link
Contributor Author

I''ll revive the old PR ASAP but it might that might not be till September (impending thesis deadline).

bors added a commit to rust-lang/rust that referenced this pull request Aug 23, 2016
Implement 1581 (FusedIterator)

* [ ] Implement on patterns. See #27721 (comment).
* [ ] Handle OS Iterators. A bunch of iterators (`Args`, `Env`, etc.) in libstd wrap platform specific iterators. The current ones all appear to be well-behaved but can we assume that future ones will be?
* [ ] Does someone want to audit this? On first glance, all of the iterators on which I implemented `FusedIterator` appear to be well-behaved but there are a *lot* of them so a second pair of eyes would be nice.
* I haven't touched rustc internal iterators (or the internal rand) because rustc doesn't actually call `fuse()`.
* `FusedIterator` can't be implemented on `std::io::{Bytes, Chars}`.

Closes: #35602 (Tracking Issue)
Implements: rust-lang/rfcs#1581
cuviper pushed a commit to cuviper/rayon that referenced this pull request Mar 28, 2017
Implement 1581 (FusedIterator)

* [ ] Implement on patterns. See rust-lang/rust#27721 (comment).
* [ ] Handle OS Iterators. A bunch of iterators (`Args`, `Env`, etc.) in libstd wrap platform specific iterators. The current ones all appear to be well-behaved but can we assume that future ones will be?
* [ ] Does someone want to audit this? On first glance, all of the iterators on which I implemented `FusedIterator` appear to be well-behaved but there are a *lot* of them so a second pair of eyes would be nice.
* I haven't touched rustc internal iterators (or the internal rand) because rustc doesn't actually call `fuse()`.
* `FusedIterator` can't be implemented on `std::io::{Bytes, Chars}`.

Closes: #35602 (Tracking Issue)
Implements: rust-lang/rfcs#1581
@Centril Centril added A-traits-libstd Standard library trait related proposals & ideas A-iterators Iterator related proposals & ideas A-specialization Proposals relating to specialization. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-iterators Iterator related proposals & ideas A-specialization Proposals relating to specialization. A-traits-libstd Standard library trait related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants