Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Sep 13, 2017

Fixes #18352

In #17910 I had replaced isInPropertyAssignment with a forEachAncestor loop that bailed out on any non-Expression; however, a PropertyAssignment may appear in an expression, but isPartOfExpression returns false for it.
Are there any other nodes like this? Should we just make isPartOfExpression return true for a PropertyAssignment?

@ghost ghost requested a review from sandersn September 13, 2017 21:02
@ghost ghost force-pushed the useBeforeDeclaration_propertyAssignment branch from 0f842d5 to 5ec6c91 Compare September 13, 2017 21:02
@sandersn
Copy link
Member

Seems reasonable to me, but I thought much about isPartOfExpression. @weswigham do you have opinions about PropertyAssignment being true for isPartOfExpression?

@sandersn
Copy link
Member

After skimming the code and uses of isPartOfExpression it still seems like a good idea.

@weswigham
Copy link
Member

isPartOfExpression feels like a best-effort cheapish thing. If you 100% must know if a node is within an expression context, you can't actually always trust it, I think. Its more like isPossiblyPartOfExpression. Its implementation just feels... Underspecified a little?

Having said that, it's mostly used by the language service and test harness, so changing it is unlikely to have too many significant ramifications.

@ghost
Copy link
Author

ghost commented Sep 14, 2017

It looks like that won't work because it tries to make us baseline the type of a PropertyAccess. isPartOfExpression is misnamed -- it's not returning whether something is part of an expression, it's only true if the node itself is an expression. It should be called isExpression, but the function currently named isExpression is specific to transforms and can't look at the parent.

@ghost ghost merged commit cf53743 into master Sep 14, 2017
@ghost ghost deleted the useBeforeDeclaration_propertyAssignment branch September 14, 2017 15:00
@ghost ghost mentioned this pull request Sep 14, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
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