fixes #259, clone ast before mutation

[email protected]

Review URL: https://codereview.chromium.org/1245013005 .

Author: John Messerly

pkg/dev_compiler/lib/src/checker/checker.dart

@@ -1001,7 +1001,11 @@ class CodeChecker extends RecursiveAstVisitor {
     reporter.onError(error);
 
     if (info is CoercionInfo) {
-      assert(CoercionInfo.get(info.node) == null);
+      // TODO(jmesserly): if we're run again on the same AST, we'll produce the
+      // same annotations. This should be harmless. This might go away once
+      // CodeChecker is integrated better with analyzer, as it will know that
+      // checking has already been performed.
+      // assert(CoercionInfo.get(info.node) == null);
       CoercionInfo.set(info.node, info);
     }
   }

pkg/dev_compiler/lib/src/codegen/js_codegen.dart

@@ -2802,6 +2802,8 @@ class JSGenerator extends CodeGenerator {
   }
 
   String generateLibrary(LibraryUnit unit) {
+    // Clone the AST first, so we can mutate it.
+    unit = unit.clone();
     var library = unit.library.element.library;
     var fields = findFieldsNeedingStorage(unit);
     var codegen =

pkg/dev_compiler/lib/src/codegen/reify_coercions.dart

@@ -111,37 +111,31 @@ class CoercionReifier extends analyzer.GeneralizingAstVisitor<Object> {
   // to discharging the collected definitions.
   // Returns the set of new type identifiers added by the reifier
   Map<Identifier, NewTypeIdDesc> reify() {
-    _library.partsThenLibrary.forEach(generateUnit);
+    _library.partsThenLibrary.forEach(visitCompilationUnit);
     return _tm.addedTypes;
   }
 
-  void generateUnit(CompilationUnit unit) {
-    visitCompilationUnit(unit);
-  }
-
   @override
   Object visitExpression(Expression node) {
     var info = CoercionInfo.get(node);
     if (info is InferredTypeBase) {
-      return _visitInferredTypeBase(info);
+      return _visitInferredTypeBase(info, node);
     } else if (info is DownCast) {
-      return _visitDownCast(info);
+      return _visitDownCast(info, node);
     }
     return super.visitExpression(node);
   }
 
   ///////////////// Private //////////////////////////////////
 
-  Object _visitInferredTypeBase(InferredTypeBase node) {
-    var expr = node.node;
+  Object _visitInferredTypeBase(InferredTypeBase node, Expression expr) {
     var success = _inferrer.inferExpression(expr, node.type, <String>[]);
     assert(success);
     expr.visitChildren(this);
     return null;
   }
 
-  Object _visitDownCast(DownCast node) {
-    var expr = node.node;
+  Object _visitDownCast(DownCast node, Expression expr) {
     var parent = expr.parent;
     expr.visitChildren(this);
     Expression newE = _cm.coerceExpression(expr, node.cast);

pkg/dev_compiler/lib/src/compiler.dart

@@ -128,19 +128,10 @@ class BatchCompiler extends AbstractCompiler {
     if (_jsGen != null) {
       var unit = units.first;
       var parts = units.skip(1).toList();
-
-      // TODO(jmesserly): this hack is to avoid compiling the same compilation
-      // unit to JS twice. We mutate the AST, so it's not safe to run more than
-      // once on the same unit.
-      if (unit.getProperty(_propertyName) == true) return;
-      unit.setProperty(_propertyName, true);
-
       _jsGen.generateLibrary(new LibraryUnit(unit, parts));
     }
   }
 
-  static const String _propertyName = 'dev_compiler.BatchCompiler.isCompiled';
-
   void compileHtml(Source source) {
     // TODO(jmesserly): reuse DartScriptsTask instead of copy/paste.
     var contents = context.getContents(source);

pkg/dev_compiler/lib/src/info.dart

@@ -9,6 +9,7 @@ library dev_compiler.src.info;
 import 'package:analyzer/src/generated/ast.dart';
 import 'package:analyzer/src/generated/element.dart';
 import 'package:analyzer/src/generated/error.dart';
+import 'package:analyzer/src/generated/parser.dart';
 
 import 'package:dev_compiler/src/checker/rules.dart';
 import 'package:dev_compiler/src/utils.dart' as utils;
@@ -53,6 +54,43 @@ class LibraryUnit {
     yield* parts;
     yield library;
   }
+
+  /// Creates a clone of this library's AST.
+  LibraryUnit clone() {
+    return new LibraryUnit(
+        _cloneUnit(library), parts.map(_cloneUnit).toList(growable: false));
+  }
+
+  static CompilationUnit _cloneUnit(CompilationUnit oldNode) {
+    var newNode = oldNode.accept(new _AstCloner());
+    ResolutionCopier.copyResolutionData(oldNode, newNode);
+    return newNode;
+  }
+}
+
+class _AstCloner extends AstCloner {
+  void _cloneProperties(AstNode clone, AstNode node) {
+    if (clone != null) {
+      CoercionInfo.set(clone, CoercionInfo.get(node));
+      DynamicInvoke.set(clone, DynamicInvoke.get(node));
+    }
+  }
+
+  @override
+  AstNode cloneNode(AstNode node) {
+    var clone = super.cloneNode(node);
+    _cloneProperties(clone, node);
+    return clone;
+  }
+
+  @override
+  List cloneNodeList(List list) {
+    var clone = super.cloneNodeList(list);
+    for (int i = 0, len = list.length; i < len; i++) {
+      _cloneProperties(clone[i], list[i]);
+    }
+    return clone;
+  }
 }
 
 // The abstract type of coercions mapping one type to another.

pkg/dev_compiler/pubspec.yaml

@@ -6,7 +6,7 @@ description: >
 author: Dart Dev Compiler team <[email protected]>
 homepage: https://github.com/dart-lang/dev_compiler
 dependencies:
-  analyzer: ^0.25.1
+  analyzer: ^0.25.3-alpha.2
   args: ^0.13.0
   cli_util: ^0.0.1
   crypto: ^0.9.0

pkg/dev_compiler/test/codegen/expect/sunflower/dom.js

@@ -1,4 +1,4 @@
-dart_library.library('sunflower/dom', window, /* Imports */[
+dart_library.library('dom', window, /* Imports */[
   "dart_runtime/dart",
   'dart/core'
 ], /* Lazy imports */[

pkg/dev_compiler/test/codegen/expect/syncstar_yield_test.txt

@@ -1,3 +1,5 @@
 // Messages from compiling syncstar_yield_test.dart
+info: [DynamicCast] expected (dynamic) will need runtime check to cast to type String (test/codegen/expect.dart, line 97, col 51)
+info: [DynamicCast] actual (dynamic) will need runtime check to cast to type String (test/codegen/expect.dart, line 97, col 61)
 info: [DynamicInvoke] check(e) requires dynamic invoke (test/codegen/expect.dart, line 372, col 14)
 info: [DynamicCast] p (dynamic) will need runtime check to cast to type num (test/codegen/syncstar_yield_test.dart, line 17, col 28)

pkg/dev_compiler/test/codegen/expect/syncstar_yieldstar_test.txt

@@ -1,4 +1,6 @@
 // Messages from compiling syncstar_yieldstar_test.dart
+info: [DynamicCast] expected (dynamic) will need runtime check to cast to type String (test/codegen/expect.dart, line 97, col 51)
+info: [DynamicCast] actual (dynamic) will need runtime check to cast to type String (test/codegen/expect.dart, line 97, col 61)
 info: [DynamicInvoke] check(e) requires dynamic invoke (test/codegen/expect.dart, line 372, col 14)
 info: [DynamicCast] foo().take(9).toList() (dynamic) will need runtime check to cast to type List<dynamic> (test/codegen/syncstar_yieldstar_test.dart, line 28, col 53)
 info: [DynamicInvoke] foo().take(9).toList() requires dynamic invoke (test/codegen/syncstar_yieldstar_test.dart, line 28, col 53)