niklasf / chessops

Chess and chess variant rules and operations in TypeScript
https://niklasf.github.io/chessops/
GNU General Public License v3.0
118 stars 34 forks source link

FEN parsing and validation: regression or refactoring? #151

Closed acherm closed 1 year ago

acherm commented 1 year ago

Hi,

I'm using chessops and would like to share my experience, with the hope of better understanding this awesome library, but also to report on potential issues.

Let us consider this program:

import { parseFen } from 'chessops/fen';
import { Chess } from 'chessops/chess';

const setup = parseFen('1b1B4/1P6/2B5/3r2kN/2r4P/8/5K2/3B4 b').unwrap();
const pos = Chess.fromSetup(setup).unwrap();
console.log(pos);

The position is "illegal": https://lichess.org/analysis/standard/1b1B4/1P6/2B5/3r2kN/2r4P/8/5K2/3B4_b since there are two checks, coming from a pawn and a bishop. It's simply impossible in practice.

However, when you execute this piece of code with chessops >= 0.12.0 eg "chessops": "^0.12.0" It's fine!

When you execute this code with chessops < 0.12.0 like "chessops": "^0.11.0", there is an error

You can play with code/versions here: https://replit.com/@acherm/FENChess

/home/runner/FENChess/node_modules/chessops/src/chess.ts:496
            return Result.err(new PositionError(IllegalSetup.ImpossibleCheck));
                              ^
PositionError: ERR_IMPOSSIBLE_CHECK
    at Chess.validateCheckers (/home/runner/FENChess/node_modules/chessops/src/chess.ts:496:31)
    at Chess.validate (/home/runner/FENChess/node_modules/chessops/src/chess.ts:470:70)
    at Function.fromSetup (/home/runner/FENChess/node_modules/chessops/src/chess.ts:449:16)
    at <anonymous> (/home/runner/FENChess/index.ts:5:19)
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:530:24)
    at async loadESM (node:internal/process/esm_loader:91:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)

Node.js v18.12.1

I come to this conclusion through tries and errors, but also looking at @lichess-org and specifically at https://github.com/lichess-org/lila/blob/master/ui/ceval/src/ctrl.ts#L64-L70 https://github.com/lichess-org/lila/blob/master/ui/ceval/src/view.ts#L29 that somehow explains why there is the message "Engine cannot analyze this position" when loading the FEN.

I also looked at the history of chessops and closed to 0.12.0 there is a huge refactoring that makes disappear the use of validateCheckers https://github.com/niklasf/chessops/commit/77b905f95bd8434dd56f735c2db96651970698c6

It has been moved in isImpossibleCheck but this function is never called... except in test cases!

I have spent a lot of time getting the source of error and my conclusion is that you should do something like:

const fenPosCase = parseFen("1b1B4/1P6/2B5/3r2kN/2r4P/8/5K2/3B4 b").chain(setup => setupPosition("chess", setup));
if (fenPosCase.isErr) {
    console.log("FEN position is non valid....");    
}
else {
    console.log("FEN position is valid");
    if (isImpossibleCheck(fenPosCase.unwrap())) {
        console.log("but impossible check!");
    }    
}

However, I highly suggest it should be documented somewhere. An open and interested question (not a critic!) is to understand the rationale:

Another related point: I don't understand why lichess.org does get it's an illegal position, despite using 0.12.7 https://github.com/lichess-org/lila/blob/master/ui/ceval/package.json#L19C1-L19C27 that is not supposed to catch the issue.
Or did I miss something? Perhaps the deployed version of lichess.org is actually using an older version of chessops, cc @ornicar

Final point: I also come across this nice project https://github.com/tromp/ChessPositionRanking/blob/main/src/Legality.hs that apparently checks doubles checks https://github.com/tromp/ChessPositionRanking/blob/main/src/Legality.hs#L88-L90 and that found numerous illegal FEN positions

niklasf commented 1 year ago

Sorry. I messed up when publishing v0.12.8, which includes more changes than intended, including some which were not ready. v0.12.7 does not have this issue, which is why Lichess is working as expected.

Some changes to validation are planned, but work in progress, and will have a proper changelog.

acherm commented 1 year ago

OK thanks a lot! I've tested again with v0.12.7, and you're right, I mess up with ~ in package.json during my tests. Only v0.12.8 is impacted.

niklasf commented 1 year ago

Marked v0.12.8 as deprecated. I hope that's less disruptive than unpublishing.

niklasf commented 11 months ago

v0.13.0 now has these breaking changes: https://github.com/niklasf/chessops/blob/master/CHANGELOG.md#v0130