-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
ES private field check #44648
Conversation
add support for the 'private-fields-in-in' TC39 proposal Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
The TypeScript team hasn't accepted the linked issue #42574. If you can get it accepted, this PR will have a better chance of being reviewed. |
I'm looking into the CI failures. Tests were passing locally before I pushed. I'll start with a fresh checkout this time to make sure I don't miss something. |
tests/baselines/reference/privateNameInInExpressionTransform.js
Outdated
Show resolved
Hide resolved
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.
Mostly questions in the checker, plus a background question in emitHelpers.
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
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.
Please remove the lexical lookup code and open an issue+PR pair to discuss how it should work in general. I don't think it should be private-identifier specific.
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 need to do a more thorough review, but my primary concern is that I don't think we need to add a new node for this.
tests/cases/conformance/classes/members/privateNames/privateNameInInExpression.ts
Show resolved
Hide resolved
tests/cases/conformance/classes/members/privateNames/privateNameInInExpressionTransform.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
} | ||
} | ||
if (!symbolResolved) { | ||
error(privId, Diagnostics.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?
src/compiler/checker.ts
Outdated
checkExternalEmitHelpers(left, ExternalEmitHelpers.ClassPrivateFieldIn); | ||
} | ||
// Unlike in 'checkPrivateIdentifierExpression' we now have access to the RHS type | ||
// which provides us with the oppotunity to emit more detailed errors |
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.
// which provides us with the oppotunity to emit more detailed errors | |
// which provides us with the opportunity to emit more detailed errors |
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.
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.
src/compiler/checker.ts
Outdated
if (leftType === silentNeverType || rightType === silentNeverType) { | ||
return silentNeverType; | ||
} |
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.
If we have a silentNeverType
, we're generally in an inference path and bail out of checking. Instead of splitting up the function body around privIdOnLeft
, should we just move this to the top of the function?
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 been meaning to look into the difference between never and silentNever, thanks!
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 moved this to the top now. I hadn't done that because I thought we wouldn't get the checkExternalEmitHelpers
error when the RHS was never
but I checked and as you suggested - this is still OK as the silentNeverType
has a different use case.
|
||
const privateId = expr.left; | ||
Debug.assertNode(privateId, isPrivateIdentifier); | ||
const symbol = lookupSymbolForPrivateIdentifierDeclaration(privateId.escapedText, privateId); |
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 by isExpressionNode
:
TypeScript/src/compiler/utilities.ts
Line 1904 in 40bd336
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.
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
tslib PR opened microsoft/tslib#157 |
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
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.
A few more requests for the parser and checker.
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
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.
The new changes look good to me. I'll sign off once the new message baselines are updated and then we'll be ready to merge once @rbuckton signs off.
Signed-off-by: Ashley Claymore <acutmore@users.noreply.github.com>
@rbuckton is out for a family emergency so I checked that his comments were resolved. I'm going to merge this now and look at the tslib change next. |
Ergonomic brand checks for Private Fields
This PR implements the TC39 Stage-4 Ergonomic brand checks for Private Fields proposal
Fixes #42574
Remaining Work
tslib
Add new private class element helper: __classPrivateFieldIn tslib#157Type Checking
Checking the presence of a private field provides a strong type narrowing hint.
Unsoundness
Whilst the runtime check is accurate, this static check is not fool-proof. The super constructor return pattern can add a private field to an object that is not an instance of the class (different prototype). This PR opts to narrow the type without checking for this case under the assumption that this pattern is uncommon and discoverable by looking at the constructor of the super class.
Example
Downlevel
Builds on top of the existing downlevel support for ES private class elements. If the private field is an instance field then the coresponding WeakMap is consulted. Private methods and accessors consult the WeakSet. Private static fields/methods/accessors do a direct equality check with the class constructor.
Example transform
TypeScript
JavaScript - ES2020 emit
References
Credits
This PR includes contributions from the following Bloomberg engineers: