Bug: Shakmaty produces incorrect position and generates misleading list of moves in it

[kirillbobyrev]

Hi, thanks for an amazing crate! I'm using it to validate my own clunky move generator and fuzz both & compare the results using shakmaty as the reference. Unfortunately, I've run into a problem early on: there is a position which both my generator (does not reliably check whether the position is truly valid or not) and shakmaty accept but shakmaty produces an incorrect list of moves (if we take the en-passant square out of the equation). Because the position is incorrect, I guess the real problem is both programs accepting the position rather than producing different move lists. Here's the reproducer:

use shakmaty::{CastlingMode, Chess, Position};

fn main() {
    let shakmaty_setup: shakmaty::fen::Fen =
        "rnbqk1nr/bb3p1p/1q2r3/2pPp3/3P4/7P/1PP1NpPP/R1BQKBNR w KQkq c6"
            .parse()
            .expect("The FEN is fine but the position is not (en passant with a check delivered by a piece other than a pawn).");
    let shakmaty_position: Chess = shakmaty_setup
        .position(CastlingMode::Standard)
        .expect("Oh-oh, this is not a legal position!");
    println!("{:?}", shakmaty_position.legal_moves());
}

Gives:

[EnPassant { from: D5, to: C6 }, Normal { role: King, from: E1, capture: None, to: D2, promotion: None }, Normal { role: King, from: E1, capture: Some(Pawn), to: F2, promotion: None }]

The problematic move is EnPassant { from: D5, to: C6 } because it doesn't actually resolve the check given by an F2 pawn! I guess, generating moves in invalid position is undefined/should not be reliable.

Here's the position in the editor for esier inspection: https://lichess.org/editor/rnbqk1nr/bb3p1p/1q2r3/2pPp3/3P4/7P/1PP1NpPP/R1BQKBNR_w_KQkq_c6_0_1

[kirillbobyrev]

I'm not very familiar with the code itself but it seems that this is the problematic snippet: https://docs.rs/shakmaty/latest/src/shakmaty/position.rs.html#2596-2600

If our king is in check and there is en-passant square and there's a check, the pawn that was pushed last turn should be the only checker.

[niklasf]

Took a couple of attempts, but I think I got it now. Thanks for reporting!

For validation, either the pushed pawn is the only checker, or moving the pawn back to its original position would block check.

For move generation, in case validation is deliberately ignored, do not assume that the king is safe from knights and unrelated pawns.

[kirillbobyrev]

Oh, right, I implemented the "pushed pawn is the only checker" but forgot that it could be a discovery 😞 Thanks a lot, that is very helpful!

[niklasf]

Published in v0.20.7.