Extend require-parseInt-radix to computed and global-object parseInt calls - #42027
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42027 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. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR expands the require-parseInt-radix ESLint rule in eslint-factory to catch additional parseInt(...) call shapes that previously slipped through, specifically computed member access and common global-object receivers, while explicitly keeping alias/destructure tracking out of scope.
Changes:
- Extend detection to computed
Number["parseInt"](...)calls missing a radix. - Extend detection to
globalThis|window|global .parseInt(...)(and computed"parseInt") missing a radix, while avoiding reports when those identifiers are locally bound. - Add a new test suite covering the newly supported access forms and explicit “out of scope”/shadowing cases.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-parseInt-radix.ts | Adds scope-aware detection for computed and global-object parseInt member calls without radix. |
| eslint-factory/src/rules/require-parseInt-radix.test.ts | Introduces tests for the expanded detection surface and shadowing/non-goal cases. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
| const isDirectAccess = property.type === "Identifier" && property.name === "parseInt"; | ||
| const isComputedAccess = property.type === "Literal" && property.value === "parseInt"; | ||
|
|
||
| return isDirectAccess || isComputedAccess; |
| it("invalid: computed Number.parseInt access without radix is flagged", () => { | ||
| cjsRuleTester.run("require-parseInt-radix", requireParseIntRadixRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `Number["parseInt"](value);`, | ||
| errors: [{ messageId: "requireRadix" }], | ||
| }, | ||
| ], | ||
| }); | ||
| }); |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (4 tests analyzed)
Language breakdown: Go: 0; JavaScript/TypeScript: 4 ( Test notes:
Verdict
|
![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 /improve-codebase-architecture — requesting changes on test coverage and naming.
📋 Key Themes & Highlights
Key Themes
- Missing baseline regression test (
/tdd): The refactoredhasLocalBindingpath for bareparseInt(value)has no invalid test case — the most important gap since this is the rule's original behaviour and the first-ever test file for it. - Test granularity (
/tdd): Multiple assertions bundled into singleitblocks make failure diagnosis harder; the sibling rule sets a one-case-per-itconvention. - ESM invalid coverage (
/tdd): All invalid cases run only undercjsRuleTester; ESM scope semantics affectinghasLocalBindingare untested. - Constant naming and coverage (
/improve-codebase-architecture):GLOBAL_PARSE_INT_OBJECTSconflates a static built-in (Number) with global-object aliases (globalThis,window,global).self(browser/Web Worker alias) is also missing.
Positive Highlights
- ✅ The
hasLocalBindingscope-walk correctly prevents false positives on locally-shadowed identifiers — the logic is sound. - ✅ Scope boundary is explicitly documented in both the PR description and inline comments, making the intentional non-goals clear.
- ✅ The computed-access (
Number["parseInt"]) and global-object detection are cleanly unified throughisParseIntProperty, keeping the rule implementation readable. - ✅ Valid cases for shadowed
globalThis/window/globalidentifiers are well thought out.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 77.8 AIC · ⌖ 7.42 AIC · ⊞ 6.6K
| }); | ||
| }); | ||
|
|
||
| it("invalid: computed Number.parseInt access without radix is flagged", () => { |
There was a problem hiding this comment.
[/tdd] Missing baseline regression test — parseInt(value) (original behaviour) has no invalid case.
This is the first test file for this rule, and the hasLocalBinding change refactored the bare-call detection path. Without an invalid case for parseInt(value), a regression in that path would go undetected.
💡 Suggested addition
Add before this block:
it("invalid: bare global parseInt without radix is flagged", () => {
cjsRuleTester.run("require-parseInt-radix", requireParseIntRadixRule, {
valid: [],
invalid: [
{
code: `parseInt(value);`,
errors: [{ messageId: "requireRadix" }],
},
],
});
});@copilot please address this.
| }); | ||
| }); | ||
|
|
||
| it("invalid: global-object parseInt access without radix is flagged", () => { |
There was a problem hiding this comment.
[/tdd] Six invalid cases share one it block — consider splitting into one it per access form.
The sibling require-json-parse-try-catch.test.ts gives each logical scenario its own it. Bundling 6 cases here makes it hard to pinpoint which specific form failed when the test suite turns red.
💡 Suggested structure
it("invalid: globalThis.parseInt without radix is flagged", () => { ... });
it("invalid: globalThis[\"parseInt\"] without radix is flagged", () => { ... });
it("invalid: window.parseInt without radix is flagged", () => { ... });
it("invalid: window[\"parseInt\"] without radix is flagged", () => { ... });
it("invalid: global.parseInt without radix is flagged", () => { ... });
it("invalid: global[\"parseInt\"] without radix is flagged", () => { ... });This also mirrors the granularity of the valid-cases suite above.
@copilot please address this.
| }); | ||
| }); | ||
|
|
||
| it("valid: aliased and destructured bindings remain out of scope", () => { |
There was a problem hiding this comment.
[/tdd] All invalid cases use cjsRuleTester — no ESM invalid coverage.
esmRuleTester is only used for the valid/aliased block. The sibling rule (require-json-parse-try-catch.test.ts) adds a dedicated ESM invalid case (line 72). ESM introduces different scope semantics that could affect hasLocalBinding; at minimum, one invalid case (e.g. Number["parseInt"](value)) should run against esmRuleTester to confirm identical behaviour in module context.
@copilot please address this.
| import { ESLintUtils, TSESTree } from "@typescript-eslint/utils"; | ||
|
|
||
| const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/actions/setup/js/eslint-factory#${name}`); | ||
| const GLOBAL_PARSE_INT_OBJECTS = new Set(["Number", "globalThis", "window", "global"]); |
There was a problem hiding this comment.
[/improve-codebase-architecture] GLOBAL_PARSE_INT_OBJECTS conflates two distinct concepts — Number is a built-in constructor, not a global-object alias.
The constant mixes Number (a built-in whose parseInt is a static method) with globalThis/window/global (runtime aliases for the global object). The name implies all four are 'global objects', which is misleading. A clearer name would be PARSE_INT_HOSTS or KNOWN_PARSE_INT_NAMESPACES. Additionally, self is a valid global-object alias in browser and Web Worker environments — self.parseInt(value) currently escapes the rule.
💡 Suggestions
Rename and extend:
const PARSE_INT_HOSTS = new Set(["Number", "globalThis", "window", "global", "self"]);Or, if you want to keep the structural split clear:
// Number holds parseInt as a static method
const PARSE_INT_STATIC_OBJECTS = new Set(["Number"]);
// Runtime aliases for the global object
const GLOBAL_OBJECT_ALIASES = new Set(["globalThis", "window", "global", "self"]);@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — two issues that should be fixed before this lands.
Blocking issues (2)
isParseIntProperty — false positive on computed variable access
The isDirectAccess branch (property.type === "Identifier" && property.name === "parseInt") does not guard against node.computed === true. A computed MemberExpression like Number[parseInt] (where parseInt is a locally declared variable) has an Identifier property node with name === "parseInt", so it matches isDirectAccess and is incorrectly reported. The original code explicitly checked !node.callee.computed and the refactor dropped that guard. Fixing it requires branching on node.computed in isParseIntProperty — see inline comment for the patch.
Test file covers only new cases; original parseInt(value) and Number.parseInt(value) paths are untested as invalid
The test file is brand new and never asserts that the two cases the rule was originally written to catch actually fire. A regression in the Identifier branch or a GLOBAL_PARSE_INT_OBJECTS edit that drops "Number" would merge silently. Inline comment has the suggested test block.
🔎 Code quality review by PR Code Quality Reviewer · 102 AIC · ⌖ 7.46 AIC · ⊞ 5.2K
| */ | ||
| function isParseIntProperty(node: TSESTree.MemberExpression): boolean { | ||
| const property = node.property; | ||
| const isDirectAccess = property.type === "Identifier" && property.name === "parseInt"; |
There was a problem hiding this comment.
isDirectAccess falsely matches Number[parseInt] (computed access with a variable named parseInt), producing a false positive.
When node.computed === true and the bracket expression is a variable named parseInt — e.g. Number[parseInt](value) — the property AST node is Identifier { name: "parseInt" }. This is identical to the Number.parseInt non-computed case, so isDirectAccess returns true and the call gets incorrectly flagged. The original code avoided this by explicitly asserting !node.callee.computed before checking property name.
💡 Suggested fix
Gate each branch on node.computed:
function isParseIntProperty(node: TSESTree.MemberExpression): boolean {
if (!node.computed) {
// Number.parseInt — property is a non-computed Identifier
return node.property.type === "Identifier" && (node.property as TSESTree.Identifier).name === "parseInt";
}
// Number["parseInt"] — property must be a string literal, not a variable
return node.property.type === "Literal" && (node.property as TSESTree.Literal).value === "parseInt";
}Concrete false-positive case that currently fires incorrectly:
const parseInt = "someKey";
Number[parseInt](value); // ← rule reports missing radix; should be silent| }, | ||
| ], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The two original cases (parseInt(value) and Number.parseInt(value)) have no invalid test coverage after the refactor — a silent regression in either path would go undetected.
The old rule used two explicit conditional blocks for these cases. The refactor merges them into a generic handler, but the new test suite only validates the newly-added code paths. The parseInt(value) case is the primary violation this rule exists to catch and it has zero invalid assertions anywhere in the file.
💡 Suggested addition
Add a dedicated it block (or extend an existing one) covering both original cases:
it("invalid: bare parseInt and Number.parseInt without radix are flagged", () => {
cjsRuleTester.run("require-parseInt-radix", requireParseIntRadixRule, {
valid: [],
invalid: [
{
code: `parseInt(value);`,
errors: [{ messageId: "requireRadix" }],
},
{
code: `Number.parseInt(value);`,
errors: [{ messageId: "requireRadix" }],
},
],
});
});Without these, a typo in the Identifier check on line 69 of the rule, or a change to GLOBAL_PARSE_INT_OBJECTS that accidentally drops "Number", would merge unnoticed.
|
🎉 This pull request is included in a new release. Release: |
require-parseInt-radixonly caught bareparseInt(...)and directNumber.parseInt(...), leaving the same false-negative class already closed forJSON["parse"]in the sibling rule. This change expands coverage to computed member access and global-object receivers while keeping alias/destructure tracking explicitly out of scope.Rule coverage
Number["parseInt"](...)globalThis.parseInt(...),window.parseInt(...), andglobal.parseInt(...)Intentional scope boundary
const p = parseInt; p(value)const { parseInt } = Number; parseInt(value)Tests
parseInt,globalThis,window, andglobalidentifiers are not reported