@modifies {arguments} in RemoveUnusedCode

[David Neil]

Context:
I am on a quest to make classes and properties that are modified by calls to `__decorate` (TypeScript experimental decorators) able to be removed as dead code by the compiler.

The decorators appear as two distinct variations.

```

let A = class A {

    myMethod() { return 'a';}

}

A = __decorate([ClassDecorator], A);

__decorate([MethodDecorator], A.prototype, goog.reflect.objectProperty("myMethod", A.prototype));

```

If I extern `__decorate` with `@nosideeffects`, it is able to inline the first, but not the second call to `__decorate`, this is fine, because it is suspicious code.

If I extern it with `@modifies {arguments}`, then the compiler makes no attempt to remove either call.

I have spent a few hours attempting to add support for `@modifies {arguments}` to the `RemoveUnusedCode` pass, and I've got it mostly working, but I am wondering if anyone else would like to chime in on some design decisions before I start trying to author tests.

Proposed solution:
Allow `@modifies` to accept a parameter name, e.g. `@modifies {target}`

Only change `RemoveUnusedCode` in the simple case where there is only a either a single parameter with `@modifies {arguments}`, or there is only a single `@modifies {variable}`.
In `RemoveUnusedCode`, when it is removing an assignment, it takes into account the `@modifies` tag for determining if it should keep the RHS.
In `RemoveUnusedCode` add logic to `traverseCall` to have it remove the call if the given argument is not used.  Only traverse the call in the continuation of that (if the variable is eventually used).  Only support the simple case where the argument is not computed.

My reasoning for only supporting a single argument is that I don't think there is a mechanism to add a Removable that is conditional on multiple `VarInfo` being removable. Though now that I've written that out, I suppose a class that implements `VarInfo`, but is really just a collection of `VarInfo` under the hood is not unfeasible.

Does this all seem sensible?  Is there something obvious I've missed?

Is there a better layer for this change to exist at (like `allArgsAreUnescapedLocal`)?

Let me know if it would be easier to talk about this in a different forum, or on a draft PR.

Thank you.

David Neil

[John Lenz]

I don't  believe side-effects are the right way to be thinking about this.  Decorators without side-effects would be useless.  This is generally true of any "class construction" function.  Before classes became standard, the compiler need some way to distinguish between side-effects that were *only* class assembly and more general side-effects.  For example, the "GlobalNamespace" class contains this logic:

    private boolean isClassDefiningCall(Node callNode) {
      CodingConvention convention = compiler.getCodingConvention();
      // Look for goog.inherits and J2CL mixin calls
      SubclassRelationship classes = convention.getClassesDefinedByCall(callNode);
      if (classes != null) {
        return true;
      }

      // Look for calls to goog.addSingletonGetter calls.
      String className = convention.getSingletonGetterClassName(callNode);
      return className != null;
    }

 "RemoveUnusedCode" has something similar.

The problem that I see is that somehow this needs to be decorator specific in order for there to be a general solution.  As I've seen a number of declarators that do something like "register this class to do construction" and it wouldn't be happy if they were removed.

But perhaps I'm missing some of the context, are you actually trying to remove "design time" decorators that actually do nothing at runtime and you want everything associated with them to be removed?

--

---
You received this message because you are subscribed to the Google Groups "Closure Compiler Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to closure-compiler-d...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/closure-compiler-discuss/70df98b5-090a-4b13-818a-ef2b791b618fn%40googlegroups.com.

[David Neil]

My goal is to have `class A` entirely removed from the example.
When I did this with `@nosideeffects` (so only removing unused class decorators, not method / property decorators) it led to a 2% decrease in our bundle size, which is significant, especially as some recent design decisions will have us relying on dead code elimination more heavily.

While we could special-case this as part of class setup, it seems more valuable to be able to get this gain in the non-special-case.

If I dot my i's cross my t's correctly, I think this optimization would also work for non-extern functions that provably only modify arguments (though we would need `AmbiguatedFunctionSummary` to update the underlying `Node` when it does side-effect discovery.)