Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow overloads to be specified in interface #7230

Closed
masaeedu opened this issue Feb 24, 2016 · 17 comments
Closed

Allow overloads to be specified in interface #7230

masaeedu opened this issue Feb 24, 2016 · 17 comments
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead

Comments

@masaeedu
Copy link
Contributor

TypeScript Version:

nightly (1.9.0-dev.20160217)

Code
A contrived example:

interface Convert {
  (arg: string): number;
  (arg: number): string;
}

function convert(arg: number | string): number | string {
  return typeof arg === 'number' ? arg.toString() : parseFloat(arg);
}

let foo: Convert = convert;

Expected behavior:
Code above compiles, and the following compiles and runs without errors:

let result1: string = foo(10);
let result2: number = foo(result1);
assert(result2 === 10);

Actual behavior:
Compilation error:

...(10,5): error TS2322: Type '(arg: number | string) => number | string' is not assignable to type 'Convert'.
Type 'number | string' is not assignable to type 'number'.
Type 'string' is not assignable to type 'number'.

Current approach:
Specify all overloads again above function:

interface Convert {
  (arg: string): number;
  (arg: number): string;
}

function convert(arg: string): number;
function convert(arg: number): string;
function convert(arg: number | string): number | string {
  return typeof arg === 'number' ? arg.toString() : parseFloat(arg);
}

let foo: Convert = convert;
@RyanCavanaugh RyanCavanaugh added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Feb 24, 2016
@RyanCavanaugh
Copy link
Member

This is intentional. The signature convert(arg: number | string): number | string does not have the same contract as Convert. For example, you could have written:

function convert(arg: number | string): number | string {
  return Math.random() > 0.5 ? 42 : 'foo';
}

which doesn't maintain the number -> number / string -> string input/output relationship.

@mhegazy mhegazy closed this as completed Feb 24, 2016
@masaeedu
Copy link
Contributor Author

@RyanCavanaugh Yes, but I fail to see how this is any different from regular overloads. You can just as easily write:

function convert(arg: string): number;
function convert(arg: number): string;
function convert(arg: number | string): number | string {
  return Math.random() > 0.5 ? 42 : 'foo';
}

And when you request parameter completion at the following position:

convert(|

You will get number -> string and string -> number. Could this please be reconsidered?

@DanielRosenwasser
Copy link
Member

The difference is that the implementation is free to delegate how users should call the function. Flexibility in authoring overloads, on the other hand, is strictly necessary.

Linking to #1805.

@masaeedu
Copy link
Contributor Author

Shouldn't the interface be authoritative as to how users should call the function? That is the point of an interface after all.

Thanks for the link, I'm reading through that now. The discussion in #1805 suggests this is difficult to implement (or that the implementation would be computationally expensive). Couldn't the existing logic for ensuring that the bottom overload in an overload list satisfies all prior overloads be reused, in order to ensure that the function can be called with all signatures specified in the interface?

@RyanCavanaugh
Copy link
Member

Couldn't the existing logic for ensuring that the bottom overload in an overload list satisfies all prior overloads be reused, in order to ensure that the function can be called with all signatures specified in the interface?

It certainly could, but that logic would reject the assignment anyway. It's legal to invoke your function with a number and it's legal to invoke it with a string, but the return type of the function is invariantly number|string which is not assignable to number and is not assignable to string.

@masaeedu
Copy link
Contributor Author

@DanielRosenwasser I should mention that what I'm suggesting is for a function that already accepts union types to be assignable to an interface with multiple overloads, not vice versa. The example given by @danquirk in that issue:

function foo(x: string, y: string) {}
function foo(x: number, y: number) {}

var arg: string|number;
foo(arg, arg); // should be an error

is not relevant, since that's trying to pass a union argument to an overloaded function. Instead, I'm suggesting that the following (which works):

interface Convert {
  (arg: string): number;
  (arg: number): string;
}

function convert(arg: string): number;
function convert(arg: number): string;
function convert(arg: number | string): number | string {
  return typeof arg === 'number' ? arg.toString() : parseFloat(arg);
}

let foo: Convert = convert;

behave identically to the following (which doesn't currently work):

interface Convert {
  (arg: string): number;
  (arg: number): string;
}

function convert(arg: number | string): number | string {
  return typeof arg === 'number' ? arg.toString() : parseFloat(arg);
}

let foo: Convert = convert;

since it is exactly as type safe.

@RyanCavanaugh
Copy link
Member

@masaeedu the assignability rules do work that way. For example:

declare function fn(x: string): HTMLElement;
declare function fn(x: number): Element;

function myFn(x: string|number): HTMLDivElement { return undefined };

// OK
var x: typeof fn = myFn;

@masaeedu
Copy link
Contributor Author

@RyanCavanaugh I hope I haven't been blathering on about something that already works 😱 Are you saying the last snippet in my previous comment should work?

@RyanCavanaugh
Copy link
Member

It shouldn't work because it's not "exactly as type safe.".

Just to break it down:

  1. When invoked with string, Convert returns number
  2. When invoked with string, convert returns number | string
  3. It's not valid to assign a number | string to a number
  4. Therefore, it is not valid to use convert as a Convert

Each of these should be uncontroversial, I hope? Which of these do you consider to not be correct?

@masaeedu
Copy link
Contributor Author

Okay, thanks. That helps. Let's start with 1. An instance of Convert, when invoked with string, returns number, not string.

@RyanCavanaugh
Copy link
Member

Typo fixed

@masaeedu
Copy link
Contributor Author

2 is fine. 3 is also true, but is not relevant, since we ignore this problem when creating overloads as well. When I do:

function convert(arg: string): number;
function convert(arg: number): string;
function convert(arg: number | string): number | string {
  return typeof arg === 'number' ? arg.toString() : parseFloat(arg);
}

there's no flow analysis being done to ensure that the function actually returns number when passed string and string when passed number. We just kind of close our eyes and hope the implementation does the right thing.

In other words, we don't enforce the covariance of the function in its return type. We assume a function returning number | string is acceptable wherever a function that returns number is required, and wherever a function that returns string is required. We only enforce contravariance in its arguments.

This is why I was saying it is exactly as type safe.

@RyanCavanaugh
Copy link
Member

I understand what you're suggesting, but don't see why it's at all desirable. What makes function return types special here compared to any other type system position? This line of reasoning works equally well anywhere -- we could just as well make all assignability relations completely bidirectional.

It seems like working backward from an example to produce a rule that doesn't generalize well.

@masaeedu
Copy link
Contributor Author

@RyanCavanaugh You are right that this doesn't work as a general principle. This would be a special case with respect to overload assignability only. I would imagine there's some special-cased, not broadly applicable code in the type checker to deal with function overloads anyway, no?

The reason this is desirable is because duplicating all the overloads you specify in an interface as vestigial function declarations atop the implementation is annoying, and bad for refactorability.

@masaeedu
Copy link
Contributor Author

masaeedu commented Aug 3, 2017

Please revisit this. My function implementations are starting to look like this, which is a total nightmare from a readability and DRY perspective:

  presignedGetObject(bucketName: string, objectName: string, callback: Callback<string>): void
  presignedGetObject(bucketName: string, objectName: string, expires: number, callback: Callback<string>): void
  presignedGetObject(bucketName: string, objectName: string, expires: number, respHeaders: ResponseHeaders, callback: Callback<string>): void
  presignedGetObject(bucketName: string, objectName: string, arg3: number | Callback<string>, arg4?: ResponseHeaders | Callback<string>, arg5?: Callback<string>) {

I can construct type signatures much more conveniently in type ... expressions using intersection types, and I really don't want to clutter up the implementation with all this noise.

I am able to do type F = /* complicated function signature */; const f: F = (a, b, c) => { /* use a, b, c, with full type inference */ }, except when the function signature is overloaded, so the special case is the behavior that's already baked in, not the behavior I'm proposing.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 3, 2017

Just to comment, from a readability and DRY perspective, it is much better to author APIs that take an options bag, or even a tagged union option bag instead of creating overloads with complex signatures.

@masaeedu
Copy link
Contributor Author

masaeedu commented Aug 3, 2017 via email

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead
Projects
None yet
Development

No branches or pull requests

5 participants