Clarify iterator by_ref docs #135987

hkBst · 2025-01-24T11:54:24Z

rustbot · 2025-01-24T11:54:32Z

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

ibraheemdev · 2025-02-08T04:20:17Z

library/core/src/iter/traits/iterator.rs

    ///
-    /// This is useful to allow applying iterator adapters while still
-    /// retaining ownership of the original iterator.


I don't think this part of the diff is necessary. The one-liner at the top and the explanation seems sufficient, maybe just adding a disclaimer that the original iterator is still mutated is enough.

"Borrows an iterator, rather than consuming it." is quite poorly phrased, as it is subsequent methods applied to the borrow, that will be prevented from consuming the original iterator.
"This is useful to allow applying iterator adapters while still retaining ownership of the original iterator." is also quite opaque, as it is already possible to do just that with the original iterator, except if the method happens to take self, but that crucial detail is not mentioned.

In addition to this, it is not obvious how the mutable borrow of an iterator turns into an iterator. It wasn't until I started writing this PR that I thought to myself: "wait a minute, there is no reason a mutable borrow of an iterator should be an iterator, unless there is an impl block that says so." Before that, I think I thought something like this: "I must be missing something about how mutable references work, because I don't see how this method works..." It is just like the chars method on Strings giving a Chars struct which impls iterator, except that there is no &mut impl Iterator struct to point to, therefore I think it makes sense to link to the relevant impl directly, to show that there is no magic here.

I do agree that the original iterator being mutated is still missing from my text, so I will work on that.

I see what you are getting at, but I feel that this is a bit wordy. Let me try rewording it. We should keep a one sentence header for rustdoc, Read::by_ref does a better job of this.

Alright, there should be a short one sentence summary at the top, can do.

ibraheemdev · 2025-02-08T04:24:56Z

library/core/src/iter/traits/iterator.rs

+    /// This turns an iterator into its mutably-borrowed iterator via
+    /// [impl<I: Iterator + ?Sized> Iterator for &mut I { type Item = I::Item; ...}](trait.Iterator.html#impl-Iterator-for-&mut I).
+    /// This allows using consuming methods on the mutable borrow,
+    /// which then consume the mutable borrow instead of the original,
+    /// such that you can continue to use the original iterator afterwards.


Suggested change

/// This turns an iterator into its mutably-borrowed iterator via

/// [impl<I: Iterator + ?Sized> Iterator for &mut I { type Item = I::Item; ...}](trait.Iterator.html#impl-Iterator-for-&mut I).

/// This allows using consuming methods on the mutable borrow,

/// which then consume the mutable borrow instead of the original,

/// such that you can continue to use the original iterator afterwards.

/// This method returns a mutable borrow of the iterator, which also

/// implements the [`Iterator`] trait. This allows you to use iterator adapters

/// through the reference, which will mutate but still retain ownership of

/// the original iterator.

I agree that it is important to say that the original iterator is mutated when the &mut is used.

hkBst · 2025-02-09T16:19:36Z

@ibraheemdev I've tried to reformulate. Let me know if it is better.

hkBst · 2025-02-09T16:20:15Z

@rustbot ready

ibraheemdev · 2025-02-11T02:37:56Z

library/core/src/iter/traits/iterator.rs

+    /// Most `Iterator` methods, other than the `next` method (and this one),
+    /// are consuming methods; they move the iterator, taking ownership of it,
+    /// via their `self` parameter (and then mutating it privately).
+    /// Usually, this is fine, though sometimes you want to
+    /// mutate the original iterator without giving up ownership of it.
+    /// That is where this method comes in.
+    ///
+    /// The return value of this method acts like the original iterator,
+    /// except that all consuming methods behave like mutating methods,
+    /// which mutate the original iterator, without taking ownership of it.
+    ///
+    /// This works, because the mutable reference that this method returns,
+    /// implements the `Iterator` trait in its own right via:
+    /// [impl<I: Iterator + ?Sized> Iterator for &mut I { type Item = I::Item; ...}](trait.Iterator.html#impl-Iterator-for-%26mut I).
+    /// This implementation simply passes all method calls on to the original iterator,
+    /// allowing you to call a consuming iterator method on the mutable reference,
+    /// which will mutate the original,
+    /// without giving up ownership of the original iterator.


