Match moves from MoveList instead of constructing them directly

In the future, we might want to adapt Move to contain more information
about the move in question, such as whether it's a capture or not.

If we do this, places where we currently have all the information
necessary to construct a move (e.g. when we get move information via
UCI) will no longer be able to easily construct Move directly.

In order to avoid that trouble, a better approach is to run movegen and
pick the matching Move object out, ensuring it has all the correct
metadata with essentially zero extra effort.

This did well enough in SPRT (although it did not complete) to prove
there's no critical bugs:

Elo   | 3.56 +- 12.59 (95%)
SPRT  | 8.0+0.08s Threads=1 Hash=16MB
LLR   | 0.42 (-2.94, 2.94) [-5.00, 0.00]
Games | N: 1270 W: 370 L: 357 D: 543
Penta | [36, 130, 285, 153, 31]

CHANGELOG.md

@@ -10,6 +10,7 @@
 ### Misc
 
 * Allow sending only 'go wtime' or 'go btime'
+* Avoid constructing `Move` objects directly where possible, preferring to extract them from `MoveList`
 
 ## [4.0]
 

src/chess/movegen/gen.rs

@@ -527,25 +527,23 @@ mod tests {
     use crate::chess::square::squares::all::*;
 
     #[inline(always)]
-    fn should_allow_move(fen: &str, mv: impl Into<Move>) {
+    fn should_allow_move(fen: &str, mv: (Square, Square)) {
         crate::init();
         let game = Game::from_fen(fen).unwrap();
         let mut movelist = MoveList::new();
         generate_legal_moves(&game, &mut movelist);
-        let mv = mv.into();
 
-        assert!(movelist.to_vec().iter().any(|m| *m == mv));
+        assert!(movelist.to_vec().iter().any(|m| (m.src, m.dst) == mv));
     }
 
     #[inline(always)]
-    fn should_not_allow_move(fen: &str, mv: impl Into<Move>) {
+    fn should_not_allow_move(fen: &str, mv: (Square, Square)) {
         crate::init();
         let game = Game::from_fen(fen).unwrap();
         let mut movelist = MoveList::new();
         generate_legal_moves(&game, &mut movelist);
-        let mv = mv.into();
 
-        assert!(movelist.to_vec().iter().all(|m| *m != mv));
+        assert!(movelist.to_vec().iter().all(|m| (m.src, m.dst) != mv));
     }
 
     #[test]

src/chess/moves.rs

@@ -5,6 +5,34 @@ const MAX_LEGAL_MOVES: usize = 218;
 
 pub type MoveList = ArrayVec<Move, MAX_LEGAL_MOVES>;
 
+pub trait MoveListExt {
+    fn expect_matching(
+        &self,
+        src: Square,
+        dst: Square,
+        promotion: Option<PromotionPieceKind>,
+    ) -> Move;
+}
+
+impl MoveListExt for MoveList {
+    fn expect_matching(
+        &self,
+        src: Square,
+        dst: Square,
+        promotion: Option<PromotionPieceKind>,
+    ) -> Move {
+        for i in 0..self.len() {
+            let mv = *self.get(i).unwrap();
+
+            if mv.src == src && mv.dst == dst && mv.promotion == promotion {
+                return mv;
+            }
+        }
+
+        panic!("Illegal move")
+    }
+}
+
 #[derive(PartialEq, Eq, Clone, Copy)]
 pub struct Move {
     pub src: Square,

src/chess/san/san_parser.rs

@@ -1,5 +1,5 @@
 use crate::chess::game::Game;
-use crate::chess::moves::Move;
+use crate::chess::moves::{Move, MoveListExt};
 use crate::chess::piece::{PieceKind, PromotionPieceKind};
 use crate::chess::san;
 use crate::chess::square::{squares, File, Rank, Square};
@@ -207,16 +207,18 @@ fn parse_squares(game: &Game, mv: &str) -> Result<(Square, Square), ParseError>
 
 pub fn parse_move(game: &Game, mv: &str) -> Result<Move, ParseError> {
     if mv == san::KINGSIDE_CASTLE {
-        return Ok(Move::new(
+        return Ok(game.moves().expect_matching(
             squares::king_start(game.player),
             squares::kingside_castle_dest(game.player),
+            None,
         ));
     }
 
     if mv == san::QUEENSIDE_CASTLE {
-        return Ok(Move::new(
+        return Ok(game.moves().expect_matching(
             squares::king_start(game.player),
             squares::queenside_castle_dest(game.player),
+            None,
         ));
     }
 
@@ -236,10 +238,7 @@ pub fn parse_move(game: &Game, mv: &str) -> Result<Move, ParseError> {
 
     let (src, dst) = parse_squares(game, mv)?;
 
-    Ok(match promotion {
-        None => Move::new(src, dst),
-        Some(promoted_to) => Move::promotion(src, dst, promoted_to),
-    })
+    Ok(game.moves().expect_matching(src, dst, promotion))
 }
 
 #[cfg(test)]
@@ -250,18 +249,39 @@ mod tests {
     use crate::chess::piece::PromotionPieceKind;
     use crate::chess::square::squares::all::*;
 
-    fn test_parse_san(fen: &'static str, expected_mv: impl Into<Move>, san: &'static str) {
+    fn test_parse_san(fen: &'static str, expected_mv: (Square, Square), san: &'static str) {
         crate::init();
 
         let game = Game::from_fen(fen).unwrap();
         let mv = parse_move(&game, san).unwrap();
 
-        assert_eq!(mv, expected_mv.into());
+        let expected_mv = game
+            .moves()
+            .expect_matching(expected_mv.0, expected_mv.1, None);
+
+        assert_eq!(mv, expected_mv);
+    }
+
+    fn test_parse_san_with_promotion(
+        fen: &'static str,
+        expected_mv: (Square, Square, PromotionPieceKind),
+        san: &'static str,
+    ) {
+        crate::init();
+
+        let game = Game::from_fen(fen).unwrap();
+        let mv = parse_move(&game, san).unwrap();
+
+        let expected_mv =
+            game.moves()
+                .expect_matching(expected_mv.0, expected_mv.1, Some(expected_mv.2));
+
+        assert_eq!(mv, expected_mv);
     }
 
     #[test]
     fn san_simple_pawn_move() {
-        test_parse_san(fen::START_POS, Move::new(E2, E4), "e4");
+        test_parse_san(fen::START_POS, (E2, E4), "e4");
     }
 
     #[test]
@@ -291,10 +311,10 @@ mod tests {
     fn san_promotion() {
         let promotion_fen = "k7/7P/8/8/8/8/8/7K w - - 0 1";
 
-        test_parse_san(promotion_fen, (H7, H8, PromotionPieceKind::Knight), "h8=N");
-        test_parse_san(promotion_fen, (H7, H8, PromotionPieceKind::Bishop), "h8=B");
-        test_parse_san(promotion_fen, (H7, H8, PromotionPieceKind::Rook), "h8=R+");
-        test_parse_san(promotion_fen, (H7, H8, PromotionPieceKind::Queen), "h8=Q+");
+        test_parse_san_with_promotion(promotion_fen, (H7, H8, PromotionPieceKind::Knight), "h8=N");
+        test_parse_san_with_promotion(promotion_fen, (H7, H8, PromotionPieceKind::Bishop), "h8=B");
+        test_parse_san_with_promotion(promotion_fen, (H7, H8, PromotionPieceKind::Rook), "h8=R+");
+        test_parse_san_with_promotion(promotion_fen, (H7, H8, PromotionPieceKind::Queen), "h8=Q+");
     }
 
     #[test]
@@ -322,7 +342,7 @@ mod tests {
 
     #[test]
     fn san_plus_for_check() {
-        test_parse_san(
+        test_parse_san_with_promotion(
             "k7/6P1/8/8/8/8/8/K7 w - - 0 1",
             (G7, G8, PromotionPieceKind::Queen),
             "g8=Q+",

src/chess/san/san_writer.rs

@@ -143,41 +143,60 @@ mod tests {
     use super::*;
     use crate::chess::fen;
     use crate::chess::game::Game;
+    use crate::chess::moves::MoveListExt;
     use crate::chess::square::squares::all::*;
+    use crate::chess::square::Square;
 
-    fn test_san_string(fen: &'static str, mv: Move, expected_san: &'static str) {
+    fn test_san_string(fen: &'static str, mv: (Square, Square), expected_san: &'static str) {
         crate::init();
 
         let game = Game::from_fen(fen).unwrap();
+        let (src, dst) = mv;
+        let mv = game.moves().expect_matching(src, dst, None);
+        let san = format_move(&game, mv);
+
+        assert_eq!(&san, expected_san);
+    }
+
+    fn test_san_string_with_promotion(
+        fen: &'static str,
+        mv: (Square, Square, PromotionPieceKind),
+        expected_san: &'static str,
+    ) {
+        crate::init();
+
+        let game = Game::from_fen(fen).unwrap();
+        let (src, dst, promotion) = mv;
+        let mv = game.moves().expect_matching(src, dst, Some(promotion));
         let san = format_move(&game, mv);
 
         assert_eq!(&san, expected_san);
     }
 
     #[test]
     fn san_simple_pawn_move() {
-        test_san_string(fen::START_POS, Move::new(E2, E4), "e4");
+        test_san_string(fen::START_POS, (E2, E4), "e4");
     }
 
     #[test]
     fn san_simple_pawn_capture() {
         test_san_string(
             "rnbqkbnr/ppp1pppp/8/3p4/4P3/8/PPPP1PPP/RNBQKBNR w KQkq d6 0 2",
-            Move::new(E4, D5),
+            (E4, D5),
             "exd5",
         );
     }
 
     #[test]
     fn san_simple_knight_move() {
-        test_san_string(fen::START_POS, Move::new(B1, C3), "Nc3");
+        test_san_string(fen::START_POS, (B1, C3), "Nc3");
     }
 
     #[test]
     fn san_test_en_passant() {
         test_san_string(
             "rnbqkbnr/ppp2ppp/4p3/3pP3/8/8/PPPP1PPP/RNBQKBNR w KQkq d6 0 3",
-            Move::new(E5, D6),
+            (E5, D6),
             "exd6",
         );
     }
@@ -186,29 +205,10 @@ mod tests {
     fn san_promotion() {
         let promotion_fen = "k7/7P/8/8/8/8/8/7K w - - 0 1";
 
-        test_san_string(
-            promotion_fen,
-            Move::promotion(H7, H8, PromotionPieceKind::Knight),
-            "h8=N",
-        );
-
-        test_san_string(
-            promotion_fen,
-            Move::promotion(H7, H8, PromotionPieceKind::Bishop),
-            "h8=B",
-        );
-
-        test_san_string(
-            promotion_fen,
-            Move::promotion(H7, H8, PromotionPieceKind::Rook),
-            "h8=R+",
-        );
-
-        test_san_string(
-            promotion_fen,
-            Move::promotion(H7, H8, PromotionPieceKind::Queen),
-            "h8=Q+",
-        );
+        test_san_string_with_promotion(promotion_fen, (H7, H8, PromotionPieceKind::Knight), "h8=N");
+        test_san_string_with_promotion(promotion_fen, (H7, H8, PromotionPieceKind::Bishop), "h8=B");
+        test_san_string_with_promotion(promotion_fen, (H7, H8, PromotionPieceKind::Rook), "h8=R+");
+        test_san_string_with_promotion(promotion_fen, (H7, H8, PromotionPieceKind::Queen), "h8=Q+");
     }
 
     #[test]
@@ -217,10 +217,14 @@ mod tests {
 
         let fen = "R6R/8/8/8/8/8/8/1k4K1 w - - 0 1";
         let game = Game::from_fen(fen).unwrap();
-        let ambiguous_move = Move::new(A8, B8);
+        let ambiguous_move = (A8, B8);
 
         assert_eq!(
-            required_ambiguity_resolution(&game, ambiguous_move),
+            required_ambiguity_resolution(
+                &game,
+                game.moves()
+                    .expect_matching(ambiguous_move.0, ambiguous_move.1, None)
+            ),
             AmbiguityResolution::File
         );
 
@@ -233,10 +237,14 @@ mod tests {
 
         let fen = "R7/8/8/8/8/1k4K1/8/R7 w - - 0 1";
         let game = Game::from_fen(fen).unwrap();
-        let ambiguous_move = Move::new(A1, A3);
+        let ambiguous_move = (A1, A3);
 
         assert_eq!(
-            required_ambiguity_resolution(&game, ambiguous_move),
+            required_ambiguity_resolution(
+                &game,
+                game.moves()
+                    .expect_matching(ambiguous_move.0, ambiguous_move.1, None)
+            ),
             AmbiguityResolution::Rank
         );
 
@@ -249,10 +257,14 @@ mod tests {
 
         let fen = "1k1K4/8/8/8/4Q2Q/8/8/7Q w - - 0 1";
         let game = Game::from_fen(fen).unwrap();
-        let ambiguous_move = Move::new(H4, E1);
+        let ambiguous_move = (H4, E1);
 
         assert_eq!(
-            required_ambiguity_resolution(&game, ambiguous_move),
+            required_ambiguity_resolution(
+                &game,
+                game.moves()
+                    .expect_matching(ambiguous_move.0, ambiguous_move.1, None)
+            ),
             AmbiguityResolution::Exact
         );
 
@@ -263,15 +275,15 @@ mod tests {
     fn san_castling() {
         let fen = "1k6/8/8/8/8/8/8/R3K2R w KQ - 0 1";
 
-        test_san_string(fen, Move::new(E1, G1), "O-O");
-        test_san_string(fen, Move::new(E1, C1), "O-O-O");
+        test_san_string(fen, (E1, G1), "O-O");
+        test_san_string(fen, (E1, C1), "O-O-O");
     }
 
     #[test]
     fn san_plus_for_check() {
-        test_san_string(
+        test_san_string_with_promotion(
             "k7/6P1/8/8/8/8/8/K7 w - - 0 1",
-            Move::promotion(G7, G8, PromotionPieceKind::Queen),
+            (G7, G8, PromotionPieceKind::Queen),
             "g8=Q+",
         );
     }

src/engine/tablebases/mod.rs

@@ -1,5 +1,5 @@
 use crate::chess::game::Game;
-use crate::chess::moves::Move;
+use crate::chess::moves::{Move, MoveListExt};
 use crate::chess::piece::PromotionPieceKind;
 use crate::chess::square::Square;
 use std::path::Path;
@@ -77,7 +77,7 @@ impl Tablebase {
         let shakmaty_pos = Self::pos_to_shakmaty_pos(game);
 
         match self.shakmaty_tb.best_move(&shakmaty_pos) {
-            Ok(m) => m.map(|(shakmaty_mv, _)| Self::shakmaty_mv_to_move(&shakmaty_mv)),
+            Ok(m) => m.map(|(shakmaty_mv, _)| Self::shakmaty_mv_to_move(game, &shakmaty_mv)),
             Err(_) => None,
         }
     }
@@ -90,19 +90,19 @@ impl Tablebase {
             .unwrap()
     }
 
-    fn shakmaty_mv_to_move(shakmaty_mv: &shakmaty::Move) -> Move {
+    fn shakmaty_mv_to_move(game: &Game, shakmaty_mv: &shakmaty::Move) -> Move {
         use shakmaty::Role;
 
-        Move {
-            src: Square::from_index(shakmaty_mv.from().unwrap() as u8),
-            dst: Square::from_index(shakmaty_mv.to() as u8),
-            promotion: shakmaty_mv.promotion().map(|p| match p {
-                Role::Knight => PromotionPieceKind::Knight,
-                Role::Bishop => PromotionPieceKind::Bishop,
-                Role::Rook => PromotionPieceKind::Rook,
-                Role::Queen => PromotionPieceKind::Queen,
-                Role::King | Role::Pawn => unreachable!(),
-            }),
-        }
+        let src = Square::from_index(shakmaty_mv.from().unwrap() as u8);
+        let dst = Square::from_index(shakmaty_mv.to() as u8);
+        let promotion = shakmaty_mv.promotion().map(|p| match p {
+            Role::Knight => PromotionPieceKind::Knight,
+            Role::Bishop => PromotionPieceKind::Bishop,
+            Role::Rook => PromotionPieceKind::Rook,
+            Role::Queen => PromotionPieceKind::Queen,
+            Role::King | Role::Pawn => unreachable!(),
+        });
+
+        game.moves().expect_matching(src, dst, promotion)
     }
 }