Repository: google/closure-compiler
Pull request #1071 (2015-08-12 20:07:56)

CommonJS: Remove redundant namespace declaration.
Namespace should already be declared and initialized by `goog.provide`. So instead of emitting this:

```
goog.provide("module$libs$foo");
var module$libs$foo = {};
module$libs$foo.sayHelloInEnglish = function() {
  return "Hello";
};
```
this should be emitted:
```
goog.provide("module$libs$foo");
module$libs$foo.sayHelloInEnglish = function() {
  return "Hello";
};
```

This is also consistent with the way "normal" Google Closure modules are written.

Adjusted the tests to match changes.

See the mailing list discussion: https://groups.google.com/forum/#!topic/closure-compiler-discuss/t8u_YhL2Onk

dimvar: Looks fine. Only a minor comment.
mneise: Ah, good to know. Made the changes as suggested. Thank you for reviewing :smiley: 
dimvar: LGTM. One more minor thing to fix and then i'll merge it.

Patch:
diff --git a/src/com/google/javascript/jscomp/Compiler.java b/src/com/google/javascript/jscomp/Compiler.java
index ff045abd7..ee206c8df 100644
--- a/src/com/google/javascript/jscomp/Compiler.java
+++ b/src/com/google/javascript/jscomp/Compiler.java
@@ -222,12 +222,11 @@ public class Compiler extends AbstractCompiler {
       DiagnosticType.error("JSC_OPTIMIZE_LOOP_ERROR",
           "Exceeded max number of code motion iterations: {0}");
 
-  private final CompilerExecutor compilerExecutor = new CompilerExecutor();
-
+  private static final CompilerExecutor compilerExecutor = new CompilerExecutor();
   /**
    * Logger for the whole com.google.javascript.jscomp domain -
    * setting configuration for this logger affects all loggers
-   * in other classes within the compiler.
+   *  in other classes within the compiler.
    */
   private static final Logger logger =
       Logger.getLogger("com.google.javascript.jscomp");
@@ -640,7 +639,7 @@ public class Compiler extends AbstractCompiler {
     compilerExecutor.setTimeout(timeout);
   }
 
-  <T> T runInCompilerThread(Callable<T> callable) {
+  <T> T runInCompilerThread(final Callable<T> callable) {
     return compilerExecutor.runInCompilerThread(callable, options != null && options.tracer.isOn());
   }
 
diff --git a/src/com/google/javascript/jscomp/DiagnosticGroups.java b/src/com/google/javascript/jscomp/DiagnosticGroups.java
index 46096f858..a790486f7 100644
--- a/src/com/google/javascript/jscomp/DiagnosticGroups.java
+++ b/src/com/google/javascript/jscomp/DiagnosticGroups.java
@@ -96,7 +96,7 @@ public class DiagnosticGroups {
       + "es5Strict, externsValidation, fileoverviewTags, globalThis, "
       + "inferredConstCheck, internetExplorerChecks, invalidCasts, "
       + "misplacedTypeAnnotation, missingGetCssName, missingProperties, "
-      + "missingProvide, missingRequire, missingReturn, msgDescriptions"
+      + "missingProvide, missingRequire, missingReturn, "
       + "newCheckTypes, nonStandardJsDocs, reportUnknownTypes, suspiciousCode, "
       + "strictModuleDepCheck, typeInvalidation, "
       + "undefinedNames, undefinedVars, unknownDefines, unnecessaryCasts, uselessCode, "
@@ -402,10 +402,6 @@ public class DiagnosticGroups {
       DiagnosticGroups.registerGroup("duplicateMessage",
           JsMessageVisitor.MESSAGE_DUPLICATE_KEY);
 
-  public static final DiagnosticGroup MESSAGE_DESCRIPTIONS =
-      DiagnosticGroups.registerGroup("msgDescriptions",
-          JsMessageVisitor.MESSAGE_HAS_NO_DESCRIPTION);
-
   public static final DiagnosticGroup MISPLACED_TYPE_ANNOTATION =
       DiagnosticGroups.registerGroup("misplacedTypeAnnotation",
           CheckJSDoc.DISALLOWED_MEMBER_JSDOC,
diff --git a/src/com/google/javascript/jscomp/ProcessCommonJSModules.java b/src/com/google/javascript/jscomp/ProcessCommonJSModules.java
index 491a6b1a8..d0fe3cce1 100644
--- a/src/com/google/javascript/jscomp/ProcessCommonJSModules.java
+++ b/src/com/google/javascript/jscomp/ProcessCommonJSModules.java
@@ -244,16 +244,14 @@ public final class ProcessCommonJSModules implements CompilerPass {
     private void processExports(Node script, String moduleName) {
       if (hasOneTopLevelModuleExportAssign()) {
         // One top-level assign: transform to
-        // /** @const */ var moduleName = rhs
+        // moduleName = rhs
         Node ref = moduleExportRefs.get(0);
         Node newName = IR.name(moduleName);
         newName.putProp(Node.ORIGINALNAME_PROP, ref.getQualifiedName());
 
-        Node newVar = IR.var(newName)
-            .copyInformationFromForTree(ref.getParent());
         Node rhsValue = ref.getNext().detachFromParent();
-        newVar.getFirstChild().addChildToFront(rhsValue);
-        newVar.setJSDocInfo(NodeUtil.createConstantJsDoc());
+        Node newExprResult = IR.exprResult(IR.assign(newName, rhsValue)
+          .copyInformationFromForTree(ref.getParent()));
 
         // If the rValue is an object literal, check each property to see if
         // it's an alias, and if it is, copy the annotation over.
@@ -279,20 +277,14 @@ public final class ProcessCommonJSModules implements CompilerPass {
 
         Node assign = ref.getParent();
         Node exprResult = assign.getParent();
-        script.replaceChild(exprResult, newVar);
+        script.replaceChild(exprResult, newExprResult);
         return;
       }
 
       if (!hasExportLValues()) {
         // Transform to:
         //
-        // /** @const */ var moduleName = {};
         // moduleName.prop0 = 0; // etc.
-        //
-        // We consider the 0-ref case a special case of this.
-        Node newVar = injectExportsObject(script, moduleName);
-        newVar.setJSDocInfo(NodeUtil.createConstantJsDoc());
-
         for (Node ref : Iterables.concat(moduleExportRefs, exportRefs)) {
           Node newRef = IR.name(moduleName).copyInformationFrom(ref);
           newRef.putProp(Node.ORIGINALNAME_PROP, ref.getQualifiedName());
@@ -301,23 +293,19 @@ public final class ProcessCommonJSModules implements CompilerPass {
         return;
       }
 
-      // The general case:
-      // At the beginning, add the stanza:
-      // var moduleName = {}; var moduleName$$exports = moduleName;
       // Transform module.exports to moduleName
-      // Transform exports to moduleName$$exports
-      Node exportsNode = injectExportsObject(script, moduleName);
-
       for (Node ref : moduleExportRefs) {
         Node newRef = IR.name(moduleName).copyInformationFrom(ref);
         ref.getParent().replaceChild(ref, newRef);
       }
 
+      // Transform exports to exports$$moduleName and set to point
+      // to module namespace: exports$$moduleName = moduleName;
       if (!exportRefs.isEmpty()) {
         String aliasName = "exports$$" + moduleName;
         Node aliasNode = IR.var(IR.name(aliasName), IR.name(moduleName))
             .copyInformationFromForTree(script);
-        script.addChildAfter(aliasNode, exportsNode);
+        script.addChildToFront(aliasNode);
 
         for (Node ref : exportRefs) {
           ref.putProp(Node.ORIGINALNAME_PROP, ref.getString());
@@ -326,17 +314,6 @@ public final class ProcessCommonJSModules implements CompilerPass {
       }
     }
 
-    /**
-     * Creates an exports object for this module.
-     * var moduleName = {};
-     */
-    private Node injectExportsObject(Node script, String moduleName) {
-      Node varNode = IR.var(IR.name(moduleName), IR.objectlit())
-          .copyInformationFromForTree(script);
-      script.addChildToFront(varNode);
-      return varNode;
-    }
-
     /**
      * Recognize export pattern [1] (see above).
      */


Test patch:
diff --git a/test/com/google/javascript/jscomp/ProcessCommonJSModulesTest.java b/test/com/google/javascript/jscomp/ProcessCommonJSModulesTest.java
index e2475e16a..7ec1936eb 100644
--- a/test/com/google/javascript/jscomp/ProcessCommonJSModulesTest.java
+++ b/test/com/google/javascript/jscomp/ProcessCommonJSModulesTest.java
@@ -68,7 +68,6 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
     testModules(
         "var name = require('other');" + "name()",
         "goog.provide('module$test');"
-            + "var module$test = {};"
             + "goog.require('module$other');"
             + "var name$$module$test = module$other;"
             + "name$$module$test();");
@@ -80,7 +79,6 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
             SourceFile.fromCode(
                 "test/sub.js", "var name = require('mod/name');" + "(function() { name(); })();")),
         "goog.provide('module$test$sub');"
-            + "var module$test$sub = {};"
             + "goog.require('module$mod$name');"
             + "var name$$module$test$sub = module$mod$name;"
             + "(function() { name$$module$test$sub(); })();");
@@ -91,7 +89,6 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
     testModules(
         "var name = require('other');" + "exports.foo = 1;",
         "goog.provide('module$test');"
-            + "/** @const */ var module$test = {};"
             + "goog.require('module$other');"
             + "var name$$module$test = module$other;"
             + "module$test.foo = 1;");
@@ -100,7 +97,7 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
         "goog.provide('module$test');"
             + "goog.require('module$other');"
             + "var name$$module$test = module$other;"
-            + "/** @const */ var module$test = function () {};");
+            + "module$test = function () {};");
   }
 
   public void testPropertyExports() {
@@ -108,7 +105,6 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
     testModules(
         "exports.one = 1;" + "module.exports.obj = {};" + "module.exports.obj.two = 2;",
         "goog.provide('module$test');"
-            + "/** @const */ var module$test = {};"
             + "module$test.one = 1;"
             + "module$test.obj = {};"
             + "module$test.obj.two = 2;");
@@ -119,7 +115,6 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
     testModules(
         "exports.one = 1;" + "module.exports = {};",
         "goog.provide('module$test');"
-            + "var module$test = {};"
             + "var exports$$module$test = module$test;"
             + "exports$$module$test.one = 1;"
             + "module$test = {};");
@@ -130,7 +125,6 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
     testModules(
         "var a = 1, b = 2;" + "(function() { var a; b = 4})()",
         "goog.provide('module$test');"
-            + "var module$test = {};"
             + "var a$$module$test = 1, b$$module$test = 2;"
             + "(function() { var a; b$$module$test = 4})();");
   }
@@ -140,7 +134,6 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
     testModules(
         "var name = require('other'); exports.foo = 1;",
         "goog.provide('module$test_test');"
-            + "var module$test_test = {};"
             + "goog.require('module$other');"
             + "var name$$module$test_test = module$other;"
             + "module$test_test.foo = 1;");
@@ -151,7 +144,6 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
     testModules(
         "var name = require('../other'); exports.bar = 1;",
         "goog.provide('module$foo$index');"
-            + "var module$foo$index = {};"
             + "goog.require('module$other');"
             + "var name$$module$foo$index = module$other;"
             + "module$foo$index.bar = 1;");
@@ -161,7 +153,7 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
     setFilename("foo/bar");
     testModules(
         "var name = require('other');",
-        "goog.provide('module$foo$bar'); var module$foo$bar = {};"
+        "goog.provide('module$foo$bar');"
             + "goog.require('module$other');"
             + "var name$$module$foo$bar = module$other;");
     ProcessEs6ModulesTest.testModules(
@@ -170,7 +162,6 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
             SourceFile.fromCode("foo/name.js", ""),
             SourceFile.fromCode("foo/bar.js", "var name = require('./name');")),
         "goog.provide('module$foo$bar');"
-            + "var module$foo$bar = {};"
             + "goog.require('module$foo$name');"
             + "var name$$module$foo$bar = module$foo$name;");
   }
@@ -182,13 +173,13 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
         "module.exports = foo;",
         "goog.provide('module$test');" +
         "var foo$$module$test=function(module){module.exports={}};" +
-        "var module$test=foo$$module$test");
+        "module$test=foo$$module$test");
     testModules(
         "var foo = function () {var module = {};module.exports = {};};" +
         "module.exports = foo;",
         "goog.provide('module$test');" +
         "var foo$$module$test=function(){var module={};module.exports={}};" +
-        "var module$test=foo$$module$test");
+        "module$test=foo$$module$test");
     testModules(
         "var foo = function () {if (true) var module = {};" +
         "module.exports = {};};" +
@@ -196,7 +187,7 @@ public final class ProcessCommonJSModulesTest extends CompilerTestCase {
         "goog.provide('module$test');" +
         "var foo$$module$test=function(){if(true)var module={};" +
         "module.exports={}};" +
-        "var module$test=foo$$module$test");
+        "module$test=foo$$module$test");
   }
 
   public void testSortInputs() throws Exception {