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

Regression; loss of some names in --debug output #3772

Open
hills opened this issue Feb 10, 2021 · 4 comments
Open

Regression; loss of some names in --debug output #3772

hills opened this issue Feb 10, 2021 · 4 comments

Comments

@hills
Copy link

hills commented Feb 10, 2021

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
@concavelenz
Copy link
Contributor

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.

@hills
Copy link
Author

hills commented Feb 12, 2021

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
Copy link
Author

hills commented Mar 16, 2021

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!

@brad4d
Copy link
Contributor

brad4d commented Apr 12, 2021

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.

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

3 participants