Fix IndexError in read_game on an unmatched ) after an illegal move - #1204
Fix IndexError in read_game on an unmatched ) after an illegal move#1204eeshsaxena wants to merge 2 commits into
Conversation
read_game recovers from an illegal move by entering skip mode, and a later unmatched ')' closes that skip by calling end_variation(). When the error happened on the mainline there was no matching begin_variation(), so the unconditional variation_stack.pop() removed the root game node, leaving the stack empty and making the next visit_move() raise IndexError. Keep the root on the stack, mirroring the assert in begin_variation().
|
Hi! Gentle nudge on this one whenever you have some bandwidth. It's a small, self-contained fix ( |
|
Hi. Thanks for reporting and analysing the issue. I think the fix belongs into the |
Instead of guarding GameBuilder.end_variation against an empty stack, keep the parser from calling end_variation() for a ")" that never opened a variation. An illegal move sets skip_variation_depth via error recovery, not through begin_variation(); track that distinction with skip_variation_from_begin so the closing ")" only ends a variation that a SKIP begin_variation() opened or a real variation whose tail the error skipped. A top-level error's trailing ")" is now ignored, keeping begin/end_variation balanced for every visitor. Reverts the GameBuilder.end_variation guard and adds a bare-visitor test that begin/end_variation stay balanced on the reported input.
|
Thanks, that makes sense. Moved the fix into The root cause is that an illegal move triggers error recovery which sets Added a test with a bare |
2 participants
Came across this feeding some scraped PGNs through read_game: a short game like
raises
IndexError: list index out of rangefromGameBuilder.visit_moveinstead of being parsed with the bad move recorded ingame.errors.What happens:
Nf3is illegal for Black, so the parser callshandle_errorand setsskip_variation_depth = 1to skip to the end of the (assumed) variation. The following)closes that skip and callsend_variation(), but since the error was on the mainline there was never a matchingbegin_variation().end_variation()popsvariation_stackunconditionally, so it removes the root game node and empties the stack; the next move (e5) then doesself.variation_stack[-1]and raises IndexError.read_gamealready guards the normal)path withlen(board_stack) > 1, andbegin_variation()asserts the root is never pushed as a variation, so I madeend_variation()symmetric: it keeps the root on the stack. With that, the example parses to thee4 e5mainline with one recorded error, and real variations are unaffected.Extended test_variation_stack with this case (it sits right next to the existing superfluous-bracket cases). It raises IndexError on master and passes with the change; the full PGN test suite still passes. Found it by fuzzing read_game with mutated PGNs.