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
7 changes: 7 additions & 0 deletions eslint-factory/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ This project hosts custom ESLint linters for `/actions/setup/js`.
|---|---|
| [`no-core-exportvariable-non-string`](#no-core-exportvariable-non-string) | Require explicit string values for `core.exportVariable` calls |
| [`no-core-setoutput-non-string`](#no-core-setoutput-non-string) | Require explicit string values for `core.setOutput` calls |
| [`no-duplicate-constant-values`](#no-duplicate-constant-values) | Report constants with duplicate static primitive values in the same file |
| [`no-child-process-interpolated-command`](#no-child-process-interpolated-command) | Disallow interpolated command strings in shell-evaluated `child_process` calls |
| [`no-github-request-interpolated-route`](#no-github-request-interpolated-route) | Disallow interpolated route arguments in Octokit `.request()` calls |
| [`no-json-stringify-error`](#no-json-stringify-error) | Disallow `JSON.stringify()` on caught error variables |
Expand All @@ -44,6 +45,12 @@ This project hosts custom ESLint linters for `/actions/setup/js`.
| [`require-execfilesync-try-catch`](#require-execfilesync-try-catch) | Require try/catch around `execFileSync(...)` calls from `child_process` |
| [`require-spawnsync-error-check`](#require-spawnsync-error-check) | Require checking `result.error` after `spawnSync` calls |

### `no-duplicate-constant-values`

Inventory module-level `const` declarations with static primitive initializers and report each declaration after the first one that uses the same value in a file. The diagnostic names both constants and shows the duplicated value.

The rule compares string, number, boolean, `null`, bigint, regular-expression, static template-literal, and signed numeric initializers. Dynamic expressions, object and array literals, destructuring declarations, function-local declarations, and `let` or `var` declarations are ignored.

### `no-github-request-interpolated-route`

Disallow template literals with interpolations or string concatenation expressions as the route argument of Octokit `.request()` calls.
Expand Down
1 change: 1 addition & 0 deletions eslint-factory/eslint.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ module.exports = [
"gh-aw-custom/no-caught-error-interpolation": "warn",
"gh-aw-custom/require-fetch-try-catch": "warn",
"gh-aw-custom/no-core-error-then-setfailed": "warn",
"gh-aw-custom/no-duplicate-constant-values": "warn",
},
},
{
Expand Down
2 changes: 2 additions & 0 deletions eslint-factory/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { noErrStackThenStringFallbackRule } from "./rules/no-err-stack-then-stri
import { noCaughtErrorInterpolationRule } from "./rules/no-caught-error-interpolation";
import { requireFetchTryCatchRule } from "./rules/require-fetch-try-catch";
import { noCoreErrorThenSetFailedRule } from "./rules/no-core-error-then-setfailed";
import { noDuplicateConstantValuesRule } from "./rules/no-duplicate-constant-values";

const plugin = {
meta: {
Expand Down Expand Up @@ -71,6 +72,7 @@ const plugin = {
"no-caught-error-interpolation": noCaughtErrorInterpolationRule,
"require-fetch-try-catch": requireFetchTryCatchRule,
"no-core-error-then-setfailed": noCoreErrorThenSetFailedRule,
"no-duplicate-constant-values": noDuplicateConstantValuesRule,
},
};

Expand Down
73 changes: 73 additions & 0 deletions eslint-factory/src/rules/no-duplicate-constant-values.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { RuleTester } from "eslint";
import { describe, expect, it } from "vitest";
import { noDuplicateConstantValuesRule } from "./no-duplicate-constant-values";

const ruleTester = new RuleTester({
languageOptions: {
ecmaVersion: 2022,
sourceType: "commonjs",
},
});

describe("no-duplicate-constant-values", () => {
it("uses the correct docs URL", () => {
expect(noDuplicateConstantValuesRule.meta.docs.url).toBe("https://github.com/github/gh-aw/tree/main/eslint-factory#no-duplicate-constant-values");
});

it("accepts unique and dynamic constant values", () => {
ruleTester.run("no-duplicate-constant-values", noDuplicateConstantValuesRule, {
valid: [
`const FIRST = "first"; const SECOND = "second";`,
`const FIRST = makeValue(); const SECOND = makeValue();`,
`const FIRST = { value: 1 }; const SECOND = { value: 1 };`,
`let first = "same"; let second = "same";`,
`const { first, second } = value;`,
`function first() { const VALUE = "same"; } function second() { const VALUE = "same"; }`,
],
invalid: [],
});
});

it("reports duplicate strings, numbers, templates, and regular expressions", () => {
ruleTester.run("no-duplicate-constant-values", noDuplicateConstantValuesRule, {
valid: [],
invalid: [
{
code: `const FIRST = "same"; const SECOND = "same";`,
errors: [{ messageId: "duplicateConstantValue", data: { name: "SECOND", originalName: "FIRST", value: `"same"` } }],
},
{
code: 'const FIRST = `same`; const SECOND = "same";',
errors: [{ messageId: "duplicateConstantValue", data: { name: "SECOND", originalName: "FIRST", value: `"same"` } }],
},
{
code: `const FIRST = -42; const SECOND = -42; const THIRD = 42;`,
errors: [{ messageId: "duplicateConstantValue", data: { name: "SECOND", originalName: "FIRST", value: "-42" } }],
},
{
code: `const FIRST = /value/gi; const SECOND = /value/gi;`,
errors: [{ messageId: "duplicateConstantValue", data: { name: "SECOND", originalName: "FIRST", value: "/value/gi" } }],
},
{
code: `const FIRST = /value/gi; const SECOND = /value/ig;`,
errors: [{ messageId: "duplicateConstantValue", data: { name: "SECOND", originalName: "FIRST", value: "/value/ig" } }],
},
],
});
});

it("reports every duplicate after the first declaration", () => {
ruleTester.run("no-duplicate-constant-values", noDuplicateConstantValuesRule, {
valid: [],
invalid: [
{
code: `const FIRST = "same"; const SECOND = "same"; const THIRD = "same";`,
errors: [
{ messageId: "duplicateConstantValue", data: { name: "SECOND", originalName: "FIRST", value: `"same"` } },
{ messageId: "duplicateConstantValue", data: { name: "THIRD", originalName: "FIRST", value: `"same"` } },
],
},
],
});
});
});
81 changes: 81 additions & 0 deletions eslint-factory/src/rules/no-duplicate-constant-values.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import { AST_NODE_TYPES, ESLintUtils, TSESTree } from "@typescript-eslint/utils";

const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`);

interface ConstantDeclaration {
name: string;
}

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.

[/codebase-design] The ConstantDeclaration interface only holds name but is used just to carry the string — it adds indirection with no benefit. Use a plain Map<string, string> (value key → name) instead.

💡 Simplified approach
const constantsByValue = new Map<string, string>(); // key → original name
// ...
const original = constantsByValue.get(valueKey);
if (!original) {
  constantsByValue.set(valueKey, declaration.id.name);
  continue;
}
context.report({ ..., data: { name: ..., originalName: original, value: ... } });

Removing the wrapper type makes the intent clearer.

@copilot please address this.


function getStaticValueKey(node: TSESTree.Expression): string | null {
if (node.type === AST_NODE_TYPES.Literal) {
if ("regex" in node && node.regex) {
return `regexp:${node.regex.pattern}/${[...node.regex.flags].sort().join("")}`;
}

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] Regex flag-order normalization is missing — /value/gi and /value/ig are semantically identical but produce different keys, so the rule silently misses that duplicate.

💡 Suggested fix

Sort flags before building the key:

return `regexp:${node.regex.pattern}/${[...node.regex.flags].sort().join('')}`;

Add a test case: const A = /x/gi; const B = /x/ig; should report a duplicate.

@copilot please address this.

return `${typeof node.value}:${String(node.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.

BigInt literals aren't handled correctly and can collide with unrelated null constants.

💡 Details

For a BigInt literal (e.g. const A = 10n;), typescript-eslint's parser may leave node.value as null (bigint values aren't always populated depending on parser/runtime support), producing the key object:null — identical to the key generated for an actual null literal (typeof null === "object"). This causes a BigInt constant to be falsely reported as duplicating an unrelated const B = null;, or silently merged with other BigInts that have different numeric values. Use the literal's bigint string property (node.bigint) to build a dedicated key, e.g. `bigint:${node.bigint}`, and add a test case covering it.

}

if (node.type === AST_NODE_TYPES.TemplateLiteral && node.expressions.length === 0) {
return `string:${node.quasis[0].value.cooked ?? node.quasis[0].value.raw}`;
}

if (node.type === AST_NODE_TYPES.UnaryExpression && (node.operator === "+" || node.operator === "-") && node.argument.type === AST_NODE_TYPES.Literal && typeof node.argument.value === "number") {
return `number:${String(node.operator === "-" ? -node.argument.value : node.argument.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.

[/tdd] A no-expressions template literal const A = `hello`; const B = `hello`; is not tested as an invalid case — only the template-matches-string-literal case is. Add a test to confirm the rule fires for two identical template literals.

@copilot please address this.

}

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.

[/tdd] The unary + operator case (e.g. const A = +42; const B = +42;) is handled in getStaticValueKey but has no test coverage. It is also worth confirming that +42 and 42 are intentionally treated as duplicates — if so, add a test; if not, the key should encode the operator.

@copilot please address this.

export const noDuplicateConstantValuesRule = createRule({
name: "no-duplicate-constant-values",
meta: {
type: "suggestion",
docs: {
description: "List module-level constant declarations by their static primitive values and report later declarations that duplicate a value in the same file.",
},
schema: [],
messages: {
duplicateConstantValue: "Constant '{{name}}' duplicates the value of constant '{{originalName}}' ({{value}}).",
},
},
defaultOptions: [],
create(context) {
const constantsByValue = new Map<string, ConstantDeclaration>();

return {
VariableDeclaration(node) {
if (node.kind !== "const" || node.parent.type !== AST_NODE_TYPES.Program) {

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.

This guard silently skips every export const declaration, making the rule a near no-op on typical modules.

💡 Details

For export const X = ...;, the VariableDeclaration node's parent is ExportNamedDeclaration (or ExportDefaultDeclaration), not Program. The check node.parent.type !== AST_NODE_TYPES.Program therefore returns early and the loop over node.declarations never runs for exported constants — arguably the most common form of top-level constant in real modules.

Fix: also allow the export wrapper:

const isTopLevel =
  node.parent.type === AST_NODE_TYPES.Program ||
  (node.parent.type === AST_NODE_TYPES.ExportNamedDeclaration && node.parent.parent.type === AST_NODE_TYPES.Program);
if (node.kind !== "const" || !isTopLevel) return;

No test in this PR exercises export const, so this false-negative went undetected.

return;
}

for (const declaration of node.declarations) {
if (declaration.id.type !== AST_NODE_TYPES.Identifier || !declaration.init) {
continue;
}

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] No test covers a const inside a namespace or module block — the node.parent.type !== Program guard correctly blocks function-local scope, but TypeScript module blocks have TSModuleBlock as the parent, not Program. Verify whether that case should be flagged or ignored and add a test.

@copilot please address this.


const valueKey = getStaticValueKey(declaration.init);
if (valueKey === null) {
continue;
}

const original = constantsByValue.get(valueKey);
if (!original) {
constantsByValue.set(valueKey, {

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.

Any two unrelated constants that coincidentally share a common trivial value (0, 1, "", true, null) will be flagged, which will likely be extremely noisy in real production .cjs files.

💡 Details

The rule's premise (matching by structural equality of the initializer value alone, with no relation to identifier name/semantics) means any file with e.g. const RETRY_COUNT = 3; and const MAX_WORKERS = 3; gets a warning even though these constants are unrelated. Since this is enabled as warn on all production .cjs files, expect a wave of false-positive warnings on merge, especially for common values like 0, 1, "", true/false. Consider restricting the rule to only flag duplicates of "non-trivial" values (e.g., strings longer than N chars, or numbers outside a small allow-list like -1/0/1) to keep signal-to-noise reasonable, or gate it behind an opt-in option.

name: declaration.id.name,
});
continue;
}

context.report({
node: declaration,
messageId: "duplicateConstantValue",
data: {
name: declaration.id.name,
originalName: original.name,
value: context.sourceCode.getText(declaration.init),
},
});
}
},
};
},
});