Skip to content

prefer-number-isnan: treat ESM import bindings as local shadows #43500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Jul 5, 2026
+22 −1
Merged

prefer-number-isnan: treat ESM import bindings as local shadows #43500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5c8615
Initial plan
Copilot Jul 5, 2026
69d37b5
fix(eslint-factory): treat import bindings as local shadows in prefer…
Copilot Jul 5, 2026
1e59acd
test(eslint-factory): add globalThis ESM import-shadow case and ESM i…
Copilot Jul 5, 2026
7857489
Merge remote-tracking branch 'origin/main' into copilot/fix-prefer-nu…
Copilot Jul 5, 2026
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
20 changes: 20 additions & 0 deletions eslint-factory/src/rules/prefer-number-isnan.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,13 @@ describe("prefer-number-isnan", () => {
valid: [
`function isNaN(value) { return false; } isNaN(value);`,
`const isNaN = Number.isNaN; isNaN(value);`,
`import { isNaN } from "lodash"; isNaN(value);`,
`const globalThis = { isNaN(value) { return value; } }; globalThis.isNaN(value);`,
`const window = { isNaN(value) { return value; } }; window["isNaN"](value);`,
`const global = { isNaN(value) { return value; } }; global.isNaN(value);`,
`import { globalThis } from "./global-shim"; globalThis.isNaN(value);`,
`import { window } from "./browser-shim"; window.isNaN(value);`,
`import { global } from "./server-shim"; global["isNaN"](value);`,

@github-actions github-actions Bot Jul 5, 2026

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.

Missing ESM invalid test: no test proves the rule still flags isNaN() in ESM modules after this change, leaving a correctness regression undetectable.

💡 Explanation and suggested fix

All invalid: test cases use cjsRuleTester. The fix changes scope-walking behavior, which is determined per sourceType. If the new variable && variable.defs.length > 0 branch misbehaves in ESM mode (e.g., due to how eslint scope analysis treats globals under sourceType: "module"), no test would catch it.

Consider adding to the new esmRuleTester.run block:

esmRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, {
  valid: [ /* existing */ ],
  invalid: [
    {
      code: `isNaN(value);`,
      errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }],
    },
    {
      code: `window.isNaN(value);`,
      errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }],
    },
  ],
});

Without this, the PR's test coverage only validates the non-regression path (valid cases), not that the rule still fires in ESM when no import shadowing is present.

// Dynamic computed access — identifier property reference, not string literal "isNaN"

@github-actions github-actions Bot Jul 5, 2026

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.

Incomplete test symmetry: globalThis is in GLOBAL_IS_NAN_OBJECTS but has no import-shim valid case.

💡 Suggested addition

The three new valid ESM tests cover import { isNaN }, import { window }, and import { global } — but not import { globalThis }. Since globalThis is an equal member of GLOBAL_IS_NAN_OBJECTS, a missing test here leaves asymmetric coverage.

Consider adding:

`import { globalThis } from "./global-shim"; globalThis.isNaN(value);`,

Low risk by itself, but the gap creates a mental model mismatch between the rule's constant and the test suite.

`globalThis[isNaN](value);`,
],
Expand Down Expand Up @@ -99,4 +103,20 @@ describe("prefer-number-isnan", () => {
],
});
});

it("invalid: global isNaN() is still flagged in ESM mode without a shadow", () => {
esmRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, {
valid: [],
invalid: [
{
code: `isNaN(value);`,
errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }],
},
{
code: `window.isNaN(value);`,
errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }],
},
],
});
});
});
3 changes: 2 additions & 1 deletion eslint-factory/src/rules/prefer-number-isnan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ export const preferNumberIsNanRule = createRule({
while (scope) {
const variable = scope.set.get(name);

if (variable?.defs.some(d => d.type !== "ImportBinding")) {
// Any local definition shadows the global (including ESM ImportBinding).
if (variable && variable.defs.length > 0) {
return true;
}

Expand Down
Loading