Added support for static private fields accessors and methods. #50

dragomirtitian · 2021-02-16T14:03:43Z

Draft PR for private static fields methods and accessors.

…rs. There is no spec to go with this behavior as of yet.

…initialization in esnext. Fixed issue where nested privates produce incorrect brand check.

…d-accessors' into es-private-static-fields-methods-and-accessors

…c-fields-methods-and-accessors

joeywatts

Couple of initial thoughts. Will try to look over again

tests/baselines/reference/privateNameStaticAccessors.js

src/compiler/transformers/classFields.ts

mkubilayk

I think @joeywatts also mentioned this in one of his comments but the following example doesn't work correctly right now:

class C {
    static #x = 42;
    static #y = this.#z();
    static #z() {
        return this.#x;
    }

    static m() {
        return C.#y;
    }
}

console.log(C.m()); // should log 42

We should fix the checker (produces TS2334) and the emit (should probably replace this in the field initializers with the class constructor?).

src/compiler/transformers/classFields.ts

…seDefineForClassFields is not specified and target is esnext.

…h `target:esnext`

src/compiler/transformers/classFields.ts

…efore it was declared.

mkubilayk

I'm halfway through the fixture changes but reviewed everything else. Leaving a few comments but overall it looks good.

src/compiler/checker.ts

mkubilayk · 2021-02-25T11:14:03Z

src/compiler/checker.ts

@@ -32296,6 +32309,9 @@ namespace ts {
                    getNodeLinks(lexicalScope).flags |= NodeCheckFlags.ContainsClassWithPrivateIdentifiers;
                }
            }
+            if (isPrivateIdentifier(node.name) && hasStaticModifier(node) && node.initializer && languageVersion === ScriptTarget.ESNext && !compilerOptions.useDefineForClassFields) {


This line will need to be updated to use getUseDefineForClassFields() when microsoft#42663 gets in.

Yup. After we get that in we should do a merge and do a sweep for useDefineForClassFields again. I'll leave this open so we don't forget

src/compiler/transformers/classFields.ts

tests/baselines/reference/privateNameAndAny.js

mkubilayk · 2021-02-25T12:17:29Z

tests/baselines/reference/privateNameDuplicateField.errors.txt

-tests/cases/conformance/classes/members/privateNames/privateNameDuplicateField.ts(47,9): error TS2300: Duplicate identifier '#foo'.
-tests/cases/conformance/classes/members/privateNames/privateNameDuplicateField.ts(48,9): error TS2300: Duplicate identifier '#foo'.
-tests/cases/conformance/classes/members/privateNames/privateNameDuplicateField.ts(49,9): error TS2300: Duplicate identifier '#foo'.
+tests/cases/conformance/classes/members/privateNames/privateNameDuplicateField.ts(6,9): error TS2300: Duplicate identifier '#foo'.


Haha what happened here? I this because we replaced the early return with continue in the checker?

Nope. Since duplication detection was so unreliable (combinations of type and order matter) I decided to add all possible combinations between instance and static private class elements.

mkubilayk · 2021-02-25T12:26:46Z

tests/baselines/reference/privateNameES5Ban(target=es3).errors.txt

-        ~~~~~~
-!!! error TS18019: 'static' modifier cannot be used with a private identifier.
+               ~~~~~~~
+!!! error TS18028: Private identifiers are only available when targeting ECMAScript 2015 and higher.


I guess technically private static emit can be implemented in earlier versions because it doesn't use WeakSet or WeakMap but we wan't to have a consistent minimum target?

Yes, you are right. I think it would cause more confusion than benefit.

mkubilayk

Looks good! One small issue in a test.

mkubilayk · 2021-02-26T09:34:31Z

tests/baselines/reference/privateNameStaticFieldUnaryMutation.js

+        __classStaticPrivateFieldSet(_a = C, C, _C_test, +__classStaticPrivateFieldGet(_a, C, _C_test) + 1);
+        __classStaticPrivateFieldSet(_b = C, C, _C_test, +__classStaticPrivateFieldGet(_b, C, _C_test) - 1);
+        __classStaticPrivateFieldSet(_c = C, C, _C_test, +__classStaticPrivateFieldGet(_c, C, _C_test) + 1);


This is not specific to private statics but it's a bit weird that C.#test++ and ++C.#test evaluates to the same thing as standalone expressions. They are correctly handled if the expression is used, though.

mkubilayk · 2021-02-26T09:53:55Z

tests/cases/conformance/classes/members/privateNames/privateNameDuplicateField.ts

+    // Error
+    class A_StaticGetter_StaticField {
+        static get #foo() { return ""}
+        static #foo() { }


Suggested change

static #foo() { }

static #foo = "foo";

* Update editorServices.ts * Fix baselines Co-authored-by: Orta <[email protected]>

* Fixed three typo errors * Reverted * Reverted * Fixed typo

@filename

* Initial implementation+first big transitions * about 10 more * Change baseline filename + more baselines 1. Use containing file name instead of first @filename. 2. Switch the rest of the tests I need for @link over to baselines. * fix lint * Remove unused/incorrectly named fourslash baselines * fix incorrectly updated baselines * dedupe non-unique filenames * Add names to marker baselines Also rename another duped test filename. * Fix semicolon lint

…rver (microsoft#42542) * Move fixed chunk size polling as a watch option and move it out of server Fixes microsoft#41549 * Feedback

…soft#43052)

…c-fields-methods-and-accessors

…5877) * fix receiver of imported and exported functions fixes: microsoft#35420 * Rebase against master and clean up substitution flow * Add evaluator tests * Fix evaluator tests Co-authored-by: Ron Buckton <[email protected]>

…ields Filter transient flags to fix useDefineForClassFields

Fixes: microsoft#35476

* Do a shallow clone for docker tests. * Remove the 'pull' step from the Docker files.

* Update the DOM: March 03, 2021 Mostly Typescript-DOM-lib-generator#915 * update baselines

…riables (microsoft#43083)

… JavaScript files (microsoft#42392)

…ains a string index signature to 'any' (microsoft#43065) * Added test. * Accepted baselines. * Allow other index signatures to 'any' if there is a string index signature to 'any'. * Accepted baselines.

…soft#42747) * fix combined type mapper in getConditionalType * Add regression tests

…microsoft#42952)

* feat(lib): improve parseInt type definition and docstring * Accepted baselines * update tests

…natures (microsoft#43086) * Remove undefined from optional properties when inferring to index signatures * Add tests

…osoft#42472) * Exclude old number/enum literal compatibility rule from comparable relation * Add tests * Accept new baselines

…element.

…static-fields-methods-and-accessors

dragomirtitian requested review from joeywatts and mkubilayk February 16, 2021 14:03

dragomirtitian force-pushed the es-private-static-fields-methods-and-accessors branch 2 times, most recently from 61ead3a to d65136f Compare February 16, 2021 16:26

dragomirtitian added 7 commits February 18, 2021 16:15

Added support for static private fields accessors and methods.

5ebb3d6

Added error message for private identifiers used with static decorato…

7006c09

…rs. There is no spec to go with this behavior as of yet.

Fixed emit static bug that used private names outside of classes for …

b876613

…initialization in esnext. Fixed issue where nested privates produce incorrect brand check.

Added tests for private static fields methods and accessors.

31a8c00

Merge remote-tracking branch 'remotes/bloomberg/es-private-methods-an…

5a27a32

…d-accessors' into es-private-static-fields-methods-and-accessors

Merge branch 'es-private-methods-and-accessors' into es-private-stati…

d2ab6fc

…c-fields-methods-and-accessors

Accept new baseline.

23f6c67

dragomirtitian force-pushed the es-private-static-fields-methods-and-accessors branch from 427cce4 to 23f6c67 Compare February 19, 2021 08:03

Improved duplicate identifier checks for static private class elements.

374e6f3

dragomirtitian force-pushed the es-private-static-fields-methods-and-accessors branch from b564ef6 to 374e6f3 Compare February 19, 2021 18:36

joeywatts reviewed Feb 19, 2021

View reviewed changes

tests/baselines/reference/privateNameStaticAccessors.js Show resolved Hide resolved

src/compiler/transformers/classFields.ts Show resolved Hide resolved

mkubilayk reviewed Feb 22, 2021

View reviewed changes

dragomirtitian added 3 commits February 23, 2021 19:54

Added error when using initializers with private static fields when u…

62b4f2f

…seDefineForClassFields is not specified and target is esnext.

Fixed code review issues.

b2a692d

Removed semantically wrong emit on useDefineForClassFields:true wit…

363041e

…h `target:esnext`

mkubilayk reviewed Feb 24, 2021

View reviewed changes

dragomirtitian added 2 commits February 25, 2021 10:11

Changed emit for uninitialized private static fields.

64757e0

Added runtime error in helper if a static private field is accessed b…

68a7f80

…efore it was declared.

mkubilayk reviewed Feb 25, 2021

View reviewed changes

Fixed code review comments for private identifier static class elements.

7e7b0b1

mkubilayk approved these changes Feb 26, 2021

View reviewed changes

mkubilayk mentioned this pull request Feb 26, 2021

add debug.assertNever for unknown node type #53

Merged

pushkine and others added 3 commits March 2, 2021 06:35

typo (microsoft#42899)

2834737

* Update editorServices.ts * Fix baselines Co-authored-by: Orta <[email protected]>

Fixed typo error. (microsoft#42816)

82de139

* Fixed three typo errors * Reverted * Reverted * Fixed typo

Update project.ts (microsoft#42900)

0b337f1

a-tarasyuk and others added 28 commits March 2, 2021 16:32

fix(43006): skip trivia in a function name (microsoft#43021)

f1c911b

Move fixed chunk size polling as a watch option and move it out of se…

0cf834c

…rver (microsoft#42542) * Move fixed chunk size polling as a watch option and move it out of server Fixes microsoft#41549 * Feedback

Delete old, different-case name of quickInfoJSDocTags.baseline (micro…

6cd87d0

…soft#43052)

Added tests

652199a

Update package-lock.json

a8742e3

Merge branch 'es-private-methods-and-accessors' into es-private-stati…

09a5aae

…c-fields-methods-and-accessors

Fixed code review issues for private class elements.

497e47f

Merge pull request microsoft#43055 from armanio123/FixDefineForClassF…

8a81a67

…ields Filter transient flags to fix useDefineForClassFields

capture thisArg of optionalChaining in parens (microsoft#35494)

15e69ac

Fixes: microsoft#35476

Update package-lock.json

3e05aa6

Do a shallow clone for Docker tests. (microsoft#42966)

1f6a8d7

* Do a shallow clone for docker tests. * Remove the 'pull' step from the Docker files.

Update the DOM: March 03, 2021 (microsoft#43070)

e234f0c

* Update the DOM: March 03, 2021 Mostly Typescript-DOM-lib-generator#915 * update baselines

Expose 'reservedInNestedScopes' option when creating temp and loop va…

38fdce9

…riables (microsoft#43083)

fix(39836): allow type declaration/unknown type in catch arguments in…

ca8d9e4

… JavaScript files (microsoft#42392)

Relax requirements on index signatures to 'any' when a type also cont…

9862b03

…ains a string index signature to 'any' (microsoft#43065) * Added test. * Accepted baselines. * Allow other index signatures to 'any' if there is a string index signature to 'any'. * Accepted baselines.

Update package-lock.json

f0a72e2

Properly instantiate inferred constraints in conditional types (micro…

408c804

…soft#42747) * fix combined type mapper in getConditionalType * Add regression tests

Unnecessary elaboration about not being assignable to type parameters (…

25375a2

…microsoft#42952)

Update parseInt parameter name and jsdoc (microsoft#42756)

612a96e

* feat(lib): improve parseInt type definition and docstring * Accepted baselines * update tests

Update package-lock.json

7394efc

Remove undefined from optional properties when inferring to index sig…

ba56fca

…natures (microsoft#43086) * Remove undefined from optional properties when inferring to index signatures * Add tests

Exclude enum/number compatibility rule from comparable relation (micr…

f918bd9

…osoft#42472) * Exclude old number/enum literal compatibility rule from comparable relation * Add tests * Accept new baselines

Update package-lock.json

f7cd702

Update package-lock.json

4d50624

Fixes class shadowing when checking access to a private static class …

294cc48

…element.

Merge remote-tracking branch 'remotes/origin/master' into es-private-…

027bdb3

…static-fields-methods-and-accessors

dragomirtitian merged commit fbd749c into es-private-methods-and-accessors Mar 17, 2021

dragomirtitian deleted the es-private-static-fields-methods-and-accessors branch November 17, 2023 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for static private fields accessors and methods. #50

Added support for static private fields accessors and methods. #50

dragomirtitian commented Feb 16, 2021

joeywatts left a comment

mkubilayk left a comment

mkubilayk left a comment

mkubilayk Feb 25, 2021

dragomirtitian Feb 25, 2021

mkubilayk Feb 25, 2021

dragomirtitian Feb 25, 2021

mkubilayk Feb 25, 2021

dragomirtitian Feb 25, 2021

mkubilayk left a comment

mkubilayk Feb 26, 2021

mkubilayk Feb 26, 2021 •

edited

Loading

Added support for static private fields accessors and methods. #50

Added support for static private fields accessors and methods. #50

Conversation

dragomirtitian commented Feb 16, 2021

joeywatts left a comment

Choose a reason for hiding this comment

mkubilayk left a comment

Choose a reason for hiding this comment

mkubilayk left a comment

Choose a reason for hiding this comment

mkubilayk Feb 25, 2021

Choose a reason for hiding this comment

dragomirtitian Feb 25, 2021

Choose a reason for hiding this comment

mkubilayk Feb 25, 2021

Choose a reason for hiding this comment

dragomirtitian Feb 25, 2021

Choose a reason for hiding this comment

mkubilayk Feb 25, 2021

Choose a reason for hiding this comment

dragomirtitian Feb 25, 2021

Choose a reason for hiding this comment

mkubilayk left a comment

Choose a reason for hiding this comment

mkubilayk Feb 26, 2021

Choose a reason for hiding this comment

mkubilayk Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

mkubilayk Feb 26, 2021 •

edited

Loading