Rule proposal: Disallow calling Number.isNaN on non-number types

[Sweater-Baron]

Before You File a Proposal Please Confirm You Have Done The Following...

• I have searched for related issues and found none that match my proposal.
• I have searched the current rule list and found no rules that match my proposal.
• I have read the FAQ and my problem is not listed.

My proposal is suitable for this project

• My proposal specifically checks TypeScript syntax, or it proposes a check that requires type information to be accurate.
• My proposal is not a "formatting rule"; meaning it does not just enforce how code is formatted (whitespace, brace placement, etc).
• I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

Several years ago, the type signature of Number.isNaN in TypeScript was changed to accept unknown: microsoft/TypeScript#24436

I think this was a mistake, because it breaks type safety on this function and doesn't provide a benefit for most use cases, but the decision seems unlikely to be reversed at this point.

In any codebase I've worked in, there's no use case for calling Number.isNaN on any type aside from number, and any time Number.isNaN is called on a non-number, it's guaranteed to be a bug. It's a pretty easy mistake to make, and currently there is no automated way to catch it. It would be valuable to have an eslint rule to detect this.

E.g.:

const numberFromUserInputTextbox = '123abc';
// Whoops, we forgot to convert this string to a number before checking if it's NaN.
// Now it looks like the user entered a valid number because isNaN always returns
// false when called on a string:
const userEnteredValidNumber = !Number.isNaN(numberFromUserInputTextbox);

Anecdotally: In the current codebase that I work in, after finding one bug that was caused by calling this function on a string instead of number, I searched the codebase for other uses of isNaN on a string and found one additional bug that way. I suspect this is a common mistake.

Fail Cases

Number.isNaN("1");
Number.isNaN(undefined);
Number.isNaN([]);

Pass Cases

Number.isNaN(1);

Additional Info

If someone can point me to an example PR where someone added a similar rule before, I'm happy to take a crack at implementing this myself.

[kirkwaiblinger]

Interesting!

