Merge pull request DeusData#1042 from DeusData/fix/871-commonjs-require

fix(extract): stop CommonJS require() bindings from shadowing call resolution

Author: DeusData

internal/cbm/extract_defs.c

@@ -4646,6 +4646,37 @@ static void extract_destructured_vars(CBMExtractCtx *ctx, TSNode pattern, TSNode
     }
 }
 
+/* True for `require("...")` call_expressions with a string-literal path — the
+ * CommonJS import form that extract_imports records with local_name = the
+ * enclosing declarator's identifier. Same string-argument kinds as
+ * process_commonjs_require so the two stay in lockstep. */
+static bool is_require_import_call(TSNode value, const char *source, CBMArena *a) {
+    if (strcmp(ts_node_type(value), "call_expression") != 0) {
+        return false;
+    }
+    TSNode fn = ts_node_child_by_field_name(value, TS_FIELD("function"));
+    if (ts_node_is_null(fn) || strcmp(ts_node_type(fn), "identifier") != 0) {
+        return false;
+    }
+    char *fn_name = cbm_node_text(a, fn, source);
+    if (!fn_name || strcmp(fn_name, "require") != 0) {
+        return false;
+    }
+    TSNode args = ts_node_child_by_field_name(value, TS_FIELD("arguments"));
+    if (ts_node_is_null(args)) {
+        return false;
+    }
+    uint32_t argc = ts_node_named_child_count(args);
+    for (uint32_t i = 0; i < argc; i++) {
+        const char *ak = ts_node_type(ts_node_named_child(args, i));
+        if (strcmp(ak, "string") == 0 || strcmp(ak, "string_literal") == 0 ||
+            strcmp(ak, "template_string") == 0) {
+            return true;
+        }
+    }
+    return false;
+}
+
 // JS/TS variable extraction: skip function-assigned declarators.
 static void extract_js_vars(CBMExtractCtx *ctx, TSNode node, CBMArena *a) {
     uint32_t n = ts_node_named_child_count(node);
@@ -4654,13 +4685,15 @@ static void extract_js_vars(CBMExtractCtx *ctx, TSNode node, CBMArena *a) {
         if (strcmp(ts_node_type(child), "variable_declarator") != 0) {
             continue;
         }
+        bool is_require = false;
         TSNode value = ts_node_child_by_field_name(child, TS_FIELD("value"));
         if (!ts_node_is_null(value)) {
             const char *vk = ts_node_type(value);
             if (strcmp(vk, "arrow_function") == 0 || strcmp(vk, "function_expression") == 0 ||
                 strcmp(vk, "generator_function") == 0) {
                 continue;
             }
+            is_require = is_require_import_call(value, ctx->source, a);
         }
         TSNode vname = ts_node_child_by_field_name(child, TS_FIELD("name"));
         if (!ts_node_is_null(vname)) {
@@ -4670,6 +4703,15 @@ static void extract_js_vars(CBMExtractCtx *ctx, TSNode node, CBMArena *a) {
             if (strcmp(nk, "object_pattern") == 0 || strcmp(nk, "array_pattern") == 0) {
                 extract_destructured_vars(ctx, vname, child, a);
             } else {
+                /* `const foo = require('./foo')` is an import binding, not a
+                 * definition — extract_imports records local_name="foo". A
+                 * Variable node here shadows call resolution onto the alias
+                 * and orphans the imported callee (#871); ESM `import`
+                 * bindings emit no Variable either. Destructured requires
+                 * keep theirs: the import row only records the module. */
+                if (is_require) {
+                    continue;
+                }
                 push_var_def(ctx, cbm_node_text(a, vname, ctx->source), child);
             }
         }

tests/test_lang_contract.c

@@ -1202,6 +1202,68 @@ TEST(contract_edge_imports_alias_resolves_real_file_issue767) {
     PASS();
 }
 
+/* True if some CALLS edge's TARGET node carries `label` and a QN ending with
+ * `qn_suffix` — i.e. the call resolved to that specific definition. */
+static int calls_edge_targets(cbm_store_t *store, const char *project, const char *label,
+                              const char *qn_suffix) {
+    cbm_edge_t *edges = NULL;
+    int n = 0;
+    if (cbm_store_find_edges_by_type(store, project, "CALLS", &edges, &n) != CBM_STORE_OK)
+        return 0;
+    int found = 0;
+    size_t sl = strlen(qn_suffix);
+    for (int i = 0; i < n && !found; i++) {
+        cbm_node_t tgt;
+        if (cbm_store_find_node_by_id(store, edges[i].target_id, &tgt) != CBM_STORE_OK)
+            continue;
+        const char *qn = tgt.qualified_name;
+        if (qn && tgt.label && strcmp(tgt.label, label) == 0) {
+            size_t ql = strlen(qn);
+            if (ql >= sl && strcmp(qn + ql - sl, qn_suffix) == 0)
+                found = 1;
+        }
+        cbm_node_free_fields(&tgt);
+    }
+    cbm_store_free_edges(edges, n);
+    return found;
+}
+
+/* #871: a CommonJS require() binding shadowed call resolution. `const doThing
+ * = require("../bs/doThing")` emitted a Variable def for `doThing` in the
+ * importing module, so the call `doThing(...)` resolved to that same-module
+ * Variable (an import ALIAS, not a definition) instead of following the
+ * recorded import to the exported function — which stayed at zero inbound
+ * CALLS (the reporter's disconnected-node symptom). ESM `import` bindings
+ * never materialize as Variables; the identifier-bound require form must
+ * behave identically: no shadowing Variable, CALLS edge lands on the real
+ * Function. Fixture = the reporter's exact two-file repro. */
+TEST(contract_edge_commonjs_require_call_resolves_issue871) {
+    LangProj lp;
+    static const LangFile f[] = {
+        {"src/bs/doThing.js",
+         "module.exports = async function doThing({ id }) {\n  return id;\n};\n"},
+        {"src/mutations/doThing.js",
+         "const doThing = require(\"../bs/doThing\");\n\n"
+         "module.exports = async (parent, args) => {\n  return doThing({ id: args.id });\n};\n"}};
+    cbm_store_t *store = lang_index_files(&lp, f, 2);
+    ASSERT_TRUE(store != NULL);
+    /* The call resolves THROUGH the require to the exported function. */
+    int resolved = calls_edge_targets(store, lp.project, "Function", ".bs.doThing.doThing");
+    /* The alias must not swallow the call: no CALLS edge may terminate on a
+     * Variable in the importing module (ESM parity: the binding is an import,
+     * not a definition). */
+    int shadowed = calls_edge_targets(store, lp.project, "Variable", ".mutations.doThing.doThing");
+    if (!resolved || shadowed) {
+        fprintf(stderr, "  [871] FAIL resolved=%d shadowed=%d (require binding must resolve to "
+                        "the exported Function, not the local alias Variable)\n",
+                resolved, shadowed);
+    }
+    ASSERT_TRUE(resolved);
+    ASSERT_TRUE(!shadowed);
+    lang_cleanup(&lp, store);
+    PASS();
+}
+
 /* DEPENDS_ON — Helm Chart.yaml `dependencies:` -> per-dependency Chart node.
  * Basename must be exactly "Chart.yaml"; pass_k8s runs in both pipeline paths. */
 TEST(contract_edge_depends_on) {
@@ -1411,6 +1473,7 @@ SUITE(lang_contract) {
     RUN_TEST(contract_edge_workspaces_imports_issue408);
     RUN_TEST(contract_edge_imports_alias_no_phantom_folder_edge_issue767);
     RUN_TEST(contract_edge_imports_alias_resolves_real_file_issue767);
+    RUN_TEST(contract_edge_commonjs_require_call_resolves_issue871);
     RUN_TEST(contract_edge_depends_on);
     RUN_TEST(contract_edge_parallel_service_edges);
     RUN_TEST(contract_edge_file_changes_with);