Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 90 additions & 1 deletion eslint-factory/src/rules/command-initializer-utils.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,95 @@
import { AST_NODE_TYPES, TSESLint, TSESTree } from "@typescript-eslint/utils";

/**
* Resolves the element of an array literal bound to `name` by an array
* destructuring pattern (for example `const [cmd] = [command]`). Returns null
* when the binding cannot be resolved precisely — a non-literal right-hand
* side, a rest element before the binding, a spread element in the array
* literal, a hole, or a default value.
*/
function resolveArrayPatternElement(pattern: TSESTree.ArrayPattern, init: TSESTree.Expression, name: string): TSESTree.Expression | null {
if (init.type !== AST_NODE_TYPES.ArrayExpression) return null;
for (let index = 0; index < pattern.elements.length; index++) {
const element = pattern.elements[index];
// A rest element consumes the remaining values, so positions after it no
// longer line up with the array literal.
if (element !== null && element.type === AST_NODE_TYPES.RestElement) return null;
if (element === null || element.type !== AST_NODE_TYPES.Identifier || element.name !== name) continue;
// 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;
return value;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

}
return null;
}

/**
* Returns the static property name of a non-computed property key, or null
* when the key is not statically known.
*/
function getStaticPropertyName(key: TSESTree.Node): string | null {
if (key.type === AST_NODE_TYPES.Identifier) return key.name;
if (key.type === AST_NODE_TYPES.Literal && (typeof key.value === "string" || typeof key.value === "number")) return String(key.value);
return null;
}

/**
* Narrows a property value to a plain expression, rejecting binding patterns
* and other non-expression property values.
*/
function asExpression(value: TSESTree.Property["value"]): TSESTree.Expression | null {
switch (value.type) {
case AST_NODE_TYPES.ArrayPattern:
case AST_NODE_TYPES.AssignmentPattern:
case AST_NODE_TYPES.ObjectPattern:
case AST_NODE_TYPES.TSEmptyBodyFunctionExpression:
return null;
default:
return value;
}
}

