-
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? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # 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; | ||||
} | ||||
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. 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 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. Thinking about this overnight, it's probably enough to create 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 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 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. 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 commentThe reason will be displayed to describe this comment to others. Learn more. I could put in a call to 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. For find-all-refs, you're possibly being tripped up by the fact TypeScript/src/compiler/utilities.ts Line 1904 in 40bd336
called from here: For an 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 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! Yes it was the I rather than 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 commentThe 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 Is there an alternative approach than checking for |
||||
|
||||
function narrowTypeByPrivateIdentifierInInExpression(type: Type, expr: BinaryExpression, assumeTrue: boolean): Type { | ||||
const target = getReferenceCandidate(expr.right); | ||||
if (!isMatchingReference(reference, target)) { | ||||
return type; | ||||
} | ||||
|
||||
Debug.assertNode(expr.left, isPrivateIdentifier); | ||||
const symbol = getSymbolForPrivateIdentifierExpression(expr.left); | ||||
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. | ||||
|
@@ -27730,6 +27751,40 @@ namespace ts { | |||
} | ||||
} | ||||
|
||||
function checkGrammarPrivateIdentifierExpression(privId: PrivateIdentifier): boolean { | ||||
if (!getContainingClass(privId)) { | ||||
return grammarErrorOnNode(privId, Diagnostics.Private_identifiers_are_not_allowed_outside_class_bodies); | ||||
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 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 commentThe 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:
@DanielRosenwasser any thoughts on wording? 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 that new error, and split the check into 3 levels.
|
||||
} | ||||
if (!isExpressionNode(privId)) { | ||||
sandersn marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
return grammarErrorOnNode(privId, Diagnostics.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); | ||||
} | ||||
if (!getSymbolForPrivateIdentifierExpression(privId)) { | ||||
return grammarErrorOnNode(privId, Diagnostics.Cannot_find_name_0, idText(privId)); | ||||
} | ||||
return false; | ||||
} | ||||
|
||||
function checkPrivateIdentifierExpression(privId: PrivateIdentifier): Type { | ||||
checkGrammarPrivateIdentifierExpression(privId); | ||||
const symbol = getSymbolForPrivateIdentifierExpression(privId); | ||||
if (symbol) { | ||||
markPropertyAsReferenced(symbol, /* nodeForCheckWriteOnly: */ undefined, /* isThisAccess: */ false); | ||||
} | ||||
return anyType; | ||||
} | ||||
|
||||
function getSymbolForPrivateIdentifierExpression(privId: PrivateIdentifier): Symbol | undefined { | ||||
if (!isExpressionNode(privId)) { | ||||
return undefined; | ||||
} | ||||
|
||||
const links = getNodeLinks(privId); | ||||
if (links.resolvedSymbol === undefined) { | ||||
links.resolvedSymbol = lookupSymbolForPrivateIdentifierDeclaration(privId.escapedText, privId); | ||||
} | ||||
return links.resolvedSymbol; | ||||
} | ||||
|
||||
function getPrivateIdentifierPropertyOfType(leftType: Type, lexicallyScopedIdentifier: Symbol): Symbol | undefined { | ||||
return getPropertyOfType(leftType, lexicallyScopedIdentifier.escapedName); | ||||
} | ||||
|
@@ -32050,11 +32105,29 @@ namespace ts { | |||
if (leftType === silentNeverType || rightType === silentNeverType) { | ||||
return silentNeverType; | ||||
sandersn marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
} | ||||
leftType = checkNonNullType(leftType, left); | ||||
if (isPrivateIdentifier(left)) { | ||||
if (languageVersion < ScriptTarget.ESNext) { | ||||
checkExternalEmitHelpers(left, ExternalEmitHelpers.ClassPrivateFieldIn); | ||||
} | ||||
// Unlike in 'checkPrivateIdentifierExpression' we now have access to the RHS type | ||||
// which provides us with the opportunity to emit more detailed errors | ||||
if (!getNodeLinks(left).resolvedSymbol && getContainingClass(left)) { | ||||
const isUncheckedJS = isUncheckedJSSuggestion(left, rightType.symbol, /*excludeClasses*/ true); | ||||
reportNonexistentProperty(left, rightType, isUncheckedJS); | ||||
} | ||||
sandersn marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
} | ||||
else { | ||||
leftType = checkNonNullType(leftType, left); | ||||
// TypeScript 1.0 spec (April 2014): 4.15.5 | ||||
// Require the left operand to be of type Any, the String primitive type, or the Number 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_a_private_identifier_or_of_type_any_string_number_or_symbol); | ||||
} | ||||
} | ||||
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, | ||||
|
@@ -32072,10 +32145,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) || | ||||
rightTypeConstraint && ( | ||||
|
@@ -33457,6 +33526,8 @@ namespace ts { | |||
switch (kind) { | ||||
case SyntaxKind.Identifier: | ||||
return checkIdentifier(node as Identifier, checkMode); | ||||
case SyntaxKind.PrivateIdentifier: | ||||
return checkPrivateIdentifierExpression(node as PrivateIdentifier); | ||||
case SyntaxKind.ThisKeyword: | ||||
return checkThisExpression(node); | ||||
case SyntaxKind.SuperKeyword: | ||||
|
@@ -40236,6 +40307,9 @@ namespace ts { | |||
} | ||||
return result; | ||||
} | ||||
else if (isPrivateIdentifier(name)) { | ||||
return getSymbolForPrivateIdentifierExpression(name); | ||||
} | ||||
else if (name.kind === SyntaxKind.PropertyAccessExpression || name.kind === SyntaxKind.QualifiedName) { | ||||
const links = getNodeLinks(name); | ||||
if (links.resolvedSymbol) { | ||||
|
@@ -41651,6 +41725,7 @@ namespace ts { | |||
case ExternalEmitHelpers.MakeTemplateObject: return "__makeTemplateObject"; | ||||
case ExternalEmitHelpers.ClassPrivateFieldGet: return "__classPrivateFieldGet"; | ||||
case ExternalEmitHelpers.ClassPrivateFieldSet: return "__classPrivateFieldSet"; | ||||
case ExternalEmitHelpers.ClassPrivateFieldIn: return "__classPrivateFieldIn"; | ||||
case ExternalEmitHelpers.CreateBinding: return "__createBinding"; | ||||
default: return Debug.fail("Unrecognized helper"); | ||||
} | ||||
|
Uh oh!
There was an error while loading. Please reload this page.