Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Private named instance fields #30829

Merged

Conversation

@mheiber
Copy link
Contributor

mheiber commented Apr 9, 2019

Private-Named Instance Fields

This PR implements 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

PR merge checklist

  • BB: incorporate remaining feedback
  • BB: add multiple @targets to conformance tests esp this one
  • MS: reviewer 1 馃憤 remaining feedback
  • MS: reviewer 2 馃憤 remaining feedback
  • BB: squash and rebase
  • MS: run rw test suite
  • merge companion PR to tslib

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;
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 lead to the following related work by the team:

This PR includes work by the following engineers:

@msftclas

This comment has been minimized.

Copy link

msftclas commented Apr 9, 2019

CLA assistant check
All CLA requirements met.

@mheiber mheiber force-pushed the bloomberg:private-named-instance-fields branch 2 times, most recently from b242db5 to f2c5233 Apr 9, 2019
@mihailik

This comment has been minimized.

Copy link
Contributor

mihailik commented Apr 15, 2019

Amazing achievement!

Are the helpers _classPrivateFieldGet/_classPrivateFieldSet treated in the same way as __extends, __awaiter, __generator?

(that would include modifications to tslib)

@mheiber

This comment has been minimized.

Copy link
Contributor Author

mheiber commented Apr 17, 2019

@mihailik thanks! As far as I know, the helpers work the same way. Is any additional work required to get the helpers into tslib? I was hoping the syncing was semi-automated.

@weswigham

This comment has been minimized.

Copy link
Member

weswigham commented Apr 17, 2019

I was hoping the syncing was semi-automated.

Sadly, they are not. A paired PR against tslib with the finalized helpers will be needed.

@Neuroboy23

This comment has been minimized.

Copy link

Neuroboy23 commented Apr 17, 2019

@weswigham: I have asked our IP team to set up a fork of the tslib repo.

@joeywatts and @mheiber: Will you take a look at this other repo? I think it will be an easy mod.

@G-Rath

This comment has been minimized.

Copy link

G-Rath commented May 30, 2019

Forgive me if this is obvious, but I've look around on the issues and couldn't see this covered anywhere:

Is there any particular reason why we can't do private #bar = 3;?
(In case its not clear, I mean this purely as a meaningless sugar that would never make it into compiled code)

Would there be any chance of getting that syntax supported?

In my eyes it'd give us the best of both worlds, as then all class properties can maintain their indentation level, along with making refactoring easy for people like me who prefix all our private properties with _.

I mean you could pretty much just use a global find-and-replace using /private #/ regex, since # is only valid for private class properties - the only issue with this I can think of would be with strings.

Also, could someone confirm for me if its planned for TS somewhere down the line to transform private to #? (I suspect it is, but again there's a lot of heavy comments, PRs, & issues on the matter, so it'd be nice to have a simple one-liner comment about it if possible).

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented May 30, 2019

@G-Rath no, no chance of it being supported by JS itself (see the FAQ). I suspect/hope TS would not be likely to support non-type-related syntax that isn't standard to JS.

@G-Rath

This comment has been minimized.

Copy link

G-Rath commented May 30, 2019

@ljharb fair enough - so then I assume TS is aiming to deprecate & remove the private keyword? (over time of course)

@robpalme

This comment has been minimized.

Copy link

robpalme commented May 30, 2019

@G-Rath Your question on the future of TypeScript's keyword private is one that many people will have, and may provoke more discussion. Would you like to raise it as a new top-level issue?

@G-Rath

This comment has been minimized.

Copy link

G-Rath commented May 30, 2019

@robpalme more than happy to - done via #31670

@septs

This comment has been minimized.

Copy link

septs commented May 31, 2019

"private field" decorator "private" only?

class Sample {
    private #field: number = 1;
}
@robpalme

This comment has been minimized.

Copy link

robpalme commented Jun 18, 2019

@rbuckton has re-landed the refactoring of class properties. The next step is to rebase this PR.

@Aqours

This comment has been minimized.

Copy link

Aqours commented Jun 21, 2019

Does private name use same private fields transformation of #name?

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Jun 21, 2019

@Aqours no, private is meant to be strictly design-time.

@mheiber

This comment has been minimized.

Copy link
Contributor Author

mheiber commented Jun 21, 2019

The rebase is in progress and we're expecting to update soon!

@mheiber

This comment has been minimized.

Copy link
Contributor Author

mheiber commented Jun 21, 2019

I noticed an issue with our Language Service changes. Type inference and red underlines work correctly, but autocomplete is borked.

Steps to reproduce:

In a class with private field #foo#, start typing this.#foo

