Skip to content

Conversation

weswigham
Copy link
Member

Fixes #16778. isMatchingReferenceDiscriminant needed to use the previous type witnessed for the reference in question rather than its declared type to check if it was discriminable.

@weswigham weswigham force-pushed the flow-control-nested-discriminant branch from b4b404b to 6c6d1c2 Compare July 26, 2017 21:12
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.

I found a better name for prevType: computedType. Otherwise looks good.

isMatchingReference(reference, (<PropertyAccessExpression>expr).expression) &&
isDiscriminantProperty(declaredType, (<PropertyAccessExpression>expr).name.escapedText);
isDiscriminantProperty(prevType, (<PropertyAccessExpression>expr).name.escapedText);
Copy link
Member

Choose a reason for hiding this comment

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

prevType has a specific name elsewhere in the control flow code. I'll go find out what it is.

Copy link
Member

Choose a reason for hiding this comment

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

Aha! "computed type"

@sandersn
Copy link
Member

Note: I would have been a lot more enthusiastic about this fix if you had used our real Node code from the compiler in the test.

@weswigham
Copy link
Member Author

Note: I would have been a lot more enthusiastic about this fix if you had used our real Node code from the compiler in the test.

That would have been significantly greater than the minimum required code to repro and examine the bug. XD

@weswigham weswigham merged commit b080aa9 into microsoft:master Jul 26, 2017
@weswigham weswigham deleted the flow-control-nested-discriminant branch July 26, 2017 22:27
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants