improve error reporting in PGN parser - resolves #958 default tip

Wed, 02 Sep 2026 17:11:56 +0200

author
Mike Becker <universe@uap-core.de>
date
Wed, 02 Sep 2026 17:11:56 +0200
changeset 209
dc4f34426c66
parent 208
e9c3d3e89465

improve error reporting in PGN parser - resolves #958

src/chess/pgn.c file | annotate | diff | comparison | revisions
src/chess/pgn.h file | annotate | diff | comparison | revisions
src/main.c file | annotate | diff | comparison | revisions
--- a/src/chess/pgn.c	Tue Sep 01 17:47:07 2026 +0200
+++ b/src/chess/pgn.c	Wed Sep 02 17:11:56 2026 +0200
@@ -41,24 +41,17 @@
     "Missing closing brace '}' for comment.",
     "Tags must be enclosed in square brackets: '[Key \"Value\"]'.",
     "Move numbers must be terminated with a dot (e.g. '13.' - not '13').",
-    "Move is syntactically incorrect.",
-    "Move is not valid according to chess rules.",
     "Result syntax is incorrect. Expected 1-0, 0-1, 1/2-1/2, or *.",
     "Result at end of move text does not match result in tag roster.",
     "Unexpected end of data.",
+    "Invalid move.",
 };
 
 const char* pgn_error_str(int code) {
     return pgn_error_strings[code];
 }
 
-size_t pgn_error_pos;
-
-size_t pgn_error_position(void) {
-    return pgn_error_pos;
-}
-
-static const char *pgn_result(const GameState *gamestate) {
+static const char *pgn_game_result(const GameState *gamestate) {
     if (is_game_drawn(gamestate)) {
         return "1/2-1/2";
     } else if (gamestate->wresign) {
@@ -76,9 +69,28 @@
     }
 }
 
-#define return(code) pgn_error_pos = pgndata - pgndata0; return code
+static pgn_result create_pgn_result(
+        const char *datastart, const char *datacur,
+        enum pgn_error_code code, unsigned move_half_number) {
+    return (pgn_result) {
+        .position = (size_t)(datacur - datastart),
+        .code = code,
+        .move_half_number = move_half_number
+    };
+}
 
-int parse_pgn(const char *pgndata, GameState *gamestate) {
+static pgn_result create_pgn_move_error(
+        const char *datastart, const char *datacur,
+        int move_error_code, unsigned move_half_number) {
+    return (pgn_result) {
+        .position = (size_t)(datacur - datastart),
+        .code = pgn_error_move,
+        .move_error = move_error_code,
+        .move_half_number = move_half_number
+    };
+}
+
+pgn_result parse_pgn(const char *pgndata, GameState *gamestate) {
     const char * const pgndata0 = pgndata;
     int i;
     char c;
@@ -95,7 +107,8 @@
             break;
         }
         if (c != '[') {
-            return(pgn_error_missing_bracket);
+            return create_pgn_result(pgndata0, pgndata,
+                pgn_error_missing_bracket, 0);
         }
         while (isspace(c = *(pgndata++)));
         i = 0;
@@ -105,18 +118,21 @@
         tagkey[i] = '\0';
         while (isspace(c = *(pgndata++)));
         if (c != '"') {
-            return(pgn_error_missing_quote);
+            return create_pgn_result(pgndata0, pgndata,
+                pgn_error_missing_quote, 0);
         }
         i = 0;
         while ((c = *(pgndata++)) != '"') {
             if (c == '\n' || c == 0) {
-                return(pgn_error_missing_quote);
+                return create_pgn_result(pgndata0, pgndata,
+                    pgn_error_missing_quote, 0);
             }
             tagvalue[i++] = c;
         }
         tagvalue[i] = '\0';
         if (*(pgndata++) != ']') {
-            return(pgn_error_missing_bracket);
+            return create_pgn_result(pgndata0, pgndata,
+                pgn_error_missing_bracket, 0);
         }
 
         // TODO: read clock info
@@ -128,13 +144,15 @@
     
     /* read moves */
     if (*(pgndata++) != '.') {
-        return(pgn_error_missing_dot);
+        return create_pgn_result(pgndata0, pgndata,
+            pgn_error_missing_dot, 1);
     }
     
     char movestr[10];
     Move move;
     Color curcol = WHITE;
     bool movetext_ends_with_result = false;
+    unsigned move_half_number = 1;
     
     while (true) {
         /* move */
@@ -143,17 +161,18 @@
         do {
             movestr[i++] = c;
             if (i >= 10) {
-                return(pgn_error_move_syntax);
+                return create_pgn_move_error(pgndata0, pgndata,
+                    INVALID_MOVE_SYNTAX, move_half_number);
             }
         } while (!isspace(c = *(pgndata++)));
         movestr[i] = '\0';
-        if (eval_move_strict(gamestate, movestr, curcol, &move)
-                != VALID_MOVE_SYNTAX) {
-            return(pgn_error_move_syntax);
+        int move_result = eval_move_strict(gamestate, movestr, curcol, &move);
+        if (!move_result) {
+            move_result = validate_move(gamestate, &move);
         }
-        int move_validate_result = validate_move(gamestate, &move);
-        if (move_validate_result != VALID_MOVE_SEMANTICS) {
-            return(pgn_error_move_semantics);
+        if (move_result) {
+            return create_pgn_move_error(pgndata0, pgndata,
+                move_result, move_half_number);
         }
         format_move(gamestate, &move);
         apply_move(gamestate, &move);
@@ -168,7 +187,8 @@
                 c = *(pgndata++);
             } while (c != '}' && c != 0);
             if (c == 0) {
-                return(pgn_error_missing_brace);
+                return create_pgn_result(pgndata0, pgndata,
+                    pgn_error_missing_brace, move_half_number);
             }
             /* skip spaces */
             while (isspace(c = *(pgndata++)));
@@ -188,7 +208,8 @@
                     } else if (c == '0') {
                         gamestate->bresign = true;
                     } else {
-                        return(pgn_error_result_syntax);
+                        return create_pgn_result(pgndata0, pgndata,
+                            pgn_error_result_syntax, 0);
                     }
                 }
                 movetext_ends_with_result = true;
@@ -211,25 +232,28 @@
         if (curcol == BLACK) {
             while (isdigit(c = *(pgndata++)));
             if (c != '.') {
-                return(pgn_error_missing_dot);
+                return create_pgn_result(pgndata0, pgndata,
+                    pgn_error_missing_dot, move_half_number + 1);
             }
         }
         curcol = opponent_color(curcol);
+        move_half_number++;
     }
 
     /* sanity check result - if it was specified */
     if (movetext_ends_with_result && result[0]) {
-        if (strncmp(result, pgn_result(gamestate), 8) != 0) {
-            return(pgn_error_result_mismatch);
+        if (strncmp(result, pgn_game_result(gamestate), 8) != 0) {
+            return create_pgn_result(pgndata0, pgndata,
+                pgn_error_result_mismatch, 0);
         }
     }
     
-    return(pgn_no_error);
+    return create_pgn_result(pgndata0, pgndata, pgn_no_error, 0);
 }
 
 #undef return
 
-int read_pgn(FILE *stream, GameState *gamestate) {
+pgn_result read_pgn(FILE *stream, GameState *gamestate) {
     fseek(stream, 0, SEEK_END);
     size_t size = ftell(stream);
     fseek(stream, 0, SEEK_SET);
@@ -239,12 +263,14 @@
     do {
         size_t r = fread(data + read, 1, size - read, stream);
         if (r == 0) {
-            pgn_error_pos = read;
-            return pgn_error_unexpected_eof;
+            return (pgn_result) {
+                .position = read,
+                .code = pgn_error_unexpected_eof
+            };
         }
         read += r;
     } while (read < size);
-    int result = parse_pgn(data, gamestate);
+    pgn_result result = parse_pgn(data, gamestate);
     free(data);
     return result;
 }
@@ -328,7 +354,7 @@
     resp += sprintf(resp, "[Round \"%s\"]\n", "-");
     resp += sprintf(resp, "[White \"%s\"]\n", pgn_player_name(gamestate, WHITE));
     resp += sprintf(resp, "[Black \"%s\"]\n", pgn_player_name(gamestate, BLACK));
-    const char *gameresult = pgn_result(gamestate);
+    const char *gameresult = pgn_game_result(gamestate);
     resp += sprintf(resp, "[Result \"%s\"]\n\n", gameresult);
 
     // TODO: add optional clock info
--- a/src/chess/pgn.h	Tue Sep 01 17:47:07 2026 +0200
+++ b/src/chess/pgn.h	Wed Sep 02 17:11:56 2026 +0200
@@ -38,29 +38,36 @@
 extern "C" {
 #endif
 
-enum {
+enum pgn_error_code {
     pgn_no_error = 0,
     pgn_error_missing_quote,
     pgn_error_missing_bracket,
     pgn_error_missing_brace,
     pgn_error_missing_dot,
-    pgn_error_move_syntax,
-    pgn_error_move_semantics,
     pgn_error_result_syntax,
     pgn_error_result_mismatch,
     pgn_error_unexpected_eof,
+    pgn_error_move,
 };
 
-int read_pgn(FILE *stream, GameState *gamestate);
+typedef struct {
+    size_t position;
+    enum pgn_error_code code;
+    int move_error; // TODO: should probably also be an enum eventually
+    unsigned move_half_number;
+} pgn_result;
+
+
+pgn_result read_pgn(FILE *stream, GameState *gamestate);
 int write_pgn(FILE* stream, const GameState *gamestate, bool export_comments);
 
-int parse_pgn(const char *data, GameState *gamestate);
+pgn_result parse_pgn(const char *data, GameState *gamestate);
 char *create_pgn(const GameState *gamestate, bool export_comments);
 
+// TODO: this function looks a bit misplaced
 const char *pgn_player_name(const GameState *gamestate, Color color);
 
 const char* pgn_error_str(int code);
-size_t pgn_error_position(void);
 
 #ifdef	__cplusplus
 }
--- a/src/main.c	Tue Sep 01 17:47:07 2026 +0200
+++ b/src/main.c	Wed Sep 02 17:11:56 2026 +0200
@@ -1289,11 +1289,20 @@
     if (settings.continuepgn) {
         FILE *pgnfile = fopen(settings.continuepgn, "r");
         if (pgnfile) {
-            int result = read_pgn(pgnfile, &gamestate);
+            pgn_result result = read_pgn(pgnfile, &gamestate);
             fclose(pgnfile);
-            if (result) {
-                printw("Invalid PGN file content at position %zu:\n%s\n",
-                        pgn_error_position(), pgn_error_str(result));
+            if (result.code) {
+                printw("Invalid PGN content at position %zu", result.position);
+                if (result.move_half_number) {
+                    unsigned hm = result.move_half_number - 1;
+                    printw(" (move %u, %s)", 1+hm/2, hm%2 ? "black":"white");
+                }
+                printw(":\n%s", pgn_error_str(result.code));
+                if (result.code == pgn_error_move) {
+                    addch(' ');
+                    eval_move_failed_msg(result.move_error);
+                }
+                printw("\n");
                 exitcode = EXIT_FAILURE;
             }
             /* clear resignation and remis flags to allow game continuation */

mercurial