Improve error messages to show problematic position in the filter (#3292)

This commit improves the parser and compiler error messages, making
it more easier to understand where in the filter the problem is.
Like modern compilers, jq will show the line in the filter where the
error occurred, will display squiggly line under the invalid token.

src/jq_test.c

@@ -70,8 +70,6 @@ static void test_err_cb(void *data, jv e) {
     e = jv_dump_string(e, JV_PRINT_INVALID);
   if (!strncmp(jv_string_value(e), "jq: error", sizeof("jq: error") - 1))
     snprintf(err_data->buf, sizeof(err_data->buf), "%s", jv_string_value(e));
-  if (strchr(err_data->buf, '\n'))
-    *(strchr(err_data->buf, '\n')) = '\0';
   jv_free(e);
 }
 
@@ -108,18 +106,7 @@ static void run_jq_tests(jv lib_dirs, int verbose, FILE *testdata, int skip, int
 
     if (skip > 0) {
       skip--;
-
-      // skip past test data
-      while (fgets(buf, sizeof(buf), testdata)) {
-        lineno++;
-        if (buf[0] == '\n' || (buf[0] == '\r' && buf[1] == '\n'))
-          break;
-      }
-
-      must_fail = 0;
-      check_msg = 0;
-
-      continue;
+      goto next;
     } else if (skip == 0) {
       printf("Skipped %d tests\n", tests_to_skip);
       skip = -1;
@@ -139,37 +126,40 @@ static void run_jq_tests(jv lib_dirs, int verbose, FILE *testdata, int skip, int
 
     if (must_fail) {
       jq_set_error_cb(jq, NULL, NULL);
-      if (!fgets(buf, sizeof(buf), testdata)) { invalid++; break; }
-      lineno++;
-      if (buf[strlen(buf)-1] == '\n') buf[strlen(buf)-1] = 0;
       if (compiled) {
-        printf("*** Test program compiled that should not have at line %u: %s\n", lineno, prog);
-        must_fail = 0;
-        check_msg = 0;
-        invalid++;
-        continue;
+        printf("*** Test program compiled successfully, but should fail at line number %u: %s\n", lineno, prog);
+        goto fail;
       }
-      if (check_msg && strcmp(buf, err_msg.buf) != 0) {
-        printf("*** Erroneous test program failed with wrong message (%s) at line %u: %s\n", err_msg.buf, lineno, prog);
+      char *err_buf = err_msg.buf;
+      while (fgets(buf, sizeof(buf), testdata)) {
+        lineno++;
+        if (skipline(buf)) break;
+        if (check_msg) {
+          if (buf[strlen(buf)-1] == '\n') buf[strlen(buf)-1] = '\0';
+          if (strncmp(buf, err_buf, strlen(buf)) != 0) {
+            if (strchr(err_buf, '\n')) *strchr(err_buf, '\n') = '\0';
+            printf("*** Erroneous program failed with '%s', but expected '%s' at line number %u: %s\n", err_buf, buf, lineno, prog);
+            goto fail;
+          }
+          err_buf += strlen(buf);
+          if (*err_buf == '\n') err_buf++;
+        }
+      }
+      if (check_msg && *err_buf != '\0') {
+        if (strchr(err_buf, '\n')) *strchr(err_buf, '\n') = '\0';
+        printf("*** Erroneous program failed with extra message '%s' at line %u: %s\n", err_buf, lineno, prog);
         invalid++;
-      } else {
-        passed++;
+        pass = 0;
       }
       must_fail = 0;
       check_msg = 0;
+      passed += pass;
       continue;
     }
 
     if (!compiled) {
       printf("*** Test program failed to compile at line %u: %s\n", lineno, prog);
-      invalid++;
-      // skip past test data
-      while (fgets(buf, sizeof(buf), testdata)) {
-        lineno++;
-        if (buf[0] == '\n' || (buf[0] == '\r' && buf[1] == '\n'))
-          break;
-      }
-      continue;
+      goto fail;
     }
     if (verbose) {
       printf("Disassembly:\n");
@@ -181,8 +171,7 @@ static void run_jq_tests(jv lib_dirs, int verbose, FILE *testdata, int skip, int
     jv input = jv_parse(buf);
     if (!jv_is_valid(input)) {
       printf("*** Input is invalid on line %u: %s\n", lineno, buf);
-      invalid++;
-      continue;
+      goto fail;
     }
     jq_start(jq, input, verbose ? JQ_DEBUG_TRACE : 0);
 
@@ -192,8 +181,7 @@ static void run_jq_tests(jv lib_dirs, int verbose, FILE *testdata, int skip, int
       jv expected = jv_parse(buf);
       if (!jv_is_valid(expected)) {
         printf("*** Expected result is invalid on line %u: %s\n", lineno, buf);
-        invalid++;
-        continue;
+        goto fail;
       }
       jv actual = jq_next(jq);
       if (!jv_is_valid(actual)) {
@@ -226,12 +214,25 @@ static void run_jq_tests(jv lib_dirs, int verbose, FILE *testdata, int skip, int
         printf("*** Superfluous result: ");
         jv_dump(extra, 0);
         printf(" for test at line number %u, %s\n", lineno, prog);
+        invalid++;
         pass = 0;
       } else {
         jv_free(extra);
       }
     }
-    passed+=pass;
+    passed += pass;
+    continue;
+
+fail:
+    invalid++;
+
+next:
+    while (fgets(buf, sizeof(buf), testdata)) {
+      lineno++;
+      if (skipline(buf)) break;
+    }
+    must_fail = 0;
+    check_msg = 0;
   }
   jq_teardown(&jq);
 

src/locfile.c

@@ -7,7 +7,7 @@
 #include "jq.h"
 #include "jv_alloc.h"
 #include "locfile.h"
-
+#include "util.h"
 
 struct locfile* locfile_init(jq_state *jq, const char *fname, const char* data, int length) {
   struct locfile* l = jv_mem_alloc(sizeof(struct locfile));
@@ -63,13 +63,6 @@ static int locfile_line_length(struct locfile* l, int line) {
 void locfile_locate(struct locfile* l, location loc, const char* fmt, ...) {
   va_list fmtargs;
   va_start(fmtargs, fmt);
-  int startline;
-  int offset;
-
-  if (loc.start != -1) {
-    startline = locfile_get_line(l, loc.start);
-    offset = l->linemap[startline];
-  }
 
   jv m1 = jv_string_vfmt(fmt, fmtargs);
   va_end(fmtargs);
@@ -78,16 +71,23 @@ void locfile_locate(struct locfile* l, location loc, const char* fmt, ...) {
     return;
   }
   if (loc.start == -1) {
-    jq_report_error(l->jq, jv_string_fmt("jq: error: %s\n<unknown location>", jv_string_value(m1)));
+    jq_report_error(l->jq, jv_string_fmt("jq: error: %s", jv_string_value(m1)));
     jv_free(m1);
     return;
   }
-  jv m2 = jv_string_fmt("%s at %s, line %d, column %d:\n%.*s%*s",
+
+  int startline = locfile_get_line(l, loc.start);
+  int offset = l->linemap[startline];
+  int end = MIN(loc.end, l->linemap[startline+1] - 1);
+  assert(end > loc.start);
+  jv underline = jv_string_repeat(jv_string("^"), end - loc.start);
+  jv m2 = jv_string_fmt("%s at %s, line %d, column %d:\n    %.*s\n    %*s",
                         jv_string_value(m1), jv_string_value(l->fname),
                         startline + 1, loc.start - offset + 1,
                         locfile_line_length(l, startline), l->data + offset,
-                        loc.start - offset, "");
+                        end - offset, jv_string_value(underline));
   jv_free(m1);
+  jv_free(underline);
   jq_report_error(l->jq, m2);
   return;
 }

src/parser.y

@@ -139,15 +139,7 @@ struct lexer_param {
 void yyerror(YYLTYPE* loc, block* answer, int* errors,
              struct locfile* locations, struct lexer_param* lexer_param_ptr, const char *s){
   (*errors)++;
-  if (strstr(s, "unexpected")) {
-#ifdef WIN32
-      locfile_locate(locations, *loc, "jq: error: %s (Windows cmd shell quoting issues?)", s);
-#else
-      locfile_locate(locations, *loc, "jq: error: %s (Unix shell quoting issues?)", s);
-#endif
-  } else {
-      locfile_locate(locations, *loc, "jq: error: %s", s);
-  }
+  locfile_locate(locations, *loc, "jq: error: %s", s);
 }
 
 int yylex(YYSTYPE* yylval, YYLTYPE* yylloc, block* answer, int* errors,
@@ -300,11 +292,11 @@ Module:
 } |
 "module" Query ';' {
   if (!block_is_const($2)) {
-    FAIL(@$, "Module metadata must be constant");
+    FAIL(@2, "Module metadata must be constant");
     $$ = gen_noop();
     block_free($2);
   } else if (block_const_kind($2) != JV_KIND_OBJECT) {
-    FAIL(@$, "Module metadata must be an object");
+    FAIL(@2, "Module metadata must be an object");
     $$ = gen_noop();
     block_free($2);
   } else {
@@ -431,12 +423,12 @@ ImportWhat ';' {
 } |
 ImportWhat Query ';' {
   if (!block_is_const($2)) {
-    FAIL(@$, "Module metadata must be constant");
+    FAIL(@2, "Module metadata must be constant");
     $$ = gen_noop();
     block_free($1);
     block_free($2);
   } else if (block_const_kind($2) != JV_KIND_OBJECT) {
-    FAIL(@$, "Module metadata must be an object");
+    FAIL(@2, "Module metadata must be an object");
     $$ = gen_noop();
     block_free($1);
     block_free($2);
@@ -472,7 +464,7 @@ ImportWhat:
 ImportFrom:
 String {
   if (!block_is_const($1)) {
-    FAIL(@$, "Import path must be constant");
+    FAIL(@1, "Import path must be constant");
     $$ = gen_const(jv_string(""));
     block_free($1);
   } else {
@@ -826,7 +818,7 @@ String ':' Pattern {
 '(' Query ')' ':' Pattern {
   jv msg = check_object_key($2);
   if (jv_is_valid(msg)) {
-    FAIL(@$, jv_string_value(msg));
+    FAIL(@2, jv_string_value(msg));
   }
   jv_free(msg);
   $$ = gen_object_matcher($2, $5);
@@ -942,13 +934,13 @@ Keyword {
 '(' Query ')' ':' DictExpr {
   jv msg = check_object_key($2);
   if (jv_is_valid(msg)) {
-    FAIL(@$, jv_string_value(msg));
+    FAIL(@2, jv_string_value(msg));
   }
   jv_free(msg);
   $$ = gen_dictpair($2, $5);
 } |
 error ':' DictExpr {
-  FAIL(@$, "May need parentheses around object key expression");
+  FAIL(@1, "May need parentheses around object key expression");
   $$ = $3;
 }
 

tests/jq.test

@@ -59,6 +59,12 @@ null
 "Aa\r\n\t\b\f\u03bc"
 "Aa\u000d\u000a\u0009\u0008\u000c\u03bc"
 
+%%FAIL
+"u\vw"
+jq: error: Invalid escape at line 1, column 4 (while parsing '"\v"') at <top-level>, line 1, column 3:
+    "u\vw"
+      ^^
+
 "inter\("pol" + "ation")"
 null
 "interpolation"
@@ -119,11 +125,21 @@ null
 
 %%FAIL
 {(0):1}
-jq: error: Cannot use number (0) as object key at <top-level>, line 1, column 2:
+jq: error: Cannot use number (0) as object key at <top-level>, line 1, column 3:
+    {(0):1}
+      ^
+
+%%FAIL
+{1+2:3}
+jq: error: May need parentheses around object key expression at <top-level>, line 1, column 2:
+    {1+2:3}
+     ^^^
 
 %%FAIL
 {non_const:., (0):1}
-jq: error: Cannot use number (0) as object key at <top-level>, line 1, column 15:
+jq: error: Cannot use number (0) as object key at <top-level>, line 1, column 16:
+    {non_const:., (0):1}
+                   ^
 
 #
 # Field access, piping
@@ -303,6 +319,8 @@ null
 %%FAIL
 . as $foo | break $foo
 jq: error: $*label-foo is not defined at <top-level>, line 1, column 13:
+    . as $foo | break $foo
+                ^^^^^^^^^^
 
 [.[]|[.,1]|until(.[0] < 1; [.[0] - 1, .[1] * .[0]])|.[1]]
 [1,2,3,4,5]
@@ -518,13 +536,29 @@ null
 [0]
 [0]
 
-%%FAIL IGNORE MSG
+%%FAIL
 . as [] | null
-jq: error: syntax error, unexpected ']', expecting '$' or '[' or '{' (Unix shell quoting issues?) at <top-level>, line 1, column 7:
+jq: error: syntax error, unexpected ']', expecting BINDING or '[' or '{' at <top-level>, line 1, column 7:
+    . as [] | null
+          ^
 
-%%FAIL IGNORE MSG
+%%FAIL
 . as {} | null
-jq: error: syntax error, unexpected '}' (Unix shell quoting issues?) at <top-level>, line 1, column 7:
+jq: error: syntax error, unexpected '}' at <top-level>, line 1, column 7:
+    . as {} | null
+          ^
+
+%%FAIL
+. as $foo | [$foo, $bar]
+jq: error: $bar is not defined at <top-level>, line 1, column 20:
+    . as $foo | [$foo, $bar]
+                       ^^^^
+
+%%FAIL
+. as {(true):$foo} | $foo
+jq: error: Cannot use boolean (true) as object key at <top-level>, line 1, column 8:
+    . as {(true):$foo} | $foo
+           ^^^^
 
 # [.,(.[] | {x:.},.),.,.[]]
 
@@ -1840,27 +1874,39 @@ null
 
 %%FAIL
 module (.+1); 0
-jq: error: Module metadata must be constant at <top-level>, line 1, column 1:
+jq: error: Module metadata must be constant at <top-level>, line 1, column 8:
+    module (.+1); 0
+           ^^^^^
 
 %%FAIL
 module []; 0
-jq: error: Module metadata must be an object at <top-level>, line 1, column 1:
+jq: error: Module metadata must be an object at <top-level>, line 1, column 8:
+    module []; 0
+           ^^
 
 %%FAIL
 include "a" (.+1); 0
-jq: error: Module metadata must be constant at <top-level>, line 1, column 1:
+jq: error: Module metadata must be constant at <top-level>, line 1, column 13:
+    include "a" (.+1); 0
+                ^^^^^
 
 %%FAIL
 include "a" []; 0
-jq: error: Module metadata must be an object at <top-level>, line 1, column 1:
+jq: error: Module metadata must be an object at <top-level>, line 1, column 13:
+    include "a" []; 0
+                ^^
 
 %%FAIL
 include "\ "; 0
 jq: error: Invalid escape at line 1, column 4 (while parsing '"\ "') at <top-level>, line 1, column 10:
+    include "\ "; 0
+             ^^
 
 %%FAIL
 include "\(a)"; 0
 jq: error: Import path must be constant at <top-level>, line 1, column 9:
+    include "\(a)"; 0
+            ^^^^^^
 
 modulemeta
 "c"
@@ -1876,11 +1922,15 @@ modulemeta | .defs | length
 
 %%FAIL IGNORE MSG
 import "syntaxerror" as e; .
-jq: error: syntax error, unexpected ';', expecting $end (Unix shell quoting issues?) at /home/nico/ws/jq/tests/modules/syntaxerror/syntaxerror.jq, line 1, column 8:
+jq: error: syntax error, unexpected ';', expecting end of file at tests/modules/syntaxerror/syntaxerror.jq, line 1, column 4:
+    wat;
+       ^
 
-%%FAIL IGNORE MSG
+%%FAIL
 %::wat
-jq: error: syntax error, unexpected '%', expecting $end (Unix shell quoting issues?) at <top-level>, line 1, column 1:
+jq: error: syntax error, unexpected '%', expecting end of file at <top-level>, line 1, column 1:
+    %::wat
+    ^
 
 import "test_bind_order" as check; check::check
 null
@@ -1959,9 +2009,17 @@ true
 [range(-99/2;99/2;1)] as $orig | [$orig[]|pow(2;.)|log2] as $back | ($orig|keys)[]|. as $k | (($orig|.[$k])-($back|.[$k]))|if . < 0 then . * -1 else . end|select(.>.00005)
 null
 
-%%FAIL IGNORE MSG
+%%FAIL
+{
+jq: error: syntax error, unexpected end of file at <top-level>, line 1, column 1:
+    {
+    ^
+
+%%FAIL
 }
-jq: error: syntax error, unexpected INVALID_CHARACTER, expecting $end (Unix shell quoting issues?) at <top-level>, line 1, column 1:
+jq: error: syntax error, unexpected INVALID_CHARACTER, expecting end of file at <top-level>, line 1, column 1:
+    }
+    ^
 
 (.[{}] = 0)?
 null

tests/shtest

@@ -634,6 +634,29 @@ EOF
   fi
 fi
 
+# Test syntax error messages of multiline filter (we can't test in jq.test)
+if $VALGRIND $Q $JQ -n '[
+  try if .
+         then 1
+         else 2
+  catch ]' > $d/out 2>&1; then
+  echo 'syntax error not detected'
+  exit 1
+fi
+cat > $d/expected <<EOF
+jq: error: syntax error, unexpected catch, expecting end or '|' or ',' at <top-level>, line 5, column 3:
+      catch ]
+      ^^^^^
+jq: error: Possibly unterminated 'if' statement at <top-level>, line 2, column 7:
+      try if .
+          ^^^^
+jq: error: Possibly unterminated 'try' statement at <top-level>, line 2, column 3:
+      try if .
+      ^^^^^^^^
+jq: 3 compile errors
+EOF
+diff $d/out $d/expected
+
 # Comments!
 if ! x=$($JQ -n '123 # comment') || [ "$x" != 123 ]; then
   echo 'comment did not work'