Disallow importing injected repos

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java

@@ -49,7 +49,6 @@
 import net.starlark.java.eval.StarlarkValue;
 import net.starlark.java.eval.Structure;
 import net.starlark.java.eval.Tuple;
-import net.starlark.java.syntax.Location;
 
 /** A collection of global Starlark build API functions that apply to MODULE.bazel files. */
 @GlobalMethods(environment = Environment.MODULE)
@@ -171,11 +170,10 @@ public void module(
     }
     if (repoName.isEmpty()) {
       repoName = name;
-      context.addRepoNameUsage(name, "as the current module name", thread.getCallerLocation());
+      context.addRepoNameUsage(name, "as the current module name", thread.getCallStack());
     } else {
       RepositoryName.validateUserProvidedRepoName(repoName);
-      context.addRepoNameUsage(
-          repoName, "as the module's own repo name", thread.getCallerLocation());
+      context.addRepoNameUsage(repoName, "as the module's own repo name", thread.getCallStack());
     }
     Version parsedVersion;
     try {
@@ -293,7 +291,7 @@ public void bazelDep(
               name, parsedVersion, maxCompatibilityLevel.toInt("max_compatibility_level")));
     }
 
-    context.addRepoNameUsage(repoName, "by a bazel_dep", thread.getCallerLocation());
+    context.addRepoNameUsage(repoName, "by a bazel_dep", thread.getCallStack());
   }
 
   @StarlarkMethod(
@@ -541,9 +539,13 @@ static class ModuleExtensionProxy implements Structure, StarlarkExportable {
       usageBuilder.addProxyBuilder(proxyBuilder);
     }
 
-    void addImport(String localRepoName, String exportedName, String byWhat, Location location)
+    void addImport(
+        String localRepoName,
+        String exportedName,
+        String byWhat,
+        ImmutableList<StarlarkThread.CallStackEntry> stack)
         throws EvalException {
-      usageBuilder.addImport(localRepoName, exportedName, byWhat, location);
+      usageBuilder.addImport(localRepoName, exportedName, byWhat, stack);
       proxyBuilder.addImport(localRepoName, exportedName);
     }
 
@@ -635,13 +637,13 @@ public void useRepo(
       throws EvalException {
     ModuleThreadContext context = ModuleThreadContext.fromOrFail(thread, "use_repo()");
     context.setNonModuleCalled();
-    Location location = thread.getCallerLocation();
+    ImmutableList<StarlarkThread.CallStackEntry> stack = thread.getCallStack();
     for (String arg : Sequence.cast(args, String.class, "args")) {
-      extensionProxy.addImport(arg, arg, "by a use_repo() call", location);
+      extensionProxy.addImport(arg, arg, "by a use_repo() call", stack);
     }
     for (Map.Entry<String, String> entry :
         Dict.cast(kwargs, String.class, String.class, "kwargs").entrySet()) {
-      extensionProxy.addImport(entry.getKey(), entry.getValue(), "by a use_repo() call", location);
+      extensionProxy.addImport(entry.getKey(), entry.getValue(), "by a use_repo() call", stack);
     }
   }
 
@@ -829,7 +831,7 @@ public void call(
                   .setContainingModuleFilePath(
                       usageBuilder.getContext().getCurrentModuleFilePath()));
       extensionProxy.getValue(tagName).call(kwargs, thread);
-      extensionProxy.addImport(name, name, "by a repo rule", thread.getCallerLocation());
+      extensionProxy.addImport(name, name, "by a repo rule", thread.getCallStack());
     }
   }
 

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java

@@ -91,14 +91,21 @@ Location location() {
     }
   }
 
-  record RepoNameUsage(String how, Location where) {}
+  record RepoNameUsage(String how, ImmutableList<StarlarkThread.CallStackEntry> stack) {
+    Location location() {
+      // Skip over the override_repo builtin frame.
+      return stack.reverse().get(1).location;
+    }
+  }
 
