google / closure-compiler

A JavaScript checker and optimizer.
https://developers.google.com/closure/compiler/
Apache License 2.0
7.4k stars 1.15k forks source link

Please do not modify 'super' in static methods! #3345

Open blooddy opened 5 years ago

blooddy commented 5 years ago

input:

class A {
    static ['method']() {
        return this.name;
    }
}
class B extends A {
    static ['method']() {
        return '+' + super['method']();
    }
}
console.log( A['method']() ); // A
console.log( B['method']() ); // +B

After compilation, you replace the super with a direct link to the class, which leads to incorrect work of the code.

output:

class a {
  static ["method"]() {
    return this.name;
  }
}
class b extends a {
  static ["method"]() {
    return "+" + a.method();
  }
}
console.log(a.method()); // a
console.log(b.method()); // a
brad4d commented 5 years ago

Filed internal issue http://b/131155331

brad4d commented 5 years ago

This is part of a more general problem that closure-compiler can end up breaking static methods because it doesn't recognize the types of this and super correctly in static methods and for historical reasons assumes it is safe to collapse these methods (MyClass.staticMethod() becomes MyClass$staticMethod()).

We're working on a fix for the general problems of failure to recognize the type of this and super and incorrect collapsing of static methods this quarter (2019Q2). Not sure if that will be enough on its own to fix this, but it's a necessary first step.

blooddy commented 5 years ago

This is nice, but in previous versions everything worked like a clock) We have version 20181210.0.0 installed and there it’s ok

brad4d commented 5 years ago

Thanks for pointing out that this is a regression since 20181210.0.0.

FYI, @lauraharker . Looks like this is the result of a change to AggressiveInlineAliases to support collapsing of static properties. I

Enabling such collapsing made a really big code size difference for internal Google products, so we're unlikely to reverse it, just work on making it smarter to avoid code breakage.

I believe we just need to fix this line & associated tests. https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/StaticSuperPropReplacer.java#L108

instead of BaseClass.method() we should generate BaseClass.method.call(this)

brad4d commented 5 years ago

Actually, we can't do the simple change I mentioned above, because it will definitely undo the code size improvement work, since CollapseProperties will either break such methods or fail to collapse them.

I think fixing this is going to have to wait until we fix the more general problems I mentioned above.

blooddy commented 5 years ago

we will wait =)

it would be nice to fix the transformation ['name'] to name declarations

brad4d commented 5 years ago

@blooddy

re: the x['name'] to x.name conversions you should probably file a separate issue.

You're doing something with those that looks pretty weird to me, so you're likely to have issues with that. There may be a better way to get the effect you're going for, but let's not discuss that further on this issue.

blooddy commented 5 years ago

https://github.com/google/closure-compiler/issues/3105