eslint-factory: resolve destructured bindings precisely in resolveInitializer - #53960
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Ponytail review: reviewed diff for over-engineering only. New helper functions (resolveArrayPatternElement, resolveObjectPatternProperty, getStaticPropertyName, asExpression) each encode a single required precision guard (rest/spread/hole/computed-key/default rejection) mandated by the linked issue acceptance criteria, not speculative abstraction. No reinvented stdlib, no unneeded deps, no dead code, no unused flexibility. Test additions are necessary regression coverage. Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53960 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No actionable blocking issues found in the changed lines.
Review notes
I checked the new destructuring resolution logic and the added regression coverage for the affected rules. The implementation is intentionally conservative around spreads, defaults, computed properties, and non-literal right-hand sides, which avoids introducing false precision while fixing the missed dynamic-command cases this PR targets.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.82 AIC · ⌖ 9.04 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The implementation correctly resolves destructured bindings in resolveInitializer for both array and object patterns. Key correctness observations:
- Array patterns: Rest elements and spread elements in the RHS literal both cause early null returns, preserving soundness. Holes are handled. Default values in the pattern are correctly rejected (the element won't be an
Identifier, so the loop skips it). - Object patterns: Any spread in the object literal causes an immediate null return (conservative but sound). Duplicate keys: the loop overwrites
resolvedfor each match, so the last property wins — matching JS runtime semantics. asExpression: Correctly rejectsAssignmentPatternand other non-expression property values from slipping through.- Tests: Valid (no-flag) and invalid (must-flag) cases cover the new code paths across all three rule test files.
No blocking issues found. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.8 AIC · ⌖ 8.83 AIC · ⊞ 5.7K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (10 test scenarios)
Verdict
|
There was a problem hiding this comment.
Pull request overview
Improves initializer resolution for array/object destructuring used by security-focused ESLint rules.
Changes:
- Resolves literal destructured values precisely.
- Adds conservative guards for ambiguous patterns.
- Adds regression coverage across three consuming rules.
Show a summary per file
| File | Description |
|---|---|
command-initializer-utils.ts |
Implements destructured initializer resolution. |
no-child-process-interpolated-command.test.ts |
Tests destructured child-process commands. |
no-exec-interpolated-command.test.ts |
Tests destructured exec commands. |
no-github-request-interpolated-route.test.ts |
Tests destructured GitHub routes. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
| // A spread element at or before this position shifts the value positions. | ||
| if (init.elements.slice(0, index + 1).some(value => value !== null && value.type === AST_NODE_TYPES.SpreadElement)) return null; | ||
| const value = init.elements[index]; | ||
| if (value === null || value.type === AST_NODE_TYPES.SpreadElement) return null; |
| if (property.type !== AST_NODE_TYPES.Property || property.computed || property.kind !== "init") continue; | ||
| if (getStaticPropertyName(property.key) !== key) continue; |
| if (property.type !== AST_NODE_TYPES.Property || property.computed || property.kind !== "init") continue; | ||
| if (getStaticPropertyName(property.key) !== key) continue; | ||
| // Later properties win over earlier duplicates. | ||
| resolved = asExpression(property.value); |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with a few test-coverage gaps and one minor performance note. No correctness bugs found; the fix and its guards are sound.
📋 Key Themes & Highlights
Key Themes
- Missing edge-case tests: three patterns (default values, duplicate object keys, nested/aliased destructuring) are handled correctly by the implementation but have no corresponding test entries — future refactors could silently break them.
- Minor O(n2) loop: the spread-before-index check in
resolveArrayPatternElementre-scans already-visited elements on each iteration; a single-pass flag would be cleaner.
Positive Highlights
- ✅ Precision guards are comprehensive: rest elements, spread elements, array holes, computed props, accessor props, and defaults are all handled defensively.
- ✅ Duplicate object property semantics (last-value-wins) are correctly implemented.
- ✅ Regression tests cover all three consuming rules with valid, invalid, and unresolvable cases.
- ✅
asExpressioncleanly narrows property values, keeping the object-pattern resolver readable. - ✅ Root cause (dispatching on
declarator.id.type) is addressed directly rather than papering over the symptom.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 45.9 AIC · ⌖ 10.3 AIC · ⊞ 7.8K
Comment /matt to run again
| if (init.elements.slice(0, index + 1).some(value => value !== null && value.type === AST_NODE_TYPES.SpreadElement)) return null; | ||
| const value = init.elements[index]; | ||
| if (value === null || value.type === AST_NODE_TYPES.SpreadElement) return null; | ||
| return value; |
There was a problem hiding this comment.
[/diagnosing-bugs] The spread guard (slice(0, index + 1).some(...)) is correct but re-scans previously visited elements on every loop iteration, making the function O(n2) for wide array patterns.
💡 Suggested fix — single-pass spread tracking
Track whether a spread element has been seen as you iterate:
let spreadSeen = false;
for (let index = 0; index < pattern.elements.length; index++) {
const element = pattern.elements[index];
if (element !== null && element.type === AST_NODE_TYPES.RestElement) return null;
const initEl = init.elements[index];
if (initEl !== null && initEl.type === AST_NODE_TYPES.SpreadElement) spreadSeen = true;
if (element === null || element.type !== AST_NODE_TYPES.Identifier || element.name !== name) continue;
if (spreadSeen) return null;
const value = init.elements[index];
if (value === null || value.type === AST_NODE_TYPES.SpreadElement) return null;
return value;
}Not a correctness issue, but improves readability and avoids redundant work for patterns with many elements.
@copilot please address this.
| if (init.type !== AST_NODE_TYPES.ObjectExpression) return null; | ||
| // A spread can override any property, so the literal is no longer authoritative. | ||
| if (init.properties.some(property => property.type === AST_NODE_TYPES.SpreadElement)) return null; | ||
|
|
There was a problem hiding this comment.
[/diagnosing-bugs] asExpression rejects AssignmentPattern (default values), which is correct for object shorthand defaults — but there's no test for const { cmd = fallback } = { cmd: template }. Without coverage, a future reader may not know this is intentionally unsupported.
💡 Suggested test
Add a valid-case entry in each rule's test suite:
// Default value in destructuring — unresolvable, must not be flagged
{ code: `const { execSync } = require("child_process"); const { cmd = "default" } = { cmd: \`git checkout \${branch}\` }; execSync(cmd);` }If the intent is to be conservative (return null), the test documents that; if the intent should be to resolve through defaults, the missing logic becomes visible.
@copilot please address this.
| resolved = asExpression(property.value); | ||
| } | ||
| return resolved; | ||
| } |
There was a problem hiding this comment.
[/tdd] The object-pattern resolver handles duplicate property keys by returning the last one (matching JS runtime semantics). This edge case is not covered by any test, so a future refactor could break it silently.
💡 Suggested test
// Duplicate property — last value wins; if it is interpolated it must be flagged
{
code: `const { execSync } = require("child_process"); const { cmd } = { cmd: "git status", cmd: \`git checkout \${branch}\` }; execSync(cmd);`,
errors: [{ messageId: "interpolatedCommand", ... }],
}And the reverse — last value is safe even though the first is not:
{ code: `... const { cmd } = { cmd: \`git checkout \${branch}\`, cmd: "git status" }; execSync(cmd);` }@copilot please address this.
| code: `const { execSync } = require("child_process"); const [cmd] = [\`git checkout \${branch}\`]; execSync(cmd);`, | ||
| errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "execSync" } }], | ||
| }, | ||
| // Object-destructured dynamic command must resolve to the destructured property |
There was a problem hiding this comment.
[/tdd] The new invalid test cases don't cover nested array destructuring (e.g., const [[cmd]] = [[template]]) or aliased object keys (e.g., const { cmd: alias } = { cmd: template }). The implementation returns null for these (correct conservative behaviour), but without valid-case tests a reader cannot verify that they are intentionally unsupported.
💡 Suggested additions to valid cases
// Nested destructuring — unresolvable, must not be flagged
{ code: `const { execSync } = require("child_process"); const [[cmd]] = [[\`git checkout \${branch}\`]]; execSync(cmd);` },
// Aliased object key — verify alias is resolved, not the original key
{ code: `const { execSync } = require("child_process"); const { cmd: alias } = { cmd: \`git checkout \${branch}\` }; execSync(alias);` },Even if these remain unresolvable by design, explicit tests act as executable documentation.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
resolveInitializerreaddeclarator.initwithout checking the shape ofdeclarator.id, so a destructured binding resolved to the entire right-hand side rather than the value actually bound. Since it backsresolveWriteOnceInitializerChain/getDynamicCommandKind, dynamic commands and routes were silently missed byno-child-process-interpolated-command,no-exec-interpolated-command, andno-github-request-interpolated-route.Changes
resolveInitializernow dispatches ondeclarator.id:Identifier— unchanged.ArrayPattern— resolves to the array-literal element at the matching index.ObjectPattern— resolves to the matching object-literal property value.null.nullrather than a wrong sub-expression): non-literal RHS, rest element before the binding, spread at/before the position, array holes, any spread in an object literal, computed or accessor properties, defaults, and nested patterns.const [cmd] = ["git status"]) and unresolvable ones (const [cmd] = parts) remain unflagged.The issue proposed simply returning
nullfor destructured ids, but that would leave the two "should now be flagged" acceptance criteria unmet — resolving the bound value satisfies both those and the no-false-positive criterion.