Code Advice (whether this gets accepted or you'd like to try implementing it as a simple custom rule)...
You can maybe look to something like prefer-find for how to query a (member) function call.
and you'll probably want to use the ts-api-utils isIntrinsicNumberType function to check the argument for being a number.

Will wait to see what others have to say, though, on whether this ought to be part of typescript-eslint or not. FYI this is probably something that you could solve without a lint rule by modifying the global types, but that has its own downsides. You might be interested to peruse the conversation for use-unknown-in-catch-callback-variable, where this was discussed in some detail #7526 (comment) 🙂

[bradzacher]

Seems like this is similar to #5360
The TS types are intentionally loose here because there is well-defined and expected behaviour for non-number input.

I personally don't think this should be a lint rule and instead if your codebase wants different behaviour:

• patching the inbuilt lib types is possible and a well-defined process (eg https://github.com/uhyo/better-typescript-lib).
• wrapping the function with a better type and using eg no-restricted-properties to ban the global

So I'm a -1 on this.

[Sweater-Baron]

The well-defined and expected behavior for calling Number.isNaN on a non-number is just return false, which makes sense because the JavaScript implementation of this function has to do something if you pass it a non-number. But I don't think this is desirable behavior in a typed language.

I can see an argument for allowing calling this on a union type that includes number as a possible type, so maybe my proposed lint rule can include a config option that allows possibly-number union types (though that still seems like a bad practice and I think most codebases shouldn't allow it).

But for all other cases, where Number.isNaN is called on a value that is known at compile time not to be a number, it's essentially guaranteed that the programmer made a mistake. If the function can be known at compile time to always return false, then one of the branches based on the return value is actually unreachable.

Like, I don't think there's anyone who uses eslint who would actively not want this to be flagged as an error at compile time by something, whether by the compiler or an eslint rule:

const exampleFunction = (x: string) => {
  if (Number.isNaN(x)) {
    // This branch can be known at compile time to be unreachable.
    console.log("It was NaN");
  } else {
    console.log("It was not NaN");
  }
};

Patching lib types or using something like no-restricted-properties would solve this problem for me personally, but my ultimate goal here is to help other people easily catch this class of bug, not just myself.

typescript-eslint is much more widely adopted compared to something like better-typescript-lib, so I think adding a lint rule for this is the best way to help people avoid this problem. A rule that says "don't call Number.isNaN on things that are known at compile time to definitely not be a number" would help many people to catch bugs, and doesn't seem to have any downsides.

I'll amend my proposal a bit:

Configuration option allow-number-union-types:

• Errors only if the function is called on a value that is known at compile time not to be a number, but allows union types that include number.
• Seems pretty unopinionated to me - it's almost impossible to imagine it flagging any code that wasn't actually a mistake, and it genuinely would catch bugs, so I think it makes sense to add to the recommended ruleset.

// Fail case:
const exampleFailure = (x: string) => Number.isNaN(x);

// Pass cases:
const examplePassOnUnion = (x: string | number) => Number.isNaN(x);
const examplePass = (x: number) => Number.isNaN(x);

Configuration option allow-only-number:

• Errors unless Number.isNaN's parameter is precisely known to be a number type.
• Very slightly opinionated, but most people probably should want this? 99% of the things it flags will be mistakes by the programmer, and it's easy to work around with typeof value === 'number' && or just with an eslint disable comment if it genuinely is intentional. I'd argue it's inoffensive enough that this should be in the recommended ruleset instead of the version allowing union types, but if not that, then at least the strict ruleset should include this one.

// Fail cases:
const exampleFailure = (x: string) => Number.isNaN(x);
const exampleFailureOnUnion = (x: string | number) => Number.isNaN(x);

// Pass case:
const examplePass = (x: number) => Number.isNaN(x);

Does that make sense?

[Josh-Cena]

But I don't think this is desirable behavior in a typed language.

Number.isNaN is a predicate. A predicate is designed to accept anything and tell you whether it's a particular thing. For example, Array.isArray accepts anything and tells you if it's an array. React.isValidElement tells you if anything is a ReactElement.

Any predicate has this pitfall of "accepting unrelated types". For example, your own isDog predicate may accidentally accept a Cat and always return false. Array.isArray may always receive a boolean. This is why Array<T>#includes, which also functions somewhat like a predicate, is only allowed to take T (which inconveniently disallows testing if a string is in a set of known strings). This kind of problem can be fixed once TS has super constraints, but before then, it will always be up to the programmer's discretion to understand if they are using predicates as they intend. Number.isNaN is not unique enough as a rule.

There are other ways you can shoot yourself in the foot while doing type narrowing, by the way:

const a = 1;
if (typeof a === "string") {
  // Not even caught by no-unnecessary-condition!
}

[Sweater-Baron]

Any usage of a predicate where the result can be known at compile time is pretty suspicious, but as you point out, there's currently no way in TypeScript to detect that systematically for all predicates.

I do think Number.isNaN is unique in this respect as far as predicates go: It's commonly used to validate numeric user inputs, and numeric user inputs usually start out as string and not number, and it's conceptually very easy for a programmer to do Number.isNaN(valueTheUserEntered) without converting it to a number first.

Anecdotally, I have seen this class of bug several times in my career, and at at one point at my most recent job, 2/12 uses of Number.isNaN were making this exact mistake (committed by different people). That's of course a small sample size, but it's a pretty high error rate! I also just skimmed the results of a sourcegraph search for occurrences of "Number.isNaN(input" for a couple minutes and found 2 examples of this kind of erroneous usage [1][2] (it says there were 127 total results but I didn't check them exhaustively).

I have never seen people have similar problems with incorrect usage of other predicates, so it seems like it'd be a waste of time for anyone to write lint rules for them, but with Number.isNaN specifically, it does seem like a common enough mistake that there would be value in having a rule for it.

If I did the work and made a PR for this, is anyone strongly enough opposed to this that they'd be opposed to merging it? Or is it more at the level of "I don't see much point in this but it doesn't seem harmful if someone else wants to do the work"?

[Josh-Cena]

That doesn't sound unreasonable to me, but remember that contributions also imply future work by the team to maintain them, so I'll wait for more feedback from Brad.

[bradzacher]

yes - to be clear it's not that we don't think there's value in such a check. It's a question of maintenance cost against user value.

We are volunteer maintainers. We have barely enough bandwidth to keep everything maintained as it is. So we are very careful about adding new rules. Once a contributor builds a rule and it merges - in general the contributor never contributes again. Which means the maintenance team is on-the-hook for fixing bugs and ensuring future TS version compatibility. I.e. every new rule or option carries with it a maintenance burden - a cost to the maintenance team.

When we change the AST - we need to update every rule. When we support new TS versions we need to ensure every rule doesn't break. When new ES versions are released we need to ensure existing rules support new semantics/syntax. When new ESLint versions (particularly majors) are released we need to ensure existing rules work with the changes. If we want to refactor our codebase we need to refactor every rule. Etc. Even the simplest of rules have a maintenance burden.

When we are evaluating a new rule what we are looking at is the cost-to-value ratio. Is the cost of bringing in a new rule higher or lower than the value it gives to our users? Value is a nebulous thing because it is subjective - to each person it is different. For you the perceived value of this rule is high - this bug has impacted you directly so preventing it in future is high value to you.
But for someone that's never been impacted by this bug before - the rule is likely low value because from their POV it's checking for a problem that they've never seen.

So if value is subjective - how do we as a maintenance team evaluate it? Well we apply our own experiences - each of us have had our own careers in various spaces and we've seen code at many different companies solving many different problems. We also have our experiences from talking to users as part of this project - it's 2nd hand, anecdotal experience but it helps guide us all the same.
So we leverage our experience and try to extrapolate to what we think the broader userbase of tens of millions of users will find value in.

We also evaluate it against the history of the project. The plugin itself is ~8y old (longer if you consider its tslint legacy) so a good question to ask is "why has it taken 8y+ for someone to suggest this check?" Often times the answer is just that it took 8y+ to get someone to go "this should be a lint rule". Other times the answer is also "the problem is just so rare that people barely ever hit it".

If we're ever unsure - we default to the wisdom of the collective and leave an issue open for reactions / comments. 3-6m later we come back and see if anyone else has come and reacted. If few people do then it's a good signal that people either aren't running into the issue or they aren't looking to lint against it.

---

Phew. So with all that being said - is does this rule pass the cost:value bar?

I think it has some limited value, for sure - it catches a potential bug vector. But it is - as you've researched - a reasonably rare one.

I do think there are other (better) ways around this.
For example Number.isNaN always returns false for strings - but isNaN coerces strings to a number. So one alternative is to just use the global instead of the namespaced version. I.e. using no-restricted-properties to enforce isNaN over Number.isNaN would prevent this class of bugs too based on my understanding.
Or as I suggested above - wrapping Number.isNaN with stricter types, or using the well-defined TS lib
type patch processes would work too and allow you to control the allowed input types.

For me right now this doesn't seem like something that the general userbase would really see value in turning on - it would likely need to be included in a recommended set for anyone to use it. I'm still sitting as a -1 - but I'm happy to leave this to evaluate community engagement.

[Josh-Cena]

Worth noting that the TS type for isNaN only allows numbers due to coercion, so if you enforce isNaN you actually get the "strict" behavior you desire.

[bradzacher]

TS type for isNaN only allows numbers due to coercion

I'm so confused lol.. "This thing is specced to coerce everything to a number - so it must only accept numbers" and "This thing is specced to always return false for non-numbers - so it must accept anything".

Seems like such contradictory thinking?!?

[Josh-Cena]

It is a longstanding confusion of TS users. See microsoft/TypeScript#37449 and the many issues linked. In essence, TS wants us to only use a function in the domain where no type coercion happens.