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

goog.reflect.objectProperty does not prevent inlining of methods breaking method annotations #2437

Closed
WearyMonkey opened this issue Apr 14, 2017 · 5 comments

Comments

@WearyMonkey
Copy link
Contributor

Closure Compiler Service example

We are using TypeScript method decorators which transpiles:

class MyClass {
  @myAnnotation
  myMethod() { }
}

to

__decorate([myAnnotation], MyClass.prototype, "myMethod", null);

Which we further transpile into the CC compatible form:

__decorate([myAnnotation], MyClass.prototype, goog.reflect.objectProperty("myMethod"), null);

However, in some instances CC is inlining the invocation of the annotated methods, which effecively by passes the annotation.

This is breaking our usage of modern annotated driven libraries such as MobX.

Can compiler treatment of goog.reflect.objectProperty be extended to prevent inlining?
Is there canonical way we can prevent inlining of specific methods which we can include in a transpilation?

Cheers!

@ChadKillingsworth
Copy link
Collaborator

Can compiler treatment of goog.reflect.objectProperty be extended to prevent inlining?

No. That's beyond the scope of that intrinsic.

Can you post an example of what it might look like when inlined?

@WearyMonkey
Copy link
Contributor Author

WearyMonkey commented Apr 14, 2017

TypeScript input (target es6):

function logger(target, key, descriptor) {
  const org = descriptor.value;
  descriptor.value = function() {
    console.log(`${key} ran`);
    return org.apply(this, arguments);
  };
  return descriptor;
}

class MyClass {
  @logger
  myMethod() {
    console.log('myMethod');
  }
}

new MyClass().myMethod();

TypeScript output and CC input (with goog.reflect.objectPropery injected):

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 logger(target, key, descriptor) {
    const org = descriptor.value;
    descriptor.value = function () {
        console.log(`${key} ran`);
        return org.apply(this, arguments);
    };
    return descriptor;
}
class MyClass {
    myMethod() {
        console.log('myMethod');
    }
}
__decorate([
    logger
], MyClass.prototype, goog.reflect.objectProperty("myMethod", MyClass.prototype), null);
new MyClass().myMethod();

CC Advanced mode output:

(this && this.b || function(f, c, a, e) {
  var g = arguments.length, d = 3 > g ? c : null === e ? e = Object.getOwnPropertyDescriptor(c, a) : e, h;
  if ("object" === typeof Reflect && "function" === typeof Reflect.a) {
    d = Reflect.a(f, c, a, e);
  } else {
    for (var k = f.length - 1;0 <= k;k--) {
      if (h = f[k]) {
        d = (3 > g ? h(d) : 3 < g ? h(c, a, d) : h(c, a)) || d;
      }
    }
  }
  return 3 < g && d && Object.defineProperty(c, a, d), d;
})([function(f, c, a) {
  var e = a.value;
  a.value = function() {
    console.log(c + " ran");
    return e.apply(this, arguments);
  };
  return a;
}], function() {
}.prototype, "myMethod", null);
console.log("myMethod");

From the above output, you can see new MyClass().myMethod(); was inlined to: console.log("myMethod"); which would not invoke the logger decoration as expected.

@ChadKillingsworth
Copy link
Collaborator

This is a problem with property reflection - the method call is hidden from the compiler. Polymer suffers from the same problem.

What's needed to block this is a sink value. See https://google.github.io/closure-library/api/goog.reflect.html#sinkValue

Ideally tsickle would be updated to output this for you.

Example

@WearyMonkey
Copy link
Contributor Author

Great thanks, we'll integrate sinkValue.

@ChadKillingsworth
Copy link
Collaborator

@WearyMonkey After thinking on this overnight - this may not be what you want. Adding the sink value will block inlining, but it will also block dead code elimination.

I don't know of a way to block inlining but not block dead code elimination. @concavelenz might have ideas though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants