Dhghomon / rust-fsharp

Rust - F# - Rust reference
MIT License
239 stars 14 forks source link

Never implement `Iterator` for a collection! #24

Closed ChayimFriedman2 closed 3 years ago

ChayimFriedman2 commented 3 years ago

Implement IntoIterator instead. This is what allows for loop, and should be implemented for iterables (As opposed to Iterator which should be implemented for iterators).

Here is a rewrite of your example, including non-consuming iterators:

struct Metropolis {
    cities: Vec<String>,
    population: u32
}

impl IntoIterator for Metropolis {
    type Item = String;
    type IntoIter = IntoIter;
    fn into_iter(self) -> Self::IntoIter {
        IntoIter(self)
    }
}

pub struct IntoIter(Metropolis);
impl Iterator for IntoIter {
    type Item = String;
    fn next(&mut self) -> Option<Self::Item> {
        if !self.0.cities.is_empty() {
            Some(self.0.cities.remove(0))
        } else {
            None
        }
    }
}
Dhghomon commented 3 years ago

Yeah...the thing is though, this book is intended to be a simple easing into one language for users of the other (we're not trying to scare the Fsharpers away). Examples should be simple like the one in here:

https://doc.rust-lang.org/book/ch13-02-iterators.html

impl Iterator for Counter {
    type Item = u32;

    fn next(&mut self) -> Option<Self::Item> {
        if self.count < 5 {
            self.count += 1;
            Some(self.count)
        } else {
            None
        }
    }
}

I don't think we can expect someone new to the language to implement IntoIterator with two associated types, followed by yet another implementation.

The same goes with the "".to_string() example elsewhere which is maybe not idiomatic but keep in mind that the reader has probably only just learned what the difference between a &str and a String is. Following that up with String::new() String::default() (and .into() or .to_owned() no less depending on the context!) is certainly frightening.

Although for that .to_string() example I think I can just rewrite it to avoid the "".to_string().

Happypig375 commented 3 years ago

In .NET land there are also System.Collections.Generic.IEnumerable<'T> (seq<'T>) and System.Collections.Generic.IEnumerator<'T> which seem to parallel this.

Happypig375 commented 3 years ago

@ChayimFriedman2 Does this look good?

type Metropolis = {
    cities: string ResizeArray
    population: uint
} with
    interface seq<string> with
        member this.GetEnumerator() = new Iterator(this) :> _
    interface System.Collections.IEnumerable with
        member this.GetEnumerator() = new Iterator(this) :> _
and Iterator(metropolis) =
    let mutable current = Unchecked.defaultof<_>
    interface System.Collections.Generic.IEnumerator<string> with
        member _.Current = current
        member _.Dispose() = failwith "No equivalent"
    interface System.Collections.IEnumerator with
        member this.MoveNext() =
            if metropolis.cities.Count > 0 then
                current <- metropolis.cities.[0]
                metropolis.cities.RemoveAt 0
                true
            else false
        member _.Reset() = failwith "No equivalent"
        member _.Current = current :> _