Type hole with compatibility between optional parameters/extra parameters

TypeScript Version: Whatever runs https://www.typescriptlang.org/play/

Code

const x = (a: number = 1): number => a;
const y: () => number = x;
// TypeScript error: "Supplied parameters do not match signature of call target."
// OK
y('x').toFixed();

const z: (a: string) => number = y;
// No TypeScript error
// Runtime error: Uncaught TypeError: z(...).toFixed is not a function
z('x').toFixed();

Expected behavior:

TypeScript error that either (a?: number) => number is incompatible with () => number, or that () => number is incompatible with (a: string) => number. I'm not sure which one is most correct, but permitting both of these things in combination is definitely unsound.

Actual behavior:

No TypeScript error on the const z: (a: string) => number = y; line nor the z('x').toFixed(); line., but an error when the code runs.

You've identified the problem and why there isn't a solution -- preventing either assignment is problematic. TypeScript does not guarantee soundness and this is one of the holes.

Even Flow, which generally tries to be more sound at the expense of convenience, allows this (update: I see you just logged this on their tracker as well).

If you somehow find yourself in this situation and need to track down how the wrong function got assigned in, you can declare out the optional parameter in the nullary case assuming you can figure out which call signature originated the problem in the first place:

// Error
const y: (_?: "must be missing!") => number = x;

Ryan Cavanaugh (@RyanCavanaugh)