Suggested change

/// Most `Iterator` methods, other than the `next` method (and this one),

/// are consuming methods; they move the iterator, taking ownership of it,

/// via their `self` parameter (and then mutating it privately).

/// Usually, this is fine, though sometimes you want to

/// mutate the original iterator without giving up ownership of it.

/// That is where this method comes in.

///

/// The return value of this method acts like the original iterator,

/// except that all consuming methods behave like mutating methods,

/// which mutate the original iterator, without taking ownership of it.

///

/// This works, because the mutable reference that this method returns,

/// implements the `Iterator` trait in its own right via:

/// [impl<I: Iterator + ?Sized> Iterator for &mut I { type Item = I::Item; ...}](trait.Iterator.html#impl-Iterator-for-%26mut I).

/// This implementation simply passes all method calls on to the original iterator,

/// allowing you to call a consuming iterator method on the mutable reference,

/// which will mutate the original,

/// without giving up ownership of the original iterator.

/// Creates a "by reference" adapter for this instance of `Iterator`.

///

/// The returned adapter also implements `Iterator` and will simply borrow

/// this current iterator. Any iteration performed through the reference

/// will mutate the original iterator.

///

/// Most `Iterator` methods are consuming methods, returning adapters

/// that take ownership of the iterator. This method allows you to mutate

/// the original iterator without giving up ownership of it, consuming the

/// reference instead of the iterator itself.

Oh, consuming methods is not very good, since the book says: Methods that call next are called consuming adapters, because calling them uses up the iterator. The original text already talks about consuming the iterator, which is also no good.

And, Iterator adapters are methods defined on the Iterator trait that don’t consume the iterator. Instead, they produce different iterators by changing some aspect of the original iterator.. You're using the term adapter for the return value of a method here instead of for the method itself.

Oh, consuming methods is not very good, since the book says: Methods that call next are called consuming adapters, because calling them uses up the iterator. The original text already talks about consuming the iterator, which is also no good.

That part is from your version :) Maybe we can just say "most Iterator methods return adapters that take ownership of the original iterator" instead?

And, Iterator adapters are methods defined on the Iterator trait that don’t consume the iterator. Instead, they produce different iterators by changing some aspect of the original iterator.. You're using the term adapter for the return value of a method here instead of for the method itself.

I think we use the two interchangeably. For example, the docs of Iterator::for_each say:

In some cases for_each may also be faster than a loop, because it will use internal iteration on adapters like Chain.

We don't have to be too nitpicky with the terminology as long as it gets the point across, I don't want to have three paragraphs to explain that by_ref returns an iterator that mutates the original when it is used.

ibraheemdev · 2025-02-11T02:39:29Z

library/core/src/iter/traits/iterator.rs

@@ -4019,6 +4033,7 @@ where
    }
 }

+/// Implements Iterator for mutable references to Iterator, such as those produced by [Iterator::by_ref]


Suggested change

/// Implements Iterator for mutable references to Iterator, such as those produced by [Iterator::by_ref]

/// Implements `Iterator` for mutable references to iterators, such as those produced by [`Iterator::by_ref`].

hkBst · 2025-02-11T06:35:43Z

Another attempt, less wordy and with a one-sentence summary. Also no more confusing uses of consuming. Let me know what you think.

ibraheemdev · 2025-02-11T08:09:29Z

library/core/src/iter/traits/iterator.rs

+    /// Most `Iterator` methods, other than the `next` method (and this one),
+    /// take ownership (via a `self` parameter) of the iterator and then mutate it.
+    /// This method retuns a mutable reference to the original iterator,
+    /// that acts like the original iterator in all respects,
+    /// except that ownership-taking methods take ownership of the reference instead of of the original.
+    /// All mutation by ownership-taking methods will still mutate the original iterator.
+    ///
+    /// # Technical details
+    ///
+    /// The mutable reference that this method returns, implements the `Iterator` trait via:
+    /// [impl<I: Iterator + ?Sized> Iterator for &mut I { type Item = I::Item; ...}](trait.Iterator.html#impl-Iterator-for-%26mut I).
+    /// This implementation simply passes all method calls on to the original iterator.


Suggested change

/// Most `Iterator` methods, other than the `next` method (and this one),

