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

Use contextual signature instantiation to compare signatures #138

Closed
JsonFreeman opened this issue Jul 18, 2014 · 7 comments
Closed

Use contextual signature instantiation to compare signatures #138

JsonFreeman opened this issue Jul 18, 2014 · 7 comments
Labels
Fixed A PR has been merged for this issue Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@JsonFreeman
Copy link
Contributor

Now that we have a heuristic algorithm for infinitely expanding generics, we can go back to comparing signatures using contextual signature instantiation. For example:

var f: (s: string) => number;
var g: <T>(s: T) => T;
f = g;
g = f;

Both of these should ideally fail. The reason we could not do this earlier was because we could not detect infinitely expanding generics properly. But the current algorithm would prevent us from blowing up in the cases where the previous algorithm failed.

@JsonFreeman
Copy link
Contributor Author

Here is another very surprising example:

function arrayize<T extends String>(x: T) {
    return [x];
}

var xs = [1, 2, 3];
var z = xs.map(arrayize);
var s: string = z;

The type argument to map is inferred to be String[], so we are trying to pass arrayize into something that takes a number => String[]. However, the assignability still succeeds because arrayize is instantiated to any => any[], which is compatible with number => String[]. @ahejlsberg and I looked at this, and it was surprising to both of us that this was allowed.

@RyanCavanaugh
Copy link
Member

Looking for formal description + a rundown of potential changes

@myitcv
Copy link

myitcv commented Apr 8, 2016

@RyanCavanaugh @JsonFreeman thanks for the responses on #241

What needs to happen to move this forward?

@RyanCavanaugh
Copy link
Member

I believe it's basically an expert-level "bug fix" in the checker code somewhere, plus some spec language. I'm not familiar with how contextual signature instantiation actually works so I don't have any more details.

@JsonFreeman
Copy link
Contributor Author

@myitcv The fix would be to replace the two calls to getErasedSignature in compareSignaturesRelated. Instead you would call instantiateSignatureInContextOf(source, target, undefined). I believe this would return an instantiated version of source that would then be assignable to target (in terms of parameters and return type).

There are a few other things that would need to be worked out though. You'd have to detect an instantiation failure, which would indicate that the signatures are not compatible. Another thing to consider is the type parameter constraints of the source signature, which is trickier than it used to be because of the new F-bounded polymorphism feature.

Contextual signature instantiation was indeed used to compare signatures before 1.0. We switched to a loose check for a few reasons. Chiefly, many users found it too strict - there were frequent errors on code that seems okay. And it is a more expensive check, particularly when comparing a generic signature member of an infinitely expanding generic type. These are two caveats to watch out for when trying this fix.

@myitcv
Copy link

myitcv commented Apr 9, 2016

@JsonFreeman thanks for the notes, almost certainly well beyond my capabilities to even think about contributing to this one.

When combined with #7234, this is biting us pretty hard at the moment. So any progress on both issues would be massively appreciated!

@mhegazy
Copy link
Contributor

mhegazy commented May 18, 2017

This is essentially the same as #5616

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants