Fix bugs in Peekable and Flatten when using non-fused iterators #68915

timvermeulen · 2020-02-07T02:31:11Z

I fixed a couple of bugs with regard to the Peekable and Flatten/FlatMap iterators when the underlying iterator isn't fused. For testing, I also added a NonFused iterator wrapper that panics when next or next_back is called on an iterator that has returned None before, which will hopefully make it easier to spot these mistakes in the future.

Peekable

Peekable::next_back was implemented as

self.iter.next_back().or_else(|| self.peeked.take().and_then(|x| x))

which is incorrect because when the peeked field is Some(None), then None has already been returned from the inner iterator and what it returns from next_back can no longer be relied upon. test_peekable_non_fused tests this.

Flatten

When a FlattenCompat instance only has a backiter remaining (i.e. self.frontiter is None and self.iter is empty), then next will call self.iter.next() every time, so the iter field needs to be fused. I fixed it by giving it the type Fuse<I> instead of I, I think this is the only way to fix it. test_flatten_non_fused_outer tests this.

Furthermore, previously FlattenCompat::next did not set self.frontiter to None after it returned None, which is incorrect when the inner iterator type isn't fused. I just delegated it to try_fold because that already handles it correctly. test_flatten_non_fused_inner tests this.

r? @scottmcm

rust-highfive · 2020-02-10T12:51:53Z

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-02-10T12:33:13.8508919Z ========================== Starting Command Output ===========================
2020-02-10T12:33:13.8511553Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/aa165ab7-07ad-4243-8557-f7e311981708.sh
2020-02-10T12:33:13.8511794Z 
2020-02-10T12:33:13.8517054Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-10T12:33:13.8525396Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/68915/merge to s
2020-02-10T12:33:13.8527003Z Task         : Get sources
2020-02-10T12:33:13.8527039Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-10T12:33:13.8527121Z Version      : 1.0.0
2020-02-10T12:33:13.8527155Z Author       : Microsoft
---
2020-02-10T12:33:18.0376313Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-10T12:33:18.9307459Z ##[command]git config gc.auto 0
2020-02-10T12:33:18.9312936Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-10T12:33:18.9316211Z ##[command]git config --get-all http.proxy
2020-02-10T12:33:18.9325617Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/68915/merge:refs/remotes/pull/68915/merge
---
2020-02-10T12:39:36.4728816Z    Compiling serde_json v1.0.40
2020-02-10T12:39:38.1407656Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2020-02-10T12:39:48.5632803Z     Finished release [optimized] target(s) in 1m 34s
2020-02-10T12:39:48.5743153Z tidy check
2020-02-10T12:39:49.5921458Z tidy error: /checkout/src/libcore/tests/iter.rs: too many lines (3012) (add `// ignore-tidy-filelength` to the file to suppress this error)
2020-02-10T12:39:51.2949138Z Found 487 error codes
2020-02-10T12:39:51.2949948Z Found 0 error codes with no tests
2020-02-10T12:39:51.2950280Z Done!
2020-02-10T12:39:51.2950386Z some tidy checks failed
2020-02-10T12:39:51.2950386Z some tidy checks failed
2020-02-10T12:39:51.2954264Z 
2020-02-10T12:39:51.2954571Z 
2020-02-10T12:39:51.2962046Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2020-02-10T12:39:51.2966162Z 
2020-02-10T12:39:51.2966330Z 
2020-02-10T12:39:51.3015501Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2020-02-10T12:39:51.3015630Z Build completed unsuccessfully in 0:01:44
2020-02-10T12:39:51.3015630Z Build completed unsuccessfully in 0:01:44
2020-02-10T12:39:51.3022491Z == clock drift check ==
2020-02-10T12:39:51.3033675Z   local time: Mon Feb 10 12:39:51 UTC 2020
2020-02-10T12:39:51.8509186Z   network time: Mon, 10 Feb 2020 12:39:51 GMT
2020-02-10T12:39:51.8509275Z == end clock drift check ==
2020-02-10T12:39:52.6497893Z 
2020-02-10T12:39:52.6625090Z ##[error]Bash exited with code '1'.
2020-02-10T12:39:52.6641289Z ##[section]Finishing: Run build
2020-02-10T12:39:52.6659781Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/68915/merge to s
2020-02-10T12:39:52.6661912Z Task         : Get sources
2020-02-10T12:39:52.6661959Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-10T12:39:52.6662024Z Version      : 1.0.0
2020-02-10T12:39:52.6662066Z Author       : Microsoft
2020-02-10T12:39:52.6662066Z Author       : Microsoft
2020-02-10T12:39:52.6662112Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-02-10T12:39:52.6662181Z ==============================================================================
2020-02-10T12:39:53.0909843Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-02-10T12:39:53.0990401Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/68915/merge to s
2020-02-10T12:39:53.1111515Z Cleaning up task key
2020-02-10T12:39:53.1112318Z Start cleaning up orphan processes.
2020-02-10T12:39:53.1232744Z Terminate orphan process: pid (3790) (python)
2020-02-10T12:39:53.1694456Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Dylan-DPC-zz · 2020-02-13T16:52:14Z

r? @Amanieu

Amanieu · 2020-03-02T11:14:39Z

Furthermore, previously FlattenCompat::next did not set self.frontiter to None after it returned None, which is incorrect when the inner iterator type isn't fused. I just delegated it to try_fold because that already handles it correctly. test_flatten_non_fused_inner tests this.

I am concerned that the generated code is now substantially more complicated. Wouldn't it have been better to simply set frontiter to None after the if?

All the other changes look fine.

JohnCSimon · 2020-03-09T07:08:57Z

Ping from triage:
@timvermeulen can you please address the comments from Amanieu? Thank you.

timvermeulen · 2020-03-13T22:47:24Z

Sorry, yep, I'll address them soon.

timvermeulen · 2020-03-16T23:08:07Z

@Amanieu Thanks for taking a look! I assumed that the generated code for next / next_back would be the same, but benchmarks show that this was indeed a regression, so I've fixed it in the way you suggested.

Amanieu · 2020-03-17T07:26:43Z

@bors r+

bors · 2020-03-17T07:26:45Z

📌 Commit 8cf33b0 has been approved by Amanieu

@scottmcm

Fix bugs in Peekable and Flatten when using non-fused iterators I fixed a couple of bugs with regard to the `Peekable` and `Flatten`/`FlatMap` iterators when the underlying iterator isn't fused. For testing, I also added a `NonFused` iterator wrapper that panics when `next` or `next_back` is called on an iterator that has returned `None` before, which will hopefully make it easier to spot these mistakes in the future. ### Peekable `Peekable::next_back` was implemented as ```rust self.iter.next_back().or_else(|| self.peeked.take().and_then(|x| x)) ``` which is incorrect because when the `peeked` field is `Some(None)`, then `None` has already been returned from the inner iterator and what it returns from `next_back` can no longer be relied upon. `test_peekable_non_fused` tests this. ### Flatten When a `FlattenCompat` instance only has a `backiter` remaining (i.e. `self.frontiter` is `None` and `self.iter` is empty), then `next` will call `self.iter.next()` every time, so the `iter` field needs to be fused. I fixed it by giving it the type `Fuse<I>` instead of `I`, I think this is the only way to fix it. `test_flatten_non_fused_outer` tests this. Furthermore, previously `FlattenCompat::next` did not set `self.frontiter` to `None` after it returned `None`, which is incorrect when the inner iterator type isn't fused. I just delegated it to `try_fold` because that already handles it correctly. `test_flatten_non_fused_inner` tests this. r? @scottmcm

bors · 2020-03-18T03:09:03Z

⌛ Testing commit 8cf33b0 with merge d939f70...

bors · 2020-03-18T06:31:18Z

☀️ Test successful - checks-azure
Approved by: Amanieu
Pushing d939f70 to master...

rust-highfive · 2020-03-18T06:32:47Z

📣 Toolstate changed by #68915!

Tested on commit d939f70.
Direct link to PR: #68915

💔 rls on linux: test-pass → test-fail (cc @Xanewok).

@Xanewok

Tested on commit rust-lang/rust@d939f70. Direct link to PR: <rust-lang/rust#68915> 💔 rls on linux: test-pass → test-fail (cc @Xanewok).

rust-highfive assigned scottmcm Feb 7, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 7, 2020

timvermeulen added 2 commits February 10, 2020 13:32

Fix Peekable::next_back

14bcf0a

Fuse FlattenCompat's inner iterator

16a23e7

rust-highfive assigned Amanieu and unassigned scottmcm Feb 13, 2020

timvermeulen added 2 commits March 17, 2020 00:03

Fix FlattenCompat::{next, next_back}

cff1182

Add tests

8cf33b0

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 17, 2020

Manishearth mentioned this pull request Mar 17, 2020

Rollup of 12 pull requests #70085

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 18, 2020

bors merged commit d939f70 into rust-lang:master Mar 18, 2020

timvermeulen deleted the non_fused_iter branch March 18, 2020 14:23

cuviper mentioned this pull request Jan 21, 2021

Flatten and FlatMap have incorrect bounds for their FusedIterator impls #81248

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bugs in Peekable and Flatten when using non-fused iterators #68915

Fix bugs in Peekable and Flatten when using non-fused iterators #68915

timvermeulen commented Feb 7, 2020

rust-highfive commented Feb 10, 2020

Dylan-DPC-zz commented Feb 13, 2020

Amanieu commented Mar 2, 2020

JohnCSimon commented Mar 9, 2020

timvermeulen commented Mar 13, 2020

timvermeulen commented Mar 16, 2020

Amanieu commented Mar 17, 2020

bors commented Mar 17, 2020

bors commented Mar 18, 2020

bors commented Mar 18, 2020

rust-highfive commented Mar 18, 2020

Fix bugs in Peekable and Flatten when using non-fused iterators #68915

Fix bugs in Peekable and Flatten when using non-fused iterators #68915

Conversation

timvermeulen commented Feb 7, 2020

Peekable

Flatten

rust-highfive commented Feb 10, 2020

Dylan-DPC-zz commented Feb 13, 2020

Amanieu commented Mar 2, 2020

JohnCSimon commented Mar 9, 2020

timvermeulen commented Mar 13, 2020

timvermeulen commented Mar 16, 2020

Amanieu commented Mar 17, 2020

bors commented Mar 17, 2020

bors commented Mar 18, 2020

bors commented Mar 18, 2020

rust-highfive commented Mar 18, 2020