Actual behavior:

langservice

Autocomplete completes incorrectly (see screenshot). It is actually completing with secret variables we use in the transformation

Expected behavior:

Autocomplete completes as #foo.

Advice welcome on how to fix autocomplete in this PR!

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Jun 24, 2019

That is actually really weird. Are you sure that it's not just picking up the output .js files? Are you able to reproduce in a fourslash test?

@mheiber mheiber force-pushed the bloomberg:private-named-instance-fields branch from f2c5233 to a9cb10e Jun 25, 2019
@mheiber

This comment has been minimized.

Copy link
Contributor Author

mheiber commented Jun 25, 2019

Thanks for your suggestion, @DanielRosenwasser: the completions were picking up the JS output. I added an export to let TS know the file is a module, and no longer see weird completions.
Unfortunately, I also don't see the desired completion, which is this.#foo:

image

@joeywatts joeywatts force-pushed the bloomberg:private-named-instance-fields branch from a9cb10e to 36a1648 Jun 26, 2019
@mheiber mheiber force-pushed the bloomberg:private-named-instance-fields branch from 36a1648 to d2adc3c Jun 27, 2019
@joeywatts joeywatts force-pushed the bloomberg:private-named-instance-fields branch 2 times, most recently from 71c384a to afcc88c Jun 27, 2019
@mheiber

This comment has been minimized.

Copy link
Contributor Author

mheiber commented Dec 24, 2019

@DanielRosenwasser , thanks for the heads-up. We rebased again and checks are passing.

@johnnyreilly

This comment has been minimized.

Copy link

johnnyreilly commented Dec 24, 2019

Apropos of nothing, Merry Christmas! 馃巺馃巹馃

@mheiber

This comment has been minimized.

Copy link
Contributor Author

mheiber commented Dec 24, 2019

Likewise!

@DanielRosenwasser DanielRosenwasser merged commit 36c87ac into microsoft:master Dec 27, 2019
8 checks passed
8 checks passed
build (8.x)
Details
build (10.x)
Details
build (12.x)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
node10 Build #58639 succeeded
Details
node12 Build #58637 succeeded
Details
node8 Build #58638 succeeded
Details
@DanielRosenwasser

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Dec 27, 2019

Congrats!! 馃帀馃帀馃帀馃帀

@mheiber

This comment has been minimized.

Copy link
Contributor Author

mheiber commented Dec 27, 2019

Thanks so much for the opportunity for us to contribute and for the time reviewing, especially @rbuckton , @sandersn , and @DanielRosenwasser .

@mikeconley12

This comment has been minimized.

Copy link

mikeconley12 commented Dec 29, 2019

Hi!

I think #private fields can reduce flexibility of TypeScript (and JavaScript).
For example, how can I access #private fields from prototype methods in the following example:

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

// another-module.ts
Greeter.prototype.myAwesomeGreet = function() {
    // how can I access this.#name from here?
}
@mheiber

This comment has been minimized.

Copy link
Contributor Author

mheiber commented Dec 29, 2019

@mikeconley12 , this is a good question. The short answer is that you cannot access the private field from another-module.ts in your example. There was a trade-off here between 'hard privacy' and expressiveness. In your example, perhaps a WeakMap or Symbol might work better.

Hi!

I think #private fields can reduce flexibility of TypeScript (and JavaScript).
For example, how can I access #private fields from prototype methods in the following example:

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

// another-module.ts
Greeter.prototype.myAwesomeGreet = function() {
    // how can I access this.#name from here?
}

I recommend seeing discussion at tc39/proposal-class-fields. ES private fields are a JS feature, already implemented in Chrome, Node, Babel, and (soon) Safari.

@falsandtru

This comment has been minimized.

Copy link
Contributor

falsandtru commented Dec 29, 2019

Where is the plan to implement private methods? I want to use it on TS like Babel.

@GulajavaMinistudio GulajavaMinistudio mentioned this pull request Dec 29, 2019
6 of 6 tasks complete
@mheiber

This comment has been minimized.

Copy link
Contributor Author

mheiber commented Dec 29, 2019

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Dec 29, 2019

A non-arrow function, stored outside the class and assigned to the private field, would be a bit closer in semantics.

@falsandtru

This comment has been minimized.

Copy link
Contributor

falsandtru commented Dec 29, 2019

Thanks. I'm not sure what is unimplemented or not but looks like parameter properties are not supported for ES private fields (may have to consider syntax compatibility with ES).

class C {
    constructor(#prop: number) {
    }
}

I think we need the place like a separated issue to manage these considerations.

  • Properties
  • Methods
  • Parameters (?)
  • Static fields (?)
