Fix NPEs in ProcessCommonJSModules caused by multiple var declarations

Closes #2579 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=163482326
google · Jul 28, 2017 · 8fc8229 · 8fc8229
1 parent dc1458f
commit 8fc8229
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 10 deletions.
diff --git a/src/com/google/javascript/jscomp/ProcessCommonJSModules.java b/src/com/google/javascript/jscomp/ProcessCommonJSModules.java
@@ -830,12 +830,21 @@ private void visitExport(NodeTraversal t, Node export) {
       if (root.matchesQualifiedName("module.exports")
           && rValue != null
           && t.getScope().getVar("module.exports") == null
-          && root.getParent().isAssign()
-          && root.getParent().getParent().isExprResult()) {
-        // Rewrite "module.exports = foo;" to "var moduleName = foo;"
-        Node parent = root.getParent();
-        Node var = IR.var(updatedExport, rValue.detach()).useSourceInfoFrom(root.getParent());
-        parent.getParent().replaceWith(var);
+          && root.getParent().isAssign()) {
+        if (root.getParent().getParent().isExprResult()) {
+          // Rewrite "module.exports = foo;" to "var moduleName = foo;"
+          Node parent = root.getParent();
+          Node var = IR.var(updatedExport, rValue.detach()).useSourceInfoFrom(root.getParent());
+          parent.getParent().replaceWith(var);
+        } else if (root.getNext() != null && root.getNext().isName() && rValueVar.isGlobal()) {
+          // This is a where a module export assignment is used in a complex expression.
+          // Before: `SOME_VALUE !== undefined && module.exports = SOME_VALUE`
+          // After: `SOME_VALUE !== undefined && module$name`
+          root.getParent().replaceWith(updatedExport);
+        } else {
+          // Other references to "module.exports" are just replaced with the module name.
+          export.replaceWith(updatedExport);
+        }
       } else {
         // Other references to "module.exports" are just replaced with the module name.
         export.replaceWith(updatedExport);
@@ -1062,6 +1071,13 @@ private void updateNameReference(
         case VAR:
         case LET:
         case CONST:
+          // Multiple declaration - needs split apart.
+          if (parent.getChildCount() > 1) {
+            splitMultipleDeclarations(parent);
+            parent = nameRef.getParent();
+            newNameDeclaration = t.getScope().getVar(newName);
+          }
+
           if (newNameIsQualified) {
             // Refactor a var declaration to a getprop assignment
             Node getProp = NodeUtil.newQName(compiler, newName, nameRef, originalName);
@@ -1078,6 +1094,14 @@ private void updateNameReference(
             }
           } else if (newNameDeclaration != null) {
             // Variable is already defined. Convert this to an assignment.
+            // If the variable declaration has no initialization, we simply
+            // remove the node. This can occur when the variable which is exported
+            // is declared in an outer scope but assigned in an inner one.
+            if (!nameRef.hasChildren()) {
+              parent.detachFromParent();
+              break;
+            }
+
             Node name = NodeUtil.newName(compiler, newName, nameRef, originalName);
             Node assign = IR.assign(name, nameRef.removeFirstChild());
             JSDocInfo info = parent.getJSDocInfo();
@@ -1382,5 +1406,14 @@ private void fixTypeNode(NodeTraversal t, Node typeNode) {
         fixTypeNode(t, child);
       }
     }
+
+    private void splitMultipleDeclarations(Node var) {
+      checkState(NodeUtil.isNameDeclaration(var));
+      while (var.getSecondChild() != null) {
+        Node newVar = new Node(var.getToken(), var.removeFirstChild());
+        newVar.useSourceInfoFrom(var);
+        var.getParent().addChildBefore(newVar, var);
+      }
+    }
   }
 }
diff --git a/test/com/google/javascript/jscomp/ProcessCommonJSModulesTest.java b/test/com/google/javascript/jscomp/ProcessCommonJSModulesTest.java
@@ -190,13 +190,12 @@ public void testVarRenaming() {
     testModules(
         "test.js",
         LINE_JOINER.join(
-            "module.exports = {};",
-            "var a = 1, b = 2;",
-            "(function() { var a; b = 4})();"),
+            "module.exports = {};", "var a = 1, b = 2;", "(function() { var a; b = 4})();"),
         LINE_JOINER.join(
             "goog.provide('module$test');",
             "/** @const */ var module$test={};",
-            "var a$$module$test = 1, b$$module$test = 2;",
+            "var a$$module$test = 1;",
+            "var b$$module$test = 2;",
             "(function() { var a; b$$module$test = 4})();"));
   }
 
@@ -868,4 +867,44 @@ public void testIssue2510() {
             "}",
             "module$test.a = 1"));
   }
+
+  public void testIssue2450() {
+    testModules(
+        "test.js",
+        LINE_JOINER.join(
+            "var BCRYPT_BLOCKS = 8,",
+            "    BCRYPT_HASHSIZE = 32;",
+            "",
+            "module.exports = {",
+            "  BLOCKS: BCRYPT_BLOCKS,",
+            "  HASHSIZE: BCRYPT_HASHSIZE,",
+            "};"),
+        LINE_JOINER.join(
+            "goog.provide('module$test');",
+            "/** @const */ var module$test={};",
+            "module$test.BLOCKS = 8;",
+            "module$test.HASHSIZE = 32;"));
+  }
+
+  public void testWebpackAmdPattern() {
+    testModules(
+        "test.js",
+        LINE_JOINER.join(
+            "var __WEBPACK_AMD_DEFINE_ARRAY__, __WEBPACK_AMD_DEFINE_RESULT__;",
+            "!(__WEBPACK_AMD_DEFINE_ARRAY__ = [__webpack_require__(1), __webpack_require__(2)],",
+            "      __WEBPACK_AMD_DEFINE_RESULT__ = function (b, c) {",
+            "          console.log(b, c.exportA, c.exportB);",
+            "      }.apply(exports, __WEBPACK_AMD_DEFINE_ARRAY__),",
+            "    __WEBPACK_AMD_DEFINE_RESULT__ !== undefined",
+            "    && (module.exports = __WEBPACK_AMD_DEFINE_RESULT__));"),
+        LINE_JOINER.join(
+            "goog.provide('module$test');",
+            "var module$test = {};",
+            "var __WEBPACK_AMD_DEFINE_ARRAY__$$module$test;",
+            "!(__WEBPACK_AMD_DEFINE_ARRAY__$$module$test = ",
+            "    [__webpack_require__(1), __webpack_require__(2)],",
+            "    module$test = function(b,c){console.log(b,c.exportA,c.exportB)}",
+            "        .apply(module$test,__WEBPACK_AMD_DEFINE_ARRAY__$$module$test),",
+            "    module$test!==undefined && module$test)"));
+  }
 }