Investigate using private fields in extHostTypes

[mjbvz]

Problem
Our APIs expose a few classes such as WorkspaceEdit or Range to extensions. Some of these classes have private properties. However TypeScript's private keyword does not actual prevent extensions from accessing these fields. Extensions may mistakenly enumerate over these properties or mistakenly override these properties during subclassing if they are not careful! (although hopefully no one is subclassing WorkspaceEdit:))

Investigation
We should see if we can switch to use ECMAScript private fields for these properties instead. Private fields ensure the property is only visible within the class (and not to consumers of our API). While targeting ES6, private fields compile down to use WeakMap

Not having truly private properties on these types is not a huge issue but I do think it is worth investigating a little since it is a best practice.

[jrieken]

Yes! A little afraid of negative perf-impact when using WeakMap (measure) but using real private all the way is something I am waiting for. I believe that our current version of Electron/V8 already supports this

[mjbvz]

Quick micro benchmark in Chrome:

---

Native Private fields

• Average time: 24ms

function native(len) {
    class F {
        #a = 1;
        #b = 2;
        doSomething() {
            return this.#a++ + this.#b++;
        }
    }
    let out = 0
    const f = new F();
    for (let i = 0; i < len; ++i) {
        out += f.doSomething();
    }
    return out;
}
console.time(); native(10000000); console.timeEnd();

---

Generated

• Average time: 400ms

function generated(len) {
    class F {
        #a = 1;
        #b = 2;
        doSomething() {
            return this.#a++ + this.#b++;
        }
    }
    let out = 0
    const f = new F();
    for (let i = 0; i < len; ++i) {
        out += f.doSomething();
    }
    return out;
}
console.time(); native(10000000); console.timeEnd();

"use strict";
var __classPrivateFieldGet = (this && this.__classPrivateFieldGet) || function (receiver, privateMap) {
    if (!privateMap.has(receiver)) {
        throw new TypeError("attempted to get private field on non-instance");
    }
    return privateMap.get(receiver);
};
var __classPrivateFieldSet = (this && this.__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;
};
function generated(len) {
    var _a, _b;
    class F {
        constructor() {
            _a.set(this, 1);
            _b.set(this, 2);
        }
        doSomething() {
            var _c, _d;
            return (__classPrivateFieldSet(this, _a, (_c = +__classPrivateFieldGet(this, _a)) + 1), _c) + (__classPrivateFieldSet(this, _b, (_d = +__classPrivateFieldGet(this, _b)) + 1), _d);
        }
    }
    _a = new WeakMap(), _b = new WeakMap();
    let out = 0;
    const f = new F();
    for (let i = 0; i < len; ++i) {
        out += f.doSomething();
    }
    return out;
}
console.time();
native(10000000);
console.timeEnd();

---

So using private fields with weak maps does have some overhead.

[mjbvz]

As a test, I converted Disposable.callOnDispose to use a private field. The idea behind this change:

• Test to see if private fields happen to break any scenarios (or extensions that were somehow relying on our internals)
• Disposable.callOnDispose should not generally be used inside loops or other performance critical code paths

[jrieken]

We have util that strips out the boilerplate preamble that TS emits before the actual code (see below) and we should teach it the private field get/set utils so that they don't get duplicated (when used in more files). Alternatively we emit the private field as-is - since our version of Electron supports this (we already have different targets in tsconfig.json and tsconfig.monaco.json)

var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
    var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
    else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
    return c > 3 && r && Object.defineProperty(target, key, r), r;
};
var __classPrivateFieldSet = (this && this.__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 = (this && this.__classPrivateFieldGet) || function (receiver, privateMap) {
    if (!privateMap.has(receiver)) {
        throw new TypeError("attempted to get private field on non-instance");
    }
    return privateMap.get(receiver);
};

[mjbvz]

We have to target esnext for to emit native private fields. es2020 doesn't include it yet

[jrieken]

We have to target esnext for to emit native private fields. es2020 doesn't include it yet

It's a shame we don't have fine grained control over what is down-level compiled. I don't feel good to use target esnext because I don't know whats all in it but I would prefer to selectively target a feature like native privates...

fyi - this where boiler plate removal happens:

vscode/build/lib/bundle.ts

Line 337 in d729e50

function removeDuplicateTSBoilerplate(destFiles: IConcatFile[]): IConcatFile[] {

[mjbvz]

I don't think we need a comprehensive push to adopt private properties and can instead handle this on a case by case basis

I've adopted them on webviews since a few extensions were trying to access out existing _properties and since the webview properties are typically not accessed inside tight loops where performance is critical