Private Name Support in the Checker #5

mheiber · 2018-07-26T13:07:31Z

treat private names as unique:
- case 1: cannot say that a variable is of a class type unless the variable points to an instance of the class
  - see test
- case 2: private names in class hierarchies do not conflict
  - see test
#constructor is reserved
- see test
- check in bindWorker, where every node is visited
Accessibility modifiers can't be used with private names
- see test
- implemented in checkAccessibilityModifiers, using ModifierFlags.AccessibilityModifier
delete #foo not allowed
Private name accesses not allowed outside of the defining class
- see test: https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNameNotAccessibleOutsideDefiningClass.ts
- see test
- implemented in checkDeleteExpression
Do the right thing for nested classes
add getPropertyForPrivateName to public compiler API

getters/setters, statics, and private methods to come in a follow-on PR

DanielRosenwasser · 2018-10-03T01:05:45Z

src/compiler/checker.ts

-        // A reserved member name starts with two underscores, but the third character cannot be an underscore
-        // or the @ symbol. A third underscore indicates an escaped form of an identifer that started
+        // A reserved member name starts with two underscores, but the third character cannot be an underscore,
+        // @ or #. A third underscore indicates an escaped form of an identifer that started


Oxford comma

DanielRosenwasser · 2018-10-03T01:11:28Z

src/compiler/checker.ts

+                                reportError(
+                                    Diagnostics.Property_0_is_missing_in_type_1_While_type_1_has_a_private_member_with_the_same_spelling_its_declaration_and_accessibility_are_distinct,
+                                    diagnosticName(privateNameDescription),
+                                    diagnosticName(source.symbol.valueDeclaration.name || ("anonymous" as __String))


For consistency, elsewhere we use the string "(anonymous)" with parentheses around it (e.g. checkNoTypeArguments)

DanielRosenwasser · 2018-10-03T01:13:17Z

src/compiler/checker.ts

+                                right,
+                                Diagnostics.This_usage_of_0_refers_to_the_private_member_declared_in_its_enclosing_class_While_type_1_has_a_private_member_with_the_same_spelling_its_declaration_and_accessibility_are_distinct,
+                                diagnosticName(right),
+                                diagnosticName(classWithShadowedPrivateName.name || ("anonymous" as __String))


"(anonymous)"

DanielRosenwasser · 2018-10-03T01:24:54Z

src/compiler/checker.ts

            }
