Skip to content

Private-named instance fields #32

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

Conversation

mheiber
Copy link

@mheiber mheiber commented Mar 27, 2019

@bloomberg teammates: The target for this PR is a branch that is currently up to date with Microsoft/TypeScript master.
Once this PR is approved, we can open a PR with the same changes from Bloomberg/private-named-instance-fields to Microsoft/private-named-instance-fields.

Private-Named Instance Fields

This PR implements part of the tc39 class fields proposal for TypeScript. It includes:

  • parse and check private-named fields, methods, and accessors
  • displayprivate names in the language server
  • transform private-named instance fields

Example:

ts

class Greeter {
    #name: string;
    constructor(name: string) {
        this.#name = name;
    }
    greet() {
        console.log(`hello ${this.#name}`);
    }
}

const greeter = new Greeter("world");
console.log(greeter);                 // logs 'Greeter {}'
console.log(Object.keys(greeter));    // logs '[]'
greeter.greet();                      // logs 'hello world'

js

var _classPrivateFieldSet = function (receiver, privateMap, value) { if (!privateMap.has(receiver)) { throw new TypeError("attempted to set private field on non-instance"); } privateMap.set(receiver, value); return value; };
var _classPrivateFieldGet = function (receiver, privateMap) { if (!privateMap.has(receiver)) { throw new TypeError("attempted to get private field on non-instance"); } return privateMap.get(receiver); };
var _nameWeakMap_1;
"use strict";
class Greeter {
    constructor(name) {
        _nameWeakMap_1.set(this, void 0);
        _classPrivateFieldSet(this, _nameWeakMap_1, name);
    }
    greet() {
        console.log(`hello ${_classPrivateFieldGet(this, _nameWeakMap_1)}`);
    }
}
_nameWeakMap_1 = new WeakMap();
const greeter = new Greeter("world");
console.log(greeter); // logs 'Greeter {}'
console.log(Object.keys(greeter)); // logs '[]'
greeter.greet(); // logs 'hello world'

This PR includes work by the following engineers:

  • Joey Watts @joeywatts (Bloomberg): private fields transformation
  • Max Heiber @mheiber (Bloomberg): parser and checker, pr review
  • Michael Gunter @Neuroboy23 (Bloomberg): pr review
  • Rob Palmer @robpalme (Bloomberg): leadership, spec expertise
  • Jaideep Bhoosreddy, Michael Molisani, Ravi Amin (Bloomberg)
  • Ron Buckton @rbuckton (Microsoft): designed the checker implementation, pr review
  • Daniel Rosenwasser @DanielRosenwasser (Microsoft): pr review

@mheiber mheiber force-pushed the private-named-instance-fields branch 2 times, most recently from a97aa6b to ecd1529 Compare March 27, 2019 20:22
@mheiber mheiber changed the base branch from master to private-named-instance-fields March 27, 2019 20:32
@mheiber mheiber changed the title WIP Private named instance fields Private-named instance fields Mar 27, 2019
Joseph Watts and others added 6 commits March 28, 2019 15:46
and check that private names not used in parameters

Signed-off-by: Max Heiber <[email protected]>
Signed-off-by: Joseph Watts <[email protected]>
- [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

mv private name tests together

more checker tests for private names

update naming and cleanup for check private names

for private name support in the checker:
- make names more consistent
- remove unnecessary checks
- add utility function to public API
- other small cleanup

Move getPropertyNameForUniqueESSymbol to utility

for consistency with other calculation of
special property names (starting with __),
move the calculation of property names for
unique es symbols to `utilities.ts`.

private name tests strict+es6

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.

error message for private names in obj literals

Disallow decorating private-named properties
because the spec is still in flux.

Signed-off-by: Max Heiber <[email protected]>
Implements private instance fields on top of the class properties refactor.
Signed-off-by: Joseph Watts <[email protected]>
Signed-off-by: Max Heiber <[email protected]>
@mheiber mheiber force-pushed the private-named-instance-fields branch from ecd1529 to 3f6b68f Compare March 28, 2019 15:50
const containingClass = getContainingClass(name.parent);
if (!containingClass) {
// we're in a case where there's a private name outside a class (invalid)
return undefined;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this is the only explicit return value of undefined. Any other undefined would have to come from an inner method call. I see some existing calling code that does things like !getDeclarationName(...). Does this undefined value here mean the same thing as any other potential undefined values? If not, maybe it's better to throw here (explicitly or implicitly) instead?

Copy link
Author

@mheiber mheiber Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Neuroboy23 these changes were reviewed internally months ago and also given a peruse by Daniel and Ron. I'm open to revisiting this after we open the PR against the MS TS repo.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re this specific question:

  • a private name outside a class body is a parse error and will be caught by the parser. If the user has noEmitOnError: false in their compilerOptions then we have to do best-effort on the emit, even though the emit almost certainly bad. In this case, I'm not sure whether it would be better to return node or undefined.

@Neuroboy23 Neuroboy23 merged commit a04bfff into bloomberg:private-named-instance-fields Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants