Use better names for captured variables in closures.

pkg/compiler/lib/src/closure.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library closureToClassMapper;
 
 import 'common.dart';
 import 'common/names.dart' show
     Identifiers;
 import 'common/resolution.dart' show
     Parsing,
     Resolution;
 import 'common/tasks.dart' show
     CompilerTask;
 import 'compiler.dart' show
     Compiler;
 import 'constants/expressions.dart';
 import 'dart_types.dart';
 import 'elements/elements.dart';
 import 'elements/modelx.dart' show
     BaseFunctionElementX,
     ClassElementX,
     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';
 import 'util/util.dart';
 import 'universe/universe.dart' show
     Universe;
 
 class ClosureTask extends CompilerTask {
   Map<Node, ClosureClassMap> closureMappingCache;
   ClosureTask(Compiler compiler)
@@ skipping 59 matching lines @@
   }
 }
 
 /// Common interface for [BoxFieldElement] and [ClosureFieldElement] as
 /// non-elements.
 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;
 
   ClosureFieldElement(String name,
                       this.local,
                       ClosureClassElement enclosing)
       : super(name, ElementKind.FIELD, enclosing);
 
   /// Use [closureClass] instead.
   @deprecated
   get enclosingElement => super.enclosingElement;
 
   ClosureClassElement get closureClass => super.enclosingElement;
 
   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 {
     throw new SpannableAssertionFailure(local,
         'Should not access node of ClosureFieldElement.');
   }
 
   bool get hasResolvedAst => hasTreeElements;
 
   ResolvedAst get resolvedAst {
@@ skipping 41 matching lines @@
   FunctionType callType;
   /// Node that corresponds to this closure, used for source position.
   final FunctionExpression node;
 
   /**
    * The element for the declaration of the function expression.
    */
   final LocalFunctionElement methodElement;
 
   final List<ClosureFieldElement> _closureFields = <ClosureFieldElement>[];
+  final NamingScope fieldScope = new NamingScope();
 
   ClosureClassElement(this.node,
                       String name,
                       Compiler compiler,
                       LocalFunctionElement closure)
       : this.methodElement = closure,
         super(name,
               closure.compilationUnit,
               // By assigning a fresh class-id we make sure that the hashcode
               // is unique, but also emit closure classes after all other
@@ skipping 38 matching lines @@
   accept(ElementVisitor visitor, arg) {
     return visitor.visitClosureClassElement(this, arg);
   }
 }
 
 /// A local variable that contains the box object holding the [BoxFieldElement]
 /// fields.
 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);
 }
 
 // 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);
 
   DartType computeType(Resolution resolution) => type;
 
   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) {
     return visitor.visitBoxFieldElement(this, arg);
   }
 
   @override
   bool get hasNode => false;
 
   @override
@@ skipping 243 matching lines @@
   /// Generate a unique name for the [id]th closure field, with proposed name
   /// [name].
   ///
   /// The result is used as the name of [ClosureFieldElement]s, and must
   /// therefore be unique to avoid breaking an invariant in the element model
   /// (classes cannot declare multiple fields with the same name).
   ///
   /// Also, the names should be distinct from real field names to prevent
   /// clashes with selectors for those fields.
   String getClosureVariableName(String name, int id) {
     return "_captured_${name}_$id";

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

Why do we still need this if we have a scope?

If we still need it, that is kind of confusing.
Could we change this mechanism to pass the container, and, say, a map from
container to the set of names already in the containser, instead of an id?  That
would be completely local to closure conversion.

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

This is the name of the element, which has to obey to Dart scoping rules not
handled by the namer. I have added a comment.

The namer still needs to understand this in case it does renaming of fields (the
minifying namer for example). I think the field namer has special support for
naming Box Fields already.

   }
 
   /// Generate a unique name for the [id]th box field, with proposed name
   /// [name].
   ///
   /// The result is used as the name of [BoxFieldElement]s, and must
   /// therefore be unique to avoid breaking an invariant in the element model
   /// (classes cannot declare multiple fields with the same name).
   ///
   /// 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";
   }
 
   bool isCapturedVariable(Local element) {
     return _capturedVariableMapping.containsKey(element);
   }
 
   void addCapturedVariable(Node node, Local variable) {
     if (_capturedVariableMapping[variable] != null) {
@@ skipping 582 matching lines @@
 
   String get name => typeVariable.name;
 
   int get hashCode => typeVariable.hashCode;
 
   bool operator ==(other) {
     if (other is! TypeVariableLocal) return false;
     return typeVariable == other.typeVariable;
   }
 }