Skip to content

Fix structural identity relations #21839

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

Merged
merged 4 commits into from
Feb 11, 2018
Merged

Fix structural identity relations #21839

merged 4 commits into from
Feb 11, 2018

Conversation

ahejlsberg
Copy link
Member

This PR adds several missing structural identity relations for higher order types, specifically index types, indexed access types, conditional types, and substitution types. Previously we'd only consider such types identical when they have the same object identity, now we properly check their constituents for structural identity.

This PR also more consistently handles distributive conditional types.

Finally, the PR introduces a new type relation: A conditional type X extends Y ? S1 : S2 is related to a conditional type X extends Y ? T1 : T2 if S1 is related to T1 and S2 is related to T2. In other words, two conditional types with identical conditions are related if their true and false branch types are pairwise related.

Fixes #21823.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good, but isIdenticalTo doesn't make sense to me:

if (result = eachTypeRelatedToSomeType(<UnionOrIntersectionType>source, <UnionOrIntersectionType>target)) {
if (result &= eachTypeRelatedToSomeType(<UnionOrIntersectionType>target, <UnionOrIntersectionType>source)) {
return result;
}
}
}
if (flags & TypeFlags.Index) {
return isRelatedTo((<IndexType>source).type, (<IndexType>target).type, /*reportErrors*/ false);
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense for isIdenticalTo to call back into isRelatedTo? isRelatedTo will use whatever the current relation is, which won't necessarily be identityRelation

Copy link
Member

Choose a reason for hiding this comment

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

Now that I've read all of isIdenticalTo, I think that it should actually be called isStructurallyIdenticalTo (for the single level at which it's called). I'm not sure that matches the intent of any of its callers.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only way we could be in the isIdenticalTo function is if the current relation is identityRelation. We need to call back to isRelatedTo because it handles primtives, etc.

Copy link
Member

Choose a reason for hiding this comment

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

OK, that was the piece that I was missing.

# Conflicts:
#	tests/baselines/reference/conditionalTypes1.errors.txt
#	tests/baselines/reference/conditionalTypes1.js
#	tests/baselines/reference/conditionalTypes1.symbols
#	tests/baselines/reference/conditionalTypes1.types
#	tests/cases/conformance/types/conditional/conditionalTypes1.ts
if (result &= isRelatedTo((<ConditionalType>source).extendsType, (<ConditionalType>target).extendsType, /*reportErrors*/ false)) {
if (result &= isRelatedTo((<ConditionalType>source).trueType, (<ConditionalType>target).trueType, /*reportErrors*/ false)) {
if (result &= isRelatedTo((<ConditionalType>source).falseType, (<ConditionalType>target).falseType, /*reportErrors*/ false)) {
if (isDistributiveConditionalType(<ConditionalType>source) === isDistributiveConditionalType(<ConditionalType>target)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not check this one first, seems the cheapest.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it is also the least selective. Practically all conditional types are distributive.

@ahejlsberg ahejlsberg merged commit 611ebc7 into master Feb 11, 2018
@ahejlsberg ahejlsberg deleted the fixStructuralIdentity branch February 11, 2018 01:14
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 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.

3 participants