-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 26 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 | ||||
---|---|---|---|---|---|---|
|
@@ -23846,6 +23846,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) { | ||||||
|
@@ -23876,6 +23879,25 @@ namespace ts { | |||||
return type; | ||||||
} | ||||||
|
||||||
function narrowTypeByPrivateIdentifierInInExpression(type: Type, expr: BinaryExpression, assumeTrue: boolean): Type { | ||||||
const target = getReferenceCandidate(expr.right); | ||||||
if (!isMatchingReference(reference, target)) { | ||||||
return type; | ||||||
} | ||||||
|
||||||
const privateId = expr.left; | ||||||
Debug.assertNode(privateId, isPrivateIdentifier); | ||||||
const symbol = lookupSymbolForPrivateIdentifierDeclaration(privateId.escapedText, privateId); | ||||||
if (symbol === undefined) { | ||||||
return type; | ||||||
} | ||||||
const classSymbol = symbol.parent!; | ||||||
const targetType = hasStaticModifier(Debug.checkDefined(symbol.valueDeclaration, "should always have a declaration")) | ||||||
? getTypeOfSymbol(classSymbol) as InterfaceType | ||||||
: getDeclaredTypeOfSymbol(classSymbol); | ||||||
return getNarrowedType(type, targetType, assumeTrue, isTypeDerivedFrom); | ||||||
} | ||||||
|
||||||
function narrowTypeByOptionalChainContainment(type: Type, operator: SyntaxKind, value: Expression, assumeTrue: boolean): Type { | ||||||
// We are in a branch of obj?.foo === value (or any one of the other equality operators). We narrow obj as follows: | ||||||
// When operator is === and type of value excludes undefined, null and undefined is removed from type of obj in true branch. | ||||||
|
@@ -32023,14 +32045,46 @@ namespace ts { | |||||
} | ||||||
|
||||||
function checkInExpression(left: Expression, right: Expression, leftType: Type, rightType: Type): Type { | ||||||
if (leftType === silentNeverType || rightType === silentNeverType) { | ||||||
return silentNeverType; | ||||||
if (isPrivateIdentifier(left)) { | ||||||
const lexicallyScopedSymbol = lookupSymbolForPrivateIdentifierDeclaration(left.escapedText, left); | ||||||
rbuckton marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should add a call to NOTE: in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done and added a test case for this. |
||||||
if (lexicallyScopedSymbol === undefined) { | ||||||
if (!getContainingClass(left)) { | ||||||
error(left, Diagnostics.Private_identifiers_are_not_allowed_outside_class_bodies); | ||||||
} | ||||||
else { | ||||||
const suggestion = getSuggestedSymbolForNonexistentProperty(left, rightType); | ||||||
if (suggestion) { | ||||||
const suggestedName = symbolName(suggestion); | ||||||
error(left, Diagnostics.Cannot_find_name_0_Did_you_mean_1, diagnosticName(left), suggestedName); | ||||||
} | ||||||
else { | ||||||
error(left, Diagnostics.Cannot_find_name_0, diagnosticName(left)); | ||||||
} | ||||||
} | ||||||
return anyType; | ||||||
} | ||||||
|
||||||
markPropertyAsReferenced(lexicallyScopedSymbol, /* nodeForCheckWriteOnly: */ undefined, /* isThisAccess: */ false); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. most of this branch ought to be in a function called Specifically, it looks strange to markPropertyAsReferenced and set resolvedSymbol here -- checkExpression is normally what does that. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added a There is still some added logic to |
||||||
getNodeLinks(left.parent).resolvedSymbol = lexicallyScopedSymbol; | ||||||
if (rightType === silentNeverType) { | ||||||
return silentNeverType; | ||||||
} | ||||||
} | ||||||
else { | ||||||
rbuckton marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if (leftType === silentNeverType || rightType === silentNeverType) { | ||||||
return silentNeverType; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regretting that when I disabled all of my extensions to free up some CPU I also disabled the code-spell-checker. I'll reenable, as mistakes like this shouldn't be getting through to the PR. |
||||||
} | ||||||
leftType = checkNonNullType(leftType, left); | ||||||
// TypeScript 1.0 spec (April 2014): 4.15.5 | ||||||
// Require the left operand to be of type Any, the String primite type, or the Number primite type. | ||||||
rbuckton marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if (!(allTypesAssignableToKind(leftType, TypeFlags.StringLike | TypeFlags.NumberLike | TypeFlags.ESSymbolLike) || | ||||||
rbuckton marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
isTypeAssignableToKind(leftType, TypeFlags.Index | TypeFlags.TemplateLiteral | TypeFlags.StringMapping | TypeFlags.TypeParameter))) { | ||||||
error(left, Diagnostics.The_left_hand_side_of_an_in_expression_must_be_of_type_any_string_number_or_symbol); | ||||||
rbuckton marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
} | ||||||
leftType = checkNonNullType(leftType, left); | ||||||
rightType = checkNonNullType(rightType, right); | ||||||
// TypeScript 1.0 spec (April 2014): 4.15.5 | ||||||
// The in operator requires the left operand to be of type Any, the String primitive type, or the Number primitive type, | ||||||
// and the right operand to be | ||||||
// The in operator requires the right operand to be | ||||||
// | ||||||
// 1. assignable to the non-primitive type, | ||||||
// 2. an unconstrained type parameter, | ||||||
|
@@ -32048,10 +32102,6 @@ namespace ts { | |||||
// unless *all* instantiations would result in an error. | ||||||
// | ||||||
// The result is always of the Boolean primitive type. | ||||||
if (!(allTypesAssignableToKind(leftType, TypeFlags.StringLike | TypeFlags.NumberLike | TypeFlags.ESSymbolLike) || | ||||||
isTypeAssignableToKind(leftType, TypeFlags.Index | TypeFlags.TemplateLiteral | TypeFlags.StringMapping | TypeFlags.TypeParameter))) { | ||||||
error(left, Diagnostics.The_left_hand_side_of_an_in_expression_must_be_of_type_any_string_number_or_symbol); | ||||||
} | ||||||
const rightTypeConstraint = getConstraintOfType(rightType); | ||||||
if (!allTypesAssignableToKind(rightType, TypeFlags.NonPrimitive | TypeFlags.InstantiableNonPrimitive) || | ||||||
sandersn marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
rightTypeConstraint && ( | ||||||
|
@@ -40219,6 +40269,15 @@ namespace ts { | |||||
return resolveEntityName(name as Identifier, /*meaning*/ SymbolFlags.FunctionScopedVariable); | ||||||
} | ||||||
|
||||||
if (isPrivateIdentifier(name) && isBinaryExpression(name.parent)) { | ||||||
const links = getNodeLinks(name.parent); | ||||||
if (links.resolvedSymbol) { | ||||||
return links.resolvedSymbol; | ||||||
} | ||||||
checkBinaryExpression(name.parent, CheckMode.Normal); | ||||||
return links.resolvedSymbol; | ||||||
} | ||||||
|
||||||
return undefined; | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ namespace ts { | |
// Class Fields Helpers | ||
createClassPrivateFieldGetHelper(receiver: Expression, state: Identifier, kind: PrivateIdentifierKind, f: Identifier | undefined): Expression; | ||
createClassPrivateFieldSetHelper(receiver: Expression, state: Identifier, value: Expression, kind: PrivateIdentifierKind, f: Identifier | undefined): Expression; | ||
createClassPrivateFieldInHelper(state: Identifier, receiver: Expression): Expression; | ||
} | ||
|
||
export function createEmitHelperFactory(context: TransformationContext): EmitHelperFactory { | ||
|
@@ -75,6 +76,7 @@ namespace ts { | |
// Class Fields Helpers | ||
createClassPrivateFieldGetHelper, | ||
createClassPrivateFieldSetHelper, | ||
createClassPrivateFieldInHelper | ||
}; | ||
|
||
/** | ||
|
@@ -395,6 +397,10 @@ namespace ts { | |
return factory.createCallExpression(getUnscopedHelperName("__classPrivateFieldSet"), /*typeArguments*/ undefined, args); | ||
} | ||
|
||
function createClassPrivateFieldInHelper(state: Identifier, receiver: Expression) { | ||
context.requestEmitHelper(classPrivateFieldInHelper); | ||
return factory.createCallExpression(getUnscopedHelperName("__classPrivateFieldIn"), /* typeArguments*/ undefined, [state, receiver]); | ||
} | ||
} | ||
|
||
/* @internal */ | ||
|
@@ -961,6 +967,29 @@ namespace ts { | |
};` | ||
}; | ||
|
||
/** | ||
* Parameters: | ||
* @param state — One of the following: | ||
* - A WeakMap when the member is a private instance field. | ||
* - A WeakSet when the member is a private instance method or accessor. | ||
* - A function value that should be the undecorated class constructor when the member is a private static field, method, or accessor. | ||
* @param receiver — The object being checked if it has the private member. | ||
* | ||
* Usage: | ||
* This helper is used to transform `#field in expression` to | ||
* `__classPrivateFieldIn(expression, <weakMap/weakSet/constructor>)` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment needs to be updated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks! |
||
*/ | ||
export const classPrivateFieldInHelper: UnscopedEmitHelper = { | ||
name: "typescript:classPrivateFieldIn", | ||
importName: "__classPrivateFieldIn", | ||
scoped: false, | ||
text: ` | ||
var __classPrivateFieldIn = (this && this.__classPrivateFieldIn) || function(state, receiver) { | ||
if (receiver === null || (typeof receiver !== "object" && typeof receiver !== "function")) throw new TypeError("Cannot use 'in' operator on non-object"); | ||
return typeof state === "function" ? receiver === state : state.has(receiver); | ||
sandersn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
};` | ||
}; | ||
|
||
let allUnscopedEmitHelpers: ReadonlyESMap<string, UnscopedEmitHelper> | undefined; | ||
|
||
export function getAllUnscopedEmitHelpers() { | ||
|
@@ -986,6 +1015,7 @@ namespace ts { | |
exportStarHelper, | ||
classPrivateFieldGetHelper, | ||
classPrivateFieldSetHelper, | ||
classPrivateFieldInHelper, | ||
createBindingHelper, | ||
setModuleDefaultHelper | ||
], helper => helper.name)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4602,8 +4602,15 @@ namespace ts { | |
} | ||
|
||
function parseBinaryExpressionOrHigher(precedence: OperatorPrecedence): Expression { | ||
// parse a BinaryExpression the LHS is either: | ||
// 1) a PrivateIdentifier when 'in' flag allowed and lookahead matches 'PrivateIdentifier in' | ||
// 2) a UnaryExpression | ||
|
||
const pos = getNodePos(); | ||
const leftOperand = parseUnaryExpressionOrHigher(); | ||
const lookaheadMatchesPrivateIdentifierIn = token() === SyntaxKind.PrivateIdentifier && !inDisallowInContext() && lookAhead(nextTokenIsInKeyword); | ||
const leftOperand = lookaheadMatchesPrivateIdentifierIn | ||
? parsePrivateIdentifier() | ||
: parseUnaryExpressionOrHigher(); | ||
return parseBinaryExpressionRest(precedence, leftOperand, pos); | ||
} | ||
|
||
|
@@ -4640,9 +4647,11 @@ namespace ts { | |
// ^^token; leftOperand = b. Return b ** c to the caller as a rightOperand | ||
// a ** b - c | ||
// ^token; leftOperand = b. Return b to the caller as a rightOperand | ||
const consumeCurrentOperator = token() === SyntaxKind.AsteriskAsteriskToken ? | ||
newPrecedence >= precedence : | ||
newPrecedence > precedence; | ||
const isPrivateIdentifierInInExpression = token() === SyntaxKind.InKeyword && isPrivateIdentifier(leftOperand); | ||
const consumeCurrentOperator = isPrivateIdentifierInInExpression || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be better to parse a private identifier unconditionally and add a grammar error in checkPrivateIdentifier if the identifier's parent is incorrect. Currently, it becomes a lot more complicated to convince myself that binary expression parsing is correct. This also means that
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are 100% on the money with this. Turns out the parsing was incorrect, and so is the parsing in v8 (about to raise a ticket). I'm changing things to the way you said and it works much better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parser logic updated now. At first I thought going with this approach had broken the tests but turned out the tests were wrong, and this new approach leads to the correct result. When I wrote the tests for this back in Febuary there were not implementations to try, and babel did not get the parsing right (they have now addressed these issues). So I unfortunately developed the wrong intuition for how this syntax should parse when nested within other expressions. Did spot an issue in V8 when trying to compare to what they do: https://bugs.chromium.org/p/v8/issues/detail?id=12259 |
||
(token() === SyntaxKind.AsteriskAsteriskToken ? | ||
newPrecedence >= precedence : | ||
newPrecedence > precedence); | ||
|
||
if (!consumeCurrentOperator) { | ||
break; | ||
|
@@ -6131,6 +6140,11 @@ namespace ts { | |
return (tokenIsIdentifierOrKeyword(token()) || token() === SyntaxKind.NumericLiteral || token() === SyntaxKind.BigIntLiteral || token() === SyntaxKind.StringLiteral) && !scanner.hasPrecedingLineBreak(); | ||
} | ||
|
||
function nextTokenIsInKeyword() { | ||
nextToken(); | ||
return token() === SyntaxKind.InKeyword; | ||
} | ||
|
||
function isDeclaration(): boolean { | ||
while (true) { | ||
switch (token()) { | ||
|
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.
Shouldn't this be cached on the private identifier? Is there a call that caches? lookupSymbolForPrivateIdentifierDeclaration doesn't, so I guess that there is a wrapper for it that does. This code should call that wrapper instead. (see comment about creating
checkPrivateIdentifier
-- that is probably the right place if it doesn't already exist.)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.
Thinking about this overnight, it's probably enough to create
checkPrivateIdentifier
in this PR, and fix caching later. The problem predates this PR, but it means that programs with lots of private identifers are likely to have performance problems.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.
You were right, the symbol was cached. So not having to re-resolve the symbol when narrowing now.
Right now I am caching the symbol on the
BinaryExpression
parent. The only reason I am doing this is because otherwise 'findAllRefs' doesn't see the symbol. everything else seems to work if I store the symbol based on thePrivateIdentifier
. I'd like to address this, I'm thinking there must be a 'set' that PrivateIds need to be added so they are valid places for 'findAllRefs' to search.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?
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 could put in a call to
checkPrivateIdentifierExpression
if the symbol comes back undefined to be 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.
For find-all-refs, you're possibly being tripped up by the fact
PrivateIdentifier
isn't handled byisExpressionNode
:TypeScript/src/compiler/utilities.ts
Line 1904 in 40bd336
called from here:
https://github.com/microsoft/TypeScript/blob/7c49552cd516da620b903edccc61af54d1872c3b/src/compiler/checker.ts#L40232
For an
Identifier
, we would perform a normal name resolution:https://github.com/microsoft/TypeScript/blob/7c49552cd516da620b903edccc61af54d1872c3b/src/compiler/checker.ts#L40245
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 oldPrivateIdentifierInInExpression
syntax kind from that predicate but not putPrivateIdentifier
s in.I rather than
PrivateIdentifier
always returning true forisExpressionNode
, it only returns true if it's in a valid position to be an 'expression'. This means the checker can useisExpressionNode
for the grammer checks.I did think about
utilities.ts
exporting a new more explicitisPrivateIdentifierInValidExpressionPosition
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.
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 isundefined
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 gettingundefined
is actually a cache-hit but because of a typo in the privateIdentifier it has no symbol to resolve to - yetcheckPrivateIdentifierExpression
would keep being called.