[7.4.0] Fix locations reported in repo rule instantiation errors for …

…extensions (#23390)

Since `Rule` logic drops a call stack entry, errors during repo rule
instantiation in a module extension weren't reported with a useful
`Location`.

Work towards #19055

Closes #23363.

PiperOrigin-RevId: 665468188
Change-Id: If67b5b6f290ac4e33b5743e9c1c361a93e1217ef

Closes #23364

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

@@ -34,9 +34,7 @@
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Starlark;
 import net.starlark.java.eval.StarlarkSemantics;
-import net.starlark.java.eval.StarlarkThread;
 import net.starlark.java.eval.StarlarkThread.CallStackEntry;
-import net.starlark.java.syntax.Location;
 
 /**
  * Creates a repo rule instance for Bzlmod. This class contrasts with the WORKSPACE repo rule
@@ -52,7 +50,7 @@ public static Rule createRule(
       BlazeDirectories directories,
       StarlarkSemantics semantics,
       ExtendedEventHandler eventHandler,
-      String callStackEntry,
+      ImmutableList<CallStackEntry> callStack,
       RuleClass ruleClass,
       Map<String, Object> attributes)
       throws InterruptedException, InvalidRuleException, NoSuchPackageException, EvalException {
@@ -71,8 +69,6 @@ public static Rule createRule(
             repoMapping);
     BuildLangTypedAttributeValuesMap attributeValues =
         new BuildLangTypedAttributeValuesMap(attributes);
-    ImmutableList<CallStackEntry> callStack =
-        ImmutableList.of(StarlarkThread.callStackEntry(callStackEntry, Location.BUILTIN));
     Rule rule;
     try {
       rule =

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

@@ -107,7 +107,7 @@ public void createRepo(StarlarkThread thread, Dict<String, Object> kwargs, RuleC
               directories,
               thread.getSemantics(),
               eventHandler,
-              "RepositoryRuleFunction.createRule",
+              thread.getCallStack(),
               ruleClass,
               Maps.transformEntries(kwargs, (k, v) -> k.equals("name") ? prefixedName : v));
 

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

@@ -785,6 +785,12 @@ public RunModuleExtensionResult run(
         String prefixedName = usagesValue.getExtensionUniqueName() + separator + name;
         Rule ruleInstance;
         AttributeValues attributesValue;
+        var fakeCallStackEntry =
+            StarlarkThread.callStackEntry("InnateRunnableExtension.run", repo.tag().getLocation());
+        // Rule creation strips the top-most entry from the call stack, so we need to add the fake
+        // one twice.
+        ImmutableList<StarlarkThread.CallStackEntry> fakeCallStack =
+            ImmutableList.of(fakeCallStackEntry, fakeCallStackEntry);
         try {
           ruleInstance =
               BzlmodRepoRuleCreator.createRule(
@@ -793,7 +799,7 @@ public RunModuleExtensionResult run(
                   directories,
                   starlarkSemantics,
                   env.getListener(),
-                  "SingleExtensionEval.createInnateExtensionRepoRule",
+                  fakeCallStack,
                   repoRule.getRuleClass(),
                   Maps.transformEntries(kwargs, (k, v) -> k.equals("name") ? prefixedName : v));
           attributesValue =

src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java

@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.skyframe;
 
-
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -53,6 +52,7 @@
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Module;
 import net.starlark.java.eval.StarlarkSemantics;
+import net.starlark.java.eval.StarlarkThread;
 import net.starlark.java.syntax.Location;
 
 /**
@@ -199,7 +199,9 @@ private BzlmodRepoRuleValue createRuleFromSpec(
               directories,
               starlarkSemantics,
               env.getListener(),
-              "BzlmodRepoRuleFunction.createRule",
+              ImmutableList.of(
+                  StarlarkThread.callStackEntry(
+                      "BzlmodRepoRuleFunction.createRuleFromSpec", Location.BUILTIN)),
               ruleClass,
               attributes);
       return new BzlmodRepoRuleValue(rule.getPackage(), rule.getName());

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

@@ -1359,6 +1359,38 @@ public void importNonExistentRepo() throws Exception {
                 + " /ws/MODULE.bazel:1:20");
   }
 
+  @Test
+  public void invalidAttributeValue() throws Exception {
+    scratch.file(
+        workspaceRoot.getRelative("MODULE.bazel").getPathString(),
+        "ext = use_extension('//:defs.bzl','ext')",
+        "bazel_dep(name='data_repo', version='1.0')",
+        "use_repo(ext,'ext')");
+    scratch.file(
+        workspaceRoot.getRelative("defs.bzl").getPathString(),
+        "load('@data_repo//:defs.bzl','data_repo')",
+        "def _ext_impl(ctx):",
+        "  data_repo(name='ext',data=42)",
+        "ext = module_extension(implementation=_ext_impl)");
+    scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
+    scratch.file(
+        workspaceRoot.getRelative("data.bzl").getPathString(),
+        "load('@ext//:data.bzl', ext_data='data')",
+        "data=ext_data");
+
+    SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
+    reporter.removeHandler(failFastHandler);
+    EvaluationResult<BzlLoadValue> result =
+        evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
+    assertThat(result.hasError()).isTrue();
+    assertContainsEvent(
+        "ERROR /ws/defs.bzl:3:12: //:_main~ext~ext: expected value of type 'string' for attribute 'data'"
+            + " in 'data_repo' rule, but got 42 (int)");
+    assertThat(result.getError().getException())
+        .hasMessageThat()
+        .isEqualTo("error evaluating module extension ext in //:defs.bzl");
+  }
+
   @Test
   public void badRepoNameInExtensionImplFunction() throws Exception {
     scratch.file(
@@ -2924,4 +2956,52 @@ public void innate_noSuchValue() throws Exception {
             "//:repo.bzl does not export a repository_rule called data_repo, yet its use is"
                 + " requested at /ws/MODULE.bazel");
   }
+
+  @Test
+  public void innate_invalidAttributeValue() throws Exception {
+    scratch.file(
+        workspaceRoot.getRelative("MODULE.bazel").getPathString(),
+        "bazel_dep(name='foo',version='1.0')",
+        "data_repo = use_repo_rule('@foo//:repo.bzl', 'data_repo')",
+        "data_repo(name='data', data=5)");
+    scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
+    scratch.file(
+        workspaceRoot.getRelative("data.bzl").getPathString(),
+        "load('@data//:data.bzl', self_data='data')",
+        "load('@foo//:data.bzl', foo_data='data')",
+        "data=self_data+' '+foo_data");
+
+    registry.addModule(
+        createModuleKey("foo", "1.0"),
+        "module(name='foo',version='1.0')",
+        "data_repo = use_repo_rule('//:repo.bzl', 'data_repo')",
+        "data_repo(name='data', data='go to bed at 11pm.')");
+    scratch.file(modulesRoot.getRelative("foo~v1.0/WORKSPACE").getPathString());
+    scratch.file(modulesRoot.getRelative("foo~v1.0/BUILD").getPathString());
+    scratch.file(
+        modulesRoot.getRelative("foo~v1.0/data.bzl").getPathString(),
+        "load('@data//:data.bzl',repo_data='data')",
+        "data=repo_data");
+    scratch.file(
+        modulesRoot.getRelative("foo~v1.0/repo.bzl").getPathString(),
+        "def _data_repo_impl(ctx):",
+        "  ctx.file('BUILD.bazel')",
+        "  ctx.file('data.bzl', 'data='+json.encode(ctx.attr.data))",
+        "data_repo = repository_rule(",
+        "  implementation=_data_repo_impl, attrs={'data':attr.string()})");
+
+    SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
+    reporter.removeHandler(failFastHandler);
+    EvaluationResult<BzlLoadValue> result =
+        evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
+    assertThat(result.hasError()).isTrue();
+    assertContainsEvent(
+        "ERROR /ws/MODULE.bazel:3:10: //:_main~_repo_rules~data: expected value"
+            + " of type 'string' for attribute 'data' in 'data_repo' rule, but got 5 (int)");
+    assertThat(result.getError().getException())
+        .hasMessageThat()
+        .isEqualTo(
+            "error creating repo data requested at /ws/MODULE.bazel:3:10: failed to instantiate"
+                + " 'data_repo' from this module extension");
+  }
 }

src/test/py/bazel/bzlmod/bazel_module_test.py

@@ -265,14 +265,16 @@ def testRepositoryRuleErrorInModuleExtensionFailsTheBuild(self):
     exit_code, _, stderr = self.RunBazel(['run', '@foo//...'],
                                          allow_failure=True)
     self.AssertExitCode(exit_code, 48, stderr)
+    stderr = '\n'.join(stderr)
     self.assertIn(
-        "ERROR: <builtin>: //pkg:_main~module_ext~foo: no such attribute 'invalid_attr' in 'repo_rule' rule",
-        stderr)
-    self.assertTrue(
-        any([
-            '/pkg/extension.bzl", line 3, column 14, in _module_ext_impl'
-            in line for line in stderr
-        ]))
+        '/pkg/extension.bzl:3:14: //pkg:_main~module_ext~foo: no such attribute'
+        " 'invalid_attr' in 'repo_rule' rule",
+        stderr,
+    )
+    self.assertIn(
+        '/pkg/extension.bzl", line 3, column 14, in _module_ext_impl',
+        stderr,
+    )
 
   def testDownload(self):
     data_path = self.ScratchFile('data.txt', ['some data'])