-  public void addRepoNameUsage(String repoName, String how, Location where) throws EvalException {
-    RepoNameUsage collision = repoNameUsages.put(repoName, new RepoNameUsage(how, where));
+  public void addRepoNameUsage(
+      String repoName, String how, ImmutableList<StarlarkThread.CallStackEntry> stack)
+      throws EvalException {
+    RepoNameUsage collision = repoNameUsages.put(repoName, new RepoNameUsage(how, stack));
     if (collision != null) {
       throw Starlark.errorf(
           "The repo name '%s' is already being used %s at %s",
-          repoName, collision.how(), collision.where());
+          repoName, collision.how(), collision.location());
     }
   }
 
@@ -176,16 +183,20 @@ boolean isForExtension(String extensionBzlFile, String extensionName) {
           && !this.isolate;
     }
 
-    void addImport(String localRepoName, String exportedName, String byWhat, Location location)
+    void addImport(
+        String localRepoName,
+        String exportedName,
+        String byWhat,
+        ImmutableList<StarlarkThread.CallStackEntry> stack)
         throws EvalException {
       RepositoryName.validateUserProvidedRepoName(localRepoName);
       RepositoryName.validateUserProvidedRepoName(exportedName);
-      context.addRepoNameUsage(localRepoName, byWhat, location);
+      context.addRepoNameUsage(localRepoName, byWhat, stack);
       if (imports.containsValue(exportedName)) {
         String collisionRepoName = imports.inverse().get(exportedName);
         throw Starlark.errorf(
             "The repo exported as '%s' by module extension '%s' is already imported at %s",
-            exportedName, extensionName, context.repoNameUsages.get(collisionRepoName).where());
+            exportedName, extensionName, context.repoNameUsages.get(collisionRepoName).location());
       }
       imports.put(localRepoName, exportedName);
     }
@@ -250,6 +261,15 @@ ModuleExtensionUsage buildUsage() throws EvalException {
         }
         String importedAs = imports.inverse().get(overriddenRepoName);
         if (importedAs != null) {
+          if (!override.getValue().mustExist) {
+            throw Starlark.errorf(
+                    "Cannot import repo '%s' that has been injected into module extension '%s' at %s. Please refer to @%s directly.",
+                    overriddenRepoName,
+                    extensionName,
+                    override.getValue().location(),
+                    overridingRepoName)
+                .withCallStack(context.repoNameUsages.get(importedAs).stack);
+          }
           context.overriddenRepos.put(importedAs, override.getValue());
         }
         context.overridingRepos.put(overridingRepoName, override.getValue());
