From fb6fd27453ef027de963516b2d92dd07fdedb376 Mon Sep 17 00:00:00 2001
From: Eryn Wells <eryn@erynwells.me>
Date: Mon, 1 Jan 2024 09:33:33 -0800
Subject: [PATCH] [board] Remove Iterator implementations form move generator
 types; replace it with an iter() method

I was given some guidance[1] on the Rust developer forums that my approach of
implementing Iterator on these types wasn't quite right.

> In general, iterators are distinct data structures than the primary struct
that owns the data, in part due to issues like this, but also due to concerns
around iterator invalidation and the need to carry around iteration state more
generally.

I took this to heart and replace the Iterator implementations with an iter()
method that returns an `impl Iterator<Item=Move>`. This works so much better and
lets me delete a bunch of code!

Additionally, the iter() methods on the piece-wise move generator types return
`impl Iterator<Item=&Move>`, and then the top-level Moves::iter() returns a
.cloned().

Overall I'm really happy with this!

[1]: https://users.rust-lang.org/t/tricky-lifetime-may-not-live-long-enough-error-on-an-iterator-wrapping-a-boxed-iterator/104650/3
---
 board/src/moves/king.rs           | 16 +++++------
 board/src/moves/move_generator.rs | 44 +++++++------------------------
 board/src/moves/pawn.rs           |  6 ++++-
 3 files changed, 23 insertions(+), 43 deletions(-)

diff --git a/board/src/moves/king.rs b/board/src/moves/king.rs
index 0884a86..9abca90 100644
--- a/board/src/moves/king.rs
+++ b/board/src/moves/king.rs
@@ -40,16 +40,16 @@ impl<'a> KingMoveGenerator<'a> {
         generator
     }
 
-    fn iter(&self) -> impl Iterator<Item = Move> + '_ {
-        self.move_lists.iter().flatten().cloned()
+    pub(super) fn iter(&self) -> impl Iterator<Item = &Move> + '_ {
+        self.move_lists.iter().flatten()
     }
 
     fn bitboard(&self) -> BitBoard {
         self.quiet_moves_bitboard() | self.capture_moves_bitboard()
     }
 
-    fn quiet_moves_iter(&self) -> impl Iterator<Item = Move> + '_ {
-        self.pushes_move_list().iter().cloned()
+    fn quiet_moves_iter(&self) -> impl Iterator<Item = &Move> + '_ {
+        self.pushes_move_list().iter()
     }
 
     fn quiet_moves_bitboard(&self) -> BitBoard {
@@ -60,8 +60,8 @@ impl<'a> KingMoveGenerator<'a> {
         self.captures
     }
 
-    fn capture_moves_iter(&self) -> impl Iterator<Item = Move> + '_ {
-        self.captures_move_list().iter().cloned()
+    fn capture_moves_iter(&self) -> impl Iterator<Item = &Move> + '_ {
+        self.captures_move_list().iter()
     }
 
     fn generate_moves(&mut self) {
@@ -191,7 +191,7 @@ mod tests {
             Move::new(Piece::king(Color::White), Square::e4(), Square::d4()),
         ];
 
-        let mut generated_moves: HashSet<Move> = generator.iter().collect();
+        let mut generated_moves: HashSet<Move> = generator.iter().cloned().collect();
 
         for ex_move in expected_moves {
             assert!(
@@ -230,7 +230,7 @@ mod tests {
             Move::new(Piece::king(Color::White), Square::a1(), Square::b2()),
         ];
 
-        let mut generated_moves: HashSet<Move> = generator.iter().collect();
+        let mut generated_moves: HashSet<Move> = generator.iter().cloned().collect();
 
         for ex_move in expected_moves {
             assert!(
diff --git a/board/src/moves/move_generator.rs b/board/src/moves/move_generator.rs
index e93d46b..e4c00d8 100644
--- a/board/src/moves/move_generator.rs
+++ b/board/src/moves/move_generator.rs
@@ -1,50 +1,26 @@
 // Eryn Wells <eryn@erynwells.me>
 
-use super::king::KingMoveGenerator;
-use super::pawn::PawnMoveGenerator;
-use super::Move;
+use super::{king::KingMoveGenerator, pawn::PawnMoveGenerator, Move};
 use crate::piece::Color;
 use crate::Position;
 
 pub struct Moves<'pos> {
-    position: &'pos Position,
-    color: Color,
-    iterators: Vec<Box<dyn Iterator<Item = Move> + 'pos>>,
-    index: usize,
+    pawn_moves: PawnMoveGenerator<'pos>,
+    king_moves: KingMoveGenerator<'pos>,
 }
 
 impl<'a> Moves<'a> {
     pub fn new(position: &Position, color: Color) -> Moves {
         Moves {
-            position,
-            color,
-            iterators: vec![
-                Box::new(PawnMoveGenerator::new(position, color)),
-                Box::new(KingMoveGenerator::new(position, color)),
-            ],
-            index: 0,
+            pawn_moves: PawnMoveGenerator::new(position, color),
+            king_moves: KingMoveGenerator::new(position, color),
         }
     }
 
-    fn current_iterator(&mut self) -> &mut Box<dyn Iterator<Item = Move> + 'a> {
-        &mut self.iterators[self.index]
-    }
-}
-
-impl<'a> std::iter::Iterator for Moves<'a> {
-    type Item = Move;
-
-    fn next(&mut self) -> Option<Self::Item> {
-        if self.index >= self.iterators.len() {
-            return None;
-        }
-
-        let next_move = self.current_iterator().next();
-
-        if next_move.is_none() && self.index < self.iterators.len() {
-            self.index += 1;
-        }
-
-        next_move
+    fn iter(&self) -> impl Iterator<Item = Move> + '_ {
+        self.pawn_moves
+            .iter()
+            .chain(self.king_moves.iter())
+            .cloned()
     }
 }
diff --git a/board/src/moves/pawn.rs b/board/src/moves/pawn.rs
index a452c83..13650d8 100644
--- a/board/src/moves/pawn.rs
+++ b/board/src/moves/pawn.rs
@@ -49,7 +49,11 @@ impl<'pos> PawnMoveGenerator<'pos> {
         }
     }
 
-    pub(super) fn generate_moves(&mut self) {
+    pub(super) fn iter(&self) -> impl Iterator<Item = &Move> + '_ {
+        self.move_lists.iter().flatten()
+    }
+
+    fn generate_moves(&mut self) {
         self.generate_move_bitboards();
         self.populate_move_lists();
     }