Merge pull request #2 from moritzmyrz/feat/perf-reliability-cli-improvements

perf: movegen, replay, stats; validate --verbose

Author: moritzmyrz

README.md

@@ -76,6 +76,9 @@ This project is **not** a top-tier competitive chess engine and does not current
 # Validate whether games can be reconstructed legally
 ply validate games.pgn
 
+# Same, but print a line per failed game (replay error)
+ply validate games.pgn --verbose
+
 # Print one-line summaries per game
 ply summarize games.pgn
 
@@ -180,8 +183,8 @@ The repository is organized as a library-first core with a small command-line fr
   - FEN parser/serializer and validation errors
 - `src/movegen.rs`
   - pseudo-legal and legal move generation, attack detection, and state transitions
-- `src/pgn.rs`
-  - PGN parsing, SAN normalization/resolution, and game reconstruction pipeline
+- `src/pgn/`
+  - PGN parsing (`parse.rs`), SAN resolution (`san.rs`), and replay (`replay.rs`)
 - `src/stats.rs`
   - summary and aggregate metrics over reconstructed games
 - `src/export.rs`

src/cli/commands.rs

@@ -14,6 +14,9 @@ pub struct Cli {
 pub enum Commands {
     Validate {
         file: PathBuf,
+        /// Print per-game replay errors for invalid games
+        #[arg(short, long)]
+        verbose: bool,
     },
     Summarize {
         file: PathBuf,

src/cli/mod.rs

@@ -8,14 +8,14 @@ use std::time::Instant;
 use ply::fen::{parse_fen, to_fen, FenError, STARTPOS_FEN};
 use ply::movegen::generate_legal_moves;
 use ply::perft::{perft, perft_divide};
-use ply::pgn::{parse_pgn_reader, reconstruct_game, PgnError, PgnReader};
-use ply::stats::{AggregateStatsAccumulator, summarize_game};
+use ply::pgn::{reconstruct_game, PgnError, PgnReader};
+use ply::stats::{summarize_game, AggregateStatsAccumulator};
 use thiserror::Error;
 
 use self::commands::{Cli, Commands};
 use self::render::{
     render_fen, render_perft, render_stats, render_summaries, render_validate, FenOutput,
-    PerftOutput, StatsOutput, SummariesOutput, SummaryEntry, ValidateOutput,
+    PerftOutput, StatsOutput, SummariesOutput, SummaryEntry, ValidateFailure, ValidateOutput,
 };
 
 #[derive(Debug, Error)]
@@ -32,64 +32,78 @@ pub enum CliError {
 
 pub fn run(cli: Cli) -> Result<(), CliError> {
     match cli.command {
-        Commands::Validate { file } => print!("{}", render_validate(&cmd_validate(&file)?)),
+        Commands::Validate { file, verbose } => {
+            print!("{}", render_validate(&cmd_validate(&file, verbose)?))
+        }
         Commands::Summarize { file } => print!("{}", render_summaries(&cmd_summarize(&file)?)),
         Commands::Stats { file, json } => print!("{}", render_stats(&cmd_stats(&file, json)?)?),
-        Commands::Fen { fen, legal_moves } => print!("{}", render_fen(&cmd_fen(&fen, legal_moves)?)),
+        Commands::Fen { fen, legal_moves } => {
+            print!("{}", render_fen(&cmd_fen(&fen, legal_moves)?))
+        }
         Commands::Perft { fen, depth, divide } => {
             print!("{}", render_perft(&cmd_perft(fen.as_deref(), depth, divide)?))
         }
     }
     Ok(())
 }
 
-fn cmd_validate(file: &std::path::Path) -> Result<ValidateOutput, CliError> {
+fn cmd_validate(file: &std::path::Path, verbose: bool) -> Result<ValidateOutput, CliError> {
     let reader = BufReader::new(File::open(file)?);
     let mut ok = 0usize;
     let mut failed = 0usize;
     let mut total = 0usize;
-    for game in PgnReader::new(reader) {
+    let mut failures = Vec::new();
+    for (idx, game) in PgnReader::new(reader).enumerate() {
         total += 1;
-        match reconstruct_game(&game?) {
+        let game = game?;
+        match reconstruct_game(&game) {
             Ok(_) => ok += 1,
-            Err(_) => failed += 1,
+            Err(err) => {
+                failed += 1;
+                if verbose {
+                    failures
+                        .push(ValidateFailure { game_index: idx + 1, message: err.to_string() });
+                }
+            }
         }
     }
-    Ok(ValidateOutput { validated_games: total, valid: ok, invalid: failed })
+    Ok(ValidateOutput { validated_games: total, valid: ok, invalid: failed, failures })
 }
 
 fn cmd_summarize(file: &std::path::Path) -> Result<SummariesOutput, CliError> {
     let reader = BufReader::new(File::open(file)?);
     let mut entries = Vec::new();
     for (idx, game) in PgnReader::new(reader).enumerate() {
         match reconstruct_game(&game?) {
-            Ok(record) => entries.push(SummaryEntry::Valid { index: idx + 1, summary: summarize_game(&record) }),
-            Err(err) => entries.push(SummaryEntry::Invalid { index: idx + 1, error: err.to_string() }),
+            Ok(record) => entries
+                .push(SummaryEntry::Valid { index: idx + 1, summary: summarize_game(&record) }),
+            Err(err) => {
+                entries.push(SummaryEntry::Invalid { index: idx + 1, error: err.to_string() })
+            }
         }
     }
     Ok(SummariesOutput { entries })
 }
 
 fn cmd_stats(file: &std::path::Path, json: bool) -> Result<StatsOutput, CliError> {
     let reader = BufReader::new(File::open(file)?);
-    if json {
-        let games = parse_pgn_reader(reader)?;
-        let records = games.iter().filter_map(|g| reconstruct_game(g).ok()).collect::<Vec<_>>();
-        let summaries = records.iter().map(summarize_game).collect::<Vec<_>>();
-        let mut acc = AggregateStatsAccumulator::default();
-        for record in &records {
-            acc.push_record(record);
-        }
-        return Ok(StatsOutput { json: true, stats: acc.finish(), summaries });
-    }
-
     let mut acc = AggregateStatsAccumulator::default();
+    let mut summaries = Vec::new();
+
     for game in PgnReader::new(reader) {
-        if let Ok(record) = reconstruct_game(&game?) {
-            acc.push_record(&record);
+        let game = game?;
+        if let Ok(record) = reconstruct_game(&game) {
+            if json {
+                let summary = summarize_game(&record);
+                acc.push_record_with_summary(&record, &summary);
+                summaries.push(summary);
+            } else {
+                acc.push_record(&record);
+            }
         }
     }
-    Ok(StatsOutput { json: false, stats: acc.finish(), summaries: Vec::new() })
+
+    Ok(StatsOutput { json, stats: acc.finish(), summaries })
 }
 
 fn cmd_fen(fen: &str, legal_moves: bool) -> Result<FenOutput, CliError> {

src/cli/render.rs

@@ -3,10 +3,17 @@ use ply::stats::{AggregateStats, GameSummary};
 
 use super::CliError;
 
+#[derive(Debug, Clone)]
+pub struct ValidateFailure {
+    pub game_index: usize,
+    pub message: String,
+}
+
 pub struct ValidateOutput {
     pub validated_games: usize,
     pub valid: usize,
     pub invalid: usize,
+    pub failures: Vec<ValidateFailure>,
 }
 
 pub enum SummaryEntry {
@@ -38,10 +45,17 @@ pub struct PerftOutput {
 }
 
 pub fn render_validate(output: &ValidateOutput) -> String {
-    format!(
+    let mut rendered = format!(
         "validated games: {}\nvalid: {}\ninvalid: {}\n",
         output.validated_games, output.valid, output.invalid
-    )
+    );
+    if !output.failures.is_empty() {
+        rendered.push_str("\nfailures:\n");
+        for f in &output.failures {
+            rendered.push_str(&format!("  game {}: {}\n", f.game_index, f.message));
+        }
+    }
+    rendered
 }
 
 pub fn render_summaries(output: &SummariesOutput) -> String {
@@ -132,21 +146,16 @@ pub fn render_stats(output: &StatsOutput) -> Result<String, CliError> {
         "  games_with_queenside_castle: {}\n",
         output.stats.games_with_queenside_castle
     ));
-    rendered.push_str(&format!(
-        "  games_with_no_castling: {}\n",
-        output.stats.games_with_no_castling
-    ));
+    rendered
+        .push_str(&format!("  games_with_no_castling: {}\n", output.stats.games_with_no_castling));
 
     rendered.push_str("move_events:\n");
     rendered.push_str(&format!("  total_captures: {}\n", output.stats.total_captures));
     rendered.push_str(&format!("  average_captures: {:.2}\n", output.stats.average_captures));
     rendered.push_str(&format!("  total_checks: {}\n", output.stats.total_checks));
     rendered.push_str(&format!("  average_checks: {:.2}\n", output.stats.average_checks));
     rendered.push_str(&format!("  total_promotions: {}\n", output.stats.total_promotions));
-    rendered.push_str(&format!(
-        "  average_promotions: {:.2}\n",
-        output.stats.average_promotions
-    ));
+    rendered.push_str(&format!("  average_promotions: {:.2}\n", output.stats.average_promotions));
     Ok(rendered)
 }
 

src/movegen.rs

@@ -1,4 +1,6 @@
-use crate::board::{CastleSide, CastlingRights, ChessMove, Color, Piece, PieceKind, Position, Square};
+use crate::board::{
+    CastleSide, CastlingRights, ChessMove, Color, Piece, PieceKind, Position, Square,
+};
 
 #[derive(Debug, Clone, Copy)]
 pub struct Undo {
@@ -23,17 +25,21 @@ pub fn generate_legal_moves_into(position: &Position, out: &mut Vec<ChessMove>)
     out.clear();
     let mut pseudo = Vec::with_capacity(64);
     generate_pseudo_legal_moves_into(position, &mut pseudo);
+    let mut scratch = position.clone();
+    let side = position.side_to_move;
     for mv in pseudo {
-        let mut next = position.clone();
-        apply_move(&mut next, mv);
-        if !is_in_check(&next, position.side_to_move) {
+        let undo = apply_move_with_undo(&mut scratch, mv);
+        if !is_in_check(&scratch, side) {
             out.push(mv);
         }
+        undo_move(&mut scratch, mv, undo);
     }
 }
 
 pub fn is_in_check(position: &Position, color: Color) -> bool {
-    let king_sq = position.king_square(color).expect("valid positions must track kings");
+    let Some(king_sq) = position.king_square(color) else {
+        return false;
+    };
     is_square_attacked(position, king_sq, color.opposite())
 }
 
@@ -350,8 +356,12 @@ fn push_castling_moves(position: &Position, from: Square, piece: Piece, out: &mu
         return;
     }
     let (rank, king_side_ok, queen_side_ok) = match piece.color {
-        Color::White => (0u8, position.castling.white_king_side, position.castling.white_queen_side),
-        Color::Black => (7u8, position.castling.black_king_side, position.castling.black_queen_side),
+        Color::White => {
+            (0u8, position.castling.white_king_side, position.castling.white_queen_side)
+        }
+        Color::Black => {
+            (7u8, position.castling.black_king_side, position.castling.black_queen_side)
+        }
     };
 
     if from != Square::from_coords(4, rank).expect("e-file") {

src/opening.rs

@@ -1,4 +1,4 @@
-use crate::board::ChessMove;
+use crate::board::{ChessMove, PieceKind};
 
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct OpeningInfo {
@@ -18,84 +18,77 @@ const OPENINGS: &[OpeningLine] = &[
         line: &["e2e4", "e7e5", "g1f3", "b8c6", "f1b5"],
     },
     OpeningLine {
-        info: OpeningInfo {
-            eco: "C50",
-            opening: "Italian Game",
-            variation: None,
-        },
+        info: OpeningInfo { eco: "C50", opening: "Italian Game", variation: None },
         line: &["e2e4", "e7e5", "g1f3", "b8c6", "f1c4"],
     },
     OpeningLine {
-        info: OpeningInfo {
-            eco: "B20",
-            opening: "Sicilian Defense",
-            variation: None,
-        },
+        info: OpeningInfo { eco: "B20", opening: "Sicilian Defense", variation: None },
         line: &["e2e4", "c7c5"],
     },
     OpeningLine {
-        info: OpeningInfo {
-            eco: "C00",
-            opening: "French Defense",
-            variation: None,
-        },
+        info: OpeningInfo { eco: "C00", opening: "French Defense", variation: None },
         line: &["e2e4", "e7e6"],
     },
     OpeningLine {
-        info: OpeningInfo {
-            eco: "B10",
-            opening: "Caro-Kann Defense",
-            variation: None,
-        },
+        info: OpeningInfo { eco: "B10", opening: "Caro-Kann Defense", variation: None },
         line: &["e2e4", "c7c6"],
     },
     OpeningLine {
-        info: OpeningInfo {
-            eco: "D06",
-            opening: "Queen's Gambit",
-            variation: None,
-        },
+        info: OpeningInfo { eco: "D06", opening: "Queen's Gambit", variation: None },
         line: &["d2d4", "d7d5", "c2c4"],
     },
     OpeningLine {
-        info: OpeningInfo {
-            eco: "D20",
-            opening: "Queen's Gambit Accepted",
-            variation: None,
-        },
+        info: OpeningInfo { eco: "D20", opening: "Queen's Gambit Accepted", variation: None },
         line: &["d2d4", "d7d5", "c2c4", "d5c4"],
     },
     OpeningLine {
-        info: OpeningInfo {
-            eco: "D30",
-            opening: "Queen's Gambit Declined",
-            variation: None,
-        },
+        info: OpeningInfo { eco: "D30", opening: "Queen's Gambit Declined", variation: None },
         line: &["d2d4", "d7d5", "c2c4", "e7e6"],
     },
     OpeningLine {
         info: OpeningInfo { eco: "D10", opening: "Slav Defense", variation: None },
         line: &["d2d4", "d7d5", "c2c4", "c7c6"],
     },
     OpeningLine {
-        info: OpeningInfo {
-            eco: "A00",
-            opening: "Van't Kruijs Opening",
-            variation: None,
-        },
+        info: OpeningInfo { eco: "A00", opening: "Van't Kruijs Opening", variation: None },
         line: &["e2e3"],
     },
 ];
 
 pub fn classify_opening(plies: &[ChessMove]) -> Option<OpeningInfo> {
-    let coordinates = plies.iter().map(|mv| mv.to_coordinate()).collect::<Vec<_>>();
     OPENINGS
         .iter()
-        .filter(|opening| has_prefix(&coordinates, opening.line))
+        .filter(|opening| has_uci_prefix(plies, opening.line))
         .max_by_key(|opening| opening.line.len())
         .map(|opening| opening.info.clone())
 }
 
-fn has_prefix(moves: &[String], prefix: &[&str]) -> bool {
-    moves.len() >= prefix.len() && moves.iter().zip(prefix.iter()).all(|(mv, expected)| mv == expected)
+/// Compare against UCI-like strings (`e2e4`, optional promotion suffix).
+fn move_matches_uci(mv: ChessMove, uci: &str) -> bool {
+    let mut buf = [0u8; 6];
+    let n = write_move_uci(mv, &mut buf);
+    uci.len() == n && uci.as_bytes() == &buf[..n]
+}
+
+fn write_move_uci(mv: ChessMove, out: &mut [u8; 6]) -> usize {
+    out[0] = b'a' + mv.from.file();
+    out[1] = b'1' + mv.from.rank();
+    out[2] = b'a' + mv.to.file();
+    out[3] = b'1' + mv.to.rank();
+    if let Some(p) = mv.promotion {
+        out[4] = match p {
+            PieceKind::Knight => b'n',
+            PieceKind::Bishop => b'b',
+            PieceKind::Rook => b'r',
+            PieceKind::Queen => b'q',
+            PieceKind::Pawn | PieceKind::King => b'q',
+        };
+        return 5;
+    }
+    4
+}
+
+fn has_uci_prefix(plies: &[ChessMove], prefix: &[&str]) -> bool {
+    plies.len() >= prefix.len()
+        && plies.iter().zip(prefix.iter()).all(|(mv, expected)| move_matches_uci(*mv, expected))
 }