Document the behaviour of infinite iterators on potentially-computable methods #47547

varkor · 2018-01-18T15:29:07Z

It’s not entirely clear from the current documentation what behaviour
calling a method such as min on an infinite iterator like RangeFrom
is. One might expect this to terminate, but in fact, for infinite
iterators, min is always nonterminating (at least in the standard
library). This adds a quick note about this behaviour for clarification.

cc @rkruppe

…e methods It’s not entirely clear from the current documentation what behaviour calling a method such as `min` on an infinite iterator like `RangeFrom` is. One might expect this to terminate, but in fact, for infinite iterators, `min` is always nonterminating (at least in the standard library). This adds a quick note about this behaviour for clarification.

rust-highfive · 2018-01-18T15:29:11Z

r? @Kimundi

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

varkor · 2018-01-18T15:30:44Z

src/libcore/iter/iterator.rs

@@ -24,6 +24,10 @@ fn _assert_is_object_safe(_: &Iterator<Item=()>) {}
 /// This is the main iterator trait. For more about the concept of iterators
 /// generally, please see the [module-level documentation]. In particular, you
 /// may want to know how to [implement `Iterator`][impl].
+/// 
+/// Note: Methods on infinite iterators that generally require traversing every


I thought it was probably helpful to point this out in the trait documentation, but if this feels like too much of a niche case to mention here, the module-level documentation could be sufficient.

kennytm · 2018-01-18T17:45:25Z

src/libcore/iter/mod.rs

+//! let least = positives.min().unwrap(); // Oh no! An infinite loop!
+//! // `positives.min` causes an infinite loop, so we won't reach this point!
+//! println!("The least positive number is {}.", least);
+//! ```


This block should be marked as ```no_run, otherwise I don't think the CI will accept it.

Ah, of course!

kennytm · 2018-01-18T17:45:51Z

src/libcore/iter/iterator.rs

@@ -24,6 +24,10 @@ fn _assert_is_object_safe(_: &Iterator<Item=()>) {}
 /// This is the main iterator trait. For more about the concept of iterators
 /// generally, please see the [module-level documentation]. In particular, you
 /// may want to know how to [implement `Iterator`][impl].
+/// 


Trailing white space.

[00:05:27] tidy error: /checkout/src/libcore/iter/mod.rs:300: trailing whitespace [00:05:27] tidy error: /checkout/src/libcore/iter/mod.rs:306: trailing whitespace [00:05:27] tidy error: /checkout/src/libcore/iter/iterator.rs:27: trailing whitespace

scottmcm · 2018-01-19T09:38:59Z

src/libcore/iter/iterator.rs

@@ -25,6 +25,10 @@ fn _assert_is_object_safe(_: &Iterator<Item=()>) {}
 /// generally, please see the [module-level documentation]. In particular, you
 /// may want to know how to [implement `Iterator`][impl].
 ///
+/// Note: Methods on infinite iterators that generally require traversing every
+/// element to produce a result may not terminate, even on traits for which a
+/// result is determinable in finite time.


Given that the trait's doc-comment is essentially just a redirect to the module docs, I don't think this note needs to be here as well. Maybe mention diverging in fold specifically, as the most general case of the methods that diverge when given infinite input?

Yeah, I was divided 50:50 on this one. I've taken it out and replaced it with a comment on fold as you suggested.

scottmcm · 2018-01-19T09:45:41Z

src/libcore/iter/mod.rs

@@ -298,7 +298,21 @@
 //!
 //! This will print the numbers `0` through `4`, each on their own line.
 //!
+//! Bear in mind that methods on infinite iterators, even those for which a
+//! result can be computed in finite time, may not terminate. Specifically,


I think the words "can be computed" lead to a "well, how do I compute it then?" feeling. Perhaps it could be phrased along the lines of "even if a value for them could be determined mathematically", or something?

And instead of "may not terminate", what about being more explicit, with something like "will not return successfully"? (I don't know if talking about the different ways something could diverge would be useful.)

scottmcm · 2018-01-19T09:46:44Z

src/libcore/iter/mod.rs

+//! ```no_run
+//! let positives = 1..;
+//! let least = positives.min().unwrap(); // Oh no! An infinite loop!
+//! // `positives.min` causes an infinite loop, so we won't reach this point!


Nitpick: in debug mode this panics due to overflow, and doesn't infinite-loop
https://play.rust-lang.org/?gist=bff007ce8e876616bbd965ec4c6dcba0&version=stable

abonander · 2018-01-19T20:49:51Z

