diff --git a/crates/codegraph-core/src/extractors/javascript.rs b/crates/codegraph-core/src/extractors/javascript.rs index daf2ff9d..e5c73d2e 100644 --- a/crates/codegraph-core/src/extractors/javascript.rs +++ b/crates/codegraph-core/src/extractors/javascript.rs @@ -2245,12 +2245,22 @@ fn extract_implements_depth(node: &Node, source: &[u8], result: &mut Vec } } -/// Callee names that idiomatically accept callback references. Member-expression -/// args (e.g. `auth.validate`) are only emitted as dynamic callback calls when -/// the callee is in this set; otherwise plain property reads passed as data -/// (`store.set(user.id, user)`) would emit spurious `id` calls with receiver -/// `user`. Identifier args are always emitted — collateral damage from dropping -/// them outweighs the FP risk for plain identifier data args. +/// Callee names that idiomatically accept callback references. Both identifier +/// (e.g. `handleToken`) and member-expression (e.g. `auth.validate`) args are +/// only emitted as dynamic callback calls when the callee is in this set; +/// otherwise plain values passed as data (`store.set(user.id, user)`, +/// `findMergeCandidates(communities)`) would emit spurious calls — e.g. `id` +/// with receiver `user`, or a fabricated edge to an unrelated same-named +/// function (issue #1741). +/// +/// Known gap: arbitrary user-defined higher-order functions (e.g. +/// `processEach(users, fn: UserProcessor)`) are neither name-allowlisted nor +/// position-mapped (see `positional_callback_arg_index`), so `processEach(users, +/// logUser)` no longer emits a `logUser` reference from the caller. +/// Recognizing these would require looking at the callee's own parameter type +/// (is it function-shaped?) rather than its name — a resolver-level, +/// cross-engine feature, not a small extension of this gate. Tracked as a +/// follow-up. /// /// Mirrors `CALLBACK_ACCEPTING_CALLEES` in `src/extractors/javascript.ts`. const CALLBACK_ACCEPTING_CALLEES: &[&str] = &[ @@ -2285,6 +2295,39 @@ const HTTP_VERB_CALLEES: &[&str] = &[ "get", "post", "put", "delete", "patch", "options", "head", "all", ]; +/// Callees whose callback argument sits at one specific positional index +/// rather than "any position" (the assumption behind `CALLBACK_ACCEPTING_CALLEES`, +/// needed for variadic Express/Router middleware chains like +/// `app.get(path, mw1, mw2, handler)`). +/// +/// `Array.from(arrayLike, mapFn, thisArg)` (also every TypedArray constructor, +/// e.g. `Uint8Array.from`) is the motivating case: `arrayLike` (index 0) is +/// plain data — treating it as a callback candidate would reintroduce the +/// exact name-collision false-positive class issue #1741 fixes — while +/// `mapFn` (index 1) is a genuine callback reference that should still +/// resolve. A callee listed here is implicitly callback-accepting (no +/// separate `CALLBACK_ACCEPTING_CALLEES` entry needed); only the arg at its +/// listed index is eligible. +/// +/// Invariant: this map and `CALLBACK_ACCEPTING_CALLEES` must stay disjoint. +/// A callee name present in both would have its any-position intent silently +/// narrowed to the single listed index (positional wins — see the gate in +/// `extract_callback_reference_calls`), with no error or warning. +/// +/// Name-based, not receiver-typed, so it can't distinguish `Array.from(x, +/// mapFn)` from an unrelated `.from(x, y)` shaped differently (e.g. +/// `Buffer.from(data, encoding)`) — that residual risk is far narrower than +/// the unconditional-emission bug this gate fixes, so it's accepted rather +/// than adding receiver-type tracking. +/// +/// Mirrors `POSITIONAL_CALLBACK_ARG_INDEX` in `src/extractors/javascript.ts`. +fn positional_callback_arg_index(callee_name: &str) -> Option { + match callee_name { + "from" => Some(1), + _ => None, + } +} + /// Extract the callee's final name (function identifier or member expression /// property) for callback-eligibility filtering. Returns `None` if the callee /// shape is not analyzable (e.g. computed subscripts, IIFEs). @@ -2327,22 +2370,35 @@ fn extract_callback_reference_calls(call_node: &Node, source: &[u8], calls: &mut if matches!(callee_name, Some("call") | Some("apply") | Some("bind")) { return; } - let mut member_expr_args_allowed = callee_name + let mut callback_args_allowed = callee_name .map(|n| CALLBACK_ACCEPTING_CALLEES.contains(&n)) .unwrap_or(false); - if member_expr_args_allowed { + if callback_args_allowed { if let Some(name) = callee_name { if HTTP_VERB_CALLEES.contains(&name) { // HTTP verbs require a string-literal route path to be treated as a // callback-accepting API; otherwise `cache.get(user.id)` etc. would // still emit `id` as a dynamic call. - member_expr_args_allowed = first_arg_is_string_literal(&args); + callback_args_allowed = first_arg_is_string_literal(&args); } } } - for i in 0..args.child_count() { - let Some(child) = args.child(i) else { continue }; + let positional_index = callee_name.and_then(positional_callback_arg_index); + if !callback_args_allowed && positional_index.is_none() { + return; + } + + for (arg_index, child) in iter_children(&args, PUNCTUATION_TOKENS).enumerate() { + // A positional entry restricts eligibility to its one designated + // index, regardless of what the generic (any-position) gate above + // decided. + if let Some(idx) = positional_index { + if arg_index != idx { + continue; + } + } + match child.kind() { "identifier" => { calls.push(Call { @@ -2353,7 +2409,7 @@ fn extract_callback_reference_calls(call_node: &Node, source: &[u8], calls: &mut ..Default::default() }); } - "member_expression" if member_expr_args_allowed => { + "member_expression" => { if let Some(prop) = child.child_by_field_name("property") { let receiver = child.child_by_field_name("object") .map(|obj| extract_receiver_name(&obj, source)); @@ -4195,6 +4251,148 @@ mod tests { assert_eq!(cb.unwrap().receiver.as_deref(), Some("handlers")); } + #[test] + fn no_identifier_callback_for_non_allowlisted_callee_issue_1741() { + // Regression guard for #1741: `findMergeCandidates(communities)` and + // `analyzeDrift(communities, communityDirs)` pass `communities` as a + // plain DATA argument, not a callback reference. Neither + // `findMergeCandidates` nor `analyzeDrift` is a callback-accepting + // callee, so identifier args must be gated exactly like + // member_expression args — otherwise the global-fallback resolver + // can bind the identifier to an unrelated same-named function + // elsewhere in the repo, fabricating a call edge (and, transitively, + // a phantom cycle — see codegraph's own src/features/communities.ts + // vs src/presentation/communities.ts). + let s = parse_js("findMergeCandidates(communities);"); + assert!( + !s.calls.iter().any(|c| c.dynamic == Some(true) && c.name == "communities"), + "findMergeCandidates non-allowlisted callee must not emit `communities` as dynamic call; got: {:?}", + s.calls, + ); + + let s2 = parse_js("analyzeDrift(communities, communityDirs);"); + assert!( + !s2.calls.iter().any(|c| c.dynamic == Some(true)), + "analyzeDrift non-allowlisted callee must not emit any dynamic calls; got: {:?}", + s2.calls, + ); + } + + #[test] + fn emits_identifier_callback_for_allowlisted_callee_issue_1741() { + // Positive companion to the #1741 fix: identifier args passed to a + // genuine callback-accepting callee must still be resolved, e.g. + // `arr.forEach(myNamedCallback)` — the exact pattern the original + // "identifier args are always emitted" trade-off existed to preserve. + let s = parse_js("arr.forEach(myNamedCallback);"); + assert!( + s.calls.iter().any(|c| c.dynamic == Some(true) && c.name == "myNamedCallback"), + "arr.forEach must still emit myNamedCallback as dynamic call; got: {:?}", + s.calls, + ); + } + + #[test] + fn no_identifier_callback_for_cache_or_map_get() { + // Identifier-arg counterpart to `no_member_expr_callback_for_cache_or_map_get`: + // `cache.get(someKey)` shares the verb name `get` with Express routes + // but has no string-literal route path first arg, so the identifier + // arg must not be emitted as a dynamic call either. + let s = parse_js("cache.get(someKey);"); + assert!( + !s.calls.iter().any(|c| c.dynamic == Some(true) && c.name == "someKey"), + "cache.get(someKey) must not emit `someKey` as dynamic call; got: {:?}", + s.calls, + ); + } + + #[test] + fn emits_array_from_mapfn_but_not_arraylike() { + // Regression guard for #1741 follow-up: `Array.from(arrayLike, mapFn)` is + // a well-known stdlib callback pattern (also every TypedArray.from), but + // the callback is the SECOND positional argument, not the first. Emitting + // `arrayLike` too would reintroduce the exact name-collision false-positive + // class #1741 fixes for the data argument; only `mapFn` should resolve. + let s = parse_js("Array.from(arr, mapCallback);"); + assert!( + s.calls.iter().any(|c| c.dynamic == Some(true) && c.name == "mapCallback"), + "Array.from(arr, mapCallback) must emit mapCallback as dynamic call; got: {:?}", + s.calls, + ); + assert!( + !s.calls.iter().any(|c| c.dynamic == Some(true) && c.name == "arr"), + "Array.from(arr, mapCallback) must not emit `arr` (index 0) as dynamic call; got: {:?}", + s.calls, + ); + } + + #[test] + fn emits_only_index_one_for_array_from_with_this_arg() { + // `Array.from(arrayLike, mapFn, thisArg)` — thisArg (index 2) is a `this` + // binding context, not a callback, and must not be emitted either. + let s = parse_js("Array.from(arr, mapCallback, thisArg);"); + let dynamic_names: Vec<&str> = s.calls.iter() + .filter(|c| c.dynamic == Some(true)) + .map(|c| c.name.as_str()) + .collect(); + assert_eq!( + dynamic_names, vec!["mapCallback"], + "only index-1 mapCallback should be dynamic; got: {:?}", s.calls, + ); + } + + #[test] + fn applies_array_from_positional_gate_to_typed_array_constructors() { + // Every TypedArray constructor (Uint8Array, Int32Array, etc.) mirrors + // Array.from's (arrayLike, mapFn, thisArg) signature; the gate is + // name-based on the property `from`, not receiver-typed, so it applies + // uniformly. + let s = parse_js("Uint8Array.from(arr, mapCallback);"); + assert!( + s.calls.iter().any(|c| c.dynamic == Some(true) && c.name == "mapCallback"), + "Uint8Array.from(arr, mapCallback) must emit mapCallback; got: {:?}", + s.calls, + ); + assert!( + !s.calls.iter().any(|c| c.dynamic == Some(true) && c.name == "arr"), + "Uint8Array.from(arr, mapCallback) must not emit `arr`; got: {:?}", + s.calls, + ); + } + + #[test] + fn applies_array_from_positional_gate_to_member_expression_args_too() { + // Mirrors the TS test of the same intent: the old member_expression + // guard was an explicit `&& memberExprArgsAllowed` inline check; the + // positional restructuring moved that responsibility to the shared + // early-return above the loop. `Array.from(arr, obj.mapper)` exercises + // that a member_expression at the positional index (1) is still + // emitted with its receiver, while one at index 0 is not. + let s = parse_js("Array.from(arr, obj.mapper);"); + assert!( + s.calls.iter().any(|c| c.dynamic == Some(true) && c.name == "mapper" && c.receiver.as_deref() == Some("obj")), + "Array.from(arr, obj.mapper) must emit mapper with receiver obj; got: {:?}", + s.calls, + ); + assert!( + !s.calls.iter().any(|c| c.dynamic == Some(true) && c.name == "arr"), + "Array.from(arr, obj.mapper) must not emit `arr` (index 0); got: {:?}", + s.calls, + ); + + let s2 = parse_js("Array.from(obj.arrayLike, mapCallback);"); + assert!( + !s2.calls.iter().any(|c| c.dynamic == Some(true) && c.name == "arrayLike"), + "Array.from(obj.arrayLike, mapCallback) must not emit `arrayLike` (index 0); got: {:?}", + s2.calls, + ); + assert!( + s2.calls.iter().any(|c| c.dynamic == Some(true) && c.name == "mapCallback"), + "Array.from(obj.arrayLike, mapCallback) must emit mapCallback; got: {:?}", + s2.calls, + ); + } + #[test] fn no_dynamic_call_for_dynamic_import_arg() { // Parity with TS walk path: callback-reference extraction must be skipped diff --git a/src/extractors/javascript.ts b/src/extractors/javascript.ts index 65326e36..b6c43417 100644 --- a/src/extractors/javascript.ts +++ b/src/extractors/javascript.ts @@ -3378,15 +3378,21 @@ function extractSubscriptCallInfo(fn: TreeSitterNode, callNode: TreeSitterNode): /** * Callee names that idiomatically accept callback references. Used to gate - * member_expression args in {@link extractCallbackReferenceCalls}: arguments - * like `user.id` are only emitted as dynamic callback calls when the callee - * is a known callback-accepting API (router/middleware, promises, array - * methods, event emitters, scheduling APIs). This avoids false positives - * from plain property reads passed as data, e.g. `store.set(user.id, user)`. + * both identifier and member_expression args in + * {@link extractCallbackReferenceCalls}: arguments are only emitted as + * dynamic callback calls when the callee is a known callback-accepting API + * (router/middleware, promises, array methods, event emitters, scheduling + * APIs). This avoids false positives from plain values passed as data, e.g. + * `store.set(user.id, user)` or `findMergeCandidates(communities)`. * - * Identifier args (e.g. `router.use(handleToken)`) are always emitted — the - * collateral damage of dropping them is larger than the FP risk, since plain - * identifier data args rarely collide with real function names. + * Identifier args used to be exempted from this gate on the theory that + * plain identifier data args rarely collide with real function names — but + * issue #1741 found a concrete counter-example (`analyzeDrift(communities, + * communityDirs)` colliding with the unrelated `communities` CLI command), + * which the global-fallback resolver then bound into a fabricated call edge + * (and, transitively, a phantom cycle). Gating identifiers the same way + * removes that FP class while still preserving legitimate callback-by- + * reference patterns like `arr.forEach(myCallback)`. */ const CALLBACK_ACCEPTING_CALLEES: ReadonlySet = new Set([ // Express / router / middleware @@ -3462,6 +3468,36 @@ const HTTP_VERB_CALLEES: ReadonlySet = new Set([ 'all', ]); +/** + * Callees whose callback argument sits at one specific positional index + * rather than "any position" (the assumption behind {@link CALLBACK_ACCEPTING_CALLEES}, + * needed for variadic Express/Router middleware chains like + * `app.get(path, mw1, mw2, handler)`). + * + * `Array.from(arrayLike, mapFn, thisArg)` (also `Int8Array.from`, `Uint8Array.from`, + * etc. — every TypedArray constructor mirrors the same signature) is the + * motivating case: `arrayLike` (index 0) is plain data — treating it as a + * callback candidate would reintroduce the exact name-collision false-positive + * class issue #1741 fixes — while `mapFn` (index 1) is a genuine callback + * reference that should still resolve. A callee listed here is implicitly + * callback-accepting (no separate {@link CALLBACK_ACCEPTING_CALLEES} entry + * needed); only the arg at its listed index is eligible. + * + * Invariant: this map and {@link CALLBACK_ACCEPTING_CALLEES} must stay + * disjoint. A callee name present in both would have its any-position intent + * silently narrowed to the single listed index (positional wins — see the + * gate in {@link extractCallbackReferenceCalls}), with no error or warning. + * + * This is name-based, not receiver-typed (consistent with the rest of this + * gate), so it can't distinguish `Array.from(x, mapFn)` from an unrelated + * `.from(x, y)` on some other object shaped differently — e.g. `Buffer.from(data, + * encoding)`, where `encoding` is conventionally a string but could in principle + * be a colliding identifier. That residual risk is far narrower than the + * unconditional-emission bug this gate fixes, so it's accepted rather than + * adding receiver-type tracking here. + */ +const POSITIONAL_CALLBACK_ARG_INDEX: ReadonlyMap = new Map([['from', 1]]); + /** * Extract the callee's final name (function identifier or member expression * property) for callback-eligibility filtering. Returns null if the callee @@ -3505,21 +3541,33 @@ function firstArgIsStringLiteral(argsNode: TreeSitterNode): boolean { * `app.use(auth.validate)` yields a call to validate with receiver auth. * Skips literals, objects, arrays, anonymous functions, and call expressions (already handled). * - * To avoid false positives where plain property reads are passed as data - * (e.g. `store.set(user.id, user)` — `user.id` is a value, not a callback), - * member_expression args are only emitted when the callee is in - * {@link CALLBACK_ACCEPTING_CALLEES}. Identifier args are always emitted. + * To avoid false positives where plain values are passed as data (e.g. + * `store.set(user.id, user)` — `user.id` is a value, not a callback; or + * `findMergeCandidates(communities)` — `communities` is a data argument, not + * a callback), both identifier and member_expression args are only emitted + * when the callee is in {@link CALLBACK_ACCEPTING_CALLEES}, or the argument + * sits at the specific index a {@link POSITIONAL_CALLBACK_ARG_INDEX} entry + * designates (e.g. `Array.from(arrayLike, mapFn)` — only index 1 is eligible; + * `arrayLike` at index 0 stays ungated data). * * HTTP-verb callees (`get`, `post`, `put`, `delete`, `patch`, `options`, * `head`, `all`) double as Map/cache/repository method names, so their - * member-expr args are only emitted when the first argument is a string - * literal route path — matching Express/router shape and skipping - * `cache.get(user.id)`-style calls. + * args are only emitted when the first argument is a string literal route + * path — matching Express/router shape and skipping `cache.get(user.id)`-style + * calls. * * `.call()` / `.apply()` / `.bind()` — the first arg is the `this` context (not a callback of * the enclosing function) and subsequent args flow into the delegated function's parameters. * Emitting them here would produce false-positive edges from the *calling* function. * This-rebinding (fn::this → ctx) is handled separately by extractThisCallBindingsWalk. + * + * Known gap: arbitrary user-defined higher-order functions (e.g. + * `processEach(users, fn: UserProcessor)`) are neither name-allowlisted nor + * position-mapped, so `processEach(users, logUser)` no longer emits a + * `logUser` reference from the caller. Recognizing these would require + * looking at the callee's own parameter type (is it function-shaped?) rather + * than its name — a resolver-level, cross-engine feature, not a small + * extension of this name/position gate. Tracked as a follow-up. */ function extractCallbackReferenceCalls(callNode: TreeSitterNode): Call[] { const args = callNode.childForFieldName('arguments') || findChild(callNode, 'arguments'); @@ -3532,24 +3580,36 @@ function extractCallbackReferenceCalls(callNode: TreeSitterNode): Call[] { // This-rebinding (fn::this → ctx) is handled separately by extractThisCallBindingsWalk. if (calleeName === 'call' || calleeName === 'apply' || calleeName === 'bind') return []; - let memberExprArgsAllowed = calleeName !== null && CALLBACK_ACCEPTING_CALLEES.has(calleeName); - if (memberExprArgsAllowed && calleeName !== null && HTTP_VERB_CALLEES.has(calleeName)) { + let callbackArgsAllowed = calleeName !== null && CALLBACK_ACCEPTING_CALLEES.has(calleeName); + if (callbackArgsAllowed && calleeName !== null && HTTP_VERB_CALLEES.has(calleeName)) { // HTTP verbs require a string-literal route path to be treated as a // callback-accepting API; otherwise `cache.get(user.id)` etc. would // still emit `id` as a dynamic call. - memberExprArgsAllowed = firstArgIsStringLiteral(args); + callbackArgsAllowed = firstArgIsStringLiteral(args); } + const positionalIndex = + calleeName !== null ? POSITIONAL_CALLBACK_ARG_INDEX.get(calleeName) : undefined; + if (!callbackArgsAllowed && positionalIndex === undefined) return []; + const result: Call[] = []; const callLine = nodeStartLine(callNode); + let argIndex = -1; for (let i = 0; i < args.childCount; i++) { const child = args.child(i); if (!child) continue; + const t = child.type; + if (t === '(' || t === ')' || t === ',') continue; + argIndex++; - if (child.type === 'identifier') { + // A positional entry restricts eligibility to its one designated index, + // regardless of what the generic (any-position) gate above decided. + if (positionalIndex !== undefined && argIndex !== positionalIndex) continue; + + if (t === 'identifier') { result.push({ name: child.text, line: callLine, dynamic: true }); - } else if (child.type === 'member_expression' && memberExprArgsAllowed) { + } else if (t === 'member_expression') { const prop = child.childForFieldName('property'); const obj = child.childForFieldName('object'); if (prop) { diff --git a/tests/benchmarks/resolution/jelly-micro.test.ts b/tests/benchmarks/resolution/jelly-micro.test.ts index 75744454..b4b8b4cd 100644 --- a/tests/benchmarks/resolution/jelly-micro.test.ts +++ b/tests/benchmarks/resolution/jelly-micro.test.ts @@ -65,11 +65,24 @@ function discoverTests(): string[] { * * Note: more1 was moved to the pts-javascript fixture set in #1383 and is * no longer part of jelly-micro. + * + * `classes` lowered 6/31 → 5/31 by #1741: `f4(x) { return x(f1); }` / + * `f4(f4)` only matched Jelly's `f4 -> f1` ground truth because the + * pre-#1741 extractor emitted a dynamic call for *every* bare identifier + * argument (here, `f1` in `x(f1)`), with no gating on the callee. `x` is an + * unresolvable parameter call — syntactically indistinguishable from a + * plain data argument like `findMergeCandidates(communities)` — so + * recognizing this specific edge would require points-to/interprocedural + * analysis (tracking that `f4(f4)` aliases `x` to `f4`, which recursively + * aliases to `f1`), not a local callee-name gate. #1741 gates identifier + * args on CALLBACK_ACCEPTING_CALLEES the same way member_expression args + * already are, trading this one coincidental match for eliminating a class + * of fabricated cross-file call edges (and phantom cycles). */ const RECALL_FLOORS: Record = { accessors3: 1.0, // 1/1 arguments: 1.0, // 1/1 - classes: 0.19, // 6/31 + classes: 0.16, // 5/31 (was 6/31 — see #1741 note above) defineProperty: 0.5, // 3/6 fun: 1.0, // 4/4 generators: 1.0, // 9/9 diff --git a/tests/benchmarks/resolution/resolution-benchmark.test.ts b/tests/benchmarks/resolution/resolution-benchmark.test.ts index 0a07b340..ccc14aba 100644 --- a/tests/benchmarks/resolution/resolution-benchmark.test.ts +++ b/tests/benchmarks/resolution/resolution-benchmark.test.ts @@ -155,6 +155,14 @@ const THRESHOLDS: Record = { // TS 0.72: Phase 8.3e adds this.method() same-class resolution (Shape.describe → Shape.area), // lifting recall from 69.4% to 72.2%. Remaining gap (interface-dispatch, CHA) is tracked // in Phase 8.5 (TSC enrichment) and Phase 8.7 (CHA on JS/TS). + // #1741 (identifier-arg callback gating) intentionally drops 3 callback-mode edges + // (runCallbackDemo -> logUser/upperUser/hasEmail in callbacks.ts — bare identifiers + // passed to the project-defined higher-order functions processEach/filterThen, which + // aren't and can't be in a name allowlist). Actual recall is now 44/47 (93.6%), still + // well above this floor — the 3 edges stay in expected-edges.json because they're real, + // decidable facts, not fabricated ones. Recognizing them needs the callee's own parameter + // type (function-shaped?), not its name; tracked as a follow-up (#1845) rather than + // expanding the name/position gate in #1741. typescript: { precision: 0.85, recall: 0.72 }, tsx: { precision: 0.85, recall: 0.8 }, // TODO: raise thresholds once bash call resolution is implemented diff --git a/tests/engines/parity.test.ts b/tests/engines/parity.test.ts index f9c5b5c4..83b54e83 100644 --- a/tests/engines/parity.test.ts +++ b/tests/engines/parity.test.ts @@ -187,6 +187,42 @@ cache.get(user.id); router.get('/users/:id', auth.check); promise.then(handlers.onSuccess); emitter?.on('tick', handlers.fn); +`, + }, + { + // Regression guard for #1741: native must gate IDENTIFIER args by + // CALLBACK_ACCEPTING_CALLEES the same way it already gates + // member_expression args. Without the gate, native (like WASM used to) + // over-emits dynamic calls for identifier args of non-allowlisted + // callees (e.g. `findMergeCandidates(communities)` → bogus call to + // `communities`), which the global-fallback resolver can then bind to + // an unrelated same-named function elsewhere in the repo. + // 1. non-allowlisted callee → drop identifier arg + // 2. cache/Map .get → drop (HTTP verb without string-literal path) + // 3. array .forEach → keep (always-allowlisted callback API) + // 4. router HTTP route → keep (HTTP verb WITH string-literal path) + name: 'JavaScript — identifier-arg callback gating must agree between engines', + file: 'identifier-callbacks.js', + code: ` +findMergeCandidates(communities); +cache.get(someKey); +arr.forEach(myNamedCallback); +router.get('/users/:id', authHandler); +`, + }, + { + // Regression guard for #1741 follow-up: native must apply the same + // positional (not just name-based) gate as WASM for Array.from-shaped + // callees. Without it, native either over-emits `arr`/`thisArg` (data + // args, reintroducing the name-collision FP class #1741 fixes) or + // under-emits `mapCallback` (a genuine, well-known stdlib callback + // reference), depending on which side of the bug it lands on. + name: 'JavaScript — Array.from positional callback gating must agree between engines', + file: 'array-from-callbacks.js', + code: ` +Array.from(arr, mapCallback); +Array.from(arr, mapCallback, thisArg); +Uint8Array.from(arr, mapCallback); `, }, { diff --git a/tests/engines/query-walk-parity.test.ts b/tests/engines/query-walk-parity.test.ts index 37f66026..32b2ade2 100644 --- a/tests/engines/query-walk-parity.test.ts +++ b/tests/engines/query-walk-parity.test.ts @@ -160,6 +160,31 @@ router.post('/api/items', async (req, res) => { save(); }); emitter.on('data', (chunk) => { process(chunk); }); server.once('listening', () => { log(); }); emitter.on('error', handleError); +`, + }, + { + // Regression guard for #1741: identifier args must be gated by + // CALLBACK_ACCEPTING_CALLEES the same way as member_expression args on + // BOTH extraction paths. `findMergeCandidates(communities)` is a plain + // data argument (non-allowlisted callee) and must not appear as a + // dynamic call on either path; `arr.forEach(myNamedCallback)` is a + // genuine callback reference (allowlisted callee) and must appear on + // both. + name: 'identifier-arg callback gating (issue #1741)', + file: 'test.js', + code: ` +findMergeCandidates(communities); +arr.forEach(myNamedCallback); +`, + }, + { + // Regression guard for #1741 follow-up: Array.from's positional callback + // gate (index 1 only) must behave identically on both extraction paths. + name: 'Array.from positional callback gating (issue #1741 follow-up)', + file: 'test.js', + code: ` +Array.from(arr, mapCallback); +Array.from(arr, mapCallback, thisArg); `, }, { diff --git a/tests/parsers/javascript.test.ts b/tests/parsers/javascript.test.ts index 5f9994fb..c1934034 100644 --- a/tests/parsers/javascript.test.ts +++ b/tests/parsers/javascript.test.ts @@ -896,6 +896,97 @@ describe('JavaScript parser', () => { ); }); + it('does not treat identifier args as callbacks for non-allowlisted callees (issue #1741)', () => { + // Regression guard for #1741: `findMergeCandidates(communities)` and + // `analyzeDrift(communities, communityDirs)` pass `communities` as a + // plain DATA argument, not a callback reference. `findMergeCandidates` + // and `analyzeDrift` are not callback-accepting callees, so identifier + // args must be gated exactly like member_expression args — otherwise + // the global-fallback resolver can bind the identifier to an unrelated + // same-named function elsewhere in the repo, fabricating a call edge + // (and, transitively, a phantom cycle — see codegraph's own + // src/features/communities.ts vs src/presentation/communities.ts). + const symbols = parseJS(`findMergeCandidates(communities);`); + expect(symbols.calls.filter((c) => c.dynamic && c.name === 'communities')).toHaveLength(0); + + const symbols2 = parseJS(`analyzeDrift(communities, communityDirs);`); + expect(symbols2.calls.filter((c) => c.dynamic)).toHaveLength(0); + }); + + it('still emits identifier args for allowlisted callees (regression guard)', () => { + // Positive companion to the #1741 fix: identifier args passed to a + // genuine callback-accepting callee must still be resolved, e.g. + // `arr.forEach(myNamedCallback)` — the exact pattern the original + // "identifier args are always emitted" trade-off existed to preserve. + const symbols = parseJS(`arr.forEach(myNamedCallback);`); + expect(symbols.calls).toContainEqual( + expect.objectContaining({ name: 'myNamedCallback', dynamic: true }), + ); + }); + + it('does not treat identifier args to cache/Map .get/.put as callback-accepting (HTTP-verb guard)', () => { + // Identifier-arg counterpart to the existing member-expression HTTP-verb + // guard: `cache.get(someKey)` shares the verb name `get` with Express + // routes but has no string-literal route path first arg, so the + // identifier arg must not be emitted as a dynamic call either. + const symbols = parseJS(`cache.get(someKey);`); + expect(symbols.calls.filter((c) => c.dynamic && c.name === 'someKey')).toHaveLength(0); + }); + + it('emits Array.from mapFn (index 1) but not arrayLike (index 0)', () => { + // Regression guard for #1741 follow-up: `Array.from(arrayLike, mapFn)` is a + // well-known stdlib callback pattern (also every TypedArray.from), but the + // callback is the SECOND positional argument, not the first. Emitting + // `arrayLike` too would reintroduce the exact name-collision false-positive + // class #1741 fixes for the data argument; only `mapFn` should resolve. + const symbols = parseJS(`Array.from(arr, mapCallback);`); + expect(symbols.calls).toContainEqual( + expect.objectContaining({ name: 'mapCallback', dynamic: true }), + ); + expect(symbols.calls.filter((c) => c.dynamic && c.name === 'arr')).toHaveLength(0); + }); + + it('emits only the index-1 mapFn for Array.from with a thisArg (index 2)', () => { + // `Array.from(arrayLike, mapFn, thisArg)` — thisArg (index 2) is a `this` + // binding context, not a callback, and must not be emitted either. + const symbols = parseJS(`Array.from(arr, mapCallback, thisArg);`); + const dynamicNames = symbols.calls.filter((c) => c.dynamic).map((c) => c.name); + expect(dynamicNames).toEqual(['mapCallback']); + }); + + it('applies the same Array.from positional gate to TypedArray constructors', () => { + // Every TypedArray constructor (Uint8Array, Int32Array, etc.) mirrors + // Array.from's (arrayLike, mapFn, thisArg) signature; the gate is + // name-based on the property `from`, not receiver-typed, so it applies + // uniformly. + const symbols = parseJS(`Uint8Array.from(arr, mapCallback);`); + expect(symbols.calls).toContainEqual( + expect.objectContaining({ name: 'mapCallback', dynamic: true }), + ); + expect(symbols.calls.filter((c) => c.dynamic && c.name === 'arr')).toHaveLength(0); + }); + + it('applies the Array.from positional gate to member_expression args too', () => { + // Greptile follow-up: the old member_expression guard was an explicit + // `&& memberExprArgsAllowed` inline check; the positional restructuring + // moved that responsibility to the shared early-return above the loop. + // `Array.from(arr, obj.mapper)` exercises that a member_expression at + // the positional index (1) is still emitted with its receiver, while + // one at index 0 is not — guarding against a future refactor that + // re-adds an inline guard on member_expression only. + const symbols = parseJS(`Array.from(arr, obj.mapper);`); + expect(symbols.calls).toContainEqual( + expect.objectContaining({ name: 'mapper', receiver: 'obj', dynamic: true }), + ); + expect(symbols.calls.filter((c) => c.dynamic && c.name === 'arr')).toHaveLength(0); + + const symbols2 = parseJS(`Array.from(obj.arrayLike, mapCallback);`); + expect(symbols2.calls.filter((c) => c.dynamic && c.name === 'arrayLike')).toHaveLength(0); + expect(symbols2.calls).toContainEqual( + expect.objectContaining({ name: 'mapCallback', dynamic: true }), + ); + }); + it('extracts callback in plain function calls like setTimeout', () => { const symbols = parseJS(`setTimeout(tick, 1000);`); expect(symbols.calls).toContainEqual(