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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Downlevel static properties initializer to be contained within the class IIFE to aid Uglify in dead code removal #15857

Closed
IgorMinar opened this issue May 15, 2017 · 25 comments
Assignees
Labels
Bug A bug in TypeScript

Comments

@IgorMinar
Copy link

IgorMinar commented May 15, 2017

TypeScript currently down-levels classes with static property initializers being outside of the class IIFE when targeting ES5. This prevents Uglify 2.8+ from removing the class from the final application bundle (after the class IIFEs have been independently annotated with the /*#__PURE__*/ annotation (see #13721) and there are no other references to the class).

Changing the down-leveling to keep the assignment within the IIFE just as static methods are down-leveled would resolve this problem and make lots of code eligible for dead code elimination by uglify (after the IIFEs have been marked with the pure annotation).

The change should be backwards compatible unless the current down-leveling approach was intentional to deal with some unusual corner-case that I'm not aware of.

TypeScript Version: 2.2.1 / nightly (2.2.0-dev.201xxxxx)

Code

class MyHappyClass {
  static myStaticProp = 'this is a simple static prop';
  static myStaticMethod() { return 'static method' };
}

Expected behavior:

var MyHappyClass = (function () {
    function MyHappyClass() {
    }
    MyHappyClass.myStaticMethod = function () { return 'static method'; };
    MyHappyClass.myStaticProp = 'this is a simple static prop';
    return MyHappyClass;
}());

Actual behavior:

The code above downlevels to the static prop being initialized

var MyHappyClass = (function () {
    function MyHappyClass() {
    }
    MyHappyClass.myStaticMethod = function () { return 'static method'; };
    ;
    return MyHappyClass;
}());
MyHappyClass.myStaticProp = 'this is a simple static prop';

Playground link

@mhegazy
Copy link
Contributor

mhegazy commented May 15, 2017

Just to clarify, this is needed in conjunction with #13721. this change without #13721 is not sufficient, correct?

@IgorMinar
Copy link
Author

IgorMinar commented May 15, 2017

@mhegazy not quite. I already have a tool that adds the pure annotations as requested in #13721. it's a bit hacky, but works. Correcting the static prop downleveling is much harder to do as a transform on the ES5 code, so I'd prefer it to be implemented in tsc rather than outside.

(just to clarify, even though I have a tool for #13721, I still think that having typescript emit this annotation is quite critical for all uglify users, which is most of the client-side web today, because one shouldn't need to resort to hacks in order to have code downleveled by typescript to be removed if not needed).

@mhegazy
Copy link
Contributor

mhegazy commented May 15, 2017

The emitted goes into phases, the first is to emit an ES6 class from a TS class, and then ES6 class to ES5 class. that is why you see the class written out in such way. doing it all in one closure is doable but will require some wrangling on our side and some hacks here and there to get it to work. not a simple change though.

@IgorMinar
Copy link
Author

@mhegazy static props are quite common and large parts of Angular and Angular Material are currently retained because of this issue, so fixing it along with #13721 would have a huge impact on all client-side code transpiled by typescript.

@kzc
Copy link

kzc commented May 16, 2017

This related Babel issue outlines a pure annotation plugin for down-levelled ES6 classes and a proof-of-concept hack for the static method IIFE problem.

@mhegazy
Copy link
Contributor

mhegazy commented May 16, 2017

@IgorMinar, would not this be considered an uglify bug? you did tell uglify that this value should is sage to be removed using /*#__PURE__*/ , but yet it keeps it around just because of a write to it.

@kzc
Copy link

kzc commented May 17, 2017

you did tell uglify that this value should is sage to be removed using /*#__PURE__*/, but yet it keeps it around just because of a write to it.

It's a write to a property, so it's trickier. Uglify does not perform SSA. Pull requests are welcome.

We're going for low hanging fruit here. It is much easier for the code generator to emit the static properties within an IIFE marked with a pure annotation comment out front.

@IgorMinar
Copy link
Author

@mhegazy I agree with @kzc, the effort on emitting this correctly is much lower than doing full static analysis, which you have done with info at much higher fidelity.

Uglify would also need to be sure that the assignment is not into a setter which could have non-local side-effect, and right there things get very complicated very quickly.

@kzc
Copy link

kzc commented May 17, 2017

I don't know the Typescript code base but the proof-of-concept Babel hack to move static properties into the IIFE was a few lines.

@rbuckton
Copy link
Member

rbuckton commented May 17, 2017

Will this also be an issue if you try to use Uglify on ES6 output? For example:

// es6 output
class MyHappyClass {
  static myStaticMethod() { return 'static method'; };
}
MyHappyClass.myStaticProp = 'this is a simple static prop';

@mhegazy mhegazy added the Bug A bug in TypeScript label May 17, 2017
@mhegazy mhegazy added this to the TypeScript 2.4 milestone May 17, 2017
@kzc
Copy link

kzc commented May 17, 2017

@rbuckton I see what you're getting at - there's no way to set a static property within an ES2015 class definition. uglify-es does not drop the unused class if it has static property assignments.

This annotation proposal is for ES5 code generation only. Typescript ES6 code generation can remain the same.

The previously mentioned Babel hack relies on the static property proposal implemented in babel-plugin-transform-class-properties, which if I'm not mistaken is also valid typescript:

class C { 
    method() {}
    static static_method() {} 
    static static_prop = 1; 
}

--> babel + babel-plugin-transform-class-properties + hack -->

// ...polyfills...

var C = /*#__PURE__*/function () {
  function C() {
    _classCallCheck(this, C);
  }

  _createClass(C, [{
    key: "method",
    value: function method() {}
  }], [{
    key: "static_method",
    value: function static_method() {}
  }]);

  C.static_prop = 1;
  return C;
}();

@IgorMinar
Copy link
Author

Interesting turn of events. So @rbuckton is basically pointing out that if we uglify were to operate on ES2015 code rather than on ES5 then there would be no way for uglify to remove the unused class with a static property, because it doesn't understand that the static prop initialization has no non-local side-effects.

It sounds like this is a problem that will need to be dealt with in uglify for es2015, but today, most of the people downlevel code to es5. So for the es2015->es5 transform, this issue is still relevant I believe.

@kzc
Copy link

kzc commented May 18, 2017

Typescript could also solve the ES2015 "unused class with static properties and decorators" problem by emitting a /*@__PURE__*/ IIFE wrapper.

Consider this typescript example:

$ cat hello.ts
function sealed(constructor: Function) {
    Object.seal(constructor);
    Object.seal(constructor.prototype);
}

@sealed
class UnusedClass {
    static static_prop = "static prop";
    static static_method() {}
}

console.log("hello");

Generate ES2015 code:

$ node_modules/.bin/tsc hello.ts --target es6 --experimentalDecorators
$ cat hello.js
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;
};
function sealed(constructor) {
    Object.seal(constructor);
    Object.seal(constructor.prototype);
}
let UnusedClass = class UnusedClass {
    static static_method() { }
};
UnusedClass.static_prop = "static prop";
UnusedClass = __decorate([
    sealed
], UnusedClass);
console.log("hello");

The code above would retain the unused class with uglify-es.

However, if typescript were to generate the following for ES2015:

$ cat hello-pure.js
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;
};
function sealed(constructor) {
    Object.seal(constructor);
    Object.seal(constructor.prototype);
}
let UnusedClass = /*@__PURE__*/function(){
    let UnusedClass = class UnusedClass {
        static static_method() { }
    };
    UnusedClass.static_prop = "static prop";
    UnusedClass = __decorate([
        sealed
    ], UnusedClass);
    return UnusedClass;
}();
console.log("hello");

Then uglify-es could drop the unused class:

$ node_modules/uglify-es/bin/uglifyjs hello-pure.js -c toplevel,passes=3
this&&this.__decorate,console.log("hello");

The ES2015 IIFE would only be necessary if there is at least one class decorator or static property for a given class.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented May 18, 2017

@IgorMinar to what extent do you expect this to operate correctly? To get the behavior in #13721, you'd need to ensure that the static initializers themselves are side-effect free, which you'd only be able to guarantee for a limited set of simple expressions (e.g. literal expressions).

Is there a common example of static literal initializers that you've found? Is it okay if other expressions can't be guaranteed as side-effect free?

@IgorMinar
Copy link
Author

for changing how the downleveling of the static props works there is nothing risky about the change as far as I can tell.

the bigger question is about whether #13721 and I'll answer that part on that issue.

@kzc
Copy link

kzc commented May 18, 2017

What about class decorators? Should they be in the IIFE as well?

@mhegazy
Copy link
Contributor

mhegazy commented May 18, 2017

class decorators can not be guaranteed to be side effect free. so even if we emit them in the same closure, the uglify flag will not be emitted.

@kzc
Copy link

kzc commented May 18, 2017

Just for the sake of argument, let's say a class decorator has a side effect. Can you provide an example where a typescript user would want that decorator to be called and the code for that class retained for an otherwise unused class? I'm not trying to be difficult, I'm just not aware of a real-life use case.

@mhegazy
Copy link
Contributor

mhegazy commented May 18, 2017

if the decorator registers a class in a global registry (e.g. MEF like registration for dynamic instantiation). that would be the only use (or call) to the class. eliding the class is exactelly the opposite of what you want.

@IgorMinar
Copy link
Author

@kzc Angular uses decorators only as a carrier for metadata during design-time or dev mode. In prod mode, we can safely delete them all and we have a tool to do that that will be rolled out via angular-cli soon. So in short, decorators, whether class or property decorators are not a concern for us because we strip completely in production bundles.

@kzc
Copy link

kzc commented May 18, 2017

eliding the class is exactelly the opposite of what you want.

On the contrary! That would be exactly what I'd want as a typescript user. :-)

If the outermost decorator is a special decorator - @elide or whatever - then the typescript user could be in charge of that decision to ignore side effects for specific classes - be they static properties or other decorators.

@mhegazy
Copy link
Contributor

mhegazy commented May 18, 2017

On the contrary! That would be exactly what I'd want as a typescript user. :-)

not sure i understand your comment..

@register("MyExtension")
class C {}

you would assume that this is an extension, and a framework at runtime would instantiate the class, and use it. eliding it would not help.

@kzc
Copy link

kzc commented May 18, 2017

I'm saying the programmer should have the option to decide on a class by class basis whether to ignore side effects for static properties and decorators.

But now that I think about it, it could be done without a language extension if Typescript were to merely preserve comments in the following example:

function deco(constructor: Function) {
    Object.seal(constructor);
    Object.seal(constructor.prototype);
}

let Greeter = /*@__PURE__*/function(){
    @deco
    class Greeter {
        static se = side_effect();
        greeting: string;
        constructor(message: string) {
            this.greeting = message;
        }
        greet() {
            return "Hello, " + this.greeting;
        }
    }
    return Greeter;
}();

It presently drops the comment in the emitted code:

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;
};
function deco(constructor) {
    Object.seal(constructor);
    Object.seal(constructor.prototype);
}
var Greeter = function () {
    var Greeter = (function () {
        function Greeter(message) {
            this.greeting = message;
        }
        Greeter.prototype.greet = function () {
            return "Hello, " + this.greeting;
        };
        return Greeter;
    }());
    Greeter.se = side_effect();
    Greeter = __decorate([
        deco
    ], Greeter);
    return Greeter;
}();

@mhegazy
Copy link
Contributor

mhegazy commented May 18, 2017

I'm saying the programmer should have the option to decide on a class by class basis whether to ignore side effects for static properties and decorators.

that is a different feature request then. generally we have not had type-directed or decorator-directed emit. it has been one of the design goals (see # 5)

But now that I think about it, it could be done without a language extension if Typescript were to merely preserve comments in the following example:

persevering comments is doable. but i doubt anyone wants to write code that looks like this. they usually want their tools to do that kind of work.

@kzc
Copy link

kzc commented May 18, 2017

persevering comments is doable.

Preserving comments before function calls would be useful. There would be no alternative to drop such a class otherwise.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants