Introduce a flag for decorator rewrite

[theseanl]

I would like to request to introduce a flag so that consumers can include googmodule transformer but not transformDecoratorsOutputForClosurePropertyRenaming transformer.

My use case is to update tsickle in tscc, but to keep a decorator transformer that has been used in tscc (using the same goog.reflect.objectProperty) before it has landed in tsickle (in #1125), so as not to break existing projects using tscc.

As I understand, the implementation in tsickle does not do any measures to prevent closure compiler for inlining decorated class methods. In my previous experience, closure compiler often breaks such codes that contain method decorators, and I've put some additional transformations in tscc to remedy this. I suppose that there already exist projects which uses tsickle and uses method decorators too -- if so, how does one prevent inlining of methods there? By some post-ts transformation? Or do recent versions of closure compiler automatically detect such patterns emitted by tsickle and do not inline those?

[evmar]

CC @rictic

[rictic]

I don't think I've ever seen a decorated method be inlined by closure compiler. Do you have a repro?

[theseanl]

I suppose this can be answered by closure compiler developers whether in principle such inlinings can be guaranteed not to occur. Here is a contrived example:

a.ts

let i = 0;

function decorator(target, prop: PropertyKey, desc: PropertyDescriptor) {
    const { value } = desc;
    desc.value = function () {
        console.log(++i);
        return Reflect.apply(value, this, arguments);
    }
    return desc;
}

class A {
    @decorator
    b() {
        return this.c();
    }
    @decorator
    c() {
        return this.d();
    }
    @decorator
    d() {
        return this.e();
    }
    @decorator
    e() {
        return this.f();
    }
    @decorator
    f() {
        return this.g();
    }
    @decorator
    g() {
        return ++i;
    }
}

console.log(new A().b());

export { };

will be transpiled by tsickle to something like

goog.module('a');
const tslib_1 = goog.require("tslib");
let i = 0;
function decorator(target, prop, desc) {
    const { value } = desc;
    desc.value = function () {
        console.log(++i);
        return Reflect.apply(value, this, arguments);
    };
    return desc;
}
class A {
    b() {
        return this.c();
    }
    c() {
        return this.d();
    }
    d() {
        return this.e();
    }
    e() {
        return this.f();
    }
    f() {
        return this.g();
    }
    g() {
        return ++i;
    }
}
tslib_1.__decorate([
    decorator
], A.prototype, goog.reflect.objectProperty("b", A.prototype), null);
tslib_1.__decorate([
    decorator
], A.prototype, goog.reflect.objectProperty("c", A.prototype), null);
tslib_1.__decorate([
    decorator
], A.prototype, goog.reflect.objectProperty("d", A.prototype), null);
tslib_1.__decorate([
    decorator
], A.prototype, goog.reflect.objectProperty("e", A.prototype), null);
tslib_1.__decorate([
    decorator
], A.prototype, goog.reflect.objectProperty("f", A.prototype), null);
tslib_1.__decorate([
    decorator
], A.prototype, goog.reflect.objectProperty("g", A.prototype), null);
console.log(new A().b());

When it is compiled with base.js, tslib.js, reflect.js

--language_out=ECMASCRIPT_2016
--compilation_level=ADVANCED_OPTIMIZATIONS
--js=base.js
--js=tslib.js
--js=reflect.js
--js=a.js
--js_output_file=out.js

(beautified for readability)

'use strict';

function d(h, b, a, e) {
    var k = arguments.length,
        c = 3 > k ? b : null === e ? e = Object.getOwnPropertyDescriptor(b, a) : e,
        l;
    if ("object" === typeof Reflect && Reflect && "function" === typeof Reflect.decorate) c = Reflect.decorate(h, b, a, e);
    else
        for (var m = h.length - 1; 0 <= m; m--)
            if (l = h[m]) c = (3 > k ? l(c) : 3 < k ? l(b, a, c) : l(b, a)) || c;
    3 < k && c && Object.defineProperty(b, a, c)
};
let f = 0;

function g(h, b, a) {
    const e = a.value;
    a.value = function() {
        console.log(++f);
        return Reflect.apply(e, this, arguments)
    };
    return a
}
class n {}
d([g], n.prototype, "a", null);
d([g], n.prototype, "b", null);
d([g], n.prototype, "g", null);
d([g], n.prototype, "h", null);
d([g], n.prototype, "c", null);
d([g], n.prototype, "f", null);
console.log(++f);

As shown above, in the closure compiler output, all the members of class n are stripped out, console.log will print a wrong value, and d() calls will throw.

[concavelenz]

We should teach the compiler to back off on goog.reflect.objectProperty where it doesn't currently.

[theseanl]

This seems to be a long-standing problem of closure compiler. google/closure-compiler#2420 and google/closure-compiler#2599 are some relevant issues. When I delved into these I couldn't find a reliable way to prevent devirtualization of decorated methods while mangling their names. I am looking forward to seeing this implemented in closure compiler, but in the meantime can we have an option to disable this problematic transformation from tsickle's side?

[concavelenz]

Those issues are not related to the discussion here. @nocollapse disables a specific optimization "collapse properties" but otherwise has no effect on the other analysis (renaming, devirtualization, etc). By design, ADVANCED_OPTIMIZATIONS optimizations disallows reflection by default , it assumes all references are visible or that an appropriate definition is provide in the externs (to assume otherwise would prevent any property optimizations which is the point of ADVANCED_OPTIMIZATIONS). Anything used reflectively (accessed by eval, etc) must be protected in some way. That is to say if it is a "problem" then ADVANCED_OPTIMIZATIONS are a "problem".

goog.reflect.objectProperty, however, exists to access a property by name and still allow renaming, and my assertion is that the only reason to do to use that is to access something reflective and that can be used as a signal to backoff on property optimizations (removal, inlining, devirtualization, etc).