Knight double disambiguation bug

[maxbergmark]

Hi,

When parsing games on lichess, I stumbled upon this game, which almost has a double disambiguation checkmate. However, the only reason that I was able to find this game was because the move was incorrectly labelled as a double disambiguation.

In this position, the move N5xf6# is causing the problem. Since all three knights are able to move to f6, two disambiguation checks are being done. The first check disambiguates with the g4 knight, and identifies that the file should be disambiguated. Then, the check is done for the d7 knight, which identifies that the rank needs to be disambiguated. However, it would be sufficient to only disambiguate the rank in this case.

I was also able to find this game, where the same problem exists.

Minimal example to reproduce

fn main() -> Result<(), Box<dyn std::error::Error>>{
    let fen: Fen = "8/2KN1p2/5p2/3N1B1k/5PNp/7P/7P/8 w - - ".parse()?;
    let position: Chess = fen.into_position(CastlingMode::Standard)?;
    let correct_san = "N5xf6#";

    let san = San::from_str(&correct_san)?;
    let m = san.to_move(&position)?;
    let san_plus = SanPlus::from_move(position, &m);

    // this fails, the actual value is "Nd5xf6#"
    assert_eq!(san_plus.to_string(), correct_san);
    Ok(())
}

The bug

It seems that the bug arises from the disambiguation logic in san.rs. Since the folding considers each disambiguation separately, it won't be able to find the correct disambiguation.

let (rank, file) = moves
    .iter()
    .filter(|c| match *c {
        Move::Normal {
            role: r,
            to: t,
            promotion: p,
            ..
        } => role == *r && to == *t && promotion == *p,
        _ => false,
    })
    .fold((false, false), |(rank, file), c| match *c {
        Move::Normal {
            from: candidate, ..
        } => {
            if from == candidate {
                (rank, file)
            } else if from.rank() == candidate.rank()
                || from.file() != candidate.file()
            {
                (rank, true)
            } else {
                (true, file)
            }
        }
        _ => (rank, file),
    });

[zanciks]

Would it be possible to use something like shakmaty::attacks::attacks here? With some bitboard manipulation, we should be able to get this somewhat easily. We take a bitboard of all pieces of the same role/color which attack the square. Remove the piece that's actually moving. BitAnd the bitboard with the rank/file of the moving piece and match it. Psuedo-rust below

// bitboard is a bitboard of all pieces of same role/color attacking the target square
// see functions like shakmaty::attacks::attacks and possibly shakmaty::board::Board::attacks_to
if bitboard.remove(moving_piece).any() {
    match ((bitboard.remove(moving_piece) & moving_piece_file_bitboard).any(), (bitboard.remove(moving_piece) & 
    moving_piece_rank_bitboard).any()) {
        (true, false) => {// this means including only the rank is sufficient},
        (false, true) => {// this means including only the file is sufficient},
        (true, true) => {// this means to include both rank and file},
        (false, false) => {// this means that either rank OR file is sufficient, so default to file (?)}
    }
}

If this makes sense, I can try and whip up some actual code for it. I personally things it's a bit more digestible, and as far as I can tell, covers all edge cases? However, if this seems unnecessary, I'll play around with the existing code :P

[maxbergmark]

@zanciks That looks promising! It goes a bit over my head with the bitboard representations, but conceptually it sounds as if it should work. Nice find!

[niklasf]

Thanks for reporting, good find! Published a bugfix release v0.27.1.