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.

Sign up for GitHub

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

Jump to bottom

Preserve literal types in contextual unions #19966

Merged
merged 14 commits into from
Dec 11, 2017
Merged

Preserve literal types in contextual unions #19966

merged 14 commits into from
Dec 11, 2017

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Nov 13, 2017 •
edited
Loading

Fixes #16457, replaces #19587, in which this code started getting reviewed.

Adds a test for #20279.
Also fixes #19837.
Also fixes #16457.

@weswigham weswigham mentioned this pull request Nov 13, 2017
Closed
@weswigham weswigham force-pushed the more-more-prolific-literals branch 2 times, most recently from b08779b to 491e464 Compare November 13, 2017 18:39
@weswigham
Copy link
Member Author

@ahejlsberg I've updated this for post-dynamic-names merge; would you like to take a look over it again so we can merge and fix #19837 and #16457?

ahejlsberg
ahejlsberg requested changes Nov 17, 2017
View reviewed changes
src/compiler/checker.ts Outdated
return shouldUseLiteralType(declaration) ? type : getWidenedLiteralType(type);
}

function shouldUseLiteralType(declaration: VariableLikeDeclaration) {
Copy link
Member

Choose a reason for hiding this comment

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

shouldKeepLiteralType may be a better name since it only matters if you already have a literal type.

src/compiler/checker.ts Outdated
getUnionType([type, checkExpressionCached(declaration.initializer)], /*subtypeReduction*/ true) :
type;
return shouldUseLiteralType(declaration) ? type : getWidenedLiteralType(type);
Copy link
Member

Choose a reason for hiding this comment

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

What are the cases that are affected by this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

You can see the changes at https://github.com/weswigham/TypeScript/pull/1/files

src/compiler/checker.ts Outdated
}

function isLiteralLikeContextualType(contextualType: Type) {
function isLiteralLikeContextualType(candidateLiteral: Type, contextualType: Type): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

isLiteralOfContextualType

src/compiler/checker.ts Outdated
return !!(((contextualType.flags & TypeFlags.StringLike) && maybeTypeOfKind(candidateLiteral, TypeFlags.StringLike)) ||
((contextualType.flags & TypeFlags.NumberLike) && maybeTypeOfKind(candidateLiteral, TypeFlags.NumberLike)) ||
((contextualType.flags & TypeFlags.BooleanLike) && maybeTypeOfKind(candidateLiteral, TypeFlags.BooleanLike)) ||
((contextualType.flags & TypeFlags.ESSymbolLike) && maybeTypeOfKind(candidateLiteral, TypeFlags.ESSymbolLike))
Copy link
Member

Choose a reason for hiding this comment

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

Why all the extra parentheses?

Copy link
Member Author

Choose a reason for hiding this comment

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

Old habit; I tend to add parens around any inner binary expression since internalizing operator precedence beyond simple PEMDAS isn't something I, personally, bother to think about. They're gone now~

@weswigham
Copy link
Member Author

@ahejlsberg Ran DT tests - Fixes 7 instances of #19837, and changes the error text of 4 tests to include or not include literals as appropriate for the new rules, same as RWC (introducing no new errors).

@mhegazy
Copy link
Contributor

mhegazy commented Nov 28, 2017

@weswigham we had an offline discussion today as a result of #20279. the main cause of the memory load is generating unions of literal types and doing sub-type reduction on them. the inclusion of this change exasperates the issue further.

@ahejlsberg has a change to limit the inference of literal types to only when the contextual type has a literal type from the same family, and that seems to alleviate the issue for these large json-like arrays/objects with a contextual type. but obviously this is going the opposite direction of this PR.

I now believe your original proposal for including the literal type in the contextual type is a better approach, and we should go with that instead. @ahejlsberg seems to have changed his mind on that issue as well after looking into #20279.

@WKenya WKenya mentioned this pull request Nov 28, 2017
Closed
@weswigham
Copy link
Member Author

I now believe your original proposal for including the literal type in the contextual type is a better approach, and we should go with that instead. @ahejlsberg seems to have changed his mind on that issue as well after looking into #20279.

Done.

@ahejlsberg has a change to limit the inference of literal types to only when the contextual type has a literal type from the same family, and that seems to alleviate the issue for these large json-like arrays/objects with a contextual type. but obviously this is going the opposite direction of this PR.

I couldn't find a PR for that, so I built it into this one, since it was a smallish change in the same place as I've been working in here. I'll add a test for #20279 hopefully shortly.

@weswigham
Copy link
Member Author

I've added a test for #20279 and confirmed that with the proposed tweaks, this fixes it.

@weswigham weswigham changed the title Cherrypick non-comparability related changes from prolific literals PR Preserve literal types in contextual unions Dec 8, 2017
@weswigham
Copy link
Member Author

weswigham commented Dec 8, 2017 •
edited
Loading

@ahejlsberg This has now been merged with master and is now just the change for preserving literal types in unions generated via contextual typing (the original proposal the fix the original issues).

@weswigham
Copy link
Member Author

@ahejlsberg getUnionType now uses a tristate for determing the level of subtype reduction to apply, per your request. Available are None, Literal, and Subtype.

@weswigham weswigham merged commit eba15b5 into microsoft:master Dec 11, 2017
@weswigham weswigham deleted the more-more-prolific-literals branch December 11, 2017 23:03
@mickdekkers mickdekkers mentioned this pull request Dec 22, 2017
Merged
8 tasks
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ahejlsberg ahejlsberg ahejlsberg requested changes

@sandersn sandersn sandersn approved these changes

@mhegazy mhegazy Awaiting requested review from mhegazy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fails to get contextual type for string literal in array Refine unions in contextual typing
4 participants
@weswigham @mhegazy @sandersn @ahejlsberg