Add jq_report_error() function; use it

    Put a stop to fprintf(stderr, ...) where we shouldn't.

7 files changed, 82 insertions, 70 deletions

diff --git a/compile.c b/compile.c
index 6005f7c3..92189687 100644
--- a/compile.c
+++ b/compile.c
@@ -818,7 +818,7 @@ static int expand_call_arglist(block* b) {
   for (inst* curr; (curr = block_take(b));) {
     if (opcode_describe(curr->op)->flags & OP_HAS_BINDING) {
       if (!curr->bound_by) {
-        locfile_locate(curr->locfile, curr->source, "error: %s/%d is not defined", curr->symbol, block_count_actuals(curr->arglist));
+        locfile_locate(curr->locfile, curr->source, "jq: error: %s/%d is not defined", curr->symbol, block_count_actuals(curr->arglist));
         errors++;
         // don't process this instruction if it's not well-defined
         ret = BLOCK(ret, inst_block(curr));
diff --git a/execute.c b/execute.c
index ec88b092..53b67c18 100644
--- a/execute.c
+++ b/execute.c
@@ -256,15 +256,18 @@ static void jq_reset(jq_state *jq) {
   jq->subexp_nest = 0;
 }
 
-static void print_error(jq_state *jq, jv value) {
-  assert(!jv_is_valid(value));
-  if (jq->err_cb) {
-    // callback must jv_free() its jv argument
-    jq->err_cb(jq->err_cb_data, jv_invalid_get_msg(jv_copy(value)));
-  }
+void jq_report_error(jq_state *jq, jv value) {
+  assert(jq->err_cb);
+  // callback must jv_free() its jv argument
+  jq->err_cb(jq->err_cb_data, value);
+}
+
+static void set_error(jq_state *jq, jv value) {
+  // Record so try/catch can find it.
   jv_free(jq->error);
   jq->error = value;
 }
+
 #define ON_BACKTRACK(op) ((op)+NUM_OPCODES)
 
 jv jq_next(jq_state *jq) {
@@ -390,8 +393,8 @@ jv jq_next(jq_state *jq) {
         stack_push(jq, jv_object_set(objv, k, v));
         stack_push(jq, stktop);
       } else {
-        print_error(jq, jv_invalid_with_msg(jv_string_fmt("Cannot use %s as object key",
-                                                          jv_kind_name(jv_get_kind(k)))));
+        set_error(jq, jv_invalid_with_msg(jv_string_fmt("Cannot use %s as object key",
+                                                        jv_kind_name(jv_get_kind(k)))));
         jv_free(stktop);
         jv_free(v);
         jv_free(k);
@@ -410,7 +413,7 @@ jv jq_next(jq_state *jq) {
       if (raising) goto do_backtrack;
       if (jv_get_kind(*var) != JV_KIND_NUMBER ||
           jv_get_kind(max) != JV_KIND_NUMBER) {
-        print_error(jq, jv_invalid_with_msg(jv_string_fmt("Range bounds must be numeric")));
+        set_error(jq, jv_invalid_with_msg(jv_string_fmt("Range bounds must be numeric")));
         jv_free(max);
         goto do_backtrack;
       } else if (jv_number_value(jv_copy(*var)) >= jv_number_value(jv_copy(max))) {
@@ -524,7 +527,7 @@ jv jq_next(jq_state *jq) {
         stack_push(jq, v);
       } else {
         if (opcode == INDEX)
-          print_error(jq, v);
+          set_error(jq, v);
         else
           jv_free(v);
         goto do_backtrack;
@@ -582,9 +585,9 @@ jv jq_next(jq_state *jq) {
       } else {
         assert(opcode == EACH || opcode == EACH_OPT);
         if (opcode == EACH) {
-          print_error(jq,
-                      jv_invalid_with_msg(jv_string_fmt("Cannot iterate over %s",
-                                                        jv_kind_name(jv_get_kind(container)))));
+          set_error(jq,
+                    jv_invalid_with_msg(jv_string_fmt("Cannot iterate over %s",
+                                                      jv_kind_name(jv_get_kind(container)))));
         }
         keep_going = 0;
       }
@@ -681,7 +684,7 @@ jv jq_next(jq_state *jq) {
       if (jv_is_valid(top)) {
         stack_push(jq, top);
       } else {
-        print_error(jq, top);
+        set_error(jq, top);
         goto do_backtrack;
       }
       break;
@@ -758,6 +761,48 @@ jv jq_next(jq_state *jq) {
   }
 }
 
+jv jq_format_error(jv msg) {
+  if (jv_get_kind(msg) == JV_KIND_NULL ||
+      (jv_get_kind(msg) == JV_KIND_INVALID && !jv_invalid_has_msg(jv_copy(msg)))) {
+    jv_free(msg);
+    fprintf(stderr, "jq: error: out of memory\n");
+    return jv_null();
+  }
+
+  if (jv_get_kind(msg) == JV_KIND_STRING)
+    return msg;                         // expected to already be formatted
+
+  if (jv_get_kind(msg) == JV_KIND_INVALID)
+    msg = jv_invalid_get_msg(msg);
+
+  if (jv_get_kind(msg) == JV_KIND_NULL)
+    return jq_format_error(msg);        // ENOMEM
+
+  // Invalid with msg; prefix with "jq: error: "
+
+  if (jv_get_kind(msg) != JV_KIND_INVALID) {
+    if (jv_get_kind(msg) == JV_KIND_STRING)
+      return jv_string_fmt("jq: error: %s", msg);
+
+    msg = jv_dump_string(msg, JV_PRINT_INVALID);
+    if (jv_get_kind(msg) == JV_KIND_STRING)
+      return jv_string_fmt("jq: error: %s", msg);
+    return jq_format_error(jv_null());  // ENOMEM
+  }
+
+  // An invalid inside an invalid!
+  return jq_format_error(jv_invalid_get_msg(msg));
+}
+
+// XXX Refactor into a utility function that returns a jv and one that
+// uses it and then prints that jv's string as the complete error
+// message.
+static void default_err_cb(void *data, jv msg) {
+  msg = jq_format_error(msg);
+  fprintf((FILE *)data, "%s\n", jv_string_value(msg));
+  jv_free(msg);
+}
+
 jq_state *jq_init(void) {
   jq_state *jq;
   jq = jv_mem_alloc_unguarded(sizeof(*jq));
@@ -772,8 +817,8 @@ jq_state *jq_init(void) {
   jq->curr_frame = 0;
   jq->error = jv_null();
 
-  jq->err_cb = NULL;
-  jq->err_cb_data = NULL;
+  jq->err_cb = default_err_cb;
+  jq->err_cb_data = stderr;
 
   jq->attrs = jv_object();
   jq->path = jv_null();
@@ -781,6 +826,7 @@ jq_state *jq_init(void) {
 }
 
 void jq_set_error_cb(jq_state *jq, jq_err_cb cb, void *data) {
+  assert(cb != NULL);
   jq->err_cb = cb;
   jq->err_cb_data = data;
 }
@@ -925,17 +971,8 @@ int jq_compile_args(jq_state *jq, const char* str, jv args) {
       nerrors = block_compile(program, &jq->bc);
     }
   }
-  if (nerrors) {
-    jv s = jv_string_fmt("%d compile %s", nerrors,
-                         nerrors > 1 ? "errors" : "error");
-    if (jq->err_cb != NULL)
-      jq->err_cb(jq->err_cb_data, s);
-    else if (!jv_is_valid(s))
-      fprintf(stderr, "Error formatting jq compilation errors: %s\n", strerror(errno));
-    else
-      fprintf(stderr, "%s\n", jv_string_value(s));
-    jv_free(s);
-  }
+  if (nerrors)
+    jq_report_error(jq, jv_string_fmt("jq: %d compile %s", nerrors, nerrors > 1 ? "errors" : "error"));
   if (jq->bc)
     jq->bc = optimize(jq->bc);
   jv_free(args);
diff --git a/jq.h b/jq.h
index eaadb597..1849f661 100644
--- a/jq.h
+++ b/jq.h
@@ -13,6 +13,8 @@ jq_state *jq_init(void);
 void jq_set_error_cb(jq_state *, jq_err_cb, void *);
 void jq_get_error_cb(jq_state *, jq_err_cb *, void **);
 void jq_set_nomem_handler(jq_state *, void (*)(void *), void *);
+jv jq_format_error(jv msg);
+void jq_report_error(jq_state *, jv);
 int jq_compile(jq_state *, const char* str);
 int jq_compile_args(jq_state *, const char* str, jv args);
 void jq_dump_disassembly(jq_state *, int);
diff --git a/linker.c b/linker.c
index 959b373d..016870d5 100644
--- a/linker.c
+++ b/linker.c
@@ -46,7 +46,7 @@ jv build_lib_search_chain(jq_state *jq, jv lib_path) {
       out_paths = jv_array_append(out_paths, path);
     } else {
       jv emsg = jv_invalid_get_msg(path);
-      fprintf(stderr, "jq: warning: skipping search path: %s\n", jv_string_value(emsg));
+      jq_report_error(jq, jv_string_fmt("jq: warning: skipping search path: %s\n",jv_string_value(emsg)));
       jv_free(emsg);
     } 
   }
@@ -108,7 +108,7 @@ static int process_dependencies(jq_state *jq, jv lib_origin, block *src_block, s
     jv lib_path = find_lib(jq, name, search);
     if (!jv_is_valid(lib_path)) {
       jv emsg = jv_invalid_get_msg(lib_path);
-      fprintf(stderr, "jq: error: %s\n",jv_string_value(emsg));
+      jq_report_error(jq, jv_string_fmt("jq: error: %s\n",jv_string_value(emsg)));
       jv_free(emsg);
       jv_free(lib_origin);
       jv_free(as);
diff --git a/locfile.c b/locfile.c
index eaead6bb..1a60f42e 100644
--- a/locfile.c
+++ b/locfile.c
@@ -59,8 +59,6 @@ static int locfile_line_length(struct locfile* l, int line) {
 }
 
 void locfile_locate(struct locfile* l, location loc, const char* fmt, ...) {
-  jq_err_cb cb;
-  void *cb_data;
   va_list fmtargs;
   va_start(fmtargs, fmt);
   int startline;
@@ -71,45 +69,20 @@ void locfile_locate(struct locfile* l, location loc, const char* fmt, ...) {
     offset = l->linemap[startline];
   }
 
-  jq_get_error_cb(l->jq, &cb, &cb_data);
-
   jv m1 = jv_string_vfmt(fmt, fmtargs);
   if (!jv_is_valid(m1)) {
-    jv_free(m1);
-    goto enomem;
+    jq_report_error(l->jq, m1);
+    return;
   }
-  jv m2;
   if (loc.start == -1) {
-    m2 = jv_string_fmt("%s\n<unknown location>", jv_string_value(m1));
-    if (cb)
-      cb(cb_data, m2);
-    else
-      fprintf(stderr, "%s", jv_string_value(m2));
+    jq_report_error(l->jq, jv_string_fmt("jq: error: %s\n<unknown location>", jv_string_value(m1)));
     jv_free(m1);
-    jv_free(m2);
     return;
   }
-  m2 = jv_string_fmt("%s\n%.*s%*s", jv_string_value(m1),
-                     locfile_line_length(l, startline), l->data + offset,
-                     loc.start - offset, "");
+  jv m2 = jv_string_fmt("%s\n%.*s%*s", jv_string_value(m1),
+                        locfile_line_length(l, startline), l->data + offset,
+                        loc.start - offset, "");
   jv_free(m1);
-  if (!jv_is_valid(m2)) {
-    jv_free(m2);
-    goto enomem;
-  }
-  if (cb)
-    cb(cb_data, m2);
-  else
-    fprintf(stderr, "%s", jv_string_value(m2));
-  jv_free(m2);
-  return;
-
-enomem:
-  if (cb != NULL)
-    cb(cb_data, jv_invalid());
-  else if (errno == ENOMEM || errno == 0)
-    fprintf(stderr, "Error formatting jq compilation error: %s", strerror(errno ? errno : ENOMEM));
-  else
-    fprintf(stderr, "Error formatting jq compilation error: %s", strerror(errno));
+  jq_report_error(l->jq, m2);
   return;
 }
diff --git a/parser.y b/parser.y
index 9f925948..c2b1b478 100644
--- a/parser.y
+++ b/parser.y
@@ -122,12 +122,12 @@ void yyerror(YYLTYPE* loc, block* answer, int* errors,
   (*errors)++;
   if (strstr(s, "unexpected")) {
 #ifdef WIN32
-      locfile_locate(locations, *loc, "error: %s (Windows cmd shell quoting issues?)", s);
+      locfile_locate(locations, *loc, "jq: error: %s (Windows cmd shell quoting issues?)", s);
 #else
-      locfile_locate(locations, *loc, "error: %s (Unix shell quoting issues?)", s);
+      locfile_locate(locations, *loc, "jq: error: %s (Unix shell quoting issues?)", s);
 #endif
   } else {
-      locfile_locate(locations, *loc, "error: %s", s);
+      locfile_locate(locations, *loc, "jq: error: %s", s);
   }
 }
 
@@ -726,7 +726,7 @@ int jq_parse_library(struct locfile* locations, block* answer) {
   int errs = jq_parse(locations, answer);
   if (errs) return errs;
   if (block_has_main(*answer)) {
-    locfile_locate(locations, UNKNOWN_LOCATION, "error: library should only have function definitions, not a main expression");
+    locfile_locate(locations, UNKNOWN_LOCATION, "jq: error: library should only have function definitions, not a main expression");
     return 1;
   }
   assert(block_has_only_binders_and_imports(*answer, OP_IS_CALL_PSEUDO));
diff --git a/tests/run b/tests/run
index 923478c6..878128de 100755
--- a/tests/run
+++ b/tests/run
@@ -129,13 +129,13 @@ if [ -n "$VALGRIND" ] && ! grep 'ERROR SUMMARY: 0 errors from 0 contexts' $d/out
     cat $d/out
     exit 1
 fi
-if ! grep '^error: syntax error,' $d/out > /dev/null; then
+if ! grep '^jq: error: syntax error,' $d/out > /dev/null; then
     echo "Module system not detecting syntax errors in modules correctly" 1>&2
     exit 1
 fi
 
 if $VALGRIND ./jq -ner -L $d '%::wat' > $d/out 2>&1 ||
-   ! grep '^error: syntax error,' $d/out > /dev/null; then
+   ! grep '^jq: error: syntax error,' $d/out > /dev/null; then
     echo "Syntax errors not detected?" 1>&2
     exit 1
 fi