(WIP) Private name class field transformation #8

joeywatts · 2018-08-28T18:13:19Z

Description of Changes

Transforms class properties with private names into WeakMap declarations
Transforms private name accesses into calls to a helper function which accesses the appropriate emitted WeakMap.
Transforms private name mutations (except unary operators) into calls to a helper function which accesses the appropriate emitted WeakMap.

Outside the Scope of this PR

Steps to Test

Build the compiler (npm run build:compiler)
Run tests (npm test).
Check compiler output of any example programs using ES private properties.

and check that private names not used in parameters Signed-off-by: Max Heiber <[email protected]>

Parse Private Names

Signed-off-by: Joseph Watts <[email protected]>

joeywatts · 2018-08-28T18:17:45Z

src/compiler/transformers/es2015.ts

@@ -3382,12 +3385,17 @@ namespace ts {
                addRange(statements, funcStatements, classBodyEnd + 1);
            }

+            // Add other class statements (such as the WeakMap declarations output by the 'esnext'
+            // transformer for private names).
+            addRange(statements, classStatements, /*start*/ 1);


Could use some guidance here. This was to avoid the WeakMaps being placed after the return statement for the class. How were end of declaration markers ending up in classStatements? classStatements is filtered by isVariableStatementWithInitializer. It seems that end of declaration markers are synthetic nodes, wouldn't they not be variable statements at all?

Not sure I follow, but would be happy to discuss

joeywatts · 2018-08-28T18:22:04Z

src/compiler/transformers/esnext.ts

+            if (nameString in environment) {
+                return environment[nameString].weakMap;
+            }
+            throw new Error("Accessing undeclared private name.");


I should probably not throw here? This should be caught by a parse error, but what should the transformer do? It seems like the approach that's usually taken is that the transformer should just keep the original node if something like this occurs, in which case, I should change this to return undefined and have callers bail on the transformation.

Given that you said "This should be caught by a parse error":

I think what you want is an assertion, Debug.assertNever, Debug.assert, etc. are used elsewhere in the transformer

What case should be caught by the parser? If you give an example, I should be able to figure out what the parser does

Thanks! I'll check out the usage of Debug.assert*

I just mean that if a private name is accessed in a class, but it is not declared, the parser will output something like "Private name not declared (or whatever the actual wording is)."

class X { something() { this.#myProperty = 1; } // "#myProperty" is not declared. }

I've resolved this to emit unchanged output for undeclared private names (which is similar to the behavior of other syntax errors).

Neuroboy23 · 2018-09-07T20:49:12Z

src/compiler/transformers/ts.ts

