- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.1k
ES private field check #44648
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.
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
ES private field check #44648
Changes from all commits
f04f22c
              30dde52
              31fa02b
              307247c
              61c677b
              8292ee2
              4723380
              9f1c176
              511ac22
              337f7e8
              d059c15
              79eeebe
              125df93
              c6b2a21
              320bc00
              cc80c7d
              ebbb063
              ea4fd4b
              8b78f01
              01c7042
              fc2b262
              c007a12
              40bd336
              7982164
              1cd313f
              97f7d30
              e672957
              2b7425d
              52b9f2a
              476bf24
              be26d0a
              f7ddce5
              01e0e60
              913d044
              7c49552
              c6b039f
              6f56c6a
              c3b6c2a
              5fbb2da
              c924a51
              27d494d
              33c3b55
              e3c25c9
              74e56a6
              8447934
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|  | @@ -23856,6 +23856,9 @@ namespace ts { | |||
| case SyntaxKind.InstanceOfKeyword: | ||||
| return narrowTypeByInstanceof(type, expr, assumeTrue); | ||||
| case SyntaxKind.InKeyword: | ||||
| if (isPrivateIdentifier(expr.left)) { | ||||
| return narrowTypeByPrivateIdentifierInInExpression(type, expr, assumeTrue); | ||||
| } | ||||
| const target = getReferenceCandidate(expr.right); | ||||
| const leftType = getTypeOfNode(expr.left); | ||||
| if (leftType.flags & TypeFlags.StringLiteral) { | ||||
|  | @@ -23886,6 +23889,24 @@ namespace ts { | |||
| return type; | ||||
| } | ||||
|          | ||||
| export function isExpressionNode(node: Node): boolean { | 
called from here:
For an Identifier, we would perform a normal name resolution:
Instead, you're storing the symbol on the parent binary expression and then looking it up at https://github.com/microsoft/TypeScript/blob/7c49552cd516da620b903edccc61af54d1872c3b/src/compiler/checker.ts#L40284, which seems strange. We don't normally store the symbol on Identifier references, since they can have multiple meanings, so it seems a bit strange to cache it for PrivateIdentifier (despite @sandersn's comment). However, since a PrivateIdentifier can't have anything other than a value meaning currently, we could probably store it on the NodeLinks for the id itself, rather than its parent expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! Yes it was the isExpressionNode that was causing the find-all-refs issue. I had removed the old PrivateIdentifierInInExpression syntax kind from that predicate but not put PrivateIdentifiers in.
I rather than PrivateIdentifier always returning true for isExpressionNode, it only returns true if it's in a valid position to be an 'expression'. This means the checker can use isExpressionNode for the grammer checks.
I did think about utilities.ts exporting a new more explicit isPrivateIdentifierInValidExpressionPosition predicate for the checker to use instead but that didn't feel like it added much value.
I am now caching the resolved symbol on the privateId itself, not the parent. As private-identifiers can only reference one member of a syntactically outer class, and not come from a different script/module. This seems safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be concerned that a code path could reach this line of code before the symbol is set?
While this wasn't happening in the tests. It did happen via the language server, and wasn't getting narrowed types on hover in vscode. I've added a call to checkPrivateIdentifierExpression when the symbol is undefined which fixes the issue.
Is there an alternative approach than checking for undefined, to see if the type-check has already happened? For the case when getting undefined is actually a cache-hit but because of a typo in the privateIdentifier it has no symbol to resolve to - yet checkPrivateIdentifierExpression would keep being called.
        
          
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is not 100% correct. We could be inside a class body though this does match the existing errors when handling invalid privateIds.
class C {
    f() {
        return #hello; // Private identifiers are not allowed outside class bodies.
    }
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like something we should be reporting this as a grammar error. Something like:
Private identifiers are only allowed in class bodies and may only be used as part of a class member declaration, property access, or on the left-hand-side of an 'in' expression
@DanielRosenwasser any thoughts on wording?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added that new error, and split the check into 3 levels.
- are we in a class, otherwise there is no valid way to have a private identifier
- is the privateIdentifier in a valid position
- does the privateIdentifier match a private member
Uh oh!
There was an error while loading. Please reload this page.