Merge branch 'main' into fix/incremental-report-trend-fallback

Author: carlos-alm

crates/codegraph-core/src/extractors/r_lang.rs

@@ -163,10 +163,14 @@ fn handle_call(node: &Node, source: &[u8], symbols: &mut FileSymbols) {
                 handle_set_class(node, source, symbols);
                 return;
             }
-            "setGeneric" | "setMethod" => {
+            "setGeneric" => {
                 handle_set_generic(node, source, symbols);
                 return;
             }
+            "setMethod" => {
+                handle_set_method(node, source, symbols);
+                return;
+            }
             _ => {}
         }
     }
@@ -332,6 +336,23 @@ fn handle_set_generic(node: &Node, source: &[u8], symbols: &mut FileSymbols) {
     }
 }
 
+// `setMethod("greet", "Person", function(x) ...)` registers an implementation
+// of the generic `greet` — it is not a new top-level definition. Emitting a
+// definition here produced two `function` nodes with the same name (one from
+// setGeneric, one from setMethod) and broke resolution. Emit a call edge to
+// the generic instead; the method body's calls are still picked up by the
+// recursive walk of the anonymous function argument.
+fn handle_set_method(node: &Node, source: &[u8], symbols: &mut FileSymbols) {
+    if let Some(name) = first_argument_value(node, source, false) {
+        symbols.calls.push(Call {
+            name,
+            line: start_line(node),
+            dynamic: None,
+            receiver: None,
+        });
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -439,6 +460,51 @@ mod tests {
         assert_eq!(d.kind, "function");
     }
 
+    #[test]
+    fn set_method_does_not_duplicate_generic_definition() {
+        // Idiomatic S4: a setGeneric followed by setMethod implementations.
+        // Only the setGeneric should emit a definition — setMethod registers
+        // an implementation of the generic, which we model as a call edge.
+        let code = r#"
+setGeneric("greet", function(x) standardGeneric("greet"))
+setMethod("greet", "Person", function(x) paste("Hello", x@name))
+setMethod("greet", "Animal", function(x) paste("Hi", x@species))
+"#;
+        let s = parse_r(code);
+        let greet_defs: Vec<&Definition> =
+            s.definitions.iter().filter(|d| d.name == "greet").collect();
+        assert_eq!(
+            greet_defs.len(),
+            1,
+            "expected exactly one `greet` definition, got {greet_defs:#?}",
+        );
+        assert_eq!(greet_defs[0].kind, "function");
+    }
+
+    #[test]
+    fn set_method_emits_call_to_generic() {
+        // setMethod registers an implementation of the generic. The fix emits
+        // a call edge to the generic so the dispatch relationship is visible
+        // in the graph.
+        let s = parse_r(
+            "setMethod(\"greet\", \"Person\", function(x) paste(\"Hello\", x@name))\n",
+        );
+        let calls: Vec<&Call> = s.calls.iter().filter(|c| c.name == "greet").collect();
+        assert_eq!(calls.len(), 1, "expected setMethod to emit one call to `greet`");
+    }
+
+    #[test]
+    fn set_method_body_calls_are_still_captured() {
+        // The recursive walk visits the anonymous function passed to
+        // setMethod, so calls inside the method body must still appear.
+        let s = parse_r(
+            "setMethod(\"greet\", \"Person\", function(x) { helper(x); validate(x) })\n",
+        );
+        let names: Vec<&str> = s.calls.iter().map(|c| c.name.as_str()).collect();
+        assert!(names.contains(&"helper"), "method body call `helper` not captured");
+        assert!(names.contains(&"validate"), "method body call `validate` not captured");
+    }
+
     #[test]
     fn function_with_double_arrow_assignment() {
         // `<<-` is super-assignment in R; the JS extractor accepts it too.

src/domain/graph/builder/pipeline.ts

@@ -658,6 +658,14 @@ async function tryNativeOrchestrator(
 
   if (result.earlyExit) {
     info('No changes detected');
+    // Even on no-op rebuilds, dropped-language files added since the last
+    // full build are still missing from `nodes`/`file_hashes` (#1083). The
+    // orchestrator's file_collector skipped them, so its earlyExit doesn't
+    // imply DB consistency. Run the gap repair before returning.
+    const gap = detectDroppedLanguageGap(ctx);
+    if (gap.missingAbs.length > 0) {
+      await backfillNativeDroppedFiles(ctx, gap);
+    }
     closeDbPair({ db: ctx.db, nativeDb: ctx.nativeDb });
     return 'early-exit';
   }
@@ -753,37 +761,55 @@ async function tryNativeOrchestrator(
   // stale native binaries). WASM handles those — backfill via WASM so both
   // engines process the same file set (#967).
   //
-  // Runs on full builds and on incrementals when the orchestrator reports
-  // any file activity (removals or changes). The orchestrator's
-  // `detect_removed_files` filter (#1070) skips files outside its narrower
-  // file_collector, so on a current binary a no-op rebuild reports
-  // `removedCount=0` and `changedCount=0`, making the backfill call pure
-  // overhead (fs walk + 2 DB queries + 48-file WASM re-parse). Legacy
-  // binaries lacking the filter still report `removedCount>0` and get the
-  // gap-repair behavior #1068 introduced. Triggering on `changedCount>0`
-  // narrows (but does not fully close) the gap where a brand-new
-  // unsupported-extension file is added on an otherwise-quiet incremental
-  // — see #1091 for the residual gap.
+  // Detect the gap once (fs walk + 2 DB queries, ~20–30ms) and use it for
+  // both gating and the backfill itself. On dirty incrementals/full builds
+  // the orchestrator signals trigger backfill, so the walk happens once
+  // (instead of redundantly inside backfill). On quiet incrementals we
+  // still pay the walk so we can detect brand-new files in dropped-language
+  // extensions — a gap that the orchestrator's `detect_removed_files`
+  // filter (#1070) leaves open (#1083, #1091). The pre-check is cheap
+  // because the expensive part (WASM re-parse of the missing set) is
+  // gated below.
   const removedCount = result.removedCount ?? 0;
   const changedCount = result.changedCount ?? 0;
-  if (result.isFullBuild || removedCount > 0 || changedCount > 0) {
-    await backfillNativeDroppedFiles(ctx);
+  const gap = detectDroppedLanguageGap(ctx);
+  if (result.isFullBuild || removedCount > 0 || changedCount > 0 || gap.missingAbs.length > 0) {
+    await backfillNativeDroppedFiles(ctx, gap);
   }
 
   closeDbPair({ db: ctx.db, nativeDb: ctx.nativeDb });
   return formatNativeTimingResult(p, structurePatchMs, analysisTiming);
 }
 
+/** Files the native orchestrator silently dropped — the working set for backfill. */
+interface DroppedLanguageGap {
+  /** Relative paths (normalized) of files missing from `nodes` or `file_hashes`. */
+  missingRel: string[];
+  /** Absolute paths, aligned by index with `missingRel`. */
+  missingAbs: string[];
+}
+
 /**
- * Backfill files that the native orchestrator silently dropped during parse.
- * Falls back to WASM + inserts file/symbol nodes so engine counts match (#967).
+ * Detect files the native orchestrator silently dropped.
+ *
+ * Walks the filesystem and compares against `nodes` + `file_hashes`. A file
+ * is "missing" if it's absent from EITHER table — both must be present for
+ * the fast-skip pre-flight (#1054) to work, and the two can diverge (e.g.
+ * legacy DBs where `nodes` was populated but `file_hashes` was not).
+ *
+ * Restricted to files with an installed WASM grammar; extensions in
+ * `LANGUAGE_REGISTRY` without a shipped grammar (e.g. groovy on minimal
+ * installs) can't be parsed by either engine, so they're not a native
+ * regression — excluding them keeps the warn count in
+ * `backfillNativeDroppedFiles` meaningful.
+ *
+ * Cheap (no DB handoff, no parsing): used both to gate the backfill call
+ * and as its working set. NativeDbProxy supports `.prepare().all()`, so
+ * this works whether `ctx.db` is a proxy or a real better-sqlite3
+ * connection — letting us skip the close-native / reopen-better-sqlite3
+ * cost when there's nothing to backfill.
  */
-async function backfillNativeDroppedFiles(ctx: PipelineContext): Promise<void> {
-  // Compute the missing-file set FIRST, before any expensive DB handoff.
-  // NativeDbProxy supports .prepare().all(), so the upfront query works
-  // whether ctx.db is a proxy or a real better-sqlite3 connection. On
-  // incremental no-op rebuilds nothing is missing, so we want to early-return
-  // without paying the close-native / reopen-better-sqlite3 cost.
+function detectDroppedLanguageGap(ctx: PipelineContext): DroppedLanguageGap {
   const collected = collectFilesUtil(ctx.rootDir, [], ctx.config, new Set<string>());
   const expected = new Set(
     collected.files.map((f) => normalizePath(path.relative(ctx.rootDir, f))),
@@ -794,12 +820,6 @@ async function backfillNativeDroppedFiles(ctx: PipelineContext): Promise<void> {
     .all() as Array<{ file: string }>;
   const existingNodes = new Set(existingNodeRows.map((r) => r.file));
 
-  // Belt-and-suspenders: also check `file_hashes`. The fast-skip pre-flight
-  // (#1054) rejects on `file_hashes` gaps, and the two tables can diverge
-  // (e.g. a DB written by old code where `nodes` was populated but
-  // `file_hashes` was not). Treating "in nodes but not in file_hashes" as
-  // missing closes the gap so the backfill repairs the file_hashes row even
-  // when the node row already exists.
   let existingHashes = new Set<string>();
   try {
     const existingHashRows = ctx.db
@@ -810,26 +830,36 @@ async function backfillNativeDroppedFiles(ctx: PipelineContext): Promise<void> {
     // file_hashes table may not exist on legacy DBs; treat as fully missing
     // so the backfill writes rows on the upsert path below.
     debug(
-      `backfillNativeDroppedFiles: file_hashes read failed (table may not exist): ${toErrorMessage(e)}`,
+      `detectDroppedLanguageGap: file_hashes read failed (table may not exist): ${toErrorMessage(e)}`,
     );
   }
 
-  // Restrict backfill to files with an installed WASM grammar. Extensions in
-  // LANGUAGE_REGISTRY without a shipped grammar file (e.g. groovy, erlang on
-  // minimal installs) can't be parsed by either engine, so they're not a
-  // native regression — excluding them keeps the warn count meaningful.
   const installedExts = getInstalledWasmExtensions();
   const missingRel: string[] = [];
   const missingAbs: string[] = [];
   for (const rel of expected) {
-    // A file is "missing" if it's absent from EITHER nodes OR file_hashes.
-    // Both must be present for fast-skip to work correctly.
     if (existingNodes.has(rel) && existingHashes.has(rel)) continue;
     const ext = path.extname(rel).toLowerCase();
     if (!installedExts.has(ext)) continue;
     missingRel.push(rel);
     missingAbs.push(path.join(ctx.rootDir, rel));
   }
+  return { missingRel, missingAbs };
+}
+
+/**
+ * Backfill files that the native orchestrator silently dropped during parse.
+ * Falls back to WASM + inserts file/symbol nodes so engine counts match (#967).
+ *
+ * Accepts a pre-computed `gap` from `detectDroppedLanguageGap` so the caller
+ * can use the same scan for both gating and the actual backfill — avoiding
+ * a redundant fs walk when the orchestrator's signals already triggered.
+ */
+async function backfillNativeDroppedFiles(
+  ctx: PipelineContext,
+  gap: DroppedLanguageGap,
+): Promise<void> {
+  const { missingRel, missingAbs } = gap;
   if (missingAbs.length === 0) return;
 
   // Now that we know there's work to do, hand off to better-sqlite3 (needed

src/extractors/r.ts

@@ -125,11 +125,16 @@ function handleCall(node: TreeSitterNode, ctx: ExtractorOutput): void {
     return;
   }
 
-  if (funcName === 'setGeneric' || funcName === 'setMethod') {
+  if (funcName === 'setGeneric') {
     handleSetGeneric(node, ctx);
     return;
   }
 
+  if (funcName === 'setMethod') {
+    handleSetMethod(node, ctx);
+    return;
+  }
+
   // Regular call
   if (funcNode.type === 'identifier') {
     ctx.calls.push({ name: funcName, line: node.startPosition.row + 1 });
@@ -212,63 +217,77 @@ function handleLibraryCall(node: TreeSitterNode, ctx: ExtractorOutput): void {
 }
 
 function handleSourceCall(node: TreeSitterNode, ctx: ExtractorOutput): void {
-  for (let i = 0; i < node.childCount; i++) {
-    const child = node.child(i);
-    if (!child || child.type !== 'arguments') continue;
-    for (let j = 0; j < child.childCount; j++) {
-      const arg = child.child(j);
-      if (!arg) continue;
-      if (arg.type === 'string') {
-        const text = arg.text.replace(/^["']|["']$/g, '');
-        ctx.imports.push({
-          source: text,
-          names: ['source'],
-          line: node.startPosition.row + 1,
-        });
-        return;
-      }
-    }
-  }
+  // source() only accepts string literals — `source(varname)` is not an import.
+  const path = firstStringArgument(node);
+  if (path === null) return;
+  ctx.imports.push({
+    source: path,
+    names: ['source'],
+    line: node.startPosition.row + 1,
+  });
 }
 
 function handleSetClass(node: TreeSitterNode, ctx: ExtractorOutput): void {
-  for (let i = 0; i < node.childCount; i++) {
-    const child = node.child(i);
-    if (!child || child.type !== 'arguments') continue;
-    for (let j = 0; j < child.childCount; j++) {
-      const arg = child.child(j);
-      if (!arg) continue;
-      if (arg.type === 'string') {
-        const name = arg.text.replace(/^["']|["']$/g, '');
-        ctx.definitions.push({
-          name,
-          kind: 'class',
-          line: node.startPosition.row + 1,
-          endLine: nodeEndLine(node),
-        });
-        return;
-      }
-    }
-  }
+  const name = firstStringArgument(node);
+  if (name === null) return;
+  ctx.definitions.push({
+    name,
+    kind: 'class',
+    line: node.startPosition.row + 1,
+    endLine: nodeEndLine(node),
+  });
 }
 
 function handleSetGeneric(node: TreeSitterNode, ctx: ExtractorOutput): void {
+  const name = firstStringArgument(node);
+  if (name === null) return;
+  ctx.definitions.push({
+    name,
+    kind: 'function',
+    line: node.startPosition.row + 1,
+    endLine: nodeEndLine(node),
+  });
+}
+
+// setMethod("greet", "Person", function(x) ...) registers an implementation of
+// the generic `greet` — it is not a new top-level definition. Emitting a
+// definition here produced two `function` nodes with the same name (one from
+// setGeneric, one from setMethod) and broke resolution. Emit a call edge to
+// the generic instead; the method body's calls are still picked up by the
+// recursive walk of the anonymous function argument.
+function handleSetMethod(node: TreeSitterNode, ctx: ExtractorOutput): void {
+  const name = firstStringArgument(node);
+  if (name === null) return;
+  ctx.calls.push({ name, line: node.startPosition.row + 1 });
+}
+
+// tree-sitter-r wraps each positional argument in an `argument` node that
+// contains the actual `string` (or `identifier`) child, so the inner string
+// must be unwrapped — checking `child.type === 'string'` directly misses it.
+// Mirrors `first_argument_value` in the Rust extractor for parity.
+function firstStringArgument(node: TreeSitterNode): string | null {
   for (let i = 0; i < node.childCount; i++) {
     const child = node.child(i);
     if (!child || child.type !== 'arguments') continue;
     for (let j = 0; j < child.childCount; j++) {
       const arg = child.child(j);
       if (!arg) continue;
       if (arg.type === 'string') {
-        const name = arg.text.replace(/^["']|["']$/g, '');
-        ctx.definitions.push({
-          name,
-          kind: 'function',
-          line: node.startPosition.row + 1,
-          endLine: nodeEndLine(node),
-        });
-        return;
+        return stripQuotes(arg.text);
+      }
+      if (arg.type === 'argument') {
+        const valueNode = arg.childForFieldName('value');
+        if (valueNode && valueNode.type === 'string') return stripQuotes(valueNode.text);
+        for (let k = 0; k < arg.childCount; k++) {
+          const inner = arg.child(k);
+          if (inner && inner.type === 'string') return stripQuotes(inner.text);
+        }
       }
     }
   }
+  return null;
+}
+
+function stripQuotes(text: string): string {
+  return text.replace(/^["']|["']$/g, '');
 }