Type definitions for (a small subset of) underscore

[danvk]

Thanks for creating this repo! Let's get things rolling.

Here is my project's lame attempt to write flow declarations for underscore. We've written declarations only as we've used new functions, so it's nowhere close to complete.

Open questions:

• What should tests look like?
• What's the best way to integrate FlowTyped into a project (should it be an NPM module?)

See also facebook/flow#946, which makes this less useful than it could be.

[jeffmo]

What should tests look like?

I've seen some people write "type tests" using things like suppress_comments (which suppress a Flow error when one is present and cause an error when no error is present). This might also be an interesting way to prove (or disprove) continued compatibility with new versions of libs like underscore.

[danvk]

Nice suggestion, @jeffmo. I've taken a crack at adding a test. If everyone's fine with this, I can make it more complete.

Library declarations seem to be a particularly buggy part of Flow — I've indicated the tests which I believe should cause an error but do not.

In general, dropping the $FlowError comments did result in errors when I ran flow check. But for the last line in the file, this wan't the case, i.e. changing this:

// $FlowError Callable signature not found in object literal
_.find([1, 2, 3], {val: 1});

to

_.find([1, 2, 3], {val: 1});

resulted in no error, but putting that line higher in the file did produce an error. Looks like another Flow bug.

@splodingsocks what do you think of this approach?

[samwgoldman]

@danvk Thanks for doing this!

I took a look into the $FlowError thing for _find with a non-callable object. I tried removing the $FlowError line, and I got no errors, which was odd, but I got the expected error when I added a blank line before the _find call. It would seem that the $FlowError comment applies to the following "block" of code, not just the following line. Will investigate to see if that is intended.

[samwgoldman]

Also, the _.isEqual thing is interesting. When you pass too few arguments, Flow (correctly) models that as passing undefined. If the annotation is such that undefined is not allowed, Flow will error. The declared type of _.isEqual has no such constraint. (Also, the type args aren't useful there, and the parameter types should probably just be any—same behavior though).

Besides, I checked the runtime behavior of _.isEqual, and _.isEqual(void 0) is true, so there's not actually an issue with this declaration, or Flow's interpretation.

[danvk]

You know you've been spending too much time in JS-land if that argument about undefined makes sense :)

This type checks:

function foo(a, b) {}
foo(1);

So I guess Flow is at least consistent, if surprising.

Is there a way to tell Flow to require each parameter? I don't want to insist that each parameter be non-null or non-undefined, but I would like to require that calls to _.isEqual have exactly two arguments.

Speaking of which, why is this valid?

function foo(a, b) {}
foo(1, 2, 3);

I realize that passing additional, undeclared arguments is technically valid JavaScript, but it certainly seems like the type of mistake that Flow would be good at catching.

[samwgoldman]

Is there a way to tell Flow to require each parameter? I don't want to insist that each parameter be non-null or non-undefined, but I would like to require that calls to _.isEqual have exactly two arguments.

Why do you want to require this behavior when no such constraint is imposed by the library itself? Calling _.isEqual with a single argument will still return a boolean. Such a constraint would not prevent a runtime error. Indeed, I might call _.isEqual(foo) as a way to check if foo is undefined.

Regardless, there isn't a way to do this today. But I should note that the constraint "neither parameter is undefined" is exactly what you want to say—the two notions (argument not provided, undefined argument provided) are completely equivalent in JS.

Furthermore, and just to be 100% clear, it isn't that Flow "doesn't check" the number of passed arguments. The issue is that the function provides no constraint on the parameter type. Considering your example above, if we add a simple constraint, Flow will find an error.

/* @flow */
function foo(a, b) { a * b }
foo(1);

test.js:3
  3: foo(1);
     ^^^^^^ function call
  2: function foo(a, b) { a * b }
                              ^ undefined (too few arguments, expected default/rest parameters). This type is incompatible with
  2: function foo(a, b) { a * b }
                          ^^^^^ number

Regarding errors for overmany arguments. As you pointed out, doing so does not cause a runtime error, so there's not much reason to restrict that. In fact, passing "too many" arguments happens pretty often. Consider an implementation of Array.prototype.forEach.

function forEach(xs, f) {
  for (let i = 0; i < xs.length; i++) {
    f(xs[i], i);
  }
}
// is this an error? callback takes 1 arg, passed 2.
forEach([1, 2, 3], x => console.log(x));

[danvk]

Your point that this is allowed by the library is well-taken, but I don't entirely agree with it.

1 * "2" is perfectly legal JavaScript, after all, but it's rejected by Flow:

$ echo 'console.log(1 * "2")' | node
2
$ echo '1 * "2"' | flow check-contents
  1: 1 * "2"
         ^^^ string. This type is incompatible with
  1: 1 * "2"
     ^^^^^^^ number

Found 1 error

and for good reason—it almost certainly represents a mistake. Calling _.isEqual with zero, one or 3+ parameters feels the same to me. I find it far more likely that _.isEqual(a) is a mistake that should have been _.isEqual(a, b) than an intentional check for a === undefined.

You're right that this doesn't come up when you have an implementation for the function, but that's not really an option for declaration files.

[samwgoldman]

Whether _.isEqual(0) should or shouldn't be an error is certainly an area where reasonable people will disagree (case in point).

I do agree that Flow needs some way to specify that a value should be non-void. For example, what if underscore explicitly checked that the arguments were non-void and raised a runtime error? We'd need some way to convey that constraint to Flow through the declaration, and no such way exists.

So, I agree that it would be useful to have some way to declare a parameter type as non-void, and I recommend we continue that discussion in an issue over in the facebook/flow repo.

Even if such a capability did exist, I'm not convinced that it would be appropriate to use it here, but that's more a matter of opinion, and mine isn't any more correct than yours.

To bring this discussion back to the current issue, since there isn't a way to declare the type of this function the way you would want, I think the point is somewhat moot. I might recommend that you change the wording in your comment above the test, which indicates a bug in Flow when the behavior is actually working as designed.

[samwgoldman]

One more thought, since this commit is introducing the style for tests (which is really great, btw). What do you think of changing $FlowError to $ExpectError, to be clear that the comment doesn't indicate an error in Flow, but rather that Flow expects an error.

[danvk]

I've reworded the comment to be more accurate and changed to $ExpectError. I've also filed an issue for non-void parameters here if you'd like to chime in.

[danvk]

FWIW, TypeScript & the DefinitelyTyped underscore declarations flag an incorrect number of arguments as an error:

One arg:

/// <reference path="typings/tsd.d.ts" />
_.isEqual(1);

$ tsc foo.ts
foo.ts(3,1): error TS2346: Supplied parameters do not match any signature of call target.

Two args:

/// <reference path="typings/tsd.d.ts" />
_.isEqual(1, 1);

$ tsc foo.ts

Three args:

/// <reference path="typings/tsd.d.ts" />
_.isEqual(1, 1, 1);

tsc foo.ts
foo.ts(2,1): error TS2346: Supplied parameters do not match any signature of call target.

[danvk]

I'm not sure if @splodingsocks is affiliated with Facebook, but if we don't hear from him within a few days, maybe we should fork this over to facebook/flowtyped? cc @gabelevi

[mrmurphy]

@danvk Thanks so much for being the first to jump in! I'm on vacation, so I haven't been keeping up with things. But we're all merged now!

[mrmurphy]

@danvk @jeffmo @samwgoldman I've added you all as collaborators on this repo, so you should have push permissions now.

I'll be back from my trip on Friday.