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

Override Cycle::try_fold #62737

Merged
merged 1 commit into from Aug 17, 2019

Conversation

@timvermeulen
Copy link
Contributor

commented Jul 16, 2019

It's not very pretty, but I believe this is the simplest way to correctly implement Cycle::try_fold. The following may seem correct:

loop {
    acc = self.iter.try_fold(acc, &mut f)?;
    self.iter = self.orig.clone();
}

...but this loops infinitely in case self.orig is empty, as opposed to returning acc. So we first have to fully iterate self.orig to check whether it is empty or not, and before that, we have to iterate the remaining elements of self.iter.

This should always call self.orig.clone() the same amount of times as repeated next() calls would.

r? @scottmcm

@Alexendoo

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Ping from triage, any updates? @scottmcm

@scottmcm

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

I'm staring at the loop in here, and as the code it totally makes sense to me, but it has me wondering how much we actually trust the Clone implementation on the iterator we're getting.

The .next() implementation will exit if the cloned iterator ends up being empty somehow. Is it reasonable for a clone of an iterator to return a different number of things? For example, to have an iterator that reads from a &File. (I guess this is a libs questions...)

Could it make sense to just loop over the version that checks for empty, to avoid spinning on the clone if the iterator does end up empty somehow? Or maybe I'm worried for nothing...

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

@scottmcm Even if .clone() erroneously returns an empty iterator, wouldn't that be caught by the emptiness check here? If the Clone implementation is incorrect I would expect .clone() to already return an empty iterator on line 400, as the original iterator will have been iterated at least once before that point.

The Clone implementation could produce an empty iterator only after multiple cycles, of course, but that seems a lot less likely to me. Or did you have a specific situation in mind where this could reasonably happen?

@scottmcm

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

I was thinking of something like this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=71cd4c47937fc4f290ec9b1c65867435

But even that doesn't have two-or-more non-empty cycles followed by an empty one.

So I have to go all the way to this:

use std::cell::Cell;

fn main() {
    let n = &Cell::new(5);
    let c = &Cell::new(0);

    let it = std::iter::from_fn(|| {
        let x = c.get();
        if x == 0 {
            let x = n.get();
            n.set(x - 1);
            c.set(x);
            None
        } else {
            c.set(x - 1);
            Some(x)
        }
    });
    let it = it.cycle();
    it.for_each(|x| {
        dbg!(x);
    });
}

https://play.rust-lang.org/?edition=2018&gist=cd0b0f3e7187e8fa5aecb042f547483a

That's certainly a bit of a stretch, but it's easy to write and kindof an interesting way to do a triangular iterator. So maybe it'd be safer to just put the loop around the empty-checked version? And it feels like that outer loop isn't the place where this is improving perf, so it shouldn't cause any of the benchmarks to get materially worse. (I normally am fine with people getting slightly-odd behaviour from mutable closures, but introducing non-termination is scary to me.)

I guess I should also ping @rust-lang/libs to see if they have conventions for how to handle people doing unusual things in FnMut closures (like Iterator::next) passed to the library...

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Ah, so intentional misuse of cycle? I wouldn't really feel bad for breaking that code... But I also wouldn't mind changing it, if it doesn't impact performance. I'll come up with some benchmarks.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

Based only on the Self: Clone bound on the signature and the doc-comment, I feel it’s pretty clear that the cycle method expects that cloning the iterator will produce "the same" iterator.

Going out of one’s way to share mutable state between clones of the iterator (through Cell) breaks this expectation. I feel that it’s not necessary a bug for cycle to have arbitrary behavior (as long as it’s memory-safe) in that case, or for that behavior to change across versions.

@scottmcm

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

Ok, I'll consider that libs approval for the somewhat-contrived behaviour change here. We have a good 10 weeks to revert if someone else has a strong opinion the other way.

@bors r+

I wonder if Iterator should document this expectation on Clone, like how Hash does?

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

📌 Commit 688c112 has been approved by scottmcm

Centril added a commit to Centril/rust that referenced this pull request Aug 17, 2019

Rollup merge of rust-lang#62737 - timvermeulen:cycle_try_fold, r=scot…
…tmcm

Override Cycle::try_fold

It's not very pretty, but I believe this is the simplest way to correctly implement `Cycle::try_fold`. The following may seem correct:
```rust
loop {
    acc = self.iter.try_fold(acc, &mut f)?;
    self.iter = self.orig.clone();
}
```
...but this loops infinitely in case `self.orig` is empty, as opposed to returning `acc`. So we first have to fully iterate `self.orig` to check whether it is empty or not, and before _that_, we have to iterate the remaining elements of `self.iter`.

This should always call `self.orig.clone()` the same amount of times as repeated `next()` calls would.

r? @scottmcm

bors added a commit that referenced this pull request Aug 17, 2019

Auto merge of #63653 - Centril:rollup-sdu26ew, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #62737 (Override Cycle::try_fold)
 - #63269 (Serialize additional data for procedural macros)
 - #63505 (Hash the remapped sysroot instead of the original.)
 - #63559 (rustc_codegen_utils: account for 1-indexed anonymous lifetimes in v0 mangling.)
 - #63621 (Modify librustc_llvm to pass -DNDEBUG while compiling.)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Aug 17, 2019

Rollup merge of rust-lang#62737 - timvermeulen:cycle_try_fold, r=scot…
…tmcm

Override Cycle::try_fold

It's not very pretty, but I believe this is the simplest way to correctly implement `Cycle::try_fold`. The following may seem correct:
```rust
loop {
    acc = self.iter.try_fold(acc, &mut f)?;
    self.iter = self.orig.clone();
}
```
...but this loops infinitely in case `self.orig` is empty, as opposed to returning `acc`. So we first have to fully iterate `self.orig` to check whether it is empty or not, and before _that_, we have to iterate the remaining elements of `self.iter`.

This should always call `self.orig.clone()` the same amount of times as repeated `next()` calls would.

r? @scottmcm

bors added a commit that referenced this pull request Aug 17, 2019

Auto merge of #63655 - Centril:rollup-ty1ot40, r=Centril
Rollup of 4 pull requests

Successful merges:

 - #62737 (Override Cycle::try_fold)
 - #63505 (Hash the remapped sysroot instead of the original.)
 - #63559 (rustc_codegen_utils: account for 1-indexed anonymous lifetimes in v0 mangling.)
 - #63621 (Modify librustc_llvm to pass -DNDEBUG while compiling.)

Failed merges:

r? @ghost

@bors bors merged commit 688c112 into rust-lang:master Aug 17, 2019

4 checks passed

pr Build #20190716.44 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.