@@ -308,7 +328,7 @@ public InterimModule buildModule(@Nullable Registry registry) throws EvalExcepti
       }
       deps.put(builtinModule, DepSpec.create(builtinModule, Version.EMPTY, -1));
       try {
-        addRepoNameUsage(builtinModule, "as a built-in dependency", Location.BUILTIN);
+        addRepoNameUsage(builtinModule, "as a built-in dependency", ImmutableList.of());
       } catch (EvalException e) {
         throw new EvalException(
             e.getMessage()

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java

@@ -2048,6 +2048,7 @@ public void extensionMetadata() throws Exception {
         "  'dev_as_non_dev_dep',",
         "  my_direct_dep = 'direct_dep',",
         ")",
+        "inject_repo(ext, my_data_repo = 'data_repo')",
         "ext_dev = use_extension('@ext//:defs.bzl', 'ext', dev_dependency = True)",
         "use_repo(",
         "  ext_dev,",
@@ -3250,7 +3251,7 @@ public void overrideRepo_inject() throws Exception {
         """
         bazel_dep(name = "data_repo", version = "1.0")
         ext = use_extension("//:defs.bzl","ext")
-        use_repo(ext, "bar", module_foo = "foo")
+        use_repo(ext, "bar")
         data_repo = use_repo_rule("@data_repo//:defs.bzl", "data_repo")
         data_repo(name = "foo", data = "overridden_data")
         inject_repo(ext, "foo")
@@ -3310,73 +3311,4 @@ def _ext_impl(ctx):
     assertThat(fooData).isInstanceOf(String.class);
     assertThat(fooData).isEqualTo("overridden_data");
   }
-
-  @Test
-  public void overrideRepo_inject_onExistingRepoFails() throws Exception {
-    scratch.file(
-        workspaceRoot.getRelative("MODULE.bazel").getPathString(),
-        """
-        bazel_dep(name = "data_repo", version = "1.0")
-        ext = use_extension("//:defs.bzl","ext")
-        use_repo(ext, "bar", module_foo = "foo")
-        data_repo = use_repo_rule("@data_repo//:defs.bzl", "data_repo")
-        data_repo(name = "override", data = "overridden_data")
-        inject_repo(ext, foo = "override")
-        """);
-    scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
-    scratch.file(
-        workspaceRoot.getRelative("data.bzl").getPathString(),
-        """
-        load("@bar//:list.bzl", _bar_list = "list")
-        load("@override//:data.bzl", _override_data = "data")
-        load("@module_foo//:data.bzl", _foo_data = "data")
-        bar_list = _bar_list
-        foo_data = _foo_data
-        override_data = _override_data
-        """);
-    scratch.file(
-        workspaceRoot.getRelative("defs.bzl").getPathString(),
-        """
-        load("@data_repo//:defs.bzl", "data_repo")
-        def _list_repo_impl(ctx):
-          ctx.file("WORKSPACE")
-          ctx.file("BUILD")
-          labels = [str(Label(l)) for l in ctx.attr.labels]
-          labels += [str(Label("@module_foo//:target3"))]
-          ctx.file("list.bzl", "list = " + repr(labels) + " + [str(Label('@foo//:target4'))]")
-        list_repo = repository_rule(
-          implementation = _list_repo_impl,
-          attrs = {
-            "labels": attr.label_list(),
-          },
-        )
-        def _fail_repo_impl(ctx):
-          fail("This rule should not be evaluated")
-        fail_repo = repository_rule(implementation = _fail_repo_impl)
-        def _ext_impl(ctx):
-          fail_repo(name = "foo")
-          list_repo(
-            name = "bar",
-            labels = [
-              # lazy extension implementation function repository mapping
-              "@foo//:target1",
-              # module repo repository mapping
-              "@module_foo//:target2",
-            ],
-          )
-        ext = module_extension(implementation = _ext_impl)
-        """);
-
-    SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
-    reporter.removeHandler(failFastHandler);
-    EvaluationResult<BzlLoadValue> result =
-        evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
-    assertThat(result.hasError()).isTrue();
-    assertThat(result.getError().getException())
-        .hasMessageThat()
-        .isEqualTo(
-            "module extension \"ext\" from \"//:defs.bzl\" generates repository \"foo\", yet it is"
-                + " injected via inject_repo() at /ws/MODULE.bazel:6:12. Use override_repo()"
-                + " instead to override an existing repository.");
-  }
 }

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java

@@ -2012,4 +2012,33 @@ public void testOverrideRepo_cycle_multipleExtensions() throws Exception {
         extension 'ext2' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:4:14, \
         which is not supported.""");
   }
+
+  @Test
+  public void testInjectRepo_imported() throws Exception {
+    scratch.overwriteFile(
+        rootDirectory.getRelative("MODULE.bazel").getPathString(),
+        """
+        module(name='aaa')
+        bazel_dep(name = 'my_repo', version = "1.0")
+        ext = use_extension('//:defs.bzl', 'ext')
+        inject_repo(ext, foo = 'my_repo')
+        use_repo(ext, bar = 'foo')
+        """);
+
+    reporter.removeHandler(failFastHandler);
+    EvaluationResult<RootModuleFileValue> result =
+        evaluator.evaluate(
+            ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext);
+    assertThat(result.hasError()).isTrue();
+
+    assertContainsEvent(
+        """
+        ERROR /workspace/MODULE.bazel:5:9: Traceback (most recent call last):
+        \tFile "/workspace/MODULE.bazel", line 5, column 9, in <toplevel>
+        \t\tuse_repo(ext, bar = 'foo')
+        Error in use_repo: Cannot import repo 'foo' that has been injected into \
+        module extension 'ext' at /workspace/MODULE.bazel:4:12. Please refer \
+        to @my_repo directly.\
+        """);
+  }
 }