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

Poor readability of closures in generated JavaScript code #4003

Closed
rakudrama opened this issue Jul 5, 2012 · 9 comments
Closed

Poor readability of closures in generated JavaScript code #4003

rakudrama opened this issue Jul 5, 2012 · 9 comments
Assignees
Labels
closed-obsolete Closed as the reported issue is no longer relevant P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug web-dart2js

Comments

@rakudrama
Copy link
Member

There are several factors that make code containing closures essentially incomprehensible:

  1. The closures are all called ClosureN for various N. They could have better names. Instead of Closure123 the name could be e.g. containingFunction_Closure_2. The names would still minify the same.
  2. The closures are far from the functions. If they can't be put in the function body at the point of closure creation, they should immediately follow the creating function (or class containing the creating function).
  3. Closed names are obfuscated by long access paths (which causes further obfuscation by CSE). See Issue Basic environment analysis: close over values instead of variables if variable is not assigned #4001.
  4. Closed names are obfuscated by suffixes. Why this.box_0.first_3 when it could be this.vars.first?
  5. Even when the ClosureN names are unavoidable, the essential superclass has a crazy name. The superclass of all closures should be $$.Closure, not $$.Closure176 today and some other number tomorrow.

$$.Closure = {"":
 ["this_0"],
 super: "Closure176",
 $call$0: function() {
  return this.this_0.onLoad$0();
 }
};
...
$$.Closure176 = {"":
 [],
 super: "Object",
 toString$0: function() {
  return 'Closure';
 }
};

I am hoping that real JavaScript functions can be used as closures since then the code could compile to the obvious JavaScript program, using JavaScript's closures. But if that is not possible, the above will still need to be addressed.

@kasperl
Copy link

kasperl commented Sep 3, 2012

Added this to the Later milestone.

@kasperl
Copy link

kasperl commented Oct 17, 2012

Removed this from the Later milestone.

@kasperl
Copy link

kasperl commented Oct 17, 2012

Added this to the Later milestone.

@kasperl
Copy link

kasperl commented May 23, 2013

Added TriageForM5 label.

@kasperl
Copy link

kasperl commented May 28, 2013

Removed TriageForM5 label.

@kasperl
Copy link

kasperl commented Jul 10, 2014

Removed this from the Later milestone.
Added Oldschool-Milestone-Later label.

@kasperl
Copy link

kasperl commented Aug 4, 2014

Removed Oldschool-Milestone-Later label.

@karlklose
Copy link
Contributor

@rakudrama Could you check what is still a problem in dart2js and update/close the bug?

@sigmundch
Copy link
Member

I believe it still is an issue:

  1. is resolved: we do use the name of the enclosing function + class
  2. is not resolved: they are in the same library, but I've seen cases where they are a few hundred lines apart
  3. I am not sure
  4. is better: the variable name is first, but the box name still contains suffixes _box_0.
  5. still appears to be an issue.

@kevmoo kevmoo added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug and removed triaged labels Feb 29, 2016
@matanlurey matanlurey added the closed-obsolete Closed as the reported issue is no longer relevant label Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-obsolete Closed as the reported issue is no longer relevant P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug web-dart2js
Projects
None yet
Development

No branches or pull requests

6 participants