Prevented skipping nodes during mergeVariablesIntoClasses (dart-archive#56)

* Prevented skipping nodes during mergeVariablesIntoClasses

Nodes were being removed from the source file as the source file was
being traversed, which was causing problems in some cases. Nodes are now
replaced by EmptyStatements rather than being removed.

* Handled AST removals/replacements using a map instead of directly modifying the AST

Author: derekxu16

lib/base.ts

@@ -282,15 +282,6 @@ export function copyNodeArrayLocation(src: ts.TextRange, dest: ts.NodeArray<any>
   dest.end = src.end;
 }
 
-// Polyfill for ES6 Array.find.
-export function arrayFindPolyfill<T extends ts.Node>(
-    nodeArray: ts.NodeArray<T>, predicate: (node: T) => boolean): T {
-  for (let i = 0; i < nodeArray.length; ++i) {
-    if (predicate(nodeArray[i])) return nodeArray[i];
-  }
-  return null;
-}
-
 export function getAncestor(n: ts.Node, kind: ts.SyntaxKind): ts.Node {
   for (let parent = n; parent; parent = parent.parent) {
     if (parent.kind === kind) return parent;
@@ -454,6 +445,22 @@ export class TranspilerBase {
     this.transpiler.reportError(n, message);
   }
 
+  /**
+   * Prevents this node from being visited.
+   */
+  suppressNode(n: ts.Node) {
+    const emptyNode = ts.createNode(ts.SyntaxKind.EmptyStatement);
+    copyLocation(n, emptyNode);
+    this.replaceNode(n, emptyNode);
+  }
+
+  /**
+   * Effectively replaces original with replacement in the AST.
+   */
+  replaceNode(original: ts.Node, replacement: ts.Node) {
+    this.transpiler.nodeSubstitutions.set(original, replacement);
+  }
+
   visitNode(n: ts.Node): boolean {
     throw new Error('not implemented');
   }

lib/main.ts

@@ -89,6 +89,11 @@ export class Transpiler {
    * Map of import library path to a Set of identifier names being imported.
    */
   imports: Map<String, ImportSummary>;
+  /**
+   * Map containing AST nodes that we have removed or replaced. This is safer than modifying the AST
+   * directly.
+   */
+  nodeSubstitutions: Map<ts.Node, ts.Node> = new Map();
   // Comments attach to all following AST nodes before the next 'physical' token. Track the earliest
   // offset to avoid printing comments multiple times.
   private lastCommentIdx = -1;
@@ -421,6 +426,9 @@ export class Transpiler {
   }
 
   visit(node: ts.Node) {
+    if (this.nodeSubstitutions.has(node)) {
+      node = this.nodeSubstitutions.get(node);
+    }
     if (!node) return;
     let comments = ts.getLeadingCommentRanges(this.currentFile.text, node.getFullStart());
     if (comments) {

lib/merge.ts

@@ -331,7 +331,7 @@ export function normalizeSourceFile(f: ts.SourceFile, fc: FacadeConverter, expli
               clazz.name = declaration.name as ts.Identifier;
               clazz.members = ts.createNodeArray();
               base.copyNodeArrayLocation(declaration, clazz.members);
-              replaceNode(n, clazz);
+              fc.replaceNode(n, clazz);
               classes.set(name, clazz);
             }
 
@@ -347,7 +347,7 @@ export function normalizeSourceFile(f: ts.SourceFile, fc: FacadeConverter, expli
               let type: ts.TypeNode = declaration.type;
               if (ts.isTypeLiteralNode(type)) {
                 if (existingClass) {
-                  removeNode(n);
+                  fc.suppressNode(n);
                 }
                 type.members.forEach((member: ts.TypeElement) => {
                   switch (member.kind) {
@@ -454,32 +454,6 @@ export function normalizeSourceFile(f: ts.SourceFile, fc: FacadeConverter, expli
     return propertyDeclarations;
   }
 
-  function removeFromArray(nodes: ts.NodeArray<ts.Node>, v: ts.Node) {
-    for (let i = 0, len = nodes.length; i < len; ++i) {
-      if (nodes[i] === v) {
-        // Small hack to get around NodeArrays being readonly
-        Array.prototype.splice.call(nodes, i, 1);
-        break;
-      }
-    }
-  }
-
-  function removeNode(n: ts.Node) {
-    let parent = n.parent;
-    switch (parent.kind) {
-      case ts.SyntaxKind.ModuleBlock:
-        let block = <ts.ModuleBlock>parent;
-        removeFromArray(block.statements, n);
-        break;
-      case ts.SyntaxKind.SourceFile:
-        let sourceFile = <ts.SourceFile>parent;
-        removeFromArray(sourceFile.statements, n);
-        break;
-      default:
-        throw 'removeNode not implemented for kind:' + parent.kind;
-    }
-  }
-
   function replaceInArray(nodes: ts.NodeArray<ts.Node>, v: ts.Node, replacement: ts.Node) {
     for (let i = 0, len = nodes.length; i < len; ++i) {
       if (nodes[i] === v) {
@@ -490,23 +464,6 @@ export function normalizeSourceFile(f: ts.SourceFile, fc: FacadeConverter, expli
     }
   }
 
-  function replaceNode(n: ts.Node, replacement: ts.Node) {
-    let parent = n.parent;
-    replacement.parent = parent;
-    switch (parent.kind) {
-      case ts.SyntaxKind.ModuleBlock:
-        let block = <ts.ModuleBlock>parent;
-        replaceInArray(block.statements, n, replacement);
-        break;
-      case ts.SyntaxKind.SourceFile:
-        let sourceFile = <ts.SourceFile>parent;
-        replaceInArray(sourceFile.statements, n, replacement);
-        break;
-      default:
-        throw 'replaceNode not implemented for kind:' + parent.kind;
-    }
-  }
-
   function gatherClasses(n: ts.Node, classes: Map<string, base.ClassLike>) {
     if (ts.isClassExpression(n) || ts.isClassDeclaration(n) || ts.isInterfaceDeclaration(n)) {
       let classDecl = <base.ClassLike>n;
@@ -520,13 +477,13 @@ export function normalizeSourceFile(f: ts.SourceFile, fc: FacadeConverter, expli
           Array.prototype.push.call(existing.members, e);
           e.parent = existing;
         });
-        removeNode(classDecl);
+        fc.suppressNode(classDecl);
       } else {
         classes.set(name, classDecl);
         // Perform other class level post processing here.
       }
     } else if (ts.isModuleDeclaration(n) || ts.isSourceFile(n)) {
-      let moduleClasses: Map<string, base.ClassLike> = new Map();
+      const moduleClasses: Map<string, base.ClassLike> = new Map();
       ts.forEachChild(n, (child) => gatherClasses(child, moduleClasses));
       ts.forEachChild(n, (child) => mergeVariablesIntoClasses(child, moduleClasses));
     } else if (ts.isModuleBlock(n)) {

test/declaration_test.ts

@@ -580,6 +580,31 @@ abstract class XStatic {
 }`);
   });
 
+  it('merges top level variables into classes when they are associated with those classes', () => {
+    expectTranslate(`interface X { a: number; }
+                     interface Y { c: number; }
+
+                     declare var X: {prototype: X, new (): X, b: string};
+                     declare var Y: {prototype: Y, new (): Y, d: string};`)
+        .to.equal(`@JS("X")
+abstract class X {
+  external num get a;
+  external set a(num v);
+  external factory X();
+  external static String get b;
+  external static set b(String v);
+}
+
+@JS("Y")
+abstract class Y {
+  external num get c;
+  external set c(num v);
+  external factory Y();
+  external static String get d;
+  external static set d(String v);
+}`);
+  });
+
   // If the lib.dom.d.ts file is compiled, it creates conflicting interface definitions which causes
   // a problem for variable declarations that we have merged into classes. The main problem was that
   // when the declaration transpiler performed the notSimpleBagOfProperties check, it was checking