feat(resolver): resolve inline-array spread call edges (fn(...[a, b, c]))#1394
Conversation
…c])) Extend extractParamBindingsWalk to handle spread arguments whose source is an inline array literal. When a call site has fn(...[a, b, c]), each element is expanded as a ParamBinding at the corresponding position so the PTS solver can propagate function references through to callees. Previously only fn(...arr) (spread of a named variable) was handled via SpreadArgBinding + ArrayElemBinding. The inline-literal form fn(...[a, b]) produced no param-flow constraints, so all 4 expected spread edges scored 0%. Also handles mixed calls: fn(x, ...[a, b]) correctly assigns positions starting from the spread's slot (x at 0, a at 1, b at 2). Benchmark: jelly-micro/spread recall 100% (11/11 TP, 0 FN, 0 FP). Closes #1379
|
Claude finished @carlos-alm's task —— View job PR Review
Root Cause AnalysisProblem Being Solved: Why This Approach: The fix is minimal and precisely targeted — it adds a new branch for Risk Assessment: Low. The change is additive-only within Phase 2: Configuration Change DetectionNo linting rules relaxed, no test timeouts increased, no TypeScript flags changed, no build config modified. Clean. Phase 3: Test IntegrityTest fixture additions are correct and honest:
The description in Phase 4: Code Quality
Correct:
Minor observation — One edge case worth noting (not a bug, but worth awareness): Architecture compliance: Phase 5: Backlog Compliance
Final RecommendationRating: ⭐⭐⭐⭐ (4/5) Action: APPROVE Reasoning: The fix is surgically correct. The root cause analysis in the PR description is accurate. The |
Greptile SummaryThis PR extends
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to inline-array spreads, the empty-spread index bug has been addressed, and the new test cases cover all four scenarios introduced by the PR. The new branch only fires when a spread argument's inner node is an array literal, leaving all other argument types (plain identifiers, variable spreads) on their existing code paths. The argIdx += elemCount; continue pattern correctly handles zero-, single-, and multi-element inline arrays, and the mixed-position case is validated by q(e, ...[h, d]). No pre-existing logic is disturbed. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[call_expression node] --> B{fn is identifier?}
B -- No --> Z[skip]
B -- Yes --> C[iterate args children]
C --> D{child type?}
D -- comma/parens --> C
D -- identifier --> E[push ParamBinding at argIdx\nthen argIdx++]
E --> C
D -- spread_element --> F[get inner = child.argument]
F --> G{inner type == array?}
G -- No --> H[fall-through: argIdx++]
H --> C
G -- Yes --> I[iterate array elements]
I --> J{elem type?}
J -- comma/brackets --> I
J -- identifier --> K[push ParamBinding at argIdx+elemCount\nelemCount++]
K --> I
J -- other --> L[elemCount++ only, no binding]
L --> I
I --> M[argIdx += elemCount\ncontinue — skip outer argIdx++]
M --> C
Reviews (6): Last reviewed commit: "Merge branch 'main' into feat/spread-inl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (inner?.type === 'array') { | ||
| let elemCount = 0; | ||
| for (let j = 0; j < inner.childCount; j++) { | ||
| const elem = inner.child(j); | ||
| if (!elem) continue; | ||
| if (elem.type === ',' || elem.type === '[' || elem.type === ']') continue; | ||
| if (elem.type === 'identifier' && !BUILTIN_GLOBALS.has(elem.text)) { | ||
| paramBindings.push({ | ||
| callee: fn.text, | ||
| argIndex: argIdx + elemCount, | ||
| argName: elem.text, | ||
| }); | ||
| } | ||
| elemCount++; | ||
| } | ||
| // argIdx++ below accounts for 1 slot; pre-advance for the remaining elements. | ||
| argIdx += Math.max(0, elemCount - 1); | ||
| } |
There was a problem hiding this comment.
argIdx by 1. When ...[ ] has zero elements, elemCount is 0, so argIdx += Math.max(0, -1) = 0, then the unconditional argIdx++ below still fires. Any positional argument appearing after ...[ ] in the same call receives an index one too high, causing a missed param binding. Replacing the pre-advance + unconditional increment with a single argIdx += elemCount; continue fully controls the slot consumption inside the branch and also simplifies the arithmetic.
| if (inner?.type === 'array') { | |
| let elemCount = 0; | |
| for (let j = 0; j < inner.childCount; j++) { | |
| const elem = inner.child(j); | |
| if (!elem) continue; | |
| if (elem.type === ',' || elem.type === '[' || elem.type === ']') continue; | |
| if (elem.type === 'identifier' && !BUILTIN_GLOBALS.has(elem.text)) { | |
| paramBindings.push({ | |
| callee: fn.text, | |
| argIndex: argIdx + elemCount, | |
| argName: elem.text, | |
| }); | |
| } | |
| elemCount++; | |
| } | |
| // argIdx++ below accounts for 1 slot; pre-advance for the remaining elements. | |
| argIdx += Math.max(0, elemCount - 1); | |
| } | |
| if (inner?.type === 'array') { | |
| let elemCount = 0; | |
| for (let j = 0; j < inner.childCount; j++) { | |
| const elem = inner.child(j); | |
| if (!elem) continue; | |
| if (elem.type === ',' || elem.type === '[' || elem.type === ']') continue; | |
| if (elem.type === 'identifier' && !BUILTIN_GLOBALS.has(elem.text)) { | |
| paramBindings.push({ | |
| callee: fn.text, | |
| argIndex: argIdx + elemCount, | |
| argName: elem.text, | |
| }); | |
| } | |
| elemCount++; | |
| } | |
| // Advance by the exact number of slots this spread occupies and skip | |
| // the unconditional argIdx++ below so that zero-element spreads (...[]) | |
| // do not shift subsequent argument indices. | |
| argIdx += elemCount; | |
| continue; | |
| } |
There was a problem hiding this comment.
Fixed. The empty inline-array spread bug is real — replaced argIdx += Math.max(0, elemCount - 1) plus the unconditional argIdx++ with argIdx += elemCount; continue, so zero-element spreads consume zero slots and do not shift subsequent argument indices. Added a test case r(...[], e) to lock in the fix: e must resolve to index 0 (not 1) in r, confirming the corrected behavior.
Codegraph Impact Analysis7 functions changed → 22 callers affected across 7 files
|
…indices (#1394) Empty `...[ ]` spread was advancing `argIdx` by 1 via the unconditional post-increment, causing any positional argument after it in the same call to receive an index one too high. Replace the pre-advance + unconditional increment with `argIdx += elemCount; continue` so zero-element spreads consume zero slots. Adds a test covering `r(...[], e)` to lock in the fix.
1 participant
Summary
extractParamBindingsWalkto handle spread arguments whose source is an inline array literal (fn(...[a, b, c]))ParamBindingat the corresponding position, so the PTS solver propagates function references through to calleesfn(x, ...[a, b])correctly assigns positions starting from the spread's slotjelly-micro/spreadfixture to include inline-literal test cases (single-element, multi-element, mixed)Root cause
extractParamBindingsWalkonly emittedParamBindingforidentifier-typed arguments. Thespread_elementnode was skipped (incrementingargIdxby 1 but emitting nothing), sofn(...[a, b])produced no param-flow constraints. The existingf(...arr)path (viaSpreadArgBinding+ArrayElemBinding) already worked — the inline-literal form was the gap.Test plan
jelly-micro/spreadbenchmark: recall 100% (11/11 TP, 0 FN, 0 FP) — up from 4/4 on the old fixture (inline cases were simply untested)prototype-method-resolutionfailures are unrelated to this change)Closes #1379