closure-compiler icon indicating copy to clipboard operation
closure-compiler copied to clipboard

Regression; loss of some names in --debug output

Open hills opened this issue 3 years ago • 4 comments

Since upgrading between the most recent releases (closure-compiler-v20210106.jar to closure-compiler-v20210202.jar) I notice a regression and a loss of useful information in the naming out of some --debug output.

Here's an example diff:

-  $pause_rtt$jscomp$2$$ = this.$Tracer$_interval$;
+  $pause_rtt$jscomp$2$$ = this.$JSC$1730__interval$;

-$JSCompiler_prototypeAlias$$.$Plug_prototype$_toggle$ = function() {
+$JSCompiler_prototypeAlias$$.$JSC$1939__toggle$ = function() {

-  }.bind($JSCompiler_StaticMethods_Door_prototype$open$self$$, $cap$jscomp$2$$), 500);
+  }.bind($JSCompiler_StaticMethods_JSC$1694_open$self$$, $cap$jscomp$2$$), 500);

I spent a few minutes trying to make a minimal test case, but time got the better of me as it was hard to stop the optimiser intervening to remove the code(!) Are these examples enough?

The compile flags are:

java -jar closure-compiler-v20210202.jar -O ADVANCED --formatting PRETTY_PRINT --debug

hills avatar Feb 10 '21 16:02 hills

This is a known regression. We recently switched the disambiguate properties implementation. It would be helpful to us if you can describe why the change is problematic for you as this would help us determine if we should try to address this.

concavelenz avatar Feb 11 '21 20:02 concavelenz

I assume I'm using debug in a relatively standard way? Which is to identify code back to its original source after the optimisation steps.

In the case above I can see code is compiled from the 'Tracer', 'Plug' and 'Door' definitions. That's especially useful if I am debugging a problem which only occurs when ADVANCED optimisation is used.

But there's no information with the new revision of the compiler which idenfies that, and so the debug functionlity becomes not very useful if it only gives the 'leaf' name as these sorts of names are often re-used source code.

hills avatar Feb 12 '21 12:02 hills

Another reason this regression is bad; it causes variation in compiled names. Diffing of compiled code was previously possible (and useful), but is no longer practical as there are many false positives with diffs looking like this:

-$JSCompiler_prototypeAlias$$.$JSC$1764__on_incoming$ = function($event$jscomp$11_p$jscomp$7$$) {
+$JSCompiler_prototypeAlias$$.$JSC$1763__on_incoming$ = function($event$jscomp$11_p$jscomp$7$$) {

-  null === this.$_timer$ || $JSCompiler_StaticMethods_JSC$1771_pause$$(this);
+  null === this.$_timer$ || $JSCompiler_StaticMethods_JSC$1770_pause$$(this);

In this example, I added an 'async' keyword to a self-contained function, but it has presumably changed the assignment of these numbers.

Frankly, I think this known regression makes the debug functionality so much less useful that this should be considered a major issue. When you say "if we should try and address this" -- yes. Because it calls into question the entire existence of --debug flag!

hills avatar Mar 16 '21 13:03 hills

This issue is a duplicate of internal issue http://b/179420668

Unfortunately because of the way GitHub issues get pulled into our internal system we cannot actually link them together the way I'd like to do.

Here are highlights from the internal issue:

  1. At least one internal team would also like to improve traceability of these names back to the original code.
  2. Putting this back the way it was just isn't something we can do because of other changes already made, and some in the works.
  3. One possibility would be to leverage sourcemaps in some way here.
  4. The old names weren't as good as you probably thought they were. From @nreid260

The typename baked into [the previous] disambiguated property names is not necessarily the type of the object on which the property is being accessed. The embedded typename comes from one arbitrary element from a cluster of types which cannot be further disambiguated with respect to the original property name.

brad4d avatar Apr 12 '21 21:04 brad4d