                setSourceMapRange(statement, moveRangePastModifiers(property));
                setCommentRange(statement, property);
-                setOriginalNode(statement, property);


This looks like a mistake.

Neuroboy23 · 2018-09-07T21:01:05Z

src/compiler/transformers/ts.ts

+                if (isPrivateProperty(property)) {
+                    continue;
+                }
+                const statement = createStatement(transformInitializedProperty(property, receiver));


By bailing out early here, it changes the order in which the property initializers are evaluated. We need to preserve a strict top-down evaluation order. Probably the right way to do this is to add the statements for private fields into the same statements array being added to below, preserving order.

Input:

let a = 0; class C { foo = ++a; #x = ++a; }

Output:

let a = 0; class C { constructor() { _x.set(this, ++a); // BAD: evaluation order is swapped this.foo = ++a; // BAD: evaluation order is swapped } } var _x = new WeakMap;

This is somewhat complicated because the TypeScript transformer will transform class property initializers to constructor statements. If the target output is ESNext, then we need to keep the private named property declaration in the class to keep valid ESNext syntax.

After further thought, I now think that we should move this behavior for ALL properties (not just private names) to the ESNext transformer, since you can now have property initializers in ESNext. (I'm also hoping that the initializer semantics are the same in the ESNext proposal as they are in the TypeScript class implementation). I'd like to get the TypeScript team's thoughts on this idea.

I've now fixed this issue with private-named property initializers. It now relies on the TS transformer moving the initializers to the constructor (in the original order) and initializing the WeakMap values to undefined at the top of the constructor (like _x.set(this, void 0);).

mheiber · 2018-09-07T21:49:41Z

Could you add some tests? Happy to talking testing strategy

mheiber · 2018-09-07T22:36:07Z

src/compiler/transformers/ts.ts

@@ -1194,6 +1194,11 @@ namespace ts {
            );
        }

+        function isPrivateProperty(member: ClassElement): member is PropertyDeclaration {


The return type of the function doesn't match what it does: the body checks not just that the member is a property declaration, but that the members' name is a private name. So which is right, the return value or the behavior?

Also, the name could be confusing because private properties are also private properties: maybe name it something like "privateNamedProperty" would be better?

This can be more correctly expressed as member is PropertyDeclaration & { name: PrivateName }. Maybe there should be a PrivateNamedPropertyDeclaration:

interface PrivateNamedPropertyDeclaration extends PropertyDeclaration { name: PrivateName; }

mheiber · 2018-09-07T22:40:25Z

src/compiler/transformers/esnext.ts

+                if (isCompoundAssignment(node.operatorToken.kind)) {
+                    let setReceiver: Expression;
+                    let getReceiver: Identifier;
+                    if (!isIdentifier(node.left.expression) && !isKeyword(node.left.expression.kind)) {


what happens in the case where the first half of the conjunction is true but the second is false?
In that case node.left.expression would not be an identifier, but you are casting to identifier on line 485. How could we correctly handle this case?

Ah, good catch. I'll see if I can get these types right. For context, this is checking whether the receiver (node.left.expression) is an identifier or a keyword (like this). If it's neither, it treats it as a non-trivial expression which could result in modification of the receiver, so we need to create a temporary variable to store the result of this expression.

Signed-off-by: Joseph Watts <[email protected]>

joeywatts · 2018-09-11T19:32:06Z

Added a bunch of test cases and addressed some bugs that I've found. Let me know if there are suggestions for more tests or any perceived issues!

Signed-off-by: Joseph Watts <[email protected]>

mheiber · 2018-09-19T16:17:43Z

tests/baselines/reference/privateNameFieldAccess.js

+    };
+    return Test;
+}());
+var _field = new WeakMap;


minor: why is the declaration of _field below the class definition?

mheiber · 2018-09-19T16:18:09Z

tests/baselines/reference/privateNameFieldAccess.js

+        console.log(this.#field);
+    }
+}
+


is there a way to test the es2015 emit? I'd like to see what this looks like with a class class

//@target: ES2015

mheiber · 2018-10-01T20:27:04Z

src/compiler/transformers/esnext.ts

+            if (isPrivateName(node.name)) {
+                const weakMapName = accessPrivateName(node.name);
+                if (!weakMapName) {
+                    return node;


if the checker doesn't catch undeclared private names do they just end up in the output unchanged?

The checker should catch them and output errors, but the checker won't have any impact on the transformer. Even if the checker errors, the transformer needs to output some code. If a private name is used and there's no declaration, we just leave it in the output unchanged.

mheiber · 2018-10-01T20:29:11Z

src/compiler/transformers/esnext.ts

+
+        function endPrivateNameEnvironment(): PrivateNameEnvironment {
+            const privateNameEnvironment = currentPrivateNameEnvironment();
+            // Destroy private name environment.


nothing uses the return value. Do we need it?

mheiber · 2018-10-01T21:20:46Z

src/compiler/transformers/esnext.ts

+        function endPrivateNameEnvironment(): PrivateNameEnvironment {
+            const privateNameEnvironment = currentPrivateNameEnvironment();
+            // Destroy private name environment.
+            delete privateNameEnvironmentStack[privateNameEnvironmentIndex--];


I had to look up what delete array[index] does. It makes the value of the array at index undefined. Is that what we want?

If the privateNameEnvironmentStack is truly a stack, then we could use pop() to remove and last() (from core.ts) to get the current environment. We wouldn't need to keep track of an index.

If the privateNameEnvironmentStack is not actually a stack, then we can still avoid having undefineds in our array with splice

If we do want undefineds in our array, I'd prefer the more explicit arr[index] = undefined

mheiber · 2018-10-01T21:54:47Z

src/compiler/utilities.ts

@@ -3738,6 +3738,28 @@ namespace ts {
            && isLeftHandSideExpression(node.left);
    }

+    export function isCompoundAssignment(kind: BinaryOperator): kind is CompoundAssignmentOperator {


note for other reviewers: these two functions were moved from generators.ts

mheiber · 2018-10-09T01:03:30Z

src/compiler/transformers/esnext.ts

+            });
+            const ctor = find(
+                members,
+                (member) => isConstructorDeclaration(member) && !!member.body


I wasn't able to come up with valid TS where there's a constructor without a body: what is the !!body check for?

This actually shouldn’t be relevant in the ESNext transformer. Constructor without a body is valid syntax with overload signatures but they should be elided by the time the code gets here

mheiber · 2018-10-13T17:26:57Z

src/compiler/transformers/esnext.ts

@@ -26,6 +26,19 @@ namespace ts {
        let enclosingFunctionFlags: FunctionFlags;
        let enclosingSuperContainerFlags: NodeCheckFlags = 0;

+


Easier said than done, but I think it would fit the style of the codebase (and also be safer and easier to manage) if we use the stack itself instead of implementing a separate privateNameEnvironmentStack.

This is done in ts.ts using saveStateAndInvoke but the general pattern is like this (I think):

let privateNameEnvironment = undefined; function visitClassDeclaration(...) { const savedPrivateNameEnvironment = privateNameEnvironment privateNameEnvironment = {} visitEachChild .... doStuff() privateNameEnvrironment = savedPrivateNameEnvironment }

Written mostly to check my own understanding. I'm experimenting with this on a branch.

PR for the change: joeywatts#6

doh! Re your follow-up: silly of me to forget that we have to keep an environment stack around to deal with lexical scoping

mheiber · 2018-10-13T19:54:01Z

src/compiler/transformers/esnext.ts

+
+    function createClassPrivateFieldGetHelper(context: TransformationContext, receiver: Expression, privateField: Identifier) {
+        context.requestEmitHelper(classPrivateFieldGetHelper);
+        return createCall(getHelperName("_classPrivateFieldGet"), /* typeArguments */ undefined, [ receiver, privateField ]);


privateField here is an AST Node (Identifier) that will be reused in multiple parts of the AST. Is that safe? It's conceivable that a later stage in the transform pipeline might modify one branch of the AST and not expect another branch to change as a result.

The transformers that are visiting the AST return new nodes (they don't mutate the existing AST as I understand it). I believe it should be fine to use the Identifier in two places, but let me check to see if there's a place in existing code where that is done. I know there are setTextRange and setCommentRange functions that actually can modify AST nodes...

I think the way you did it is how it's done elsewhere, too, but I'm curious about how it works.

Neuroboy23

@joeywatts, did you intentionally deal with statics in these commits, or is it just natural fallout that initializers of static private-named properties are handled? This transformation doesn't strip the type annotation from static properties when targeting esnext. If you had to deal with statics explicitly, is there opportunity to split that into a separate PR? If no to either of those things, please amend this PR to strip away the type annotations.

Input:

class C {
    static #name: any = 5;
}

Output (--target esnext):

class C {
    static #name: any;
}
C.#name = 5;

Also, is this related? When I use the same input with --target es2015, I get this:

C:\dev\TypeScript\built\local\tsc.js:83978
                throw e;
                ^

TypeError: Cannot use 'in' operator to search for '#name' in undefined
    at accessPrivateName (C:\dev\TypeScript\built\local\tsc.js:66538:28)
    at visitBinaryExpression (C:\dev\TypeScript\built\local\tsc.js:66745:35)
    at visitorWorker (C:\dev\TypeScript\built\local\tsc.js:66478:28)
    at visitorNoDestructuringValue (C:\dev\TypeScript\built\local\tsc.js:66454:20)
    at visitNode (C:\dev\TypeScript\built\local\tsc.js:60939:23)
    at Object.visitEachChild (C:\dev\TypeScript\built\local\tsc.js:61213:59)
    at visitExpressionStatement (C:\dev\TypeScript\built\local\tsc.js:66719:23)
    at visitorWorker (C:\dev\TypeScript\built\local\tsc.js:66504:28)
    at visitor (C:\dev\TypeScript\built\local\tsc.js:66451:20)
    at visitNodes (C:\dev\TypeScript\built\local\tsc.js:60990:48)

kpfleming and others added 19 commits August 21, 2018 13:45

Update Travis-CI configuration to build es-private-fields branch

eac1bf8

Parse Private Names

548c9f8

and check that private names not used in parameters Signed-off-by: Max Heiber <[email protected]>

Merge pull request #4 from mheiber/syntaxkind

0b8c723

Parse Private Names

Start ES2015 transformation

136e218

Signed-off-by: Joseph Watts <[email protected]>

Fix lint errors

d0a89b8

Signed-off-by: Joseph Watts <[email protected]>

Transform private name references. Generate private field initializers.

1e7326d

Signed-off-by: Joseph Watts <[email protected]>

Generate WeakMap instances for private fields. Fix field initialization.

4abc354

Signed-off-by: Joseph Watts <[email protected]>

Remove unused function.

f7fc9af

Signed-off-by: Joseph Watts <[email protected]>

Clean up private name initializer code

f65b7bb

Signed-off-by: Joseph Watts <[email protected]>

Clean up private field class transformation.

23a8d60

Signed-off-by: Joseph Watts <[email protected]>

Fix private name transformation clash with constructor overload list

7c63b54

Signed-off-by: Joseph Watts <[email protected]>

Remove unnecessary exporting of private name helper functions.

832f457

Signed-off-by: Joseph Watts <[email protected]>

Transform compound assignment expressions

fe4f98a

Signed-off-by: Joseph Watts <[email protected]>

Fix linter errors

8293248

Signed-off-by: Joseph Watts <[email protected]>

Use IIFE, transform initializers for classes with private names.

d6cc08c

Signed-off-by: Joseph Watts <[email protected]>

Fix class expression transformation, punt on IIFE class wrappers

0c22191

Signed-off-by: Joseph Watts <[email protected]>

Only transform classes when there are private names.

34be11c

Signed-off-by: Joseph Watts <[email protected]>

Fix weak map generation and class transformation

8cbf34f

Signed-off-by: Joseph Watts <[email protected]>

Fix build error and cast __String to string

4f1a883

Signed-off-by: Joseph Watts <[email protected]>

robpalme requested review from mheiber and Neuroboy23 August 28, 2018 18:22

robpalme assigned mheiber Aug 28, 2018

joeywatts commented Aug 28, 2018

View reviewed changes

Neuroboy23 requested changes Sep 7, 2018

View reviewed changes

mheiber reviewed Sep 7, 2018

View reviewed changes

Joseph Watts added 3 commits September 11, 2018 14:12

Fix out-of-order private named property initialization

d66e135

Signed-off-by: Joseph Watts <[email protected]>

Clean up private name assignment expression temp variable emit

fab8ff9

Signed-off-by: Joseph Watts <[email protected]>

Remove private named property declaration initializer in TS transform

4fb9082

Signed-off-by: Joseph Watts <[email protected]>

Emit unchanged output for undeclared private names

fec6ad8

Signed-off-by: Joseph Watts <[email protected]>

joeywatts force-pushed the private-name-transformation branch from c8090dc to fec6ad8 Compare September 11, 2018 18:12

Joseph Watts added 9 commits September 11, 2018 14:55

Update privateNameField baseline

9916d1c

Signed-off-by: Joseph Watts <[email protected]>

Accept public API changes

3b28ff7

Signed-off-by: Joseph Watts <[email protected]>

Create initialized private named field conformance test

1c4554a

Signed-off-by: Joseph Watts <[email protected]>

Create private named field accessor conformance test

8ba9238

Signed-off-by: Joseph Watts <[email protected]>

Create private named field mutation conformance test

c1d12f2

Signed-off-by: Joseph Watts <[email protected]>

Create private named field binary mutator conformance test

c8ee370

Signed-off-by: Joseph Watts <[email protected]>

Create private name non-trivial receiver mutation conformance test

ddc58f1

Signed-off-by: Joseph Watts <[email protected]>

Fix private name assignment referencing private names in the RHS expr

2824474

Signed-off-by: Joseph Watts <[email protected]>

Create private named field initialization order conformance test

e2ce860

Signed-off-by: Joseph Watts <[email protected]>

Revert unnecessary changes

a63b52d

Signed-off-by: Joseph Watts <[email protected]>

mheiber reviewed Sep 19, 2018

View reviewed changes

mheiber mentioned this pull request Sep 25, 2018

All Bloomberg Changes for Private Fields and Methods #6

Closed

mheiber reviewed Oct 1, 2018

View reviewed changes

mheiber changed the title ~~(WIP) Private name transformation~~ (WIP) Private name class field transformation Oct 4, 2018

mheiber reviewed Oct 9, 2018

View reviewed changes

mheiber reviewed Oct 13, 2018

View reviewed changes

mheiber force-pushed the es-private-fields branch from c493238 to 9767522 Compare October 15, 2018 21:20

Neuroboy23 requested changes Oct 22, 2018

View reviewed changes

joeywatts closed this Nov 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(WIP) Private name class field transformation #8

(WIP) Private name class field transformation #8

joeywatts commented Aug 28, 2018 •

edited

Loading

joeywatts Aug 28, 2018

mheiber Oct 9, 2018

joeywatts Aug 28, 2018

mheiber Sep 7, 2018

joeywatts Sep 10, 2018

joeywatts Sep 11, 2018

Neuroboy23 Sep 7, 2018

Neuroboy23 Sep 7, 2018 •

edited

Loading

Neuroboy23 Sep 7, 2018

joeywatts Sep 10, 2018

joeywatts Sep 11, 2018

mheiber commented Sep 7, 2018

mheiber Sep 7, 2018

joeywatts Sep 10, 2018

mheiber Sep 7, 2018

joeywatts Sep 10, 2018

joeywatts commented Sep 11, 2018

mheiber Sep 19, 2018

mheiber Sep 19, 2018

mheiber Oct 12, 2018 •

edited

Loading

mheiber Oct 1, 2018

joeywatts Oct 13, 2018

mheiber Oct 1, 2018

mheiber Oct 1, 2018 •

edited

Loading

mheiber Oct 1, 2018 •

edited

Loading

mheiber Oct 9, 2018

joeywatts Oct 9, 2018 •

edited

Loading

mheiber Oct 13, 2018 •

edited

Loading

mheiber Oct 13, 2018

mheiber Oct 14, 2018

mheiber Oct 13, 2018

joeywatts Oct 13, 2018

mheiber Oct 14, 2018

Neuroboy23 left a comment •

edited

Loading

		@@ -26,6 +26,19 @@ namespace ts {
		let enclosingFunctionFlags: FunctionFlags;
		let enclosingSuperContainerFlags: NodeCheckFlags = 0;

(WIP) Private name class field transformation #8

(WIP) Private name class field transformation #8

Conversation

joeywatts commented Aug 28, 2018 • edited Loading

Description of Changes

Outside the Scope of this PR

Steps to Test

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

Neuroboy23 Sep 7, 2018 • edited Loading

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

mheiber commented Sep 7, 2018

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

joeywatts commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheiber Oct 12, 2018 • edited Loading

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

mheiber Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

mheiber Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeywatts Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

mheiber Oct 13, 2018 • edited Loading

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

Neuroboy23 left a comment • edited Loading

Choose a reason for hiding this comment

joeywatts commented Aug 28, 2018 •

edited

Loading

Neuroboy23 Sep 7, 2018 •

edited

Loading

mheiber Oct 12, 2018 •

edited

Loading

mheiber Oct 1, 2018 •

edited

Loading

mheiber Oct 1, 2018 •

edited

Loading

joeywatts Oct 9, 2018 •

edited

Loading

mheiber Oct 13, 2018 •

edited

Loading

Neuroboy23 left a comment •

edited

Loading