Add ESLint rule for duplicate constant values - #48657
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (default_business_additions=0). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Adds a custom ESLint rule to detect duplicate module-level constant values in production .cjs files.
Changes:
- Implements duplicate-value detection for static initializers.
- Adds registration, configuration, tests, and documentation.
- Reports subsequent duplicates while ignoring unsupported declarations.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/no-duplicate-constant-values.ts |
Implements the rule. |
eslint-factory/src/rules/no-duplicate-constant-values.test.ts |
Adds focused rule tests. |
eslint-factory/src/index.ts |
Registers the rule. |
eslint-factory/README.md |
Documents supported behavior. |
eslint-factory/eslint.config.cjs |
Enables the rule for production CJS files. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
eslint-factory/src/rules/no-duplicate-constant-values.ts:23
- Negative bigint initializers are valid static primitive values, but this guard only accepts
number, so declarations such asconst FIRST = -1n; const SECOND = -1n;are silently ignored even though bigint and signed numeric initializers are documented as supported.
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)}`;
}
eslint-factory/src/rules/no-duplicate-constant-values.ts:22
- Serializing signed numbers with
Stringcollapses-0to"0", soconst POSITIVE = 0; const NEGATIVE = -0;is reported as a duplicate even though JavaScript preserves an observable negative-zero value (for example,1 / NEGATIVEis-Infinity). Preserve the sign when building this key.
return `number:${String(node.operator === "-" ? -node.argument.value : node.argument.value)}`;
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Medium
| if ("regex" in node && node.regex) { | ||
| return `regexp:${node.regex.pattern}/${node.regex.flags}`; | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on a correctness gap and missing test coverage.
📋 Key Themes & Highlights
Key Issues
- Correctness: Regex flag-order variance (
/givs/ig) produces different map keys, so semantically-identical regexes are not flagged as duplicates. This is the most impactful fix. - Test gaps: The unary
+path and template-literal-vs-template-literal duplicate case have no invalid test cases. - Design nit: The single-field
ConstantDeclarationinterface adds indirection; aMap<string, string>is simpler. - Scope edge:
constinside a TypeScriptnamespace/moduleblock has a different parent type (TSModuleBlock) — behaviour should be specified and tested.
Positive Highlights
- ✅ Clean
getStaticValueKeyabstraction — easy to extend to new literal types - ✅ Good scope filtering (ignores function-local,
let/var, destructuring, dynamic expressions) - ✅ Tests are well-named and serve as specifications
- ✅ Integration (registration + config + docs) is minimal and consistent with existing rules
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 40.2 AIC · ⌖ 4.95 AIC · ⊞ 6.7K
Comment /matt to run again
| if (node.type === AST_NODE_TYPES.Literal) { | ||
| if ("regex" in node && node.regex) { | ||
| return `regexp:${node.regex.pattern}/${node.regex.flags}`; | ||
| } |
There was a problem hiding this comment.
[/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.
|
|
||
| interface ConstantDeclaration { | ||
| name: string; | ||
| } |
There was a problem hiding this comment.
[/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.
| } | ||
|
|
||
| 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)}`; |
There was a problem hiding this comment.
[/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; | ||
| } | ||
|
|
There was a problem hiding this comment.
[/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.
| for (const declaration of node.declarations) { | ||
| if (declaration.id.type !== AST_NODE_TYPES.Identifier || !declaration.init) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[/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.
There was a problem hiding this comment.
Review: Add ESLint rule for duplicate constant values
Overall the implementation is clean and well-tested. One non-blocking suggestion:
Regex flag ordering — The key regexp:<pattern>/<flags> preserves the original flag order, so /x/gi and /x/ig produce different keys and won't be flagged as duplicates. Sorting the flags before joining fixes this (see inline comment).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20.6 AIC · ⌖ 5.67 AIC · ⊞ 5K
| 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}`; |
There was a problem hiding this comment.
Regex flags are not normalised before building the key, so /x/gi and /x/ig would not be detected as duplicates even though they behave identically.
Consider sorting the flags:
return `regexp:${node.regex.pattern}/${[...node.regex.flags].sort().join("")}`;@copilot please address this.
There was a problem hiding this comment.
Blocking: the rule silently skips exported top-level constants, defeating its own purpose, and its unqualified structural-value matching will produce noisy false positives on common trivial values.
💡 Themes
- Critical (functional no-op on exports):
node.parent.type !== AST_NODE_TYPES.Programfails to recognizeexport const ...declarations, since their parent isExportNamedDeclaration. Since exported top-levelconstis extremely common, this bug likely makes the rule detect almost nothing in real files. - High (noise/design): The rule flags any two unrelated constants sharing a common primitive value (0, 1, "", true, null) as "duplicates" with no semantic relationship required. Enabled as
warnon production.cjsfiles, this risks a wave of false-positive warnings. - Medium (correctness edge case): BigInt literal handling can collide with
nulldue totypeof node.valuereturningobjectwith an unpopulated value for BigInt nodes in some parser configurations. - Additional minor gaps noted only for context, not blocking: nested unary expressions (
- -1) aren't recognized as constant, and-0/0collapse to the same key — neither raised as separate comments since they're lower-impact false negatives/positives.
Please address the exported-const detection bug and reconsider the noise level of unqualified value matching before merge.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 51.6 AIC · ⌖ 4.6 AIC · ⊞ 7.4K
Comment /review to run again
|
|
||
| return { | ||
| VariableDeclaration(node) { | ||
| if (node.kind !== "const" || node.parent.type !== AST_NODE_TYPES.Program) { |
There was a problem hiding this comment.
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.
|
|
||
| const original = constantsByValue.get(valueKey); | ||
| if (!original) { | ||
| constantsByValue.set(valueKey, { |
There was a problem hiding this comment.
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.
| if ("regex" in node && node.regex) { | ||
| return `regexp:${node.regex.pattern}/${node.regex.flags}`; | ||
| } | ||
| return `${typeof node.value}:${String(node.value)}`; |
There was a problem hiding this comment.
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.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit |
|
🎉 This pull request is included in a new release. Release: |
Adds duplicate-value detection for module-level constants in production
.cjsfiles.Rule
constdeclarations with static primitive initializers.Integration
no-duplicate-constant-valuesin the custom ESLint plugin..cjsfiles.Run: https://github.com/github/gh-aw/actions/runs/30375513874