apache / royale-compiler

Apache Royale Compiler
https://royale.apache.org/
Apache License 2.0
95 stars 49 forks source link

Invalid output when using `-prevent-rename-public-symbols=false` #200

Closed Harbs closed 2 years ago

Harbs commented 2 years ago

The following code (inside a class called RuntimeOptions):

private static var _client:String;
public static function get client():String{init();return _client;}
public static function set client(value:String):void{init();_client = value;}

when I use the following two compiler options: -export-public-symbols=false -prevent-rename-public-symbols=false

is output as:

/**
 * @private
 * @type {string}
 */
RuntimeOptions._client;
.....
/**
 * @type {string}
 */
RuntimeOptions.client;

RuntimeOptions.get__client = function() {
  RuntimeOptions.init();
  return RuntimeOptions._client;
};

RuntimeOptions.set__client = function(value) {
  RuntimeOptions.init();
  RuntimeOptions._client = value;
};

which gets minified to something like this: client:{get:function(){lV();return zU},set:function(a){lV();zU=a}},

The problem is that RuntimeOptions.client is minimized to pO and that's what is used everywhere in the app and it's always undefined.

I'm not sure if there's a solution to this problem, but if not, static getters/setters should probably get @nocollapse on the variable declaration even if -prevent-rename-public-symbols=false.

joshtynjala commented 2 years ago

As I understand it, Closure compiler doesn't fully understand JS getters/setters, for some reason, so it doesn't know how to properly rename them consistently.

In general, it's probably best to avoid using the -prevent-rename-public-symbols=false compiler option entirely, and use some combination of the other, more fine-grained "prevent-rename-public-" options, so that you can specifically exclude accessors.

joshtynjala commented 2 years ago

Okay, so I determined that you are right, and @nocollapse is always needed for static accessors. Closure seems to properly handle accessors on the prototype without @nocollapse, but I guess it follows different rules for how to handle static variables and accessors.

joshtynjala commented 2 years ago

In general, it's probably best to avoid using the -prevent-rename-public-symbols=false compiler option entirely

So maybe -prevent-rename-public-symbols=false isn't unsafe after all. When I gave this advice, I was remembering that accessors caused me a lot of trouble for a while, and Google specifically mentioned it in their Closure compiler docs. However, it had slipped my mind that I had found a workaround to get renaming to work for accessors. I guess the issue that you ran into was only an edge case that I hadn't properly tested. So, with that in mind, you should continue to aim to use -prevent-rename-public-symbols=false. If you run into further issues, I'll try to get them fixed.