diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java index a05150814b86c7..77b6f0bf1ac35b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java @@ -35,9 +35,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 @@ -53,7 +51,7 @@ public static Rule createRule( BlazeDirectories directories, StarlarkSemantics semantics, ExtendedEventHandler eventHandler, - String callStackEntry, + ImmutableList callStack, RuleClass ruleClass, Map attributes) throws InterruptedException, InvalidRuleException, NoSuchPackageException, EvalException { @@ -72,8 +70,6 @@ public static Rule createRule( repoMapping); BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(attributes); - ImmutableList callStack = - ImmutableList.of(StarlarkThread.callStackEntry(callStackEntry, Location.BUILTIN)); Rule rule; try { rule = diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java index 75e77edbb1d557..685ad2fbd53db3 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java @@ -113,7 +113,7 @@ public void createRepo(StarlarkThread thread, Dict kwargs, RuleC directories, thread.getSemantics(), eventHandler, - "RepositoryRuleFunction.createRule", + thread.getCallStack(), ruleClass, Maps.transformEntries(kwargs, (k, v) -> k.equals("name") ? prefixedName : v)); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index 8f62ca9873ad97..d632c4e779e1b6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -763,6 +763,12 @@ public RunModuleExtensionResult run( String prefixedName = usagesValue.getExtensionUniqueName() + "+" + 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 fakeCallStack = + ImmutableList.of(fakeCallStackEntry, fakeCallStackEntry); try { ruleInstance = BzlmodRepoRuleCreator.createRule( @@ -771,7 +777,7 @@ public RunModuleExtensionResult run( directories, starlarkSemantics, env.getListener(), - "SingleExtensionEval.createInnateExtensionRepoRule", + fakeCallStack, repoRule.getRuleClass(), Maps.transformEntries(kwargs, (k, v) -> k.equals("name") ? prefixedName : v)); attributesValue = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java index 235bb3c8898d27..749fd34ab5edd4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java +++ b/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; @@ -52,6 +51,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; /** @@ -194,7 +194,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()); diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index 90d8e9cd242aef..5004c7d8d59350 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -1339,6 +1339,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 result = + evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); + assertThat(result.hasError()).isTrue(); + assertContainsEvent( + "ERROR /ws/defs.bzl:3:12: //:+ext+ext: expected value of type 'string' for attribute 'data'" + + " of 'data_repo', 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( @@ -2902,4 +2934,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+1.0/WORKSPACE").getPathString()); + scratch.file(modulesRoot.getRelative("foo+1.0/BUILD").getPathString()); + scratch.file( + modulesRoot.getRelative("foo+1.0/data.bzl").getPathString(), + "load('@data//:data.bzl',repo_data='data')", + "data=repo_data"); + scratch.file( + modulesRoot.getRelative("foo+1.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 result = + evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); + assertThat(result.hasError()).isTrue(); + assertContainsEvent( + "ERROR /ws/MODULE.bazel:3:10: //:+_repo_rules+data: expected value of type 'string' for" + + " attribute 'data' of 'data_repo', 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"); + } } diff --git a/src/test/py/bazel/bzlmod/bazel_module_test.py b/src/test/py/bazel/bzlmod/bazel_module_test.py index ff9e3abbb705a9..0a44e2b7a10945 100644 --- a/src/test/py/bazel/bzlmod/bazel_module_test.py +++ b/src/test/py/bazel/bzlmod/bazel_module_test.py @@ -270,16 +270,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: : //pkg:+module_ext+foo: no such attribute' + '/pkg/extension.bzl:3:14: //pkg:+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 - ])) + 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'])