fix(extractor): gate member_expression callback args on callee allowlist#974
Conversation
PR #947's extractCallbackReferenceCalls emitted a dynamic call edge for every identifier or member_expression argument of every call expression. That was correct for real callbacks (router.use, promise.then, items.map) but produced false positives for plain property reads passed as data, e.g. `store.set(user.id, user)` — the `user.id` arg wrongly resolved to `User.id@types.ts`, dropping TS resolution precision from 100% → 93.8% and tripping the regression guard. Fix: gate member_expression args on a CALLBACK_ACCEPTING_CALLEES allowlist covering router/middleware, promises, array methods, event emitters, and scheduling APIs. Identifier args remain unchanged. Adds positive and negative tests; updates the 3.9.4 resolution benchmark to reflect restored precision. Fixes #971 Impact: 2 functions changed, 7 affected
|
Claude finished @carlos-alm's task —— View job Reviewing PR: fix(extractor): gate member_expression callback args on callee allowlistReview Checklist:
Root Cause AnalysisProblem Being Solved: False positive in TypeScript callback reference extraction where Why This Approach: The allowlist approach (option 3) is well-justified:
Risk Assessment: Low risk. The change is surgical and well-scoped, with comprehensive test coverage for both negative and positive cases. Backlog Compliance
Code Quality AssessmentStrengths:
Technical correctness:
Test integrity:
Potential concerns:
Minor suggestions:
Final Recommendation
|
Greptile SummaryThis PR adds a Confidence Score: 5/5Safe to merge — the fix is targeted, well-tested, and restores benchmark precision to 100% with no regressions. All previous review concerns (HTTP-verb/Map-API overlap, optional-chaining false negative) are resolved in this commit. Five new tests cover negative, positive, HTTP-verb guard, and optional-chain cases. No P0 or P1 issues found in the implementation. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[extractCallbackReferenceCalls] --> B[extractCalleeName]
B --> C{calleeName in\nCALLBACK_ACCEPTING_CALLEES?}
C -- No --> D[memberExprArgsAllowed = false]
C -- Yes --> E{calleeName in\nHTTP_VERB_CALLEES?}
E -- No --> F[memberExprArgsAllowed = true]
E -- Yes --> G{firstArgIsStringLiteral?}
G -- Yes: Express route --> F
G -- No: Map/cache API --> D
D --> H[Iterate args]
F --> H
H --> I{arg type?}
I -- identifier --> J[always emit dynamic call]
I -- member_expression --> K{memberExprArgsAllowed?}
K -- Yes --> L[emit dynamic call with receiver]
K -- No --> M[skip — data value, not callback]
Reviews (2): Last reviewed commit: "fix(extractor): require string-literal p..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 'use', | ||
| 'get', | ||
| 'post', | ||
| 'put', | ||
| 'delete', | ||
| 'patch', | ||
| 'options', | ||
| 'head', | ||
| 'all', |
There was a problem hiding this comment.
get/HTTP-verb entries conflict with Map/cache patterns
get, post, put, delete, and patch are added for Express routes (router.get('/path', auth.check)), but they also match Map, LRU-cache, and repository APIs: cache.get(user.id) or repo.put(record.key, value) will still emit id / key as a dynamic callback call — exactly the same category of false positive the PR fixes for set. The set method isn't in the allowlist (correct), but its counterpart get is.
Consider whether the HTTP-verb entries should require a string-literal first argument (the route path) as an additional guard, or whether the get FP rate in practice is low enough to be acceptable. As-is, the remaining FP surface for Map-like patterns using these verb names isn't documented in the PR's "remaining concerns" section.
There was a problem hiding this comment.
Fixed in d51265d. Added an HTTP_VERB_CALLEES sub-allowlist gated by a string-literal first-arg check (firstArgIsStringLiteral): HTTP verbs (get, post, put, delete, patch, options, head, all) now only treat member-expr args as callbacks when the first argument is a string literal — matching the Express route signature app.get('/path', handler) while dropping cache.get(user.id), repo.put(record.key, ...), map.delete(entry.id), etc. Added a negative test (cache.get/repo.put/map.delete must not emit dynamic calls for the property name) and a positive regression guard (router.get('/users/:id', auth.check) and \ template still works). use is intentionally excluded from the sub-allowlist because app.use(handler) without a path is a valid middleware registration.
| function extractCalleeName(callNode: TreeSitterNode): string | null { | ||
| const fn = callNode.childForFieldName('function'); | ||
| if (!fn) return null; | ||
| if (fn.type === 'identifier') return fn.text; | ||
| if (fn.type === 'member_expression') { | ||
| const prop = fn.childForFieldName('property'); | ||
| return prop ? prop.text : null; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
extractCalleeName only handles identifier and member_expression callee shapes, returning null for anything else. Optional-chaining calls (obj?.on(event, handler.fn)) produce an optional_member_expression (or equivalent) in tree-sitter, so extractCalleeName returns null → memberExprArgsAllowed = false → the member-expr arg is silently dropped even though on is in the allowlist.
This is a pre-existing edge case, but the new gating logic makes it a new false-negative class. Worth adding optional_member_expression handling or at least noting it in the JSDoc.
There was a problem hiding this comment.
Verified this is a non-issue and added a regression test in d51265d. In tree-sitter-javascript and tree-sitter-typescript, obj?.on(event, handler.fn) is represented as a member_expression node (with an optional_chain child for the ?. token), not an optional_member_expression — so extractCalleeName already returns on and the allowlist gate works. Confirmed by dumping the AST for both grammars. Added test handles optional-chaining callees in allowlist (obj?.on) (emitter?.on('tick', handlers.fn) emits fn with receiver handlers) to lock in this behavior, plus a JSDoc note on extractCalleeName documenting why optional chaining is transparent here.
Codegraph Impact Analysis3 functions changed → 7 callers affected across 1 files
|
…ting (#974) Addresses Greptile review feedback on PR #974: - HTTP-verb callees (get/post/put/delete/patch/options/head/all) double as Map/cache/repository method names. Require a string-literal first argument (Express route path) for member-expr args to be emitted as dynamic calls, so `cache.get(user.id)` and `repo.put(record.key, value)` no longer leak `id`/`key` as false-positive dynamic calls while `router.get('/path', h)` still works. - Document that optional-chaining callees (`obj?.on(handlers.fn)`) are handled transparently: tree-sitter-javascript/typescript represent them as `member_expression` with an `optional_chain` child, so the existing extraction returns the property name correctly. Add a regression test. - Tests: three new cases in `tests/parsers/javascript.test.ts`: - negative: `cache.get(user.id)`, `repo.put(record.key, ...)`, `map.delete(entry.id)` - positive: `router.get('/path', auth.check)`, `app.post(\`/api\`, handlers.create)` - optional-chaining: `emitter?.on('tick', handlers.fn)` still emits All JS parser, regression-guard, and TS/JS resolution benchmarks stay green (TS precision 1.0, JS precision 1.0). Impact: 2 functions changed, 7 affected
1 participant
Summary
Fixes #971.
Why option 3 (allowlist)
Test plan
Remaining concerns