Warn on a goog.module.get without a goog.require in the top-level of a script

lauraharker · copybara-github · commit fdda0c38a161 · 2025-02-19T10:13:13.000-08:00
Normally the compiler does not enforce goog.requires for goog.module.get, as they are allowed as a way to break dependency cycles between files: a goog.module.get is acceptable if within some function body, that may not be executed until the retrieved module is later loaded.

However, when a goog.module.get executes immediately upon script load (so before any other provides/modules can load), it does need a goog.require.

PiperOrigin-RevId: 728717544
diff --git a/src/com/google/javascript/jscomp/CheckMissingRequires.java b/src/com/google/javascript/jscomp/CheckMissingRequires.java
@@ -91,12 +91,26 @@ public class CheckMissingRequires extends AbstractModuleCallback implements Comp
           "''{0}'' references the name of a module without goog.declareLegacyNamespace(), which "
               + "is not actually defined. Use goog.module.get() instead.");
 
+  public static final DiagnosticType MISSING_REQUIRE_FOR_GOOG_MODULE_GET =
+      DiagnosticType.warning(
+          "JSC_MISSING_REQUIRE_FOR_GOOG_MODULE_GET",
+          "''{0}'' references a namespace which was not required by this file.\n"
+              + "Please add a goog.require.");
+
   /** The set of template parameter names found so far in the file currently being checked. */
   private final LinkedHashSet<String> templateParamNames = new LinkedHashSet<>();
 
   /** The mapping from Closure namespace into the module that provides it. */
   private final ImmutableMap<String, ModuleMetadata> moduleByNamespace;
 
