checkJs should be as strict as TS about matching function arity

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

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):

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

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:

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.
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.

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)

Daniel Rosenwasser (@DanielRosenwasser)

Daniel Rosenwasser (@DanielRosenwasser) Wesley Wigham (@weswigham) for additional opinions.

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

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.

Require all parameters in JS

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

Wesley Wigham (@weswigham)

Wesley Wigham (@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.

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

Navigation Menu

Repository navigation

checkJs should be as strict as TS about matching function arity #31888

3 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

checkJs should be as strict as TS about matching function arity #31888

Description

Activity

sandersn commented on Aug 7, 2019

sandersn commented on Aug 7, 2019

brendankenny commented on Aug 8, 2019

sandersn commented on Aug 8, 2019

DanielRosenwasser commented on Aug 8, 2019

weswigham commented on Aug 19, 2019

3 remaining items

sandersn commented on Dec 6, 2019

sandersn commented on Dec 18, 2019

sandersn commented on Apr 2, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions