Add more tests for closures and change closure indexing to be by FunctionExpression or FunctionDecl…

tests/compiler/dart2js/closure/closure_test.dart

 // Copyright (c) 2017, 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.
 
 import 'dart:io' hide Link;
 import 'package:async_helper/async_helper.dart';
 import 'package:compiler/src/closure.dart';
 import 'package:compiler/src/commandline_options.dart';
 import 'package:compiler/src/common.dart';
 import 'package:compiler/src/compiler.dart';
 import 'package:compiler/src/diagnostics/diagnostic_listener.dart';
 import 'package:compiler/src/elements/elements.dart';
 import 'package:compiler/src/elements/entities.dart';
 import 'package:compiler/src/kernel/element_map.dart';
 import 'package:compiler/src/kernel/kernel_backend_strategy.dart';
 import 'package:compiler/src/js_model/locals.dart';
 import 'package:compiler/src/tree/nodes.dart' as ast;
 import 'package:compiler/src/util/util.dart';
 import 'package:expect/expect.dart';
 import '../equivalence/id_equivalence.dart';
 import '../equivalence/id_equivalence_helper.dart';
 import 'package:kernel/ast.dart' as ir;
 
 main(List<String> args) {
   bool verbose = args.contains('-v');
   asyncTest(() async {
     Directory dataDir = new Directory.fromUri(Platform.script.resolve('data'));
     await checkTests(dataDir, computeClosureData, computeKernelClosureData,
         // TODO(johnnniwinther,efortuna): Enable these tests for .dill.
-        skipForKernel: ['captured_variable.dart'],
+        skipForKernel: [
+          'captured_variable.dart',
+          'mixed.dart',
+          'mutations.dart',
+          'nested_closures.dart'
+        ],
         options: [Flags.disableTypeInference],
         verbose: verbose);
   });
 }
 
 /// Compute closure data mapping for [_member] as a [MemberElement].
 ///
 /// Fills [actualMap] with the data and [sourceSpanMap] with the source spans
 /// for the data origin.
 void computeClosureData(
@@ skipping 36 matching lines @@
   ClosureAstComputer(DiagnosticReporter reporter, Map<Id, ActualData> actualMap,
       ResolvedAst resolvedAst, this.closureDataLookup,
       {this.verbose: false})
       : super(reporter, actualMap, resolvedAst) {
     pushMember(resolvedAst.element as MemberElement);
   }
 
   visitFunctionExpression(ast.FunctionExpression node) {
     Entity localFunction = resolvedAst.elements.getFunctionDefinition(node);
     if (localFunction is LocalFunctionElement) {
+      pushMember(localFunction.callMethod);
       pushLocalFunction(node);
       super.visitFunctionExpression(node);
       popLocalFunction();
+      popMember();
     } else {
       super.visitFunctionExpression(node);
     }
   }
 
   @override
   String computeNodeValue(ast.Node node, [AstElement element]) {
     if (element != null && element.isLocal) {
       if (element.isFunction) {
-        return computeObjectValue(element);
+        LocalFunctionElement localFunction = element;
+        return computeObjectValue(localFunction.callMethod);
       } else {
         LocalElement local = element;
         return computeLocalValue(local);
       }
     }
     // TODO(johnniwinther,efortuna): Collect data for other nodes?
     return null;
   }
 
   @override
-  String computeElementValue(AstElement element) {
+  String computeElementValue(covariant MemberElement element) {
     // TODO(johnniwinther,efortuna): Collect data for the member
     // (has thisLocal, has box, etc.).
     return computeObjectValue(element);
   }
 }
 
 /// Kernel IR visitor for computing closure data.
 class ClosureIrChecker extends IrDataExtractor with ComputeValueMixin<ir.Node> {
   final MemberEntity member;
   final ClosureDataLookup<ir.Node> closureDataLookup;
   final KernelToLocalsMap _localsMap;
   final bool verbose;
 
   ClosureIrChecker(
       Map<Id, ActualData> actualMap,
       KernelToElementMapForBuilding elementMap,
       this.member,
       this._localsMap,
       this.closureDataLookup,
       {this.verbose: false})
       : super(actualMap) {
     pushMember(member);
   }
 
   visitFunctionExpression(ir.FunctionExpression node) {
+    ClosureRepresentationInfo info = closureDataLookup.getClosureInfo(node);
+    pushMember(info.callMethod);
     pushLocalFunction(node);
     super.visitFunctionExpression(node);
     popLocalFunction();
+    popMember();
   }
 
   visitFunctionDeclaration(ir.FunctionDeclaration node) {
+    ClosureRepresentationInfo info = closureDataLookup.getClosureInfo(node);
+    pushMember(info.callMethod);
     pushLocalFunction(node);
     super.visitFunctionDeclaration(node);
     popLocalFunction();
+    popMember();
   }
 
   @override
   String computeNodeValue(ir.Node node) {
     if (node is ir.VariableDeclaration) {
       if (node.parent is ir.FunctionDeclaration) {
-        return computeObjectValue(node.parent);
+        ClosureRepresentationInfo info =
+            closureDataLookup.getClosureInfo(node.parent);
+        return computeObjectValue(info.callMethod);
       }
       Local local = _localsMap.getLocalVariable(node);
       return computeLocalValue(local);
     } else if (node is ir.FunctionExpression) {
-      return computeObjectValue(node);
+      ClosureRepresentationInfo info = closureDataLookup.getClosureInfo(node);
+      return computeObjectValue(info.callMethod);
     }
     return null;
   }
 
   @override
   String computeMemberValue(ir.Member node) {
     return computeObjectValue(member);
   }
 }
 
 abstract class ComputeValueMixin<T> {
   bool get verbose;
+  Map<BoxLocal, String> boxNames = <BoxLocal, String>{};
   ClosureDataLookup<T> get closureDataLookup;
   Link<ScopeInfo> scopeInfoStack = const Link<ScopeInfo>();
   ScopeInfo get scopeInfo => scopeInfoStack.head;
-  CapturedScope capturedScope;
+  CapturedScope get capturedScope => capturedScopeStack.head;
+  Link<CapturedScope> capturedScopeStack = const Link<CapturedScope>();
   Link<ClosureRepresentationInfo> closureRepresentationInfoStack =
       const Link<ClosureRepresentationInfo>();
   ClosureRepresentationInfo get closureRepresentationInfo =>
       closureRepresentationInfoStack.isNotEmpty
           ? closureRepresentationInfoStack.head
           : null;
 
   void pushMember(MemberEntity member) {
     scopeInfoStack =
         scopeInfoStack.prepend(closureDataLookup.getScopeInfo(member));
-    capturedScope = closureDataLookup.getCapturedScope(member);
+    capturedScopeStack =
+        capturedScopeStack.prepend(closureDataLookup.getCapturedScope(member));
+    if (capturedScope.requiresContextBox) {
+      boxNames[capturedScope.context] = 'box${boxNames.length}';
+    }
     dump(member);
   }
 
   void popMember() {
     scopeInfoStack = scopeInfoStack.tail;
+    capturedScopeStack = capturedScopeStack.tail;
   }
 
   void pushLocalFunction(T node) {
     closureRepresentationInfoStack = closureRepresentationInfoStack
         .prepend(closureDataLookup.getClosureInfo(node));
     dump(node);
   }
 
   void popLocalFunction() {
     closureRepresentationInfoStack = closureRepresentationInfoStack.tail;
   }
 
   void dump(Object object) {
     if (!verbose) return;
 
     print('object: $object');
     if (object is MemberEntity) {
-      print(' scopeInfo (${scopeInfo.runtimeType})');
-      scopeInfo.forEachBoxedVariable((a, b) => print('  boxed1: $a->$b'));
       print(' capturedScope (${capturedScope.runtimeType})');
       capturedScope.forEachBoxedVariable((a, b) => print('  boxed2: $a->$b'));
     }
     print(
         ' closureRepresentationInfo (${closureRepresentationInfo.runtimeType})');
     closureRepresentationInfo
         ?.forEachCapturedVariable((a, b) => print('  captured: $a->$b'));
     closureRepresentationInfo
         ?.forEachFreeVariable((a, b) => print('  free3: $a->$b'));
     closureRepresentationInfo
         ?.forEachBoxedVariable((a, b) => print('  boxed3: $a->$b'));
   }
 
   /// Compute a string representation of the data stored for [local] in [info].
   String computeLocalValue(Local local) {
     List<String> features = <String>[];
     if (scopeInfo.localIsUsedInTryOrSync(local)) {
       features.add('inTry');
       // TODO(johnniwinther,efortuna): Should this be enabled and checked?
       //Expect.isTrue(capturedScope.localIsUsedInTryOrSync(local));
     } else {
       //Expect.isFalse(capturedScope.localIsUsedInTryOrSync(local));
     }
-    if (scopeInfo.isBoxed(local)) {
+    if (capturedScope.isBoxed(local)) {
       features.add('boxed');
-      Expect.isTrue(capturedScope.isBoxed(local));
-    } else {
-      Expect.isFalse(capturedScope.isBoxed(local));
     }
     if (capturedScope.context == local) {
-      features.add('local');
+      // TODO(johnniwinther): This can't happen!

[Siggi Cherem (dart-lang), 2017/08/28 17:33:45]

nit: rewrite to explain what this means? not sure what is it that it can't
happen.

+      features.add('box');

[Siggi Cherem (dart-lang), 2017/08/28 17:33:45]

maybe use a different string to denote this is an error situation? 'error-box'?

     }
     if (capturedScope is CapturedLoopScope) {
       CapturedLoopScope loopScope = capturedScope;
       if (loopScope.boxedLoopVariables.contains(local)) {
         features.add('loop');
       }
     }
     if (closureRepresentationInfo != null) {
       if (closureRepresentationInfo.createdFieldEntities.contains(local)) {
         features.add('field');
       }
-      if (closureRepresentationInfo.isVariableBoxed(local)) {
-        features.add('variable-boxed');
-      }
     }
     // TODO(johnniwinther,efortuna): Add more info?
     return (features.toList()..sort()).join(',');
   }
 
-  String computeObjectValue(Object object) {
+  String computeObjectValue(MemberEntity member) {
     Map<String, String> features = <String, String>{};
 
     void addLocals(String name, forEach(f(Local local, _))) {
       List<String> names = <String>[];
       forEach((Local local, _) {
         if (local is BoxLocal) {
-          names.add('box');
+          names.add(boxNames[local]);
         } else {
           names.add(local.name);
         }
       });
       String value = names.isEmpty ? null : '[${(names..sort()).join(',')}]';
       if (features.containsKey(name)) {
         Expect.equals(
-            features[name], value, "Inconsistent values for $name on $object.");
+            features[name], value, "Inconsistent values for $name on $member.");
       }
       features[name] = value;
     }
 
-    if (object is MemberEntity) {
-      if (scopeInfo.thisLocal != null) {
-        features['hasThis'] = '';
-      }
-      addLocals('boxed', scopeInfo.forEachBoxedVariable);
-
-      if (capturedScope.requiresContextBox) {
-        features['requiresBox'] = '';
-      }
-      addLocals('boxed', capturedScope.forEachBoxedVariable);
+    if (scopeInfo.thisLocal != null) {
+      features['hasThis'] = '';
     }
+    if (capturedScope.requiresContextBox) {
+      //print('------------- $object from $capturedScope');

[Siggi Cherem (dart-lang), 2017/08/28 17:33:45]

delete

+      features['box'] = boxNames[capturedScope.context];
+    }
+    addLocals('boxed', capturedScope.forEachBoxedVariable);

[Siggi Cherem (dart-lang), 2017/08/28 17:33:45]

an idea: since this and requiresContextBox are correlated, it might be nice to
combine them. I think this can make the comments on the test easier to read.

Instead of:
  box=box0,boxed=[x,y]

We would have something like:

  box=(box0 which holds [x,y])

or:

  box={name: "box0", variables:[x,y]}


thoughts?

 
     if (closureRepresentationInfo != null) {
-      addLocals('boxed', closureRepresentationInfo.forEachBoxedVariable);
-      addLocals('captured', closureRepresentationInfo.forEachCapturedVariable);
       addLocals('free', closureRepresentationInfo.forEachFreeVariable);
     }
 
     StringBuffer sb = new StringBuffer();
     bool needsComma = false;
     for (String name in features.keys.toList()..sort()) {
       String value = features[name];
       if (value != null) {
         if (needsComma) {
           sb.write(',');
         }
         sb.write(name);
         if (value != '') {
           sb.write('=');
           sb.write(value);
         }
         needsComma = true;
       }
     }
     return sb.toString();
   }
 }