+  /**
+   * Tracks how many "control flow scopes" we've entered, starting at 0
+   *
+   * <p>Where a "control flow scope" is here defined as any scope /except/ for either the global
+   * control flow scope, or an IIFE or goog.scope body within the global control flow scope.
+   */
+  private int controlFlowScopeDepth = 0;
+
   public CheckMissingRequires(AbstractCompiler compiler, ModuleMetadataMap moduleMetadataMap) {
     super(compiler, moduleMetadataMap);
     this.moduleByNamespace = moduleMetadataMap.getModulesByGoogNamespace();
@@ -110,6 +124,9 @@ public void process(Node externs, Node root) {
   @Override
   public boolean shouldTraverse(
       NodeTraversal t, Node n, @Nullable ModuleMetadata currentModule, Node scopeRoot) {
+    if (NodeUtil.isValidCfgRoot(n) && !isInGlobalControlFlowScope(n)) {
+      controlFlowScopeDepth++;
+    }
     if (currentModule == null) {
       return true;
     }
@@ -119,9 +136,43 @@ public boolean shouldTraverse(
     return true;
   }
 
+  /**
+   * Takes a valid "control flow root", as defined by {@link NodeUtil#isValidCfgRoot}, and returns
+   * whether it creates a new "control flow scope" from the parent scope (the global scope).
+   */
+  private boolean isInGlobalControlFlowScope(Node n) {
+    switch (n.getToken()) {
+      case SCRIPT:
+      case ROOT:
+        return true;
+      case MODULE_BODY:
+      case BLOCK: // class static blocks
+        return false;
+      case FUNCTION:
+        if (controlFlowScopeDepth > 0) {
+          return false;
+        }
+        // Check for functions invoked immediately within the global scope - so IIFEs or
+        // goog.scope callees.
+        if (isGoogScopeBody(NodeUtil.getFunctionBody(n))) {
+          return true;
+        }
+        return isIIFE(n);
+      default:
+        throw new AssertionError("Unexpected control flow root: " + n);
+    }
+  }
+
+  private static boolean isIIFE(Node n) {
+    return n.isFunction() && n.getParent().isCall() && n.isFirstChildOf(n.getParent());
+  }
+
   @Override
   public void visit(
       NodeTraversal t, Node n, @Nullable ModuleMetadata currentModule, @Nullable Node scopeRoot) {
+    if (NodeUtil.isValidCfgRoot(n) && !isInGlobalControlFlowScope(n)) {
+      controlFlowScopeDepth--;
+    }
     if (currentModule != null && n == currentModule.rootNode()) {
       // For this pass, template parameter names are only meaningful inside the file defining them.
       templateParamNames.clear();
@@ -145,6 +196,10 @@ private void visitNode(NodeTraversal t, Node n, ModuleMetadata currentModule) {
     if (n.isName() && !n.getString().isEmpty()) {
       visitMaybeDeclaration(t, n, currentModule);
     }
+
+    if (n.isCall() && controlFlowScopeDepth == 0) {
+      visitMaybeGoogModuleGet(t, n, currentModule);
+    }
   }
 
   private void visitJsDocInfo(NodeTraversal t, ModuleMetadata currentModule, JSDocInfo info) {
@@ -197,9 +252,44 @@ private void visitJsDocExpr(
     }
   }
 
+  private static final QualifiedName GOOG_SCOPE = QualifiedName.of("goog.scope");
+
+  private static boolean isGoogScopeBody(Node hoistScopeRoot) {
+    return hoistScopeRoot.isBlock()
+        && hoistScopeRoot.getParent().isFunction()
+        && hoistScopeRoot.getGrandparent().isCall()
+        && GOOG_SCOPE.matches(hoistScopeRoot.getGrandparent().getFirstChild());
+  }
+
+  /**
+   * Check for invalid goog.module.get calls executed on script load
+   *
+   * <p>We don't check for goog.module.get within function bodies though (except for goog.scope
+   * since that's basically an IIFE) - it's possible that the module will have been loaded by the
+   * time the goog.module.get is called even though there's no strong require. It's up to the caller
+   * to ensure that it's loaded.
+   */
+  private void visitMaybeGoogModuleGet(
+      NodeTraversal t, Node googModuleGet, ModuleMetadata currentFile) {
+    checkState(googModuleGet.isCall());
+    if (!NodeUtil.isGoogModuleGetCall(googModuleGet)) {
+      return;
+    }
+    String importedNamespace = googModuleGet.getSecondChild().getString();
+    ModuleMetadata requiredFile = moduleByNamespace.get(importedNamespace);
+    if (requiredFile == null) {
+      // goog.module.get of a non-existing namespace is an error in another pass.
+      return;
+    }
+    if (!hasAcceptableRequire(
+        currentFile, QualifiedName.of(importedNamespace), requiredFile, Strength.WEAK_TYPE)) {
+      t.report(googModuleGet, MISSING_REQUIRE_FOR_GOOG_MODULE_GET, importedNamespace);
+    }
+  }
+
   private void visitMaybeDeclaration(NodeTraversal t, Node n, ModuleMetadata currentFile) {
     if (!currentFile.isModule()) {
-      // This check only makes sense in goog.module files.
+      // This check only makes sense in goog.module files
       return;
     }
 
diff --git a/test/com/google/javascript/jscomp/CheckMissingRequiresTest.java b/test/com/google/javascript/jscomp/CheckMissingRequiresTest.java
@@ -1310,6 +1310,174 @@ public void tesNonLegacyGoogModule_inScript_required_noWarning() {
         "goog.provide('test.b'); goog.require('test.a'); console.log(test.a.x);");
   }
 
+  @Test
+  public void testWarning_googScope_googModuleGet_noRequire() {
+    checkRequireForGoogModuleGetWarning(
+        "foo.bar.Baz",
+        lines(
+            "goog.module('foo.bar.Baz');", //
+            "/** @constructor */",
+            "exports = function() {}"),
+        lines(
+            "goog.provide('test');", //
+            "",
+            "goog.scope(function() {",
+            "const Baz = goog.module.get('foo.bar.Baz');",
+            "exports.fn = function() { new Baz(); }",
+            "});"));
+  }
+
+  @Test
+  public void testWarning_script_googModuleGet_inProvideInitialization_noRequire() {
+    checkRequireForGoogModuleGetWarning(
+        "foo.bar.Baz",
+        lines(
+            "goog.module('foo.bar.Baz');", //
+            "/** @constructor */",
+            "exports.Boo = function() {}"),
+        lines(
+            "goog.provide('test');", //
+            "",
+            "test.boo = new (goog.module.get('foo.bar.Baz').Boo)();"));
+  }
+
+  @Test
+  public void testWarning_script_googModuleGet_inScriptIfBlock_noRequire() {
+    checkRequireForGoogModuleGetWarning(
+        "foo.bar.Baz",
+        lines(
+            "goog.module('foo.bar.Baz');", //
+            "/** @constructor */",
+            "exports.Boo = function() {}"),
+        lines(
+            "goog.provide('test');", //
+            "",
+            "if (true) { console.log(new (goog.module.get('foo.bar.Baz').Boo)()); }"));
+  }
+
+  @Test
+  public void testWarning_script_googModuleGet_inScript_inTopLevelIIFE_noRequire() {
+    checkRequireForGoogModuleGetWarning(
+        "foo.bar.Baz",
+        lines(
+            "goog.module('foo.bar.Baz');", //
+            "/** @constructor */",
+            "exports.Boo = function() {}"),
+        lines(
+            "goog.provide('test');", //
+            "",
+            "(() => console.log(new (goog.module.get('foo.bar.Baz').Boo)()))();"));
+  }
+
+  @Test
+  public void testWarning_googScope_googModuleGet_withPropAccess_noRequire() {
+    checkRequireForGoogModuleGetWarning(
+        "foo.bar.Baz",
+        lines(
+            "goog.module('foo.bar.Baz');", //
+            "/** @constructor */",
+            "exports.Boo = function() {}"),
+        lines(
+            "goog.provide('test');", //
+            "",
+            "goog.scope(function() {",
+            "const Woo = goog.module.get('foo.bar.Baz').Boo.Goo.Woo;",
+            "exports.fn = function() { new Woo(); }",
+            "});"));
+  }
+
+  @Test
+  public void testNoWarning_googScope_googModuleGet_hasRequire() {
+    checkNoWarning(
+        lines(
+            "goog.module('foo.bar.Baz');", //
+            "/** @constructor */",
+            "exports = function() {}"),
+        lines(
+            "goog.provide('test');", //
+            "goog.require('foo.bar.Baz');",
+            "",
+            "goog.scope(function() {",
+            "const Baz = goog.module.get('foo.bar.Baz');",
+            "function fn() { new Baz(); }",
+            "});"));
+  }
+
+  @Test
+  public void testWarning_googScope_googModuleGet_IIFE_noRequire() {
+    checkRequireForGoogModuleGetWarning(
+        "foo.bar.Baz",
+        lines(
+            "goog.module('foo.bar.Baz');", //
+            "/** @constructor */",
+            "exports.Boo = function() {}"),
+        lines(
+            "goog.provide('test');", //
+            "",
+            "goog.scope(function() {",
+            "  (() => console.log(new (goog.module.get('foo.bar.Baz').Boo)()))();",
+            "});"));
+  }
+
+  @Test
+  public void testWarning_script_googModuleGet_inFunctionInScript_noRequire_noWarning() {
+    checkNoWarning(
+        lines(
+            "goog.module('foo.bar.Baz');", //
+            "/** @constructor */",
+            "exports.Boo = function() {}"),
+        lines(
+            "goog.provide('test');", //
+            "",
+            // We allow this goog.module.get despite the lack of goog.require: it's possible that
+            // the foo.bar.Baz module will have been loaded by the time test.fn is called. It's up
+            // to the caller to ensure that it's loaded.
+            "test.fn = function() {",
+            "  console.log(new (goog.module.get('foo.bar.Baz').Boo)());",
+            "}"));
+  }
+
+  @Test
+  public void
+      testWarning_script_googModuleGet_inFunctionInScript_inNonTopLevelIIFE_noRequire_noWarning() {
+    checkNoWarning(
+        lines(
+            "goog.module('foo.bar.Baz');", //
+            "/** @constructor */",
+            "exports.Boo = function() {}"),
+        lines(
+            "goog.provide('test');", //
+            "",
+            // We allow this goog.module.get despite the lack of goog.require: it's possible that
+            // the foo.bar.Baz module will have been loaded by the time test.fn is called. It's up
+            // to the caller to ensure that it's loaded.
+            "test.fn = function() {",
+            "  (() => console.log(new (goog.module.get('foo.bar.Baz').Boo)()))();",
+            "}"));
+  }
+
+  @Test
+  public void testNoWarning_nestedInGoogScope_googModuleGet_noRequire() {
+    checkNoWarning(
+        lines(
+            "goog.module('foo.bar.Baz');", //
+            "/** @constructor */",
+            "exports = function() {}"),
+        lines(
+            "goog.provide('test');", //
+            "",
+            "goog.scope(function() {",
+            "test.fn = function() {",
+            // We allow this goog.module.get despite the lack of goog.require: it's possible that
+            // the foo.bar.Baz module will have been loaded by the time test.fn is called. It's up
+            // to the caller to ensure that it's loaded.
+            "  const Baz = goog.module.get('foo.bar.Baz');",
+            "  new Baz();",
+            "  new goog.module.get('foo.bar.Baz');",
+            "}",
+            "});"));
+  }
+
   private void checkNoWarning(String... js) {
     testSame(srcs(js));
   }
@@ -1368,4 +1536,11 @@ private void checkIncorrectNamespaceAliasRequireTypeWarning(String namespace, St
         warning(CheckMissingRequires.INCORRECT_NAMESPACE_ALIAS_REQUIRE_TYPE)
             .withMessageContaining("'" + namespace + "'"));
   }
+
+  private void checkRequireForGoogModuleGetWarning(String namespace, String... js) {
+    testSame(
+        srcs(js),
+        warning(CheckMissingRequires.MISSING_REQUIRE_FOR_GOOG_MODULE_GET)
+            .withMessageContaining("'" + namespace + "'"));
+  }
 }
