Type hole with compatibility between optional parameters/extra parameters

[jesseschalken]

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.

[RyanCavanaugh]

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

[RyanCavanaugh]

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;

[jesseschalken]

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

[jesseschalken]

@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 {}

[gcnew]

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 @jesseschalken's suggestion, however.

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

[jesseschalken]

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

[gcnew]

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.

[jesseschalken]

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

[gcnew]

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.

[jesseschalken]

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.

Of course JavaScript allows a TypeScript function's signature to be disobeyed. JavaScript lets you pass number into a TypeScript function expecting a HTMLElement "by definition". Does that mean that TypeScript shouldn't let you assume that your parameter is a HTMLElement? Of course not.

TypeScript also statically ensures that a function is called without extra parameters and that their runtime value is therefore undefined (apart from the loophole that is the subject of this issue). You can therefore safely add extra optional parameters, because undefined is compatible. The fact that untyped JavaScript allows TypeScript's rules to be broken at runtime is irrelevant.

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.

I don't know what you mean by "defaulting overrides". You can certainly use overriding, extra optional parameters, and overloading all at the same time, right now, in a type safe way, and my suggestion wouldn't change that.

class Foo {
    foo(x: string): void { }
}

class Blah implements Foo {
    // Foo.foo just has to be compatible with at least one of these
    foo(x: string, y?: number): void;
    foo(x: number): void;
    foo(x: Promise<number>): void;

    foo(x: string | Promise<number> | number, y: number = 1) {
        // Test x and y to determine what overload was used
    }
}

let b = new Blah();
b.foo('x');//ok
b.foo(1);//ok
b.foo(Promise.resolve(1));//ok
b.foo(Promise.resolve(1), 1);// not ok, doesn't match any signatures

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.

You could pass () => const42() as the callback instead. If the callback needs parameters threaded through they would need to be mentioned explicitly though, which is an extra few chars. ("There is no simple and generic workaround" as you said earlier.)

Having the effective ({}...) => number signature maintained across modules boundaries and through declaration files would require some syntax, but I have no problem with that. Something simple like an optional , ?), , ?..), , any...) or a keyword like allowextra or anyextra at the end of the function signature would suffice. (You would only need this when expressing the type of a function that you know ignores extra parameters. It would be implied automatically for function definitions, so would only likely show up in generated .d.ts files IMO.)

[gcnew]

I truly want to disambiguate the matter, maybe I'm not expressing myself well.

I'm not bringing in the argument that you can violate type expectations via JS. While there is nothing stopping you from doing it, programmers don't do it in practice because even if the type is not explicit it is still there and not abiding to it results in a runtime error.

On the "statically ensures" part is where see different approaches. While it is true that you are not allowed to pass extra arguments on purpose it is often implicitly the case with callbacks. Now there are two ways forward (as you've already stated in the description of the issue).

I think the approach you propose is hardly an ideal one. Why?

• it breaks local inference
• has non-uniform behaviour with local/module functions
• requires extra care or syntax

These reasons are not all true at the same time, rather they are possible consequences of the tradeoffs involved in an actual implementation.

Alternatively, I see the issue from the other side. From my point of view a (a?: number) => number function should not be assignable to a () => number reference. This is because it is a very standard practice to use functions as callbacks and maybe pass extra arguments to them. If we look at this assignability conundrum from a logical perspective and ask ourselves is it correct to allow it, for me the answer is sometimes. On the one hand yes, it will work sometimes, because defaulting happens at runtime in the presence of undefined (either explicit or because of a missing argument). On the other hand, the types are actually quite different and it is impossible to say which of the two it is at compile time. The difference is visible at runtime only having non-undefined extra arguments.

My proposal (which is not actually mine, you've mentioned it in the description) embraces the idea that extra arguments will be passed and warns you to use a safer pattern in the case of defaulting. The negatives are that overloading and overriding will be broken. I can live with that as I don't use classes and even less so class hierarchies. Having said that, I do agree it's not acceptable for everyone just as it's not acceptable for me to add boilerplate to my perfectly valid function signatures and usage.

In the end of the day it all boils down to: either functions with lower arity are not assignable to references with higher (your proposal) or functions with extra defaulted parameters are not assignable to references without them. I support the second option, because I admit calling a function with more arguments and it also rules out the uncertainty introduced by runtime defaulting.

[RyanCavanaugh]

Discussed for a while at the backlog slog. Some more detailed notes

• This should be accomplishable with just a single bit on the function type that indicates whether or not there might be additional parameters beyond the declared ones.
  • This would be false for declared functions and consts initialized by a function expression
  • true for anything else.
• Error to assign to something with more parameters when that bit is set
• We're not sure if this is a massive breaking change or not?

So if anyone wants to see this happen, we'll need a PR we can run against our internal code suite to see what the real-world impact is and assess value vs breakage.

[RyanCavanaugh]

Just thinking out loud, something that's going to really problematic is class methods. The "could there be extra parameters" bit would have to be set for class methods, because a derived class could declare new optional parameters, but it's entirely reasonable to write code like this and not expect an error:

class Base {
  bar = (n: number) => { console.log(n); }
  foo() {
    // Error, but seems ok...
    [1, 2, 3].forEach(this.bar);
  }

  // Maybe you have to write this instead?? Super annoying?
  bar = (n: number, _1: may_not_use, _2: may_not_use) => { console.log(n); }
}

// Theoretically-possible problem case
// that people will complain that they didn't do and shouldn't be bothered about
class Derived extends Base {
  bar = (n: number, s?: string) => { console.log(n); }
}

[axefrog]

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> {
  // ...
}

[RyanCavanaugh]

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

[axefrog]

@RyanCavanaugh Because I get red squigglies otherwise. Evidence below:

[gcnew]

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.

[falsandtru]

This issue seems to have fixed.

[RyanCavanaugh]

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

[mprobst]

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.