Has there been discussion on whether or not to override methods like this, e.g. Iterator::min() for RangeFrom, so that they do terminate?

varkor · 2018-01-19T20:53:53Z

@abonander: There's been some quite extensive discussion about that very recently — check #47370 and #47362 out for some of the background.

bluss · 2018-01-21T18:41:47Z

Small quibble! RangeFrom<i64> is not infinite, it's a problematic category of its own.

repeat(0) is infinite. RangeFrom<i64> does not repeat values and it uses a finite size integer; from the docs:

Note: Currently, no overflow checking is done for the Iterator implementation; if you use an integer range and the integer overflows, it might panic in debug mode or create an endless loop in release mode. This overflow behavior might change in the future.

Thus we can say it's not reasonable to use 0.. etc as an infinite iterator, reasonable use says you should not attempt to go past the wrap around.

varkor · 2018-01-21T19:05:06Z

I think RangeFrom is under-documented in general. It claims that "[RangeFrom] contains all values with x >= start", which implies that it might be finite (ending at the maximum value) for non-infinite types (like u64). I don't think it's unfair to claim it's an infinite iterator (at least with the current guarantees, the description here is correct as far as I can tell), but perhaps a repeat(0) would be better.

bluss · 2018-01-21T19:15:35Z

It is not infinite -- that leads to an argument that (1i32..).take(n).max() will complete successfully for all n -- but it does not, it might panic. It's only easier to trip over this for smaller integer types.

varkor · 2018-01-21T19:39:52Z

Yeah, that's a fair point. I'll rework the example to use repeat, which seems safer.

(On a related, but off-topic point, is there a reason [other than backwards compatibility at this stage] that RangeFrom couldn't/shouldn't be finite for integers? In fact, the documentation for RangeFrom seems to suggest that the behaviour for overflow/reaching the limit could change in the future.)

scottmcm · 2018-01-22T03:01:35Z

Small quibble! RangeFrom is not infinite

This was discussed before in #42315 (comment).

It's "infinite" in the sense of "no matter how many times you call it, it will never return None". I don't think it makes sense to consider panicking when discussing iterator lengths, because then you can't even say that repeat(0).map(f) is infinite, or that (0..10).map(f) has length 10.

is there a reason that RangeFrom couldn't/shouldn't be finite for integers?

It's hard, potentially impossible, to implement without either making the struct bigger or making it not return every possible number for the type (ie being 0..256u8, not 0..=256u8). That's one of the many places where the fact that ranges are Iterator instead of IntoIterator is a bit unfortunate.

bluss · 2018-01-22T17:12:08Z

@scottmcm That sounds fine in relation to that change, but saying it is infinite is not correct for all situations. I like my argument still. In your code, if the user knows their f, they know if the iterator is finite. If they don't know their f, then yes, they can't predict if its iteration will finish or diverge.

shepmaster · 2018-01-26T18:38:57Z

Ping from triage, @Kimundi !

kennytm · 2018-01-31T13:11:43Z

Triage ping. Since @Kimundi haven't left any comments on this PR, could ~~@rust-lang/docs or~~ @rust-lang/libs reassign this to another member? Thanks.

oberien · 2018-01-31T16:09:25Z

Coming from the previous discussion, I don't know if defining the current behaviour is the correct way. As soon as it's in the documentation, we can't change it as easily. Instead I still like the potential of optimizations for cases like Repeat, but as discussed previously, an RFC is needed.

steveklabnik · 2018-01-31T16:24:19Z

I'd like libs to take it, as we need to know what behavior is desired in order to document it.

dtolnay · 2018-01-31T17:27:53Z

I think it is valuable to document this behavior, and I like the way @varkor has phrased this as an informative warning rather than as a firm commitment for the standard library to always preserve the behavior that certain methods loop infinitely. I really like the "determined mathematically" suggestion from @scottmcm and I am glad that was incorporated. Let's merge when someone from docs is okay with this as well.

pietroalbini · 2018-02-05T10:47:09Z

Could someone from @rust-lang/libs review this?

alexcrichton · 2018-02-05T15:26:23Z

@pietroalbini oh I think we're good on the libs front! @dtolnay mentioned above and I suspect many other @rust-lang/libs members would agree, so at this point I think we're looking for a sign-off from @rust-lang/docs.

GuillaumeGomez · 2018-02-05T18:39:00Z

Looks good for me.

shepmaster · 2018-02-09T22:38:58Z

@alexcrichton We have a sign-off from docs; is that sufficient? Or were you looking for entire team discussion?

frewsxcv · 2018-02-10T15:02:46Z

just read this over as well, and it seems reasonable to document this! one question i have is why fold gets the added documentation but not other Iterator methods that also exhaust the iterator, but it's a minor point that shouldn't block this PR my opinion. i don't think we need the rest of thee docs team to sign off on this so i'm going to approve this. thanks for your contribution @varkor! 💟

@bors r+ rollup

bors · 2018-02-10T15:02:47Z

📌 Commit f129374 has been approved by frewsxcv

…oc, r=frewsxcv Document the behaviour of infinite iterators on potentially-computable methods It’s not entirely clear from the current documentation what behaviour calling a method such as `min` on an infinite iterator like `RangeFrom` is. One might expect this to terminate, but in fact, for infinite iterators, `min` is always nonterminating (at least in the standard library). This adds a quick note about this behaviour for clarification.

Rollup of 20 pull requests - Successful merges: #47790, #47835, #47854, #48015, #48047, #48051, #48058, #48059, #48064, #48078, #48080, #48086, #48098, #48101, #48107, #48100, #48085, #48120, #48124, #47547 - Failed merges:

rust-highfive assigned Kimundi Jan 18, 2018

varkor commented Jan 18, 2018

View reviewed changes

kennytm added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2018

kennytm reviewed Jan 18, 2018

View reviewed changes

Make example no_run

91668fb

scottmcm reviewed Jan 19, 2018

View reviewed changes

Adjust language as per suggestions

0be5173

Use repeat instead of RangeFrom

f129374

kennytm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Jan 31, 2018

kennytm added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 31, 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 Feb 10, 2018

kennytm mentioned this pull request Feb 10, 2018

Rollup of 20 pull requests #48113

Merged

bors merged commit f129374 into rust-lang:master Feb 10, 2018

varkor deleted the infinite-iterators-warning-doc branch February 10, 2018 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document the behaviour of infinite iterators on potentially-computable methods #47547

Document the behaviour of infinite iterators on potentially-computable methods #47547

varkor commented Jan 18, 2018

rust-highfive commented Jan 18, 2018

varkor Jan 18, 2018

kennytm Jan 18, 2018

varkor Jan 18, 2018

kennytm Jan 18, 2018

scottmcm Jan 19, 2018

varkor Jan 19, 2018

scottmcm Jan 19, 2018

scottmcm Jan 19, 2018

abonander commented Jan 19, 2018

varkor commented Jan 19, 2018

bluss commented Jan 21, 2018 •

edited

Loading

varkor commented Jan 21, 2018 •

edited

Loading

bluss commented Jan 21, 2018 •

edited

Loading

varkor commented Jan 21, 2018 •

edited

Loading

scottmcm commented Jan 22, 2018

bluss commented Jan 22, 2018

shepmaster commented Jan 26, 2018

kennytm commented Jan 31, 2018 •

edited

Loading

oberien commented Jan 31, 2018

steveklabnik commented Jan 31, 2018

dtolnay commented Jan 31, 2018

pietroalbini commented Feb 5, 2018

alexcrichton commented Feb 5, 2018

GuillaumeGomez commented Feb 5, 2018

shepmaster commented Feb 9, 2018

frewsxcv commented Feb 10, 2018

bors commented Feb 10, 2018

Document the behaviour of infinite iterators on potentially-computable methods #47547

Document the behaviour of infinite iterators on potentially-computable methods #47547

Conversation

varkor commented Jan 18, 2018

rust-highfive commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander commented Jan 19, 2018

varkor commented Jan 19, 2018

bluss commented Jan 21, 2018 • edited Loading

varkor commented Jan 21, 2018 • edited Loading

bluss commented Jan 21, 2018 • edited Loading

varkor commented Jan 21, 2018 • edited Loading

scottmcm commented Jan 22, 2018

bluss commented Jan 22, 2018

shepmaster commented Jan 26, 2018

kennytm commented Jan 31, 2018 • edited Loading

oberien commented Jan 31, 2018

steveklabnik commented Jan 31, 2018

dtolnay commented Jan 31, 2018

pietroalbini commented Feb 5, 2018

alexcrichton commented Feb 5, 2018

GuillaumeGomez commented Feb 5, 2018

shepmaster commented Feb 9, 2018

frewsxcv commented Feb 10, 2018

bors commented Feb 10, 2018

bluss commented Jan 21, 2018 •

edited

Loading

varkor commented Jan 21, 2018 •

edited

Loading

bluss commented Jan 21, 2018 •

edited

Loading

varkor commented Jan 21, 2018 •

edited

Loading

kennytm commented Jan 31, 2018 •

edited

Loading