/// take ownership (via a `self` parameter) of the iterator and then mutate it.

/// This method retuns a mutable reference to the original iterator,

/// that acts like the original iterator in all respects,

/// except that ownership-taking methods take ownership of the reference instead of of the original.

/// All mutation by ownership-taking methods will still mutate the original iterator.

///

/// # Technical details

///

/// The mutable reference that this method returns, implements the `Iterator` trait via:

/// [impl<I: Iterator + ?Sized> Iterator for &mut I { type Item = I::Item; ...}](trait.Iterator.html#impl-Iterator-for-%26mut I).

/// This implementation simply passes all method calls on to the original iterator.

/// Most `Iterator` adapters take ownership (via a `self` parameter) of the iterator.

/// This method returns a mutable reference to the original, which implements the

/// iterator trait by delegating to the original iterator. This allows iterator adapters

/// to take ownership of the reference, mutating the original iterator while retaining

/// ownership.

It's not true that only next doesn't take ownership, there are other methods like nth and advance_by.

"and mutate it" isn't exactly true, because the methods themselves don't mutate the iterator. We can just omit this.

"mutation by ownership-taking methods" is a little awkward, I updated the wording to use "adapters" to refer to the adapter methods explicitly, which I explained take ownership.

The hyperlink to the iterator implementation doesn't seem to work: Rustdoc can't create link to trait implementation with generics ([link](#impl-Trait-for-Type<G1, G2>)) #105600. I added a written explanation instead.

Does that seem reasonable?

It's not true that only next doesn't take ownership, there are other methods like nth and advance_by.

That's why the text says "Most .. methods", but I thought it important to mention next as an exception because it is so fundamental. Of the 76 methods listed only 17 don't take self, so 59 methods do, which certainly counts as most, no? But I would also be happy with "many" instead of "most"... I see that your proposed replacement differs mostly by replacing "methods" with "adapters" but all of, and advance_by are adapters and next and nth arguably even "iterator adapters" (because Option impls Iterator).

"and mutate it" isn't exactly true, because the methods themselves don't mutate the iterator. We can just omit this.

It is true! They take ownership and then you have no way of knowing what they do, but in fact they do mutate the iterator, which you can see if you use by_ref to prevent ownership-taking! I think it is important to mention this, because then it cannot be a surprise if after preventing ownership taking these methods still (!!!) mutate the original. They always mutate the original. The only difference is in whether ownership is transferred or not.

"mutation by ownership-taking methods" is a little awkward, I updated the wording to use "adapters" to refer to the adapter methods explicitly, which I explained take ownership.

The book calls next an adapter and it does not take ownership. The book names consuming adapters (next and other methods that call next), iterator adapters (methods whose return value impls iterator (next returns an Option which is an iterator, so arguably it is also an iterator adapter)), and other methods (only size_hint?).

The hyperlink to the iterator implementation doesn't seem to work:

Should work now... thanks for catching!

That's why the text says "Most .. methods", but I thought it important to mention next as an exception because it is so fundamental.

My only concern was that "other than the next method (and this one)" seems to suggest that every other iterator method is consuming.

It is true! They take ownership and then you have no way of knowing what they do, but in fact they do mutate the iterator, which you can see if you use by_ref to prevent ownership-taking

Right, but the methods themselves don't mutate it, they simply return a new iterator adapter/wrapper.

The book calls next an adapter and it does not take ownership. The book names consuming adapters (next and other methods that call next), iterator adapters (methods whose return value impls iterator (next returns an Option which is an iterator, so arguably it is also an iterator adapter)), and other methods (only size_hint?).

Sure, but we already clarified that we are talking about adapters that consume self in the previous sentence.

Should work now... thanks for catching!

Can we hyperlink to the implementation through the "which also implements the iterator trait" part of the text? I don't think it's worth an entire separate section.

ibraheemdev · 2025-02-11T08:12:03Z

library/core/src/iter/traits/iterator.rs

@@ -1825,10 +1825,20 @@ pub trait Iterator {
        Inspect::new(self, f)
    }

-    /// Borrows an iterator, rather than consuming it.
+    /// Iterator adapter that turns ownership-taking methods into mutating methods.


Suggested change

/// Iterator adapter that turns ownership-taking methods into mutating methods.

/// Creates a "by reference" adapter for this instance of `Iterator`.

I know this is a little vague, but the way you've written it reads awkwardly. The original seems fine as well with the new explanation. It's hard to fit exactly what we want to say into one sentence, so I think being general is fine.

Alright, I agree that my phrasing is perhaps not good, and the other explanation is now better, so the summary can be simpler. Going off your "by reference" iterator, how about something like: "Iterator adapter that passes on method calls to the original iterator by mutable reference."?

How about we keep my suggestion for now to mirror io::{Read, Write}::by_ref and Allocator::by_ref, and you can open a separate PR to discuss changing it? I don't want to get too hung up on this.

Alright, done.

hkBst · 2025-02-11T14:23:50Z

Alright, going back to basics. I hope nothing here is controversial. We've both invested way too much time here.

hkBst · 2025-02-11T14:24:07Z

@rustbot ready

rustbot assigned ibraheemdev Jan 24, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 24, 2025

ibraheemdev requested changes Feb 8, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 8, 2025

hkBst force-pushed the patch-20 branch from b1484f5 to 5022938 Compare February 9, 2025 16:17

hkBst requested a review from ibraheemdev February 9, 2025 16:19

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 9, 2025

ibraheemdev requested changes Feb 11, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2025

ibraheemdev reviewed Feb 11, 2025

View reviewed changes

hkBst force-pushed the patch-20 branch from 5022938 to d133dda Compare February 11, 2025 06:34

ibraheemdev reviewed Feb 11, 2025

View reviewed changes

hkBst force-pushed the patch-20 branch from d133dda to daa5339 Compare February 11, 2025 08:59

Clarify iterator by_ref docs

a3903e2

hkBst force-pushed the patch-20 branch from daa5339 to a3903e2 Compare February 11, 2025 14:21

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify iterator by_ref docs #135987

Clarify iterator by_ref docs #135987

hkBst commented Jan 24, 2025

rustbot commented Jan 24, 2025

ibraheemdev Feb 8, 2025

hkBst Feb 9, 2025 •

edited

Loading

ibraheemdev Feb 11, 2025 •

edited

Loading

hkBst Feb 11, 2025

ibraheemdev Feb 8, 2025

hkBst Feb 9, 2025

hkBst commented Feb 9, 2025

hkBst commented Feb 9, 2025

ibraheemdev Feb 11, 2025

hkBst Feb 11, 2025

ibraheemdev Feb 11, 2025

ibraheemdev Feb 11, 2025

hkBst Feb 11, 2025

hkBst commented Feb 11, 2025

ibraheemdev Feb 11, 2025 •

edited

Loading

hkBst Feb 11, 2025

ibraheemdev Feb 11, 2025 •

edited

Loading

ibraheemdev Feb 11, 2025

hkBst Feb 11, 2025

ibraheemdev Feb 11, 2025

hkBst Feb 11, 2025

hkBst commented Feb 11, 2025

hkBst commented Feb 11, 2025

	/// Implements Iterator for mutable references to Iterator, such as those produced by [Iterator::by_ref]
	/// Implements `Iterator` for mutable references to iterators, such as those produced by [`Iterator::by_ref`].

	/// Iterator adapter that turns ownership-taking methods into mutating methods.
	/// Creates a "by reference" adapter for this instance of `Iterator`.

Clarify iterator by_ref docs #135987

Are you sure you want to change the base?

Clarify iterator by_ref docs #135987

Conversation

hkBst commented Jan 24, 2025

rustbot commented Jan 24, 2025

Choose a reason for hiding this comment

hkBst Feb 9, 2025 • edited Loading

Choose a reason for hiding this comment

ibraheemdev Feb 11, 2025 • edited Loading

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

hkBst commented Feb 9, 2025

hkBst commented Feb 9, 2025

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

hkBst commented Feb 11, 2025

ibraheemdev Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibraheemdev Feb 11, 2025 • edited Loading

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

hkBst commented Feb 11, 2025

hkBst commented Feb 11, 2025

hkBst Feb 9, 2025 •

edited

Loading

ibraheemdev Feb 11, 2025 •

edited

Loading

ibraheemdev Feb 11, 2025 •

edited

Loading

ibraheemdev Feb 11, 2025 •

edited

Loading