/**
* Resolves the property value of an object literal bound to `name` by an
* object destructuring pattern (for example `const { cmd } = { cmd: command }`).
* Returns null when the binding cannot be resolved precisely — a non-literal
* right-hand side, a spread element, a computed or accessor property, or a
* default value.
*/
function resolveObjectPatternProperty(pattern: TSESTree.ObjectPattern, init: TSESTree.Expression, name: string): TSESTree.Expression | null {
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

let key: string | null = null;
for (const property of pattern.properties) {
if (property.type !== AST_NODE_TYPES.Property || property.computed) continue;
if (property.value.type !== AST_NODE_TYPES.Identifier || property.value.name !== name) continue;
key = getStaticPropertyName(property.key);
break;
}
if (key === null) return null;

let resolved: TSESTree.Expression | null = null;
for (const property of init.properties) {
if (property.type !== AST_NODE_TYPES.Property || property.computed || property.kind !== "init") continue;
if (getStaticPropertyName(property.key) !== key) continue;
Comment on lines +76 to +77
// Later properties win over earlier duplicates.
resolved = asExpression(property.value);
}
return resolved;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.


/**
* When `identifier` is a write-once local variable binding, returns its
* initializer expression so the caller can apply further checks. Returns null
* for parameters, imports, multiply-assigned vars, and vars with no
* initializer.
*
* Destructured bindings (for example `const [cmd] = [command]`) resolve to the
* specific destructured value when it can be determined precisely, and to null
* otherwise — never to the whole right-hand side expression.
*/
function resolveInitializer(identifier: TSESTree.Identifier, sourceCode: TSESLint.SourceCode): TSESTree.Expression | null {
const startScope = sourceCode.getScope(identifier);
Expand All @@ -28,7 +113,11 @@ function resolveInitializer(identifier: TSESTree.Identifier, sourceCode: TSESLin
// Reject re-assigned bindings (write references that are not the initializer).
if (variable.references.some(ref => ref.isWrite() && !ref.init)) return null;
const declarator = def.node as TSESTree.VariableDeclarator;
return declarator.init ?? null;
if (declarator.init === null || declarator.init === undefined) return null;
if (declarator.id.type === AST_NODE_TYPES.Identifier) return declarator.init;
if (declarator.id.type === AST_NODE_TYPES.ArrayPattern) return resolveArrayPatternElement(declarator.id, declarator.init, identifier.name);
if (declarator.id.type === AST_NODE_TYPES.ObjectPattern) return resolveObjectPatternProperty(declarator.id, declarator.init, identifier.name);
return null;
}
scope = scope.upper;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ describe("no-child-process-interpolated-command", () => {
{ code: `const { execSync } = require("child_process"); execSync("git-status".replace("-", () => " "));` },
// Un-reassigned options object without shell — safe, must not over-flag
{ code: `const { spawnSync } = require("child_process"); const cmd = \`git checkout \${branch}\`; const opts = {}; spawnSync(cmd, [], opts);` },
// Statically destructured command — safe, must not be flagged
{ code: `const { execSync } = require("child_process"); const [cmd] = ["git status"]; execSync(cmd);` },
// Destructured from an unresolvable right-hand side — must not be flagged
{ code: `const { execSync } = require("child_process"); function run(parts) { const [cmd] = parts; execSync(cmd); }` },
],
invalid: [
{
Expand Down Expand Up @@ -141,6 +145,16 @@ describe("no-child-process-interpolated-command", () => {
code: `require("child_process").spawn(\`git checkout \${branch}\`, { shell: true });`,
errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "spawn" } }],
},
// Array-destructured dynamic command must resolve to the destructured element
{
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

{
code: `const { execSync } = require("child_process"); const { cmd } = { cmd: \`git checkout \${branch}\` }; execSync(cmd);`,
errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "execSync" } }],
},
],
});
});
Expand Down
14 changes: 14 additions & 0 deletions eslint-factory/src/rules/no-exec-interpolated-command.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ describe("no-exec-interpolated-command", () => {
{ code: `exec.exec("git-checkout".replace("-", () => " "), [branch]);` },
// A write-once interpolation resolving to a literal is safe
{ code: `function run() { const branch = "main"; const ref = branch; exec.exec(\`git checkout \${ref}\`, []); }` },
// Statically destructured command — safe, must not be flagged
{ code: `function run() { const [cmd] = ["git status"]; exec.exec(cmd, []); }` },
// Destructured from an unresolvable right-hand side — must not be flagged
{ code: `function run(parts) { const [cmd] = parts; exec.exec(cmd, []); }` },
// A digits-only sanitized interpolation is safe
{ code: `function run(port) { const safePort = String(port).replace(/[^0-9]/g, ""); exec.exec(\`netstat | grep :\${safePort}\`, []); }` },
],
Expand Down Expand Up @@ -153,6 +157,16 @@ describe("no-exec-interpolated-command", () => {
code: "function run(branch) { const dynamic = `git checkout ${branch}`; const cmd = dynamic; exec.exec(cmd, []); }",
errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }],
},
// Array-destructured dynamic command must resolve to the destructured element
{
code: "function run(branch) { const [cmd] = [`git checkout ${branch}`]; exec.exec(cmd, []); }",
errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }],
},
// Object-destructured dynamic command must resolve to the destructured property
{
code: "function run(branch) { const { cmd } = { cmd: `git checkout ${branch}` }; exec.exec(cmd, []); }",
errors: [{ messageId: "interpolatedCommand", data: { kind: "interpolated template literal", method: "exec" } }],
},
// execApi parameter-alias with array-shaped args — flagged (matches git_helpers.cjs / create_pull_request.cjs convention)
{
code: "function run(execApi, branch) { execApi.exec(`git checkout ${branch}`, []); }",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -389,4 +389,35 @@ describe("no-github-request-interpolated-route", () => {
invalid: [],
});
});

it("destructured route bindings resolve to the destructured value", () => {
cjsRuleTester.run("no-github-request-interpolated-route", noGithubRequestInterpolatedRouteRule, {
valid: [
// Statically destructured route — no interpolation, must not be flagged
`function f() { const [route] = ["GET /repos/{owner}/{repo}"]; github.request(route, {}); }`,
// Destructured from a non-literal right-hand side — unresolvable, must not be flagged
"function f(routes) { const [route] = routes; github.request(route, {}); }",
],
invalid: [
{
code: "function f(owner, repo) { const [route] = [`GET /repos/${owner}/${repo}`]; github.request(route, {}); }",
errors: [
{
messageId: "interpolatedRoute",
data: { kind: "template literal with interpolations", client: "github" },
},
],
},
{
code: "function f(owner, repo) { const { route } = { route: `GET /repos/${owner}/${repo}` }; github.request(route, {}); }",
errors: [
{
messageId: "interpolatedRoute",
data: { kind: "template literal with interpolations", client: "github" },
},
],
},
],
});
});
});
Loading