@zhuravlikjb

This comment has been minimized.

Copy link

zhuravlikjb commented Dec 30, 2019

@falsandtru See #31670 for the discussion of 'private' vs '#'.

@falsandtru

This comment has been minimized.

Copy link
Contributor

falsandtru commented Dec 30, 2019

It is helpful, thanks.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Dec 30, 2019

I'm not sure what is unimplemented or not but looks like parameter properties are not supported for ES private fields (may have to consider syntax compatibility with ES).

We have no plans to implement #-private parameter properties.

@falsandtru

This comment has been minimized.

Copy link
Contributor

falsandtru commented Dec 30, 2019

I read #31670 (comment) but static fields are not referred. What about static private fields? Babel already implemented.

@mikeconley12

This comment has been minimized.

Copy link

mikeconley12 commented Dec 31, 2019

@mikeconley12 , this is a good question. The short answer is that you cannot access the private field from another-module.ts in your example. There was a trade-off here between 'hard privacy' and expressiveness. In your example, perhaps a WeakMap or Symbol might work better.

Hi!
I think #private fields can reduce flexibility of TypeScript (and JavaScript).
For example, how can I access #private fields from prototype methods in the following example:

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

// another-module.ts
Greeter.prototype.myAwesomeGreet = function() {
    // how can I access this.#name from here?
}

I recommend seeing discussion at tc39/proposal-class-fields. ES private fields are a JS feature, already implemented in Chrome, Node, Babel, and (soon) Safari.

@mheiber Thank you for the answer!

But still, is there any way to "hack" the #private properties?

I understand that the hack will be a bad practice. But I'm curious :)

@mheiber

This comment has been minimized.

Copy link
Contributor Author

mheiber commented Dec 31, 2019

@littledan

This comment has been minimized.

Copy link

littledan commented Dec 31, 2019

Private fields are deliberately designed to be not hackable: strong encapsulation is a core design goal. I'd recommend strongly against manipulating WeakMap's prototype for this purpose--it won't hold up when you later upgrade to native private fields (which have better performance).

If you want hackable privacy, you can continue to use the TypeScript private keyword.

@mheiber

This comment has been minimized.

Copy link
Contributor Author

mheiber commented Dec 31, 2019

fwiw, I agree that the hack is a bad idea. Wouldn't want the hack to be a secret, though, since it's good to know that the privacy of the transformed code is not absolute, barring control over the WeakMap prototype. @littledan, are there other privacy hills privacy holes, as well?

@littledan

This comment has been minimized.

Copy link

littledan commented Dec 31, 2019

@mheiber What do you mean by privacy hills?

@mheiber

This comment has been minimized.

Copy link
Contributor Author

mheiber commented Dec 31, 2019

I meant "privacy holes." Autocorrect got me!

@littledan

This comment has been minimized.

Copy link

littledan commented Dec 31, 2019

I'm not aware of any privacy holes in the main design. There may be other hacks that just work on this transform, though.

@Jamesernator

This comment has been minimized.

Copy link

Jamesernator commented Jan 2, 2020

We have no plans to implement #-private parameter properties.

The feature is pretty popular, I imagine a lot of places won't adopt private fields primarily for this reason.

Has a similar feature been considered for proposing at TC39? e.g.:

class Point {
    constructor(#x: number, #y: number) {}
}

And for public fields maybe allow using this.prop as a parameter (or destructured parameter) e.g.:

class Image {
  constructor(
    this.data: ImageData, // Simple parameter
    {
      height: this.height,
      width: this.width /* in destructuring */
    }: { width: number, height: number },
  ) {}
}
@avonwyss

This comment has been minimized.

Copy link

avonwyss commented Jan 2, 2020

Given that the __classPrivateFieldGet and __classPrivateFieldSet could be customized in the way they work by implementing them differently and importing that customized tslib, wouldn't it make sense to also put the _nameWeakMap_1 = new WeakMap(); map initialization into its own tslib helper function?

This would allow to implement a version which also works without WeakMap support (e.g. IE <= 10), or for anyone who wants to use a different mechanism, such as with non-enumerable properties or Symbol-based private fields (knowing that these are not private as per tc39 proposal, but may perform better).

@mbrowne

This comment has been minimized.

Copy link

mbrowne commented Jan 3, 2020

In case anyone else is wondering the same thing I was, it looks like the first production release to include this will be version 3.8, scheduled for sometime in February:
https://github.com/microsoft/TypeScript/wiki/Roadmap#38-february-2020

sheetalkamat added a commit to microsoft/TypeScript-TmLanguage that referenced this pull request Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.