+            if (expr.kind === SyntaxKind.PropertyAccessExpression && isPrivateName((expr as PropertyAccessExpression).name)) {
+                error(expr, Diagnostics.The_operand_of_a_delete_operator_cannot_be_a_private_name);
+


Stray newline, and this is as far as my review goes before I have to get off the bus.

DanielRosenwasser

As far as I can tell, this looks good apart from the nits I've left. We'll need one or two more pairs of eyes though. My biggest feedback will be around testing.

I think one test for duplicate private fields would be useful, which is an early error in the proposal. Something like class C { #foo; #foo }.

It'd also be good to see some set of tests in the compiler folder for common scenarios people will expect to work. For example,

// @strict: true
class C {
    #a = 1;
    #b: number;
    #c;
    #d: string | null = null;
    constructor() {
        this.#c = 2;
    }
    foo() {
        this.#d.toLowerCase();
    }
}

I would expect noImplicitAny, strictClassInitialization, and strictNullChecks checks to cause issues here.

src/compiler/checker.ts

src/compiler/diagnosticMessages.json

tests/baselines/reference/privateNameConstructorReserved.errors.txt

src/compiler/binder.ts

src/compiler/checker.ts

src/compiler/utilities.ts

src/compiler/checker.ts

- [x] treat private names as unique: - case 1: cannot say that a variable is of a class type unless the variable points to an instance of the class - see [test](https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNamesUnique.ts) - case 2: private names in class hierarchies do not conflict - see [test](https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNamesNoConflictWhenInheriting.ts) - [x] `#constructor` is reserved - see [test](https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNameConstructorReserved.ts) - check in `bindWorker`, where every node is visited - [x] Accessibility modifiers can't be used with private names - see [test](https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNamesNoAccessibilityModifiers.ts) - implemented in `checkAccessibilityModifiers`, using `ModifierFlags.AccessibilityModifier` - [x] `delete #foo` not allowed - [x] Private name accesses not allowed outside of the defining class - see test: https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNameNotAccessibleOutsideDefiningClass.ts - see [test](https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNamesNoDelete.ts) - implemented in `checkDeleteExpression` - [x] Do [the right thing](https://gist.github.com/mheiber/b6fc7adb426c2e1cdaceb5d7786fc630) for nested classes Signed-off-by: Max Heiber <mheiber@bloomberg.net>

Signed-off-by: Max Heiber <mheiber@bloomberg.net>

Update private name tests to use 'strict' type checking and to target es6 instead of default. Makes the js output easier to read and tests more surface area with other checker features. Signed-off-by: Max Heiber <max.heiber@gmail.com>

joeywatts · 2019-01-24T14:46:01Z

src/compiler/types.ts

    }

-    export interface PrivateName extends Declaration {
+    export interface PrivateName extends Expression, Declaration {


Why is a PrivateName a Declaration? It seems to me like it would just be (one option for) the name of PropertyDeclaration or a PropertyAccess.

good 👀 .
I made it an Expression to debug something else and forgot to switch it back.
Should just be Declaration. Needs to be a declaration because binder::addDeclarationToSymbol expects a declaration.

Signed-off-by: Max Heiber <max.heiber@gmail.com>

mheiber force-pushed the checker branch from 15206c2 to 498daba Compare July 30, 2018 19:51

mheiber mentioned this pull request Aug 2, 2018

Parse Private Names #4

Merged

mheiber force-pushed the checker branch 5 times, most recently from 5892232 to ef4e718 Compare August 25, 2018 23:53

mheiber changed the title ~~[Work in Progress] Checker~~ Private Name Support in the Checker Aug 25, 2018

mheiber mentioned this pull request Aug 27, 2018

All Bloomberg Changes for Private Fields and Methods #6

Closed

mheiber force-pushed the checker branch from ef4e718 to b9a3d26 Compare August 27, 2018 13:50

This was referenced Aug 29, 2018

Checkerchanges mheiber/TypeScript#11

Open

Checker no mangle #9

Closed

mheiber force-pushed the checker branch 3 times, most recently from 6a0ea87 to 6482040 Compare September 20, 2018 20:47

DanielRosenwasser reviewed Oct 3, 2018

View reviewed changes

DanielRosenwasser reviewed Oct 4, 2018

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

src/compiler/diagnosticMessages.json Outdated Show resolved Hide resolved

tests/baselines/reference/privateNameConstructorReserved.errors.txt Outdated Show resolved Hide resolved

mheiber force-pushed the checker branch 2 times, most recently from b9b77f4 to 2d7e227 Compare October 8, 2018 20:42

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

mheiber force-pushed the checker branch from 2d7e227 to 0926674 Compare October 27, 2018 20:54

mheiber force-pushed the es-private-fields branch from 853486f to 221111e Compare October 29, 2018 21:36

mheiber force-pushed the checker branch from 0926674 to 19bf705 Compare October 30, 2018 20:57

rbuckton suggested changes Nov 3, 2018

View reviewed changes

mheiber force-pushed the checker branch 3 times, most recently from 5e83071 to 5aa1b7a Compare December 7, 2018 16:24

Max Heiber added 3 commits January 5, 2019 15:07

Incorporate checker PR feedback

90a6e57

Signed-off-by: Max Heiber <mheiber@bloomberg.net>

mv private name tests together

b1052a6

Signed-off-by: Max Heiber <mheiber@bloomberg.net>

mheiber force-pushed the checker branch from 7d985cb to c78a964 Compare January 5, 2019 20:09

private name tests strict+es6

351c75c

Update private name tests to use 'strict' type checking and to target es6 instead of default. Makes the js output easier to read and tests more surface area with other checker features. Signed-off-by: Max Heiber <max.heiber@gmail.com>

mheiber force-pushed the checker branch from c78a964 to 351c75c Compare January 5, 2019 20:38

joeywatts reviewed Jan 24, 2019

View reviewed changes

mheiber force-pushed the checker branch from 31d6831 to cc77c12 Compare January 24, 2019 17:45

joeywatts approved these changes Jan 24, 2019

View reviewed changes

mheiber force-pushed the checker branch from cc77c12 to c3d7db6 Compare January 24, 2019 21:13

error message for private names in obj literals

99146e5

Signed-off-by: Max Heiber <max.heiber@gmail.com>

mheiber force-pushed the checker branch from c3d7db6 to 99146e5 Compare January 24, 2019 21:52

Neuroboy23 approved these changes Jan 28, 2019

View reviewed changes

mheiber merged commit a71323b into bloomberg:es-private-fields Jan 28, 2019

mheiber deleted the checker branch January 28, 2019 20:23

Private Name Support in the Checker #5

Private Name Support in the Checker #5

Uh oh!

Conversation

mheiber commented Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielRosenwasser Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

mheiber Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joeywatts Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheiber Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mheiber commented Jul 26, 2018 •

edited

Loading

joeywatts Jan 24, 2019 •

edited

Loading