Skip to content

[WIP] Fix false positives in require-nan-check rule - #50556

Merged
pelikhan merged 2 commits into
mainfrom
copilot/require-nan-check-after-env-numeric-parse-again
Aug 5, 2026
Merged

[WIP] Fix false positives in require-nan-check rule#50556
pelikhan merged 2 commits into
mainfrom
copilot/require-nan-check-after-env-numeric-parse-again

Conversation

Copilot AI commented Aug 5, 2026

Copy link
Copy Markdown
Contributor
  • Treat Number.isFinite(x) / isFinite(x) as validating
  • Treat truthiness guards (if (x), if (!x), ternary test) on the tracked identifier as validating
  • Update rule message text
  • Add valid-case tests for all 4 reported patterns
  • Verify the 4 grounded files are no longer flagged

…nv-numeric-parse

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI requested a review from pelikhan August 5, 2026 13:22
@pelikhan
pelikhan marked this pull request as ready for review August 5, 2026 13:24
Copilot AI balanced review requested due to automatic review settings August 5, 2026 13:24
@pelikhan
pelikhan merged commit fb136c8 into main Aug 5, 2026
@pelikhan
pelikhan deleted the copilot/require-nan-check-after-env-numeric-parse-again branch August 5, 2026 13:25

Copilot AI left a comment

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.

Pull request overview

Extends NaN-validation detection for environment-derived numeric parsing.

Changes:

  • Recognizes finite checks and direct truthiness guards.
  • Updates diagnostics and adds grounded valid-case tests.
Show a summary per file
File Description
require-nan-check-after-env-numeric-parse.ts Expands validation detection.
require-nan-check-after-env-numeric-parse.test.ts Tests finite and truthiness guards.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Suppressed comments (2)

eslint-factory/src/rules/require-nan-check-after-env-numeric-parse.ts:131

  • Validation is recorded by identifier text rather than by the tracked binding. A truthiness check on a same-named parameter or local in another scope can therefore clear the diagnostic for the parsed value—for example, const port = parseInt(process.env.PORT, 10); function f(port) { if (port) use(port); } use(port);. Resolve the test identifier to its scope variable/declarator and mark only that tracked binding as validated.
      if (expr.type === "Identifier") {
        validated.add(expr.name);

eslint-factory/src/rules/require-nan-check-after-env-numeric-parse.ts:113

  • Number.isFinite is also accepted when Number is a locally shadowed object, so code such as const Number = { isFinite: () => true }; const n = parseInt(process.env.N, 10); Number.isFinite(n); incorrectly marks n validated. Require Number to resolve to the unshadowed global before accepting this call.
        callee.object.type === "Identifier" &&
        callee.object.name === "Number" &&
        !callee.computed &&
        callee.property.type === "Identifier" &&
        (callee.property.name === "isNaN" || callee.property.name === "isFinite")
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

// Global isNaN(x)
if (callee.type === "Identifier" && callee.name === "isNaN") {
// Global isNaN(x) / isFinite(x)
if (callee.type === "Identifier" && (callee.name === "isNaN" || callee.name === "isFinite")) {
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Great work on fixing the false positives in the require-nan-check-after-env-numeric-parse rule! 🎯

This PR successfully extends validation detection to recognize Number.isFinite(), isFinite(), and truthiness guards (if (!x), if (x), ternary) as valid NaN checks, addressing all four grounded false positive patterns documented in #50495. The implementation mirrors the existing isIsNaNCall pattern cleanly and the test coverage validates all reported scenarios.

The diff is focused, the tests are comprehensive, and the PR message clearly describes the work done. Ready for merge! ✨

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.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "patchdiff.githubusercontent.com"

See Network Configuration for more information.

Generated by ✅ Contribution Check · auto · 54.1 AIC · ⌖ 3.87 AIC · ⊞ 8.8K ·

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.85.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

require-nan-check-after-env-numeric-parse: false positives on Number.isFinite() and truthiness NaN guards

3 participants