Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 16 additions & 0 deletions eslint-factory/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,19 @@ This project hosts custom ESLint linters for `/actions/setup/js`.
- `npm run build` — compile rule sources.
- `npm run lint:setup-js` — build and lint all `../actions/setup/js/**/*.cjs` files.
- `npm run lint:setup-js:changed` — build and lint `../actions/setup/js/*.cjs` files.

## Rules

### `prefer-number-isnan`

Prefer `Number.isNaN()` over global `isNaN()` to avoid silent coercion of non-numeric inputs.

Global `isNaN()` coerces its argument before testing, so `isNaN("123")` returns `false` because `"123"` coerces to the number `123` — masking that the input was a string. `Number.isNaN()` is strict and does not coerce, making numeric validation reliable when handling raw inputs such as environment variables or API strings.

Flagged forms:
- `isNaN(x)`
- `globalThis.isNaN(x)` / `globalThis["isNaN"](x)`
- `window.isNaN(x)` / `window["isNaN"](x)`
- `global.isNaN(x)` / `global["isNaN"](x)`

Locally shadowed bindings (e.g. `const isNaN = Number.isNaN`) are intentionally excluded.
1 change: 1 addition & 0 deletions eslint-factory/eslint.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ module.exports = [
"gh-aw-custom/no-unsafe-catch-error-property": "warn",
"gh-aw-custom/no-unsafe-promise-catch-error-property": "warn",
"gh-aw-custom/prefer-get-error-message": "warn",
"gh-aw-custom/prefer-number-isnan": "warn",
"gh-aw-custom/require-async-entrypoint-catch": "warn",
"gh-aw-custom/require-await-core-summary-write": "warn",
"gh-aw-custom/require-json-parse-try-catch": "warn",
Expand Down
2 changes: 2 additions & 0 deletions eslint-factory/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { noCoreSetOutputNonStringRule } from "./rules/no-core-setoutput-non-stri
import { noUnsafeCatchErrorPropertyRule } from "./rules/no-unsafe-catch-error-property";
import { noUnsafePromiseCatchErrorPropertyRule } from "./rules/no-unsafe-promise-catch-error-property";
import { preferGetErrorMessageRule } from "./rules/prefer-get-error-message";
import { preferNumberIsNanRule } from "./rules/prefer-number-isnan";
import { requireAsyncEntrypointCatchRule } from "./rules/require-async-entrypoint-catch";
import { requireAwaitCoreSummaryWriteRule } from "./rules/require-await-core-summary-write";
import { requireJsonParseTryCatchRule } from "./rules/require-json-parse-try-catch";
Expand All @@ -17,6 +18,7 @@ const plugin = {
"no-unsafe-catch-error-property": noUnsafeCatchErrorPropertyRule,
"no-unsafe-promise-catch-error-property": noUnsafePromiseCatchErrorPropertyRule,
"prefer-get-error-message": preferGetErrorMessageRule,
"prefer-number-isnan": preferNumberIsNanRule,
"require-async-entrypoint-catch": requireAsyncEntrypointCatchRule,
"require-await-core-summary-write": requireAwaitCoreSummaryWriteRule,
"require-json-parse-try-catch": requireJsonParseTryCatchRule,
Expand Down
102 changes: 102 additions & 0 deletions eslint-factory/src/rules/prefer-number-isnan.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import { RuleTester } from "eslint";
import { describe, expect, it } from "vitest";
import { preferNumberIsNanRule } from "./prefer-number-isnan";

const cjsRuleTester = new RuleTester({
languageOptions: {
ecmaVersion: 2022,
sourceType: "commonjs",
},
});

const esmRuleTester = new RuleTester({
languageOptions: {
ecmaVersion: 2022,
sourceType: "module",
},
});

describe("prefer-number-isnan", () => {
it("uses the correct docs URL", () => {
expect(preferNumberIsNanRule.meta.docs.url).toBe("https://github.com/github/gh-aw/tree/main/eslint-factory#prefer-number-isnan");

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.

[/grill-with-docs] The docs URL points to a README anchor (#prefer-number-isnan) that doesn't exist yet — clicking this link in an IDE will land on a 404 fragment.

💡 Add the anchor to README.md

Add a rules table or section to eslint-factory/README.md so the URL resolves. For example:

## Rules

### `prefer-number-isnan`

Prefer `Number.isNaN()` over global `isNaN()` to avoid silent coercion of non-numeric inputs.

This makes every rule's docs URL a meaningful destination for developers who click through from their IDE lint warning.

@copilot please address this.

});

it("valid: Number.isNaN and non-global forms are accepted", () => {
// CJS-only: actions/setup/js targets CommonJS; ESM counterparts tested in the shadowed-bindings block below
cjsRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, {
valid: [`Number.isNaN(value);`, `Number["isNaN"](value);`, `foo.isNaN(value);`],
invalid: [],
});

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.

[/tdd] The valid-cases test for Number.isNaN and non-global forms runs only under the CJS RuleTester — there's no ESM counterpart for these valid paths. Given that the rule is intended for actions/setup/js (CJS) this is acceptable, but worth an explicit comment or a matching ESM valid-case block to make the intent clear.

💡 Either add an ESM counterpart or add an intent comment
// valid cases under ESM — mirrors CJS coverage above
esmRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, {
  valid: [`Number.isNaN(value);`, `foo.isNaN(value);`],
  invalid: [],
});

Or add a brief comment: // CJS-only: actions/setup/js targets CommonJS so future readers know the asymmetry is deliberate.

@copilot please address this.

});

it("valid: locally shadowed bindings are intentionally excluded", () => {
esmRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, {
valid: [
`function isNaN(value) { return false; } isNaN(value);`,
`const isNaN = Number.isNaN; 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);`,
// Dynamic computed access — identifier property reference, not string literal "isNaN"
`globalThis[isNaN](value);`,
],
invalid: [],
});
});

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 valid test for dynamic computed property access: globalThis[isNaN](x) (where isNaN is a variable reference, not a string literal) should be a valid case, but there is no test covering it — this is the regression guard for the isIsNaNProperty false-positive bug noted in the existing review comment.

💡 Suggested addition

Add to the valid tests:

esmRuleTester.run('prefer-number-isnan', preferNumberIsNanRule, {
  valid: [
    // Dynamic computed access — identifier property, not string literal 'isNaN'
    `globalThis[isNaN](value);`,
  ],
  invalid: [],
});

The isDirectAccess branch of isIsNaNProperty checks property.type === "Identifier" && property.name === "isNaN" but does not guard against node.computed === true. That means globalThis[isNaN](x) — where the property is an identifier reference, not the literal string "isNaN" — satisfies isDirectAccess and produces a false positive. The fix is !node.computed && property.type === "Identifier" && property.name === "isNaN"; this test case is the proof it holds.


it("valid: isNaN used as a callback reference is not a CallExpression and is not flagged", () => {
cjsRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, {
valid: [`values.some(isNaN);`],
invalid: [],
});
});

it("invalid: global isNaN() is flagged with a replacement suggestion", () => {
cjsRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, {
valid: [],
invalid: [
{
code: `isNaN(value);`,
errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }],
},
{
// Raw string argument (e.g. env var) — suggestion preserves argument so callers must review whether to wrap with Number(...)
code: `isNaN(process.env.PORT);`,
errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(process.env.PORT);` }] }],
},
],
});
});

it("invalid: global object isNaN() access is flagged for direct and computed forms", () => {
cjsRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, {
valid: [],
invalid: [
{
code: `globalThis.isNaN(value);`,
errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }],
},
{
code: `globalThis["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);` }] }],
},
{
code: `window["isNaN"](value);`,
errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }],
},
{
code: `global.isNaN(value);`,
errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }],
},
{
code: `global["isNaN"](value);`,
errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }],
},
],
});
});
});
88 changes: 88 additions & 0 deletions eslint-factory/src/rules/prefer-number-isnan.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import { ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils";

const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`);
const GLOBAL_IS_NAN_OBJECTS = new Set(["globalThis", "window", "global"]);

export const preferNumberIsNanRule = createRule({
name: "prefer-number-isnan",
meta: {
type: "suggestion",
hasSuggestions: true,
docs: {
description: "Prefer Number.isNaN() over global isNaN() to avoid coercion footguns when validating unknown inputs.",
},
schema: [],
messages: {
preferNumberIsNaN: "Prefer Number.isNaN(...) over global isNaN(...). Global isNaN() coerces non-number inputs and can hide invalid raw values.",
replaceWithNumberIsNaN: "Replace callee with Number.isNaN — review whether the argument should be wrapped with Number(...).",
},
},
defaultOptions: [],
create(context) {
const sourceCode = context.sourceCode;
type SourceCodeScope = ReturnType<typeof sourceCode.getScope>;

/**
* Checks whether a given identifier name is locally bound in the current scope chain.
* @param node AST node to start the scope search from.
* @param name Identifier name to search for.
* @returns true if the name has a local binding, false otherwise.
*/
function hasLocalBinding(node: TSESTree.Node, name: string): boolean {
let scope: SourceCodeScope | null = sourceCode.getScope(node);

while (scope) {

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.

[/codebase-design] The hasLocalBinding function walks the entire scope chain up to the module/program scope, which means it also returns true for module-level import bindings (which have defs.length > 0). This could suppress the rule when isNaN is imported from a utility module — an import is not a local shadow that makes the call safe.

💡 Scope the "local binding" check to user-defined variables only

ESLint's scope defs have a type property. Consider restricting to Variable and Parameter def types so that import bindings don't trigger the exclusion:

if (variable?.defs.some(d => d.type === "Variable" || d.type === "Parameter")) {
  return true;
}

Or verify this can't happen in practice by adding a test case like:

// This should still be flagged — isNaN is imported, not a real local shadow
import { isNaN } from "./compat";
isNaN(value);

@copilot please address this.

const variable = scope.set.get(name);

if (variable?.defs.some(d => d.type !== "ImportBinding")) {
return true;
}

scope = scope.upper;
}

return false;
}

/**
* Checks whether a MemberExpression property is isNaN, either direct or computed.
* @param node MemberExpression node to inspect.
* @returns true if the property is isNaN.
*/
function isIsNaNProperty(node: TSESTree.MemberExpression): boolean {
const property = node.property;
const isDirectAccess = !node.computed && property.type === "Identifier" && property.name === "isNaN";
const isComputedAccess = property.type === "Literal" && property.value === "isNaN";

return isDirectAccess || isComputedAccess;
}

function report(node: TSESTree.CallExpression): void {
context.report({
node: node.callee,
messageId: "preferNumberIsNaN",
suggest: [
{
messageId: "replaceWithNumberIsNaN",
fix(fixer: TSESLint.RuleFixer) {
return fixer.replaceText(node.callee, "Number.isNaN");
},
},
],
});
}

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.

[/tdd] Missing edge case: the suggestion replaces only the callee (isNaNNumber.isNaN) but preserves the original argument unchanged. If the argument is a raw string (e.g., isNaN(process.env.PORT)) the replacement is still wrong — Number.isNaN(process.env.PORT) will always return false. There's no test covering this unsafe-argument scenario.

💡 Consider a suggestion message that surfaces the risk

The current suggestion message says "review whether the argument should be wrapped with Number(...)" which is good, but a test that applies the suggestion to an obviously unsafe argument (e.g., isNaN(envVar)) would confirm the suggestion text actually guides the author toward the right fix.

// add to the invalid cases section
{
  code: `isNaN(process.env.PORT);`,
  errors: [{ messageId: "preferNumberIsNaN",
    suggestions: [{ messageId: "replaceWithNumberIsNaN",
      output: `Number.isNaN(process.env.PORT);` }] }],
},

This makes the "review the argument" note testable and visible in the test suite as documentation.

@copilot please address this.


return {
CallExpression(node) {
if (node.callee.type === "Identifier" && node.callee.name === "isNaN" && !hasLocalBinding(node, "isNaN")) {
report(node);
return;
}

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.

[/tdd] The CallExpression visitor handles isNaN(...) but not isNaN used as a value (e.g., passed as a callback: values.some(isNaN)). There's no test for this pattern, so the intent (flag or ignore?) is undocumented.

💡 Add a test to document the intended behaviour
// Should this be flagged?  Document the decision with a test.
it("valid/invalid: isNaN used as a callback reference", () => {
  cjsRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, {
    valid: [`values.some(isNaN);`], // intentionally not a CallExpression
    invalid: [],
  });
});

If the intent is to also catch reference-style usage, a separate Identifier visitor would be needed.

@copilot please address this.


if (node.callee.type === "MemberExpression" && node.callee.object.type === "Identifier" && GLOBAL_IS_NAN_OBJECTS.has(node.callee.object.name) && !hasLocalBinding(node, node.callee.object.name) && isIsNaNProperty(node.callee)) {
report(node);
}
},
};
},
});
Loading