Skip to content

Conversation

weswigham
Copy link
Member

Fixes #21248

We already generated these types if, for example, you say this.prop - we just didn't reflect the narrowed types on the types of the bare this reference if the this wasn't supplied by a class declaration.

const classSymbol = checkExpression(className).symbol;
if (classSymbol && classSymbol.members && (classSymbol.flags & SymbolFlags.Function)) {
return getInferredClassType(classSymbol);
return getFlowTypeOfReference(node, getInferredClassType(classSymbol));
Copy link
Member

Choose a reason for hiding this comment

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

Can you find out if there’s a performance hit for this PR? I would expect this line especially to be costly, since there are probably a lot of this.property references in an OO codebase.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, I looked the file locally, outside github’s 3 lines of context, and see that this is for JS.

@@ -0,0 +1,39 @@
=== tests/cases/compiler/controlFlowAnalysisOnBareThisKeyword.ts ===
declare function isBig(x: any): x is { big: true };
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t there be way more type differences than this, since we are now narrowing this in classes? Maybe I’m missing something about the actual change.

Copy link
Member

Choose a reason for hiding this comment

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

[Later] Yes, I did. But github doesn’t display the “Are you sure you want to delete your comment?” message on the iPad, so I can’t delete it.

@weswigham weswigham merged commit 8b81d19 into microsoft:master Feb 2, 2018
@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.

2 participants