fix(eslint-factory): require-parseInt-radix false negative on radix 0/undefined#42581
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… and guards spread fix Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the require-parseInt-radix ESLint rule in eslint-factory to close a false-negative gap where parseInt(x, 0) and parseInt(x, undefined) were being treated as “safe enough”, despite being spec-equivalent to omitting the radix.
Changes:
- Tightens the rule’s “has radix” check by validating the second argument (rejecting
0andundefined). - Avoids offering a broken
, 10suggestion when the first argument is a spread element. - Extends unit tests to cover the new invalid/valid cases and the spread-element no-suggestion behavior.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-parseInt-radix.ts | Implements second-argument validation and suppresses broken spread-argument suggestions. |
| eslint-factory/src/rules/require-parseInt-radix.test.ts | Adds coverage for radix 0/undefined, identifier radix policy, and spread-argument suggestion suppression. |
| .github/workflows/daily-team-status.lock.yml | Regenerated lockfile output for the Team Status workflow. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
| // The identifier `undefined` is equivalent to no radix | ||
| if (arg.type === "Identifier" && arg.name === "undefined") { | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Fixed in f5a6dde. isValidSecondArg() now only rejects the global undefined binding, so locally shadowed identifiers stay on the existing non-literal path.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: require-parseInt-radix false-negative fix
Good fix overall — the length >= 2 short-circuit was genuinely broken and the spread-element guard is correct. Three issues worth addressing before merge:
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | 🔴 Correctness | isValidSecondArg L67 |
parseInt(str, null) is a false negative — ToInt32(null) === 0 makes it spec-equivalent to no radix |
| 2 | 🟡 Usability | suggest L87 |
No autofix suggestion when radix is 0 or undefined; the error fires but the user gets no addRadix10 suggestion to replace the bad argument |
| 3 | 🟡 Test coverage | test L207 | undefined radix test is missing window["parseInt"] and global.parseInt variants (5 call forms tested for radix 0, only 3 for undefined) |
Why the null case matters
parseInt(str, null) is the same hazard this rule exists to prevent: a string like "0xff" will be silently parsed as 255. null is not excluded from spec hex-detection the same way a valid non-zero radix is. The fix is a one-liner alongside the existing 0 check.
Suggestion for the autofix gap
When arguments.length >= 2 but isValidSecondArg returns false, the current code emits undefined for suggest. Offering fixer.replaceText(node.arguments[1], "10") would give the same developer experience as the insert-radix path.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 43 AIC · ⌖ 7.09 AIC · ⊞ 4.9K
| */ | ||
| function isValidSecondArg(arg: TSESTree.CallExpressionArgument): boolean { | ||
| // Literal 0 is spec-equivalent to no radix | ||
| if (arg.type === "Literal" && arg.value === 0) { |
There was a problem hiding this comment.
null radix is also spec-equivalent to no radix — false negative.
parseInt(str, null) is not caught here: ToInt32(null) === 0, so the spec treats it identically to omitting the radix (implicit hex-detection hazard). A companion check is needed:
// Literal null coerces to 0 — same hazard as literal 0
if (arg.type === "Literal" && arg.value === null) {
return false;
}@copilot please address this.
There was a problem hiding this comment.
Fixed in f5a6dde. null is now treated as spec-equivalent to an omitted radix and covered by the new invalid-radix tests.
| const firstArg = node.arguments[0]; | ||
| const suggest = | ||
| node.arguments.length === 1 | ||
| node.arguments.length === 1 && firstArg.type !== "SpreadElement" |
There was a problem hiding this comment.
No autofix suggestion offered when radix is 0 or undefined.
When parseInt(str, 0) or parseInt(str, undefined) is flagged, suggest is undefined because node.arguments.length === 1 is false. The user sees the error but gets no guidance on how to fix it.
Consider adding a suggestion that replaces the bad radix argument rather than inserting one:
const suggest: TSESLint.ReportSuggestionArray<"addRadix10"> | undefined =
node.arguments.length >= 2 && !isValidSecondArg(node.arguments[1])
? [{ messageId: "addRadix10", fix: fixer => fixer.replaceText(node.arguments[1] as TSESTree.Node, "10") }]
: node.arguments.length === 1 && firstArg.type !== "SpreadElement"
? [{ messageId: "addRadix10", fix: fixer => fixer.insertTextAfter(firstArg, ", 10") }]
: undefined;@copilot please address this.
There was a problem hiding this comment.
Fixed in f5a6dde. Known-bad radix arguments now get an addRadix10 suggestion that replaces the invalid second argument with 10.
| { | ||
| code: `globalThis.parseInt(str, undefined);`, | ||
| errors: [{ messageId: "requireRadix" }], | ||
| }, |
There was a problem hiding this comment.
Incomplete call-form coverage for undefined radix.
The radix 0 test covers all 5 call forms (parseInt, Number.parseInt, globalThis.parseInt, window["parseInt"], global.parseInt). The undefined test only covers 3 of them — window["parseInt"](str, undefined) and global.parseInt(str, undefined) are missing.
Please add:
{
code: `window["parseInt"](str, undefined);`,
errors: [{ messageId: "requireRadix" }],
},
{
code: `global.parseInt(str, undefined);`,
errors: [{ messageId: "requireRadix" }],
},@copilot please address this.
There was a problem hiding this comment.
Fixed in f5a6dde. The undefined-radix tests now include the missing window["parseInt"] and global.parseInt call forms.
Review SummaryApplied 📝 Issues found (5)
Issue #1 (null radix) is a correctness gap in the same category as the bug being fixed — worth addressing before merge. Issues #2–5 strengthen the implementation and test suite but are non-blocking if you prefer a follow-up. @copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one correctness gap and a few test coverage issues.
📋 Key Themes & Highlights
Key Themes
- Correctness gap —
nullradix:parseInt(str, null)is spec-equivalent to no radix (null coerces to 0) but is not rejected byisValidSecondArg. Same category as the0case this PR fixes. - Missing fix suggestion for known-bad radix: When
0orundefinedis passed, the rule correctly flags the call but offers no suggestion — unlike the no-radix case which proposesaddRadix10. AreplaceRadix10suggestion would be consistent and helpful. - Test coverage gaps: The
undefinedradix test block covers only 3 of 5 global variants; the spread test only covers the bareparseIntform; and none of the new invalid tests explicitly assertsuggestions: [].
Positive Highlights
- ✅ Root cause correctly identified and fixed: the
length >= 2short-circuit was the right place to intervene. - ✅
isValidSecondArgis cleanly extracted, well-documented, and easy to extend. - ✅ Spread-argument guard prevents emitting broken autofix output — good defensive thinking.
- ✅ Non-literal identifier radix (
parseInt(str, base)) is correctly accepted with a clear policy comment. - ✅ The new tests are well-named and readable as specs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 62.9 AIC · ⌖ 8.87 AIC · ⊞ 6.6K
Comment /matt to run again
🧪 Test Quality Sentinel Report✅ Test Quality Score: 81/100 — Excellent
📊 Metrics & Test Classification (11 tests analyzed)
Go: 0 (
|
There was a problem hiding this comment.
One additional false negative missed by the current isValidSecondArg implementation.
### Finding: `NaN` radix — same hazard class as `null` and `0`
parseInt(str, NaN) passes isValidSecondArg unchanged because the function only rejects Identifier nodes named "undefined". NaN is a different identifier that coerces to 0 via ToInt32, triggering the same implicit base detection. Fix: add arg.name === "NaN" to the identifier check on line 74.
This is the same pattern as the null false negative already flagged in the existing review — both are known-bad spec-equivalent values not caught by the current identifier blocklist.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 155.6 AIC · ⌖ 7.08 AIC · ⊞ 1.6K
Comment /review to run again
🤖 PR Triage — §28486872548
Score breakdown: Impact 20/50 · Urgency 10/30 · Quality 14/20 Batch: Rationale: Bug fix for false-negative radix detection in Labels applied:
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
require-parseInt-radixshort-circuited onarguments.length >= 2, treating any second argument as a valid radix. This silently acceptedparseInt(x, 0)andparseInt(x, undefined)— both spec-equivalent to omitting the radix, reintroducing the implicit hex-detection hazard the rule exists to prevent.Changes
require-parseInt-radix.tslength >= 2early return withisValidSecondArg(): rejects literal0and the identifierundefined; accepts all other values (including non-literal identifiers likebase— static verification isn't possible, but they're not known-bad literals), 10autofix suggestion when the first argument is aSpreadElement—parseInt(...args, 10)is broken outputrequire-parseInt-radix.test.tsparseInt(x, base)to valid cases (non-literal identifier policy)0andundefinedacross all call forms (parseInt,Number.parseInt,globalThis.*, etc.)parseInt(...args)is flagged but offers no suggestion