Discriminate contextual types#19733
Merged
weswigham merged 5 commits intomicrosoft:masterfrom Nov 7, 2017
Merged
Conversation
sandersn
requested changes
Nov 6, 2017
Member
sandersn
left a comment
There was a problem hiding this comment.
This is a cool fix! A couple of requests:
| return type && getApparentType(type); | ||
| let contextualType = getContextualType(node); | ||
| contextualType = contextualType && getApparentType(contextualType); | ||
| if (contextualType && contextualType.flags & TypeFlags.Union && isObjectLiteralExpression(node)) { |
| contextualType = contextualType && getApparentType(contextualType); | ||
| if (contextualType && contextualType.flags & TypeFlags.Union && isObjectLiteralExpression(node)) { | ||
| let match: Type | undefined; | ||
| propLoop: for (const prop of node.properties) { |
Member
There was a problem hiding this comment.
this code is almost exactly the same as the also-highly-questionable findMatchingDiscriminantType. The only real difference, I suspect, is that this code doesn't bail if (type === match). Two things:
- Can you harmonise the two code paths so there's only one
findMatchingDiscriminantType?
2.It's quite possible that the excess-property usage and union-error usage would both benefit from the(type === match)addition, so it could be that you can just swap out the existing body for this code. - Three things! Using
findMatchingDiscriminantTypeis slow, and only makes the compiler faster because it eliminates unions early on. Can you test performance with this change?
Member
Author
There was a problem hiding this comment.
findMatchingDiscriminantTypeoperates on type members (andfindMatchingDiscriminantTypedirectly usesisRelatedTo), this operates on an actual node tree (as most contextual type operations do), so sadly I don't think they can be merged (without a bunch of inefficient abstractions over weather you're getting symbols from types or nodes and such)- Sure, I'll try modifying
findMatchingDiscriminantType, too; but I don't think it'll show up in much unless we add a test case where multiple fields are capable of acting as a discriminant for an object and discriminate to the same types. 🐱 - Sure, will do.
Member
Author
There was a problem hiding this comment.
Oh hey, waddoyaknow, we already do have a test (excessPropertyCheckWithUnions) improved by changing findMatchingDiscriminantType. Neat.
Member
Author
|
@sandersn As far as overall perf goes, this has no real effect on any codebase in our perf suite:
|
sandersn
approved these changes
Nov 6, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This was split from #19587, and the idea originated from a comment on #19322 (and then the code moved directly into
getApparentTypeOfContextualTypeinstead ofgetContextualThisParameterTypeso as to be more general), and should be merged after #19587. The primary baseline changes will be visible in tests added in that PR; however there are some small changes already visible here because we toss out uninteresting contextual types earlier based on members we know the type of.This causes a few things:
thistypes are discriminated based on the other members you wrote; meaning methods in objects contextually typed by something now have thethistype of only the discriminated member you've indicated, if possible. (This is the primary benefit the tests in the other PR can show)