Ryan Cavanaugh (@RyanCavanaugh) Yeah Flow has the same problem (facebook/flow#3049), but I found it more surprising considering they have more of an objective of soundness than TS does.

If this is just another baked-in gotcha then fair enough, I'll add it to the list.

Ryan Cavanaugh (@RyanCavanaugh)

Ryan Cavanaugh (@RyanCavanaugh) I posted a solution against the Flow issue which you may be interested in, which I'll repeat here.

Basically you store against a function signature whether or not extra parameter slots should accept anything {} (mixed in Flow) or only undefined (void in Flow), similar to a rest parameter.

For function literals, the inferred type has any extra parameters set to accept anything, since the function ignores those parameters. This way you can pass callbacks or implement interfaces without specifying all the necessary parameters, eg element.onClick(() => { alert('clicked'); }) would work even though the callback can accept an event.

For function types written directly, the type has any extra parameters set to accept only undefined. This way the same function type with an extra optional parameter is correctly considered compatible, since undefined is a valid input for an optional parameter. Any attempt to call a function with more than the specified parameters is an error (as it is currently) (unless perhaps undefined is being passed).

The only restriction is that a written function type is not compatible with the same written function type with an extra parameter, because the shorter function type could be masking a function type with additional optional parameters for which the only known acceptable input is undefined.

Example:

const x = (a: number = 1): number => a;
// x: (number|undefined, {}...) => number

const y: () => number = x;
// y: (undefined...) => number
// Assignment OK, undefined is compatible with both number|undefined and {}

const z: (a: string) => number = y;
// z: (string, undefined...) => number
// ^ Error on assignment, string is not compatible with undefined

const w = (): number => 1;
// w: ({}...) => number

const v: (a: string) => number = w;
// v: (string, undefined...) => number
// ^ Assignment OK, string and undefined are both compatible with {}

Jesse Schalken (@jesseschalken)

Passing extra arguments should always be allowed. This is just the nature of JS.

However a much easier fix, that catches the problem at its root is disallowing

const x = (a: number = 1): number => a;
const y: () => number = x;

If (x: T?) => U is not assignable to () => U the workaround is easy, just wrap it in a () => f(). There is no simple and generic workaround for Jesse Schalken (@jesseschalken)'s suggestion, however.

Optional aliasing should be much more easily detected (--noOptionalAliasing) in the checker as well.

Marin Marinov (@gcnew)

Marin Marinov (@gcnew) I think being able to override class methods and add extra optional parameters is fairly reasonable and expected behaviour, which means (x: T?) => U is assignable to () => U and that calling a function/method with extra arguments must be disallowed (or only allow passing undefined). My suggestion aims to maintain that while also being sound.

I see now where you are coming from. Unfortunately overloads and overrides are one and the same thing in JavaScript. This makes your proposal better for class hierarchies. On the other hand JavaScript is traditionally used in functional style. As such I'm not in favour of hindering virtually every callback argument.

I still think that conditionally outlawing unsafe optional param assignments is the better alternative. If we go with your proposal then undefined... should always be used for the extra parameters as inferring {}... needs the source file and creates a discrepancy with signatures obtained from declaration files. The function literal rule is also kind of artificial/confusing.

Unfortunately overloads and overrides are one and the same thing in JavaScript.

I don't see what overloading has to do with anything.

As such I'm not in favour of hindering virtually every callback argument.

Can you give me an example? You can still omit parameters when passing callbacks.

If we go with your proposal then undefined... should always be used for the extra parameters as inferring {}... needs the source file and creates a discrepancy with declaration file functions.

The only purpose {}... serves in an inferred function signature is to be able to omit unused parameters when passing callbacks or overriding/implementing methods, i.e. it only has a purpose where an actual function definition is concerned. When it comes to type definitions and the interfaces between modules, this purpose doesn't apply and undefined... is correct.

The function literal rule is also kind of artificial/confusing.

It's just capturing the knowledge that extra parameters will be ignored by the function and they will therefore accept {}.

I think being able to override class methods and add extra optional parameters is fairly reasonable and expected behaviour

I don't think so. The runtime behaviour / expectations are different. You can't safely use such a function in all contexts the original can be used, precisely because JavaScript functions can be called with extra parameters by definition. It is also not supported in languages such as C# because the type is different although compatible at times.

I don't see what overloading has to do with anything.

Even if we agreed on the above, then comes the problem that you should be able to add overloads in subclasses. Overloads in JS are basically just one function that handles all the cases. This way it clashes with overriding, because banning defaulting overrides has the side effect of banning overloads as well.

Can you give me an example? You can still omit parameters when passing callbacks.

Sure - every named function.

const const42 = () => 42;
[1, 2, 3].reduce(const42, 0)

Now, you'd say that the inferred signature for const42 should be (_: {}...) => number. This might even be the case, but if we had it in a separate module, then the only thing we are left with is its user provided signature which is () => number. If we expand on that it should turn into (_: undefined...) => number and it will no longer be applicable as a callback in the above case. This makes it a no-go for me.

One remedy is declaration files / module interfaces to include extra param information, but I don't see this happening. Also if a user provided signature is present, it should not be silently expanded, but rather taken as is. This makes adding extra: {}... to functions inapplicable and without it many valid cases will be broken.

With the current state of affairs I don't see how the situation can be improved on, without sacrificing valid expressiveness power. As both approaches have their respective flaws, I think the best way forward is adding linter rules for both and allowing programmers to choose their poison.

Is this related to this problem? I've noticed I can't create a function overload that makes a parameter optional by way of omission in the parameter list. Normally making the trailing parameters optional is a good thing for concise code, but if the overloads have different return types, making a trailing parameter optional just to resolve compatibility seems kind of hacky.

export type CurriedHasFn<K, V> = (map: HashMap<K, V>) => boolean;

export function has<K, V>(key: K): CurriedHasFn<K, V>;
export function has<K, V>(key: K, map?: HashMap<K, V>): boolean;
export function has<K, V>(key: K, map?: HashMapImpl<K, V>): boolean|CurriedHasFn<K, V> {
  // ...
}

In the above snippet, I'm forced to make the second parameter of the the second overload optional, otherwise I can't get it to compile without compatibility issues. I would think that the absence of the second parameter in the first overload should satisfy optionality in the internal/final overload, and that the presence of the parameter therein should be enough to satisfy a non-optional version of the parameter in the second overload.

This is what I actually want:

export type CurriedHasFn<K, V> = (map: HashMap<K, V>) => boolean;

export function has<K, V>(key: K): CurriedHasFn<K, V>;
export function has<K, V>(key: K, map: HashMap<K, V>): boolean; // non-optional `map`
export function has<K, V>(key: K, map?: HashMapImpl<K, V>): boolean|CurriedHasFn<K, V> {
  // ...
}

Nathan Ridley (@axefrog)

Nathan Ridley (@axefrog) I'm confused by your example - in the first block, why is the second overload specifying map optional? It makes a call to has(key) potentially ambiguous. The second block makes more sense, though in general you should put longer overloads first, not second. Without an isolated repro (lots of undefined identifiers here) and a more clear explanation of what you would expect a given call to do, it's hard to say. Best asked on Stack Overflow as this example is not related to the subject at hand.

Ryan Cavanaugh (@RyanCavanaugh)

Ryan Cavanaugh (@RyanCavanaugh) Because I get red squigglies otherwise. Evidence below:

I suspect it is because the type of the map in the overload declaration is HasMap (interface) and the implementation expects HashMapImpl (implementation). This means that the overload may be called with a different HashMap implementation which renders it incompatible. Just use the same shape in both places.

This issue seems to have fixed.

This doesn't seem to be an issue in practice, and our appetite for breaking changes diminishes every day.

FWIW just as a data point, this unsoundness has been complicit in a production outage at Google.

Authors expected type safety in a refactoring that changed the type of an optional argument (e.g. from T to U). The compiler didn't catch a breakage in a code path going from (a: string, b?: T)=>void to (a: string)=>void to (a: string, b?: U)=>void.

It's a single data point, so not necessarily something that should change the judgement on this particular issue. I just thought I'd leave this here for future reference, in case it comes up again.

Navigation Menu

Repository navigation

Type hole with compatibility between optional parameters/extra parameters #13043

7 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Type hole with compatibility between optional parameters/extra parameters #13043

Description

Activity

RyanCavanaugh commented on Dec 20, 2016

RyanCavanaugh commented on Dec 20, 2016

jesseschalken commented on Dec 20, 2016

jesseschalken commented on Dec 20, 2016

gcnew commented on Dec 23, 2016

jesseschalken commented on Dec 29, 2016

gcnew commented on Dec 29, 2016

jesseschalken commented on Dec 29, 2016

gcnew commented on Dec 30, 2016

7 remaining items

axefrog commented on Mar 11, 2017

RyanCavanaugh commented on Mar 11, 2017

axefrog commented on Mar 11, 2017

gcnew commented on Mar 12, 2017

falsandtru commented on Aug 2, 2017

RyanCavanaugh commented on Jul 26, 2019

mprobst commented on Nov 14, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions