Array<number>.sort() should be an error

[danvk]

Here's a plausible-looking implementation of a median function:

function computeMedian(xs: number[]): number {
  xs.sort();  // <---
  return xs[Math.floor(xs.length/ 2)];
}

The problem is that xs.sort() sorts the numbers lexicographically. Since Flow knows that xs is an array of numbers, rather than strings, it has enough context to complain.

This is a classic JavaScript gotcha which flow could catch. The solution is to do xs.sort((a, b) => a - b);. So my proposal is that the comparison function parameter to Array.prototype.sort should be mandatory for arrays of numbers.

[samwgoldman]

100% agree that we should catch this. This could be a fun issue for someone interested in digging into the Flow's internals. If anyone sees this and wants to take a crack at it, I'd be happy to help them get oriented around the code.

[mrecachinas]

@samwgoldman I'd be interested in diving into Flow's internals and taking a stab at this.

[samwgoldman]

@mrecachinas Cool! So the first step to hacking on Flow is to make sure you can build and run from source. As long as you can make test, you should be good to go. As for getting around, the parts that you care about for this feature are in type_inference_js.ml and flow_js.ml under src/typing. The former takes an AST (specified in spider_monkey_ast.ml) and, among other things, calls into flow_js.ml to create a structure of upper and lower bounds around type variables. The consistency of that structure implies type safety -- inconsistency implies a type error. If you have any more questions, your best bet is to catch me (or someone else) on IRC. The #flowtype channel on freenode is where you can find me.

[mrecachinas]

Hey @samwgoldman, I tried going on the IRC channel to catch you or get some help a couple weeks ago to no avail. I've been able to make test from the get-go and I've been printf-debugging anywhere that seems relevant (because I'm not familiar with ocamldebug). I imagine the Array library call portion in flow_js.ml is relevant, but I lose it after it runs through get_builtin_typeapp, since that's fairly generic code.

Could you point me to a place where a similar check is implemented (if there is one)? The reference doesn't have to be for arrays -- it can be for strings or what-not.