Use better names for captured variables in closures.

pkg/compiler/lib/src/closure.dart

Index: pkg/compiler/lib/src/closure.dart
diff --git a/pkg/compiler/lib/src/closure.dart b/pkg/compiler/lib/src/closure.dart
index 00cb34ca1a62dca2496e1f335ff3f9d295c8ab1c..f8fa7e3e5a44577dfe5eb325412bbba0a4761718 100644
--- a/pkg/compiler/lib/src/closure.dart
+++ b/pkg/compiler/lib/src/closure.dart
@@ -23,7 +23,11 @@ import 'elements/modelx.dart' show
     ElementX,
     LocalFunctionElementX;
 import 'elements/visitor.dart' show ElementVisitor;
-import 'js_backend/js_backend.dart' show JavaScriptBackend;
+import 'js_backend/js_backend.dart' show
+    JavaScriptBackend,
+    NamingScope,
+    NameProposingEntity,
+    ScopeBearingEntity;
 import 'resolution/tree_elements.dart' show TreeElements;
 import 'tokens/token.dart' show Token;
 import 'tree/tree.dart';
@@ -103,7 +107,8 @@ abstract class CapturedVariable implements Element {}
 // TODO(ahe): These classes continuously cause problems.  We need to
 // find a more general solution.
 class ClosureFieldElement extends ElementX
-    implements FieldElement, CapturedVariable {
+    implements FieldElement, CapturedVariable, ScopeBearingEntity,
+        NameProposingEntity {

[sra1, 2015/11/04 02:27:48]

I'm a bit concerned that such an early phase depends on classes in the backend.

Rather than have magical elements that are renamed later, maybe it would be
better pick names up-front and make the fields invisible to selectors.

[herhut, 2015/11/04 15:25:47]

I have renamed NameProposingEntity to JsEntity with an declaredEntity getter.
That is pretty backend independent and just a property of the code.

Likewise, ScopeBearingEntity is now a PrivatelyNamesJsEntity, which reflects
what it is about. Johnni intends to turn this into a full JS Model at some
point.

   /// The [BoxLocal] or [LocalElement] being accessed through the field.
   final Local local;
 
@@ -120,6 +125,9 @@ class ClosureFieldElement extends ElementX
 
   MemberElement get memberContext => closureClass.methodElement.memberContext;
 
+  String proposeName() => local.name;

[sra1, 2015/11/04 04:09:25]

Can this propose that the boxes are named nicely?

=> local is BoxLocal ? 'vars' : local.name


that can generate nice things like

this.vars.count++;

[herhut, 2015/11/04 15:25:47]

No, this is just responsible for the name of the fields in a box, not the box
itself. As the box is a local, this is not handled by the namer but by the JsAst
IIRC. 

[sra1, 2015/11/04 17:59:22]

The field is named after the BoxLocal.
I tried it and it looks nice.

[herhut, 2015/11/05 10:33:47]

Oh yeah, you are right. But it would be nice to give the boxlocal itself a
better name. Anyways, with the changed interface, this is harder to communicate
to the namer now, as it will use the name of the delcared element now, which is
the BoxLocal.

This also breaks the new model somehow, as the BoxLocal is not really a declared
element but synthetic itself.

I'll think about it a little more...

+  NamingScope get namingScope => closureClass.fieldScope;
+
   bool get hasNode => false;
 
   Node get node {
@@ -181,6 +189,7 @@ class ClosureClassElement extends ClassElementX {
   final LocalFunctionElement methodElement;
 
   final List<ClosureFieldElement> _closureFields = <ClosureFieldElement>[];
+  final NamingScope fieldScope = new NamingScope();
 
   ClosureClassElement(this.node,
                       String name,
@@ -239,6 +248,7 @@ class ClosureClassElement extends ClassElementX {
 class BoxLocal extends Local {
   final String name;
   final ExecutableElement executableContext;
+  final NamingScope boxScope = new NamingScope();

[sra1, 2015/11/04 02:27:48]

Is it possible to make this data emphemeral to the closure phase?

[herhut, 2015/11/04 15:25:47]

I have now changed the interface, so that all FieldElements implement
PrivatelyNamedJsEntity and return a scoping key (their special parent class) and
the namer can use that information internally, if it likes, to select the right
scopes. Does that address your concern?

 
   BoxLocal(this.name, this.executableContext);
 }
@@ -246,10 +256,12 @@ class BoxLocal extends Local {
 // TODO(ngeoffray, ahe): These classes continuously cause problems.  We need to
 // find a more general solution.
 class BoxFieldElement extends ElementX
-    implements TypedElement, CapturedVariable, FieldElement {
+    implements TypedElement, CapturedVariable, FieldElement,
+               NameProposingEntity, ScopeBearingEntity {
   final BoxLocal box;
 
-  BoxFieldElement(String name, this.variableElement, BoxLocal box)
+  BoxFieldElement(String name, this.variableElement,
+      BoxLocal box)
       : this.box = box,
         super(name, ElementKind.FIELD, box.executableContext);
 
@@ -257,6 +269,9 @@ class BoxFieldElement extends ElementX
 
   DartType get type => variableElement.type;
 
+  String proposeName() => variableElement.name;

[Siggi Cherem (dart-lang), 2015/11/03 17:21:34]

totally up to you: it seems that all these cases for proposeName() the proposed
name doesn't change over time, so it could be written as a getter instead:

  String proposedName => variableElement.name;

[herhut, 2015/11/04 15:25:47]

Done.

+  NamingScope get namingScope => box.boxScope;
+
   final VariableElement variableElement;
 
   accept(ElementVisitor visitor, arg) {
@@ -532,6 +547,8 @@ class ClosureTranslator extends Visitor {
   ///
   /// Also, the names should be distinct from real field names to prevent
   /// clashes with selectors for those fields.
+  ///
+  /// These names are not used in generated code, just as element name.
   String getBoxFieldName(int id) {
     return "_box_$id";
   }