es private fields in in #52

acutmore · 2021-02-23T15:19:10Z

*Issue number of the reported bug or feature request: *
microsoft#42574

Describe your changes
Implements the stage 3 'private-fields-in-in' proposal

https://github.com/tc39/proposal-private-fields-in-in

WIP Progress

src/compiler/checker.ts

dragomirtitian · 2021-03-03T13:15:18Z

src/compiler/parser.ts

+            const id = parsePrivateIdentifier();
+            if (token() !== SyntaxKind.InKeyword) {
+                // TODO(aclaymore) use better error
+                return createMissingNode(SyntaxKind.InKeyword, /*reportAtCurrentPosition*/ true, Diagnostics._0_expected, tokenToString(SyntaxKind.InKeyword));


Maybe we should think of common parsing mistakes like if(#prop.a in foo). This currently issues the error 'in' expected.. I would argue an error along the lines of: You might have meant this.#props.a would be better,

Looking into this there are existing errors and quickFixes for 'bare' privateFields but my branch was preventing them from running as I was consuming the field in the parser. I've changed the parser to lookahead and only match when the in keyword is present.

src/compiler/transformers/classFields.ts

tests/baselines/reference/privateNameInInExpression.errors.txt

dragomirtitian · 2021-03-03T13:35:20Z

tests/baselines/reference/privateNameInInExpression.errors.txt

+            if (#p1 in b) {
+                b; // good b is 'Bar & Foo'
+            } else {
+                b; // good b is Bar
+            }


I wonder if we shouldn't be more strict here. I mean for b instanceof Foo it makes sense since you have Symbol.species which can customize how instanceof works. But for #p1 in b there is not universe in which #p1 can be in b and it also be an instance of Bar (if Bar is a class). For other object types this still makes sense... so I'm rather torn if we should issue an error or not.

Interesting. I'm thinking not unless the rule is applied more generally, as in all productions of ClassA & ClassB would be never if at least one class had private fields, and the other was not a sub/superType.

Surprisingly there are cases where the runtime type here could be Bar and not Foo, though I wouldn't expect this to be common code:

class SuperReturn { constructor(v) { return v; } } class C extends SuperReturn { #priv; static isC(v) { return #priv in v } cMethod() {} } class Unrelated { random = 1; uMethod(); } const u = new C(new Unrelated)); if (C.isC(u)) { u.random; // 1 u.uMethod(); // fine u.cMethod(); // throws }

tests/baselines/reference/privateNameInInExpressionUnused.errors.txt

acutmore · 2021-03-09T19:38:31Z

Another thing I've noticed is that #priv in v is not coming up in 'find all references' searches. So I need to fix that too.
EDIT: done

Signed-off-by: Ashley Claymore <[email protected]>

acutmore · 2021-03-30T11:52:45Z

@dragomirtitian - I've rebased onto TypeScript post the private methods & static fields PR. And ready for another round of review when you have the time.

dragomirtitian · 2021-05-17T13:51:01Z

src/compiler/checker.ts

+                }
+                const classSymbol = symbol.parent!;
+                const classType = <InterfaceType>getTypeOfSymbol(classSymbol);
+                const firstDecl = symbol.declarations?.[0];


Wasn't there a valueDeclaration that is what is needed here ?

ah yes there is symbol.valueDeclaration - that looks much better. I'll change to that.

dragomirtitian · 2021-05-18T09:05:07Z

src/compiler/checker.ts

+                const firstDecl = symbol.declarations?.[0];
+                Debug.assert(firstDecl, "should always have a declaration");
+                let targetType: Type;
+                if (hasSyntacticModifier(firstDecl, ModifierFlags.Static)) {


use hasStaticModifier instead

dragomirtitian · 2021-05-18T09:08:17Z

src/compiler/checker.ts

+                    const ctorSigs = getSignaturesOfType(classType, SignatureKind.Construct);
+                    Debug.assert(ctorSigs.length > 0, "should always have a constructor");
+                    targetType = getReturnTypeOfSignature(ctorSigs[0]);


There seems to be a better way to get the instance type:

const classSymbol = getSymbolOfNode(classDecl); const classInstanceType = <InterfaceType>getDeclaredTypeOfSymbol(classSymbol);

At line (24161, fn: classDeclarationExtendsNull)

Not sure if 100% equivalent or if better, but please consider it.

done. Much better, thanks!

dragomirtitian · 2021-05-18T10:26:16Z

tests/baselines/reference/privateNameInInExpression.errors.txt

@@ -0,0 +1,143 @@
+tests/cases/conformance/classes/members/privateNames/privateNameInInExpression.ts(3,27): error TS2805: Static fields with private names can't have initializers when the '--useDefineForClassFields' flag is not specified with a '--target' of 'esnext'. Consider adding the '--useDefineForClassFields' flag.


Nit: Consider removing this error from the test by changing compiler settings. It is noise here IMO

dragomirtitian · 2021-05-18T10:31:25Z

tests/baselines/reference/privateNameInInExpression.errors.txt

+
+            const e = #field in (v as never);
+
+            for (let f in #field in v as any) { /**/ } // unlikely but valid


Took a second to get this was actually for (let f in (#field in v as any)) 😅.

haha! I hope no one ever does this but at least we support it if they try

mkubilayk

Tried it out in VSCode, this is looking good! There is one case I found a bit inconvenient but I'm not sure if we can improve it. When you are inside PrivateIdentifierInInExpression and editing the identifier:

if (# in this) {
     ^ cursor
}

available private identifiers are listed correctly in the suggestions (ctrl+space). But when you pick one if them, you get a property access (this.#foo). If we could use the contextual information to drive this and insert #foo instead, that would be great. If you think people might want this.#foo more often than #foo in this case, please ignore my suggestion.

mkubilayk · 2021-05-19T11:08:43Z

src/compiler/utilities.ts

@@ -3579,6 +3580,9 @@ namespace ts {
                        return getBinaryOperatorPrecedence(operatorKind);
                }

+            case SyntaxKind.PrivateIdentifierInInExpression:


nit: This can be combined with SyntaxKind.AsExpression case.

mkubilayk · 2021-05-19T11:11:27Z

src/compiler/visitorPublic.ts

@@ -793,6 +793,12 @@ namespace ts {
                    nodeVisitor(node.operatorToken, tokenVisitor, isBinaryOperatorToken),
                    nodeVisitor(node.right, visitor, isExpression));

+            case SyntaxKind.PrivateIdentifierInInExpression:
+                return factory.updatePrivateIdentifierInInExpression(<PrivateIdentifierInInExpression>node,
+                    nodeVisitor((<PrivateIdentifierInInExpression>node).name, visitor, isExpression),


Should this use isPrivateIdentifier check?

Suggested change

nodeVisitor((<PrivateIdentifierInInExpression>node).name, visitor, isExpression),

nodeVisitor((<PrivateIdentifierInInExpression>node).name, visitor, isPrivateIdentifier),

If my memory serves correctly: no. As I believe this check is also done on the transformed AST where PrivateIdentifierInInExpression becomes a methodCall.

This was over a month ago so I may be getting this mixed up with a different bit of code. I'll try it out again and confirm either way.

Played around to jog my memory. The reason this was isExpression and not isPrivateIdentifier was because when the identifier is invalid (e.g. used outside of a class) the classFields transformer emits an empty plain Identifier.

I've changed this to isMemberName now to match both private and standard identifiers.

src/compiler/transformers/classFields.ts

tests/baselines/reference/privateNameInInExpressionTransform.js

acutmore · 2021-05-19T14:53:10Z

Tried it out in VSCode, this is looking good! There is one case I found a bit inconvenient but I'm not sure if we can improve it. When you are inside PrivateIdentifierInInExpression and editing the identifier:
if (# in this) {
     ^ cursor
}
available private identifiers are listed correctly in the suggestions (ctrl+space). But when you pick one if them, you get a property access (this.#foo). If we could use the contextual information to drive this and insert #foo instead, that would be great. If you think people might want this.#foo more often than #foo in this case, please ignore my suggestion.

Yeah I was really unsure on this too. My assumption was that people are more likely to reach for accessing the private field but that's not based on any evidence just a gut feeling.

Maybe it's possible to offer both suggestions a plain #foo identifier when we're preceding an in and the this.#foo. I'll give it a go and see what the API supports.

- simplify obtaining private fields's static or instance type - make visitor node test more specific - clean up warnings about static class fields in tests Signed-off-by: Ashley Claymore <[email protected]>

acutmore · 2021-05-24T17:28:53Z

I spent a little bit of time seeing if I could change the completions private fields to sometimes complete to just the identifier but couldn't get anything working quickly. I'll have another go next time I have some time.
I believe I've addressed the other comments.

Signed-off-by: Ashley Claymore <[email protected]>

acutmore · 2021-06-09T18:52:01Z

Merged latest TypeScript main into this branch. All tests still passing.

mkubilayk

I think this looks good. Left one comment to prevent against a potential future bug.

We can use microsoft#42458 as an example for the PR description when we open the upstream PR.

mkubilayk · 2021-06-15T13:51:05Z

src/compiler/transformers/classFields.ts

+         * Visits `#id in expr`
+         */
+        function visitPrivateIdentifierInInExpression(node: PrivateIdentifierInInExpression) {
+            if (!shouldTransformPrivateElements) {


This is probably fine today but might be a problem when ES2022 is introduced. shouldTransformPrivateElements will probably be updated to say languageVersion < ScriptTarget.ES2022. But it's possible that proposal-private-fields-in-in won't make it to ES2022.

The current plan is to assume https://github.com/tc39/proposal-private-fields-in-in will make stage 4 in time for ES2022. There are positive signals of this, as it is already available in latest Chrome and Firefox.

* es private fields in in (bloomberg#52) add support for the 'private-fields-in-in' TC39 proposal Signed-off-by: Ashley Claymore <[email protected]> * [fixup] include inToken when walking forEachChild(node, cb) * [squash] re-accept lib definition baseline changes * [squash] reduce if/else to ternary Signed-off-by: Ashley Claymore <[email protected]> * [squash] drop 'originalName' and rename parameter instead Signed-off-by: Ashley Claymore <[email protected]> * [squash] extend spelling suggestion to all privateIdentifiers Signed-off-by: Ashley Claymore <[email protected]> * [squash] revert the added lexical spelling suggestions logic Signed-off-by: Ashley Claymore <[email protected]> * [squash] update baseline Signed-off-by: Ashley Claymore <[email protected]> * [squash] inline variable as per PR suggestion Signed-off-by: Ashley Claymore <[email protected]> * [squash] test targets both esnext and es2020 as per PR comment Signed-off-by: Ashley Claymore <[email protected]> * switch to using a binary expression Signed-off-by: Ashley Claymore <[email protected]> * [squash] PrivateIdentifier now extends PrimaryExpression Signed-off-by: Ashley Claymore <[email protected]> * [squash] accept public api baseline changes Signed-off-by: Ashley Claymore <[email protected]> * [squash] classPrivateFieldInHelper now has documentation Signed-off-by: Ashley Claymore <[email protected]> * [squash] type-check now follows existing in-expression path Signed-off-by: Ashley Claymore <[email protected]> * [squash] parser now follows existing binaryExpression path Signed-off-by: Ashley Claymore <[email protected]> * [squash] correct typo in comment Signed-off-by: Ashley Claymore <[email protected]> * [squash] no longer use esNext flag Signed-off-by: Ashley Claymore <[email protected]> * [squash] swap 'reciever, state' helper params Signed-off-by: Ashley Claymore <[email protected]> * [squash] remove change to parenthesizerRules Signed-off-by: Ashley Claymore <[email protected]> * [squash] apply suggested changes to checker Signed-off-by: Ashley Claymore <[email protected]> * [squash] remove need for assertion in fixSpelling Signed-off-by: Ashley Claymore <[email protected]> * [squash] improve comment hint in test Signed-off-by: Ashley Claymore <[email protected]> * [squash] fix comment typos Signed-off-by: Ashley Claymore <[email protected]> * [squash] add flow-test for Foo | FooSub | Bar Signed-off-by: Ashley Claymore <[email protected]> * [squash] add checkExternalEmitHelpers call and new test case Signed-off-by: Ashley Claymore <[email protected]> * [squash] simplify and correct parser Signed-off-by: Ashley Claymore <[email protected]> * [squash] move most of the added checker logic to expression level Signed-off-by: Ashley Claymore <[email protected]> * [squash] always error when privateId could not be resolved Signed-off-by: Ashley Claymore <[email protected]> * [squash] reword comment Signed-off-by: Ashley Claymore <[email protected]> * [squash] fix codeFixSpelling test Signed-off-by: Ashley Claymore <[email protected]> * [squash] do less work Signed-off-by: Ashley Claymore <[email protected]> * store symbol by priateId not binaryExpression Signed-off-by: Ashley Claymore <[email protected]> * moved parsePrivateIdentifier into parsePrimaryExpression Signed-off-by: Ashley Claymore <[email protected]> * [squash] checkInExpressionn bails out early on silentNeverType Signed-off-by: Ashley Claymore <[email protected]> * [squash] more detailed error messages Signed-off-by: Ashley Claymore <[email protected]> * [squash] resolves conflict in diagnosticMessages.json Signed-off-by: Ashley Claymore <[email protected]> * [squash] update baseline for importHelpersES6 Signed-off-by: Ashley Claymore <[email protected]> * [squash] remove redundent if and comment from parser Signed-off-by: Ashley Claymore <[email protected]> * [squash] split up grammar/check/symbolLookup Signed-off-by: Ashley Claymore <[email protected]> * [squash] reword message for existing left side of in-expression error Signed-off-by: Ashley Claymore <[email protected]>

acutmore force-pushed the es-private-fields-in-in branch from 0cf6e7d to 30f4d03 Compare February 23, 2021 15:20

acutmore requested a review from dragomirtitian February 23, 2021 15:21

acutmore marked this pull request as draft February 23, 2021 15:21

acutmore mentioned this pull request Feb 23, 2021

Proposal private-fields-in-in is stage-3 microsoft/TypeScript#42574

Closed

5 tasks

acutmore force-pushed the es-private-fields-in-in branch from e76ef1b to d603860 Compare February 25, 2021 18:35

dragomirtitian reviewed Mar 3, 2021

View reviewed changes

orta mentioned this pull request Mar 14, 2021

tsc generate an invalid output for js files that contains private fields when use --allowJs microsoft/TypeScript#43234

Closed

acutmore added 20 commits March 29, 2021 11:23

basic parsing of private-id-in-in

306397b

Signed-off-by: Ashley Claymore <[email protected]>

basic type check and narrow

df7e97d

Signed-off-by: Ashley Claymore <[email protected]>

narrow to instanceOf klass not typeof klass

219899f

Signed-off-by: Ashley Claymore <[email protected]>

tweaking tests

fa3a4c7

Signed-off-by: Ashley Claymore <[email protected]>

fix emit with type syntax on rhs

7df6a99

Signed-off-by: Ashley Claymore <[email protected]>

add test case for subclass union narrow

df5a86f

Signed-off-by: Ashley Claymore <[email protected]>

es2020 emit

3560223

Signed-off-by: Ashley Claymore <[email protected]>

fix linting errors

17c95e6

Signed-off-by: Ashley Claymore <[email protected]>

mark as used

ec072dc

Signed-off-by: Ashley Claymore <[email protected]>

improve error message when inside a class

45e1b48

Signed-off-by: Ashley Claymore <[email protected]>

respect whitespace and comments in emit

9127d55

Signed-off-by: Ashley Claymore <[email protected]>

dont parse single privateIdentifier to reenable quick fix

4399cf5

Signed-off-by: Ashley Claymore <[email protected]>

remove need for lookupClassForPrivateIdentifierDeclaration

0462595

Signed-off-by: Ashley Claymore <[email protected]>

add tests for never

23c8f98

Signed-off-by: Ashley Claymore <[email protected]>

give child class different structure from parent

43a7d9f

Signed-off-by: Ashley Claymore <[email protected]>

cleanup testing for unused field

170e61b

Signed-off-by: Ashley Claymore <[email protected]>

use isTypeDerivedFrom

7e33365

Signed-off-by: Ashley Claymore <[email protected]>

add TODO for static private field emit

f2efbcf

Signed-off-by: Ashley Claymore <[email protected]>

include in find-all-references results

598215b

Signed-off-by: Ashley Claymore <[email protected]>

emit as helper with runtime type check

f7ff235

Signed-off-by: Ashley Claymore <[email protected]>

acutmore force-pushed the es-private-fields-in-in branch from f0f169e to 42b35c1 Compare March 29, 2021 14:19

correct issues caused by rebase

f3d1e52

Signed-off-by: Ashley Claymore <[email protected]>

acutmore added 5 commits March 29, 2021 17:25

update emit for static fields and methods

dbd7113

Signed-off-by: Ashley Claymore <[email protected]>

narrow to class for static field

91cfecb

Signed-off-by: Ashley Claymore <[email protected]>

tidy up TODOs

5eead91

Signed-off-by: Ashley Claymore <[email protected]>

add quickFix

85a367b

Signed-off-by: Ashley Claymore <[email protected]>

update typescript.d.ts baseline

02f59a3

Signed-off-by: Ashley Claymore <[email protected]>

acutmore force-pushed the es-private-fields-in-in branch from 42b35c1 to 02f59a3 Compare March 30, 2021 11:49

acutmore marked this pull request as ready for review March 30, 2021 11:49

acutmore requested a review from dragomirtitian March 30, 2021 11:56

dragomirtitian requested changes May 18, 2021

View reviewed changes

acutmore requested a review from mkubilayk May 18, 2021 11:16

mkubilayk reviewed May 19, 2021

View reviewed changes

acutmore changed the title ~~es private fields in in [wip]~~ es private fields in in May 24, 2021

post PR review improvements:

8928039

- simplify obtaining private fields's static or instance type - make visitor node test more specific - clean up warnings about static class fields in tests Signed-off-by: Ashley Claymore <[email protected]>

acutmore force-pushed the es-private-fields-in-in branch from f0bbed9 to 8928039 Compare May 24, 2021 15:03

acutmore requested review from dragomirtitian and mkubilayk May 24, 2021 17:29

acutmore added 3 commits June 9, 2021 19:16

Merge branch 'main' into es-private-fields-in-in

211b39e

fix linting

a846027

Signed-off-by: Ashley Claymore <[email protected]>

update .d.ts baselines

d2bddf3

Signed-off-by: Ashley Claymore <[email protected]>

mkubilayk approved these changes Jun 15, 2021

View reviewed changes

dragomirtitian approved these changes Jun 17, 2021

View reviewed changes

acutmore merged commit f04f22c into bloomberg:private-fields-in-in Jun 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

es private fields in in #52

es private fields in in #52

acutmore commented Feb 23, 2021 •

edited

Loading

dragomirtitian Mar 3, 2021

acutmore Mar 9, 2021

dragomirtitian Mar 3, 2021

acutmore Mar 9, 2021

acutmore commented Mar 9, 2021 •

edited

Loading

acutmore commented Mar 30, 2021

dragomirtitian May 17, 2021

acutmore May 18, 2021

acutmore May 24, 2021

dragomirtitian May 18, 2021

acutmore May 24, 2021

dragomirtitian May 18, 2021

acutmore May 24, 2021

dragomirtitian May 18, 2021

acutmore May 24, 2021

dragomirtitian May 18, 2021

acutmore May 18, 2021

mkubilayk left a comment

mkubilayk May 19, 2021

acutmore May 24, 2021

mkubilayk May 19, 2021

acutmore May 19, 2021

acutmore May 24, 2021

acutmore commented May 19, 2021

acutmore commented May 24, 2021

acutmore commented Jun 9, 2021

mkubilayk left a comment

mkubilayk Jun 15, 2021

acutmore Jun 17, 2021

		@@ -0,0 +1,143 @@
		tests/cases/conformance/classes/members/privateNames/privateNameInInExpression.ts(3,27): error TS2805: Static fields with private names can't have initializers when the '--useDefineForClassFields' flag is not specified with a '--target' of 'esnext'. Consider adding the '--useDefineForClassFields' flag.


		const e = #field in (v as never);

		for (let f in #field in v as any) { /**/ } // unlikely but valid

	nodeVisitor((<PrivateIdentifierInInExpression>node).name, visitor, isExpression),
	nodeVisitor((<PrivateIdentifierInInExpression>node).name, visitor, isPrivateIdentifier),

es private fields in in #52

es private fields in in #52

Conversation

acutmore commented Feb 23, 2021 • edited Loading

WIP Progress

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acutmore commented Mar 9, 2021 • edited Loading

acutmore commented Mar 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkubilayk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acutmore commented May 19, 2021

acutmore commented May 24, 2021

acutmore commented Jun 9, 2021

mkubilayk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acutmore commented Feb 23, 2021 •

edited

Loading

acutmore commented Mar 9, 2021 •

edited

Loading