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

Implement new overload compatibility checking #6075

Merged
merged 21 commits into from
Dec 18, 2015

Conversation

DanielRosenwasser
Copy link
Member

Addresses #943 and #4180.

This PR loosens the compatibility rules between an implementation and an overload. We now check for both

  • whether the implementation is assignable to the overload, while disregarding return types
  • whether the implementation return type is assignable to or from the overload return type

This means that something like

function getAnimal(x: number): Dog;
function getAnimal(x: string): Cat;
function getAnimal(x: number | string): Cat | Dog {
    // ...
}

will finally work, as will

function getAnimal(x: number): Dog;
function getAnimal(x: string): Cat;
function getAnimal(x: number | string): Animal {
    // ...
}

const targetReturnType = getReturnTypeOfSignature(erasedTarget);
if (targetReturnType === voidType
|| checkTypeRelatedTo(targetReturnType, sourceReturnType, assignableRelation, /*errorNode*/ undefined)
|| checkTypeRelatedTo(sourceReturnType, targetReturnType, assignableRelation, /*errorNode*/ undefined)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In response to @weswigham's point #943 (comment), I believe the special casing of signature return types wouldn't come into play here. The return types are being compared solely as types, and not signature return types. So I think boolean would be assignable to a type predicate in this context, which is indeed desirable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, and you can see that in stringLiteralTypesAsTags01.ts. I'll have to add more test cases though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My ultimate point is that concern number 2 in #943 (comment) is not an issue, since boolean would work in the /*????*/ slot.

// Create object types to actually perform relation checks.
const anyReturningSourceType = getOrCreateTypeFromSignature(anyReturningSource);
const anyReturningTargetType = getOrCreateTypeFromSignature(anyReturningTarget);
return checkTypeRelatedTo(anyReturningSourceType, anyReturningTargetType, assignableRelation, /*errorNode*/ undefined);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're computing and caching an awful lot of types here. They'll sit in the cache of their associated signature forever and also gum up the assignable relation cache. I think we need a function similar to compareSignatures that checks for assignability instead of identity. The function could have an ignoreReturnTypes flag and then you wouldn't need to create all of these new signatures and types.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

One other possibility is to just call isSignatureAssignableTo in both directions. For the return types, doing this would yield the bivariance you are seeking. Parameter types are bivariant anyway, so each corresponding pair would just get checked in both directions twice. The only real difference is that you'd be extra permissive with the arity checks on the signatures. Namely, an overload would be allowed to be shorter (fewer parameters) than the implementation signature. You could add the necessary arity checks after the comparison to make sure the arities are only related in the direction you want. It's kind of nifty, though perhaps a bit more roundabout than the compareSignatures style approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isSignatureAssignableTo was already doing the work I was trying to avoid, so I might as well just avoid the caching.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed out a change that does as @ahejlsberg's requested. There's no caching and no new type creation.

function signatureRelatedTo(source: Signature, target: Signature, reportErrors: boolean): Ternary {
// TODO (drosen): De-duplicate code between related functions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is concerning. There is a lot of duplicated functionality here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a separate branch for that work right now, but I'd like to get it in as part of a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it's mostly down to isRelatedTo vs isSignatureAssignableTo and Ternary vs boolean. Am I missing something?

}
const sourceReturnType = getReturnTypeOfSignature(source);

// The following block preserves behavior forbidding boolean returning functions from being assignable to type guard returning functions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use fewer words here: Just say "Preserve behaviour forbidding boolean functions from being assignable to type guards."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well change the original in signatureRelatedTo also

@sandersn
Copy link
Member

Looks good enough to merge if the de-dupe cleanup is coming soon. The semantics a bit loose for my liking but they are more consistent with existing Typescript.

@DanielRosenwasser
Copy link
Member Author

If you'd like to discuss the semantics tomorrow, I'm open to it. 1.8 isn't set in stone just yet.

DanielRosenwasser added a commit that referenced this pull request Dec 18, 2015
Implement new overload compatibility checking
@DanielRosenwasser DanielRosenwasser merged commit 8d46ffd into master Dec 18, 2015
@DanielRosenwasser DanielRosenwasser deleted the overloadCompatibility branch December 18, 2015 01:15
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants