Register imported libraries in dependency order (#3620)

block_mark_referenced walks the joined block backwards and skips the
body of any definition not yet marked referenced, so a library must
precede its importers.  f58787c41 registered a library before walking
its dependencies, to have somewhere to keep the `loading` flag, which
reversed that order and made block_drop_unreferenced free definitions
that are still referenced through bound_by.

Keep the libraries being loaded on a stack threaded through the
load_library frames instead, so registration can move back after the
dependency walk.

Makefile.am

@@ -224,6 +224,8 @@ EXTRA_DIST = $(DOC_FILES) $(man_MANS) $(TESTS) $(TEST_LOG_COMPILER)     \
         libjq.pc                                                        \
         tests/modules/a.jq tests/modules/b/b.jq tests/modules/c/c.jq    \
         tests/modules/c/d.jq tests/modules/data.json                    \
+        tests/modules/deep1.jq tests/modules/deep2a.jq                  \
+        tests/modules/deep2b.jq tests/modules/deep3.jq                  \
         tests/modules/home1/.jq tests/modules/home2/.jq/g.jq            \
         tests/modules/lib/jq/e/e.jq tests/modules/lib/jq/f.jq           \
         tests/modules/shadow1.jq tests/modules/shadow2.jq               \

src/linker.c

@@ -23,11 +23,15 @@
 struct lib_entry {
   char *name;
   block def;
-  int loading;
+};
+struct loading_lib {
+  const char *name;
+  struct loading_lib *next;
 };
 struct lib_loading_state {
   struct lib_entry *entries;
   uint64_t ct;
+  struct loading_lib *loading;
 };
 static int load_library(jq_state *jq, jv lib_path,
                         int is_data, int raw, int optional,
@@ -312,16 +316,6 @@ static int process_dependencies(jq_state *jq, jv jq_origin, jv lib_origin, block
       }
 
       if (state_idx < lib_state->ct) { // Found
-        if (lib_state->entries[state_idx].loading) {
-          jq_report_error(jq, jv_string_fmt("jq: error: circular import of %s\n",
-                                            jv_string_value(resolved)));
-          jv_free(resolved);
-          jv_free(as);
-          jv_free(deps);
-          jv_free(jq_origin);
-          jv_free(lib_origin);
-          return 1;
-        }
         jv_free(resolved);
         // Bind the library to the program
         bk = block_bind_library(lib_state->entries[state_idx].def, bk, OP_IS_CALL_PSEUDO, as_str);
@@ -351,11 +345,20 @@ static int load_library(jq_state *jq, jv lib_path, int is_data, int raw, int opt
   struct locfile* src = NULL;
   block program;
   jv data;
+  for (struct loading_lib *l = lib_state->loading; l; l = l->next) {
+    if (strcmp(l->name, jv_string_value(lib_path)) == 0) {
+      jq_report_error(jq, jv_string_fmt("jq: error: circular import of %s\n",
+                                        jv_string_value(lib_path)));
+      *out_block = gen_noop();
+      jv_free(lib_path);
+      return 1;
+    }
+  }
   if (is_data && !raw)
     data = jv_load_file(jv_string_value(lib_path), 0);
   else
     data = jv_load_file(jv_string_value(lib_path), 1);
-  int state_idx;
+  uint64_t state_idx;
   if (!jv_is_valid(data)) {
     program = gen_noop();
     if (!optional) {
@@ -373,29 +376,33 @@ static int load_library(jq_state *jq, jv lib_path, int is_data, int raw, int opt
     lib_state->entries = jv_mem_realloc(lib_state->entries, lib_state->ct * sizeof(struct lib_entry));
     lib_state->entries[state_idx].name = strdup(jv_string_value(lib_path));
     lib_state->entries[state_idx].def = program;
-    lib_state->entries[state_idx].loading = 0;
   } else {
     // import "foo" as bar;
     src = locfile_init(jq, jv_string_value(lib_path), jv_string_value(data), jv_string_length_bytes(jv_copy(data)));
     nerrors += jq_parse_library(src, &program);
     locfile_free(src);
     if (nerrors == 0) {
-      // Register the library before processing its dependencies so that
-      // circular imports can be detected.
-      state_idx = lib_state->ct++;
-      lib_state->entries = jv_mem_realloc(lib_state->entries, lib_state->ct * sizeof(struct lib_entry));
-      lib_state->entries[state_idx].name = strdup(jv_string_value(lib_path));
-      lib_state->entries[state_idx].def = gen_noop();
-      lib_state->entries[state_idx].loading = 1;
-
+      // Mark the library as being loaded while its dependencies are processed,
+      // so that a recursive reference to it is reported as a circular import.
+      // The node lives on this frame and is popped before lib_path is freed,
+      // so it can borrow the path string.
+      struct loading_lib loading = { jv_string_value(lib_path), lib_state->loading };
+      lib_state->loading = &loading;
       char *lib_origin = strdup(jv_string_value(lib_path));
       nerrors += process_dependencies(jq, jq_get_jq_origin(jq),
                                       jv_string(dirname(lib_origin)),
                                       &program, lib_state);
       free(lib_origin);
+      lib_state->loading = loading.next;
       program = block_bind_self(program, OP_IS_CALL_PSEUDO);
+      // Register the library only once its dependencies have been processed,
+      // to keep lib_state->entries in dependency order.  Register it even when
+      // its dependencies failed, so that other importers reuse this entry
+      // rather than loading and diagnosing the same library again.
+      state_idx = lib_state->ct++;
+      lib_state->entries = jv_mem_realloc(lib_state->entries, lib_state->ct * sizeof(struct lib_entry));
+      lib_state->entries[state_idx].name = strdup(jv_string_value(lib_path));
       lib_state->entries[state_idx].def = program;
-      lib_state->entries[state_idx].loading = 0;
     }
   }
   *out_block = program;
@@ -435,7 +442,7 @@ jv load_module_meta(jq_state *jq, jv mod_relpath) {
 int load_program(jq_state *jq, struct locfile* src, block *out_block) {
   int nerrors = 0;
   block program;
-  struct lib_loading_state lib_state = {0,0};
+  struct lib_loading_state lib_state = {0,0,0};
   nerrors = jq_parse(src, &program);
   if (nerrors)
     return nerrors;

tests/jq.test

@@ -1950,6 +1950,23 @@ import "shadow1" as f; import "shadow2" as f; import "shadow1" as e; [e::e, f::e
 null
 [2,3]
 
+# A module reached only through another module must be linked before its
+# importers, or dead-code elimination drops the definitions that only the
+# importer's body references (#3597)
+include "deep1"; f
+null
+[1,[21,3],[22,3,1]]
+
+# ... including when the shared dependency is pulled in first by an importer
+# whose own definitions end up unused, in either import order
+import "deep2a" as a; import "deep2b" as b; b::f
+null
+[22,3,1]
+
+import "deep2b" as b; import "deep2a" as a; b::f
+null
+[22,3,1]
+
 %%FAIL
 module (.+1); 0
 jq: error: Module metadata must be constant at <top-level>, line 1, column 8:

tests/modules/deep1.jq

@@ -0,0 +1,3 @@
+import "deep2a" as a;
+import "deep2b" as b;
+def f: [1, a::f, b::f];

tests/modules/deep2a.jq

@@ -0,0 +1,2 @@
+import "deep3" as m;
+def f: [21, m::f];

tests/modules/deep2b.jq

@@ -0,0 +1,3 @@
+import "deep3" as m;
+import "data" as $d;
+def f: [22, m::f, ($d | length)];

tests/modules/deep3.jq

@@ -0,0 +1,2 @@
+def _f: 3;
+def f: _f;