checkJs should be as strict as TS about matching function arity

[golopot]

TypeScript Version: 3.6.0-dev.20190613

Search Terms:
overload
higher order function
checkjs

Code

// id.d.ts
declare type Unary = (a: number) => void;
declare type Binary = (a: number, b: number) => void;
export declare function id(fn: Unary): Unary;
export declare function id(fn: Binary): Binary;
export {};

// id.js
export const id = fn => fn;

// index.js
import {id} from './id'

// expected f: Binary
// actual   f: Unary
const f = id((a, b) => {});

// Error: Expected 1 arguments, but got 2.
f(1, 2);

// tsconfig.json
{
  "compilerOptions": {
    "allowJs": true,
    "checkJs": true,
    "outDir": "dist",
    "noEmit": true,
    "strict": false,
    "esModuleInterop": true
  }
}

Expected behavior:
f is a binary function.
Actual behavior:
f is a unary function.
Playground Link:
N/A
Related Issues:
This issue makes util.promisify in @types/node unusable with checkjs. See
DefinitelyTyped/DefinitelyTyped#33340

[sandersn]

Here's a smaller js-only repro (not required, but makes it easier for me to test):

/** @param {{
    (fn: (a: number) => void): number;
    (fn: (a: number, b: number) => void): string
}} id */
export function test(id) {
    /** @type {string} */
    var val = id((a, b) => a + b);
}

Errors (with strict on):

1. on val, because the compiler resolves the wrong overload (expected because of this bug).
2. on parameters (a, b), because their types are implicitly any (unexpected, perhaps caused by this bug?).

[sandersn]

Unfortunately, this is a side effect of the way that we handle functions in Javascript. If a function doesn't have all of its parameters annotated, then the compiler doesn't require all of them to be passed. Internally, this means the signature gets marked with minArgumentCount: 0. This, in turn, means that (a, b) => {} can match Unary — since it doesn't require any parameters, passing one parameter is better than nothing.

To work around this, annotate (a, b) => {} with types. Here's one way to do it:

/**
 * @param {number} a
 * @param {number} b
 */
(a, b) => {}

To fix this, we'd need to make minArgumentCount: 2 just like it is in TS. I have a couple of ideas:

1. Require all parameters with "strict": true. Few people have strict on in JS projects, but we already turn off index-signatures-everywhere with strict on. This actually needs to go under a specific strict flag, but noImplicitAny seems like a fine one since you'll get a no-implicit-any error in the above example without type annotations — the unary overload blocks contextual typing from the binary overload.
2. Require all parameters for arrow functions. This assumes that arrow functions are used only as expressions, which is not correct.

2a. Require all parameters for arrow functions that are not immediately assigned.
2aa. Require all parameters for arrow functions that have a contextual type.
2ab. Require all parameters for arrow functions that are immediately used as an argument.

There are a lot of options here.

[brendankenny]

• Require all parameters with "strict": true. Few people have strict on in JS projects, but we already turn off index-signatures-everywhere with strict on. This actually need to go under a specific strict flag, but noImplicitAny seems like a fine one since you'll get a no-implicit-any error in the above example without type annotations — the unary overload blocks contextual typing from the binary overload.

This option sounds great! (independent of if the others should be done, too)

As you said, the error is already given elsewhere with noImplicitAny, and this also has the benefit of alerting a call of a remote function to the problem as well (which could be helpful if e.g. the caller doesn't control the any-paramed function code and so doesn't get notified of the arity problem in their own code)

[sandersn]

@DanielRosenwasser @weswigham for additional opinions.

[DanielRosenwasser]

If the function has a contextual type, then I think it's fair game to treat it similarly to a function with parameter annotations.

[weswigham]

Unfortunately, this is a side effect of the way that we handle functions in Javascript. If a function doesn't have all of its parameters annotated, then the compiler doesn't require all of them to be passed. Internally, this means the signature gets marked with minArgumentCount: 0.

Tbh, I'm a fan of just not doing this and bringing the JS behavior inline with TS - we have a way in js to specify optional arguments, and so if errors are on, you can fix the error, and if errors aren't on, the required arity isn't going to be unjustly noisy or anything, since errors aren't on.

[sandersn]

You mean with @param [name] ? I'm not sure that checkJs is a strong enough signal from the user that we should require jsdoc annotations where we can't determine the intended arity of a function. Maybe it is though.

[weswigham]

I think it is. It's worth noting that we also still get arity hints from parameter initializers, in addition to jsdoc comments.

[sandersn]

I think this is a pretty easy change to make, so I think we should just run the user tests on a PR to see how many new errors turn up. Looking at actual code will make it easier to think about how it will affect people, I think.

[sandersn]

The simplest solution -- requiring all parameters in JS -- is up at #35153

[sandersn]

@weswigham More details at the PR, but the upshot is that lots of webpack breaks, which is evidence that JSDoc is too onerous a requirement for specifying optional parameters.

[sandersn]

I'm going to close this because I think Wesley's change in 3.9 #37173 addresses this bug adequately.