facebook / flow

Adds static typing to JavaScript to improve developer productivity and code quality.
https://flow.org/
MIT License
22.09k stars 1.86k forks source link

isNaN shouldn't be considered a number #1406

Closed gustavoguichard closed 8 years ago

gustavoguichard commented 8 years ago

One of the reasons I've chosen Flowtype is to get rid of NaN crazyness in my team's code. I expect functions not to return NaN if I annotate them to return number.

screen shot 2016-02-14 at 6 17 21 pm

IMHO this should not be valid. Any reason why we should care this JS legacy in Flow?

Cheers

avikchaudhuri commented 8 years ago

It's hard to track when operations might return NaN. Say we do ban NaN: number, In your example, what would you expect to happen when you do divide(0,0)? Right, we'd have to say that the return type of divide is number | NaN or something like that, then force NaN checks on all callers of divide. So the question really is, would that be a net benefit?

vkurchatkin commented 8 years ago

I think numbers should stay as they are, but it would be nice to have a separate type for finite numbers (no NaN or Infinity).

Then we can define signatures of operators as following:

 + : (number, number) => number
 + : (number, finite_number) => number
 + : (finite_number, number) => number
 + : (finite_number, finite_number) => finite_number

 / : (finite_number, finite_number) => number

etc
gustavoguichard commented 8 years ago

+1 for finite type. That said, it'd be a nice DUX to somehow in my first image, get an alert. It'd make more sense to get this alert due to params that I'm expecting rather than what's returning.

Elm way of handling is sort of cool: screen shot 2016-02-16 at 9 13 54 am

mroch commented 8 years ago

plot twist: you'd agree Number.MAX_VALUE is finite, right?

(Number.MAX_VALUE + Number.MAX_VALUE) === Infinity // true

dmitriz commented 8 years ago

I would strongly support to add the type finite.

To get away from the headache how exactly it should behave, I suggest to use Lodash isFinite function as Blueprint (with unavoidable minor modification, unfortunately, see below).

It has been there for ages for a good reason, even before Lodash existed, as one of the core utilities from Underscore, and Lodash still defers to it:

https://github.com/lodash/lodash/blob/master/vendor/underscore/underscore.js#L1311-L1314

// Is a given object a finite number?
_.isFinite = function(obj) {
    return !_.isSymbol(obj) && isFinite(obj) && !isNaN(parseFloat(obj));
};

Note it does not use the native Number.isFinite function (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isFinite), but instead the less predictable global isFinite (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isFinite), perhaps for historical reasons.

What we likely don't want is this:

isFinite('1') // true

Which also caused people some headaches: http://stackoverflow.com/questions/5690071/why-check-for-isnan-after-isfinite

The signature is affected by several constants incl. Number.MAX_VALUE: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number

However, the resulting type depends on the value (instead of only type):

Number.isFinite(Number.MAX_VALUE + 1) // true, because of the rounding
Number.isFinite(Number.MAX_VALUE + Number.MAX_VALUE) // false

So what can we do? I personally would stay sane and regard Number.MAX_VALUE as NOT of type finite. That makes type checking really dead simple:

The reasoning: If you have any of those inside your code, you are at clear risk of getting infinities. So the mere reason you put type finite is that you want to be warned about those risks.

And perhaps, to avoid questions like "why is that not of type finite even when Number.isFinite says so", make a clear statement that finite is not a native type.

dmitriz commented 8 years ago

Just to attempt some clarification. As a professional mathematician, I would like to have a proper mathematical type for real numbers. That would be clean and intuitive to anyone not familiar with implementation details.

The basic idea is simple. Mathematics clearly defines what real numbers are and what operations are permitted. Entities like Number.MAX_VALUE fail that definition. Entity like

Number.EPSILON: The smallest interval between two representable numbers

is not mathematically distinguishable from 0. Therefore, it must be treated as 0 for real-number typechecking purposes.

In other words, only mathematically unambiguous real numbers can be whitelisted. Only for them, algebra rules guarantee operational safety. All other entities may lead to potential bugs.

As example, this function should be considered safe:

function safeSquare (x: finite): finite {
    return x * x
}

The fact that it might get evaluated to Infinity at run-time by JS engine bares no difference, because that result can only be a side-effect of the current engine implementation. That may well change in the future. Or the result will depend on the engine. Or the platform. Or the OS... But people writing code know that. They don't need to be warned their argument x might get too large. After all, it is not the fault of the library maintainer if the consumer tries the library with all possible crazy values. Which includes Number.MAX_VALUE. In fact, those numbers normally have no place in your code.

But here is the real problem -- what if another programmer puts this number somewhere in the code for presumably "legitimate" purposes. Or your dependency third party library has it somewhere down the code. Then, if there is any risk, I want to be warned!

Here is a concrete example of unsafe function where I want to be warned:

function unsafeDivide (x: finite, y: finite): finite {
    return x / y
}

Here there is a clear risk that y is 0 or close to it. On that other hand, this function would be safe:

function safeDivide (x: finite, y: nonzero): finite {
    return x / y
}

So I need the type nonzero. Or it needs to be inferred from the context.

The type nonzero signature can be again defined mathematically following the risk-free principle:

For instance, the above function unsafeDivide cannot guarantee it, so it fails the type check. The above function safeDivide guarantees safety, so it passes.

Note that this guarantee philosophy is orthogonal to JavaScript's don't fail at any cost. The latter tells you that 1/0 === Infinity or 1/-0 === -Infinity, throw this back into your expression and hope for the best ;) And, as will all know, our hopes can be fullfilled sometimes. But sometimes they aren't. Which is why we are here ;)

Yes, it makes a perfect business sense to keep your app running suppressing all those errors that might be of no relevance to the user. Or the user will be presented with the payment bill of 1e+30 dollars. ;) That no one forces the user to accept.

And in 98% cases, it is probably ok to move on, and the user won't even notice. But the fact that users don't notice, or did not notice yet, does not mean it is ok for your code to keep that potential bug opportunity. Next time it might matter to user after all. In contrast, we do want to see this failure to catch that bug. Which is why we are here.

In contrast, the guarantee principle will give you as programmer a complete peace of mind, because it will warn you about any conceivable potential violation. It should not be a human task to write all imaginable unit tests to safeguard against all possible imaginable and non-imaginable troubles. Not even mention, many teams don't even bother writing tests at all or write only minimum possible. The right "person" to deal with this problem is the compiler!

vkurchatkin commented 8 years ago

Any expression involving Number.MAX_VALUE, Number.MIN_VALUE, Number.MAX_SAFE_INTEGER, Number.MIN_SAFE_INTEGER is automatically NOT of type finite

This is not enough, any expression involving should not evaluate to finite as well.

would like to have a proper mathematical type for real numbers

I think it's impossible to use javascript numbers for this.

dmitriz commented 8 years ago

This is not enough, any expression involving should not evaluate to finite as well.

Exactly what I tried to say by "NOT of type finite":


I think it's impossible to use javascript numbers for this.

What are the problems?

vkurchatkin commented 8 years ago

I meant to say, any expression involving finite. There is nothing special about Number.MAX_VALUE, you can get it by adding two finite numbers

What are the problems?

For starters, real numbers are an infinite set, javascript numbers are a finite set

dmitriz commented 8 years ago

I meant to say, any expression involving finite. There is nothing special about Number.MAX_VALUE, you can get it by adding two finite numbers

Adding two of those numbers will give you Infinity. That is what is special. Therefore my proposal to fail them for being finite.

What are the problems? For starters, real numbers are an infinite set, javascript numbers are a finite set

How is this a problem for my proposal?

vkurchatkin commented 8 years ago

@dmitriz

Adding two of those numbers will give you Infinity

Not true, (8.988465674311579e+307 + 8.988465674311579e+307) === Number.MAX_VALUE

How is this a problem for my proposal?

It's not a problem with your proposal, it's a problem with 'proper mathematical type for real numbers' idea

mroch commented 8 years ago

The fact that it might get evaluated to Infinity at run-time by JS engine bares no difference, because that result can only be a side-effect of the current engine implementation. That may well change in the future. Or the result will depend on the engine. Or the platform. Or the OS...

This behavior is part of the spec and is not implementation-dependent: http://www.ecma-international.org/ecma-262/6.0/#sec-ecmascript-language-types-number-type

Unfortunately I don't think this is something that Flow will be able to prove.