diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index 00101b8403decb..2a8f9bd5145365 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java @@ -45,6 +45,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.LabelValidator; +import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate; @@ -80,6 +81,7 @@ import com.google.devtools.build.lib.packages.Type.ConversionException; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.starlarkbuildapi.StarlarkRuleFunctionsApi; +import com.google.devtools.build.lib.syntax.Debug; import com.google.devtools.build.lib.syntax.Dict; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.Identifier; @@ -97,6 +99,7 @@ import java.util.Collection; import java.util.Map; import java.util.concurrent.ExecutionException; +import javax.annotation.Nullable; /** A helper class to provide an easier API for Starlark rule definitions. */ public class StarlarkRuleClassFunctions implements StarlarkRuleFunctionsApi { @@ -346,11 +349,20 @@ public StarlarkCallable rule( .requiresHostConfigurationFragmentsByStarlarkBuiltinName( Sequence.cast(hostFragments, String.class, "host_fragments")); builder.setConfiguredTargetFunction(implementation); - // Information about the .bzl module containing the call that created the rule class. + // Obtain the rule definition environment (RDE) from the .bzl module being initialized by the + // calling thread -- the label and transitive source digest of the .bzl module of the outermost + // function in the call stack. + // + // If this thread is initializing a BUILD file, then the toplevel function's Module has + // no BazelModuleContext. Such rules cannot be instantiated, so it's ok to use a + // dummy label and RDE in that case (but not to crash). BazelModuleContext bzlModule = - BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)); + BazelModuleContext.of(getModuleOfOutermostStarlarkFunction(thread)); builder.setRuleDefinitionEnvironmentLabelAndDigest( - bzlModule.label(), bzlModule.bzlTransitiveDigest()); + bzlModule != null + ? bzlModule.label() + : Label.createUnvalidated(PackageIdentifier.EMPTY_PACKAGE_ID, "dummy_label"), + bzlModule != null ? bzlModule.bzlTransitiveDigest() : new byte[0]); builder.addRequiredToolchains(parseToolchains(toolchains, thread)); builder.useToolchainTransition(useToolchainTransition); @@ -405,6 +417,20 @@ public StarlarkCallable rule( return new StarlarkRuleFunction(builder, type, attributes, thread.getCallerLocation()); } + /** + * Returns the module (file) of the outermost enclosing Starlark function on the call stack or + * null if none of the active calls are functions defined in Starlark. + */ + @Nullable + private static Module getModuleOfOutermostStarlarkFunction(StarlarkThread thread) { + for (Debug.Frame fr : Debug.getCallStack(thread)) { + if (fr.getFunction() instanceof StarlarkFunction) { + return ((StarlarkFunction) fr.getFunction()).getModule(); + } + } + return null; + } + private static void checkAttributeName(String name) throws EvalException { if (!Identifier.isValid(name)) { throw Starlark.errorf("attribute name `%s` is not a valid identifier.", name); diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java index af4d7293a678c9..63e14d09118edc 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.collect.nestedset.Depset; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventKind; +import com.google.devtools.build.lib.events.NullEventHandler; import com.google.devtools.build.lib.packages.AdvertisedProviderSet; import com.google.devtools.build.lib.packages.AspectParameters; import com.google.devtools.build.lib.packages.Attribute; @@ -1846,4 +1847,124 @@ public void testCreateExecGroup() throws Exception { assertThat(group.execCompatibleWith()) .containsExactly(makeLabel("//constraint:cv1"), makeLabel("//constraint:cv2")); } + + @Test + public void ruleDefinitionEnvironmentDigest_unaffectedByTargetAttrValueChange() throws Exception { + scratch.file( + "r/def.bzl", + "def _r(ctx): return struct(value=ctx.attr.text)", + "r = rule(implementation=_r, attrs={'text': attr.string()})"); + scratch.file("r/BUILD", "load(':def.bzl', 'r')", "r(name='r', text='old')"); + byte[] oldDigest = + createRuleContext("//r:r") + .getRuleContext() + .getRule() + .getRuleClassObject() + .getRuleDefinitionEnvironmentDigest(); + + scratch.deleteFile("r/BUILD"); + scratch.file("r/BUILD", "load(':def.bzl', 'r')", "r(name='r', text='new')"); + // Signal SkyFrame to discover changed files. + skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE); + byte[] newDigest = + createRuleContext("//r:r") + .getRuleContext() + .getRule() + .getRuleClassObject() + .getRuleDefinitionEnvironmentDigest(); + + assertThat(newDigest).isEqualTo(oldDigest); + } + + @Test + public void ruleDefinitionEnvironmentDigest_accountsForFunctionWhenCreatingRuleWithAMacro() + throws Exception { + scratch.file("r/create.bzl", "def create(impl): return rule(implementation=impl)"); + scratch.file( + "r/def.bzl", + "load(':create.bzl', 'create')", + "def f(ctx): return struct(value='OLD')", + "r = create(f)"); + scratch.file("r/BUILD", "load(':def.bzl', 'r')", "r(name='r')"); + byte[] oldDigest = + createRuleContext("//r:r") + .getRuleContext() + .getRule() + .getRuleClassObject() + .getRuleDefinitionEnvironmentDigest(); + + scratch.deleteFile("r/def.bzl"); + scratch.file( + "r/def.bzl", + "load(':create.bzl', 'create')", + "def f(ctx): return struct(value='NEW')", + "r = create(f)"); + // Signal SkyFrame to discover changed files. + skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE); + byte[] newDigest = + createRuleContext("//r:r") + .getRuleContext() + .getRule() + .getRuleClassObject() + .getRuleDefinitionEnvironmentDigest(); + + assertThat(newDigest).isNotEqualTo(oldDigest); + } + + @Test + public void ruleDefinitionEnvironmentDigest_accountsForAttrsWhenCreatingRuleWithMacro() + throws Exception { + scratch.file( + "r/create.bzl", + "def f(ctx): return struct(value=ctx.attr.to_json())", + "def create(attrs): return rule(implementation=f, attrs=attrs)"); + scratch.file("r/def.bzl", "load(':create.bzl', 'create')", "r = create({})"); + scratch.file("r/BUILD", "load(':def.bzl', 'r')", "r(name='r')"); + byte[] oldDigest = + createRuleContext("//r:r") + .getRuleContext() + .getRule() + .getRuleClassObject() + .getRuleDefinitionEnvironmentDigest(); + + scratch.deleteFile("r/def.bzl"); + scratch.file( + "r/def.bzl", + "load(':create.bzl', 'create')", + "r = create({'value': attr.string(default='')})"); + // Signal SkyFrame to discover changed files. + skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE); + byte[] newDigest = + createRuleContext("//r:r") + .getRuleContext() + .getRule() + .getRuleClassObject() + .getRuleDefinitionEnvironmentDigest(); + + assertThat(newDigest).isNotEqualTo(oldDigest); + } + + /** + * This test is crucial for correctness of {@link RuleClass#getRuleDefinitionEnvironmentDigest} + * since we use a dummy bzl transitive digest in that case. It is correct to do that only because + * a rule class created by a BUILD thread cannot be instantiated. + */ + @Test + public void ruleClassDefinedInBuildFile_fails() throws Exception { + reporter.removeHandler(failFastHandler); + reporter.addHandler(ev.getEventCollector()); + scratch.file("r/create.bzl", "def create(impl): return rule(implementation=impl)"); + scratch.file("r/def.bzl", "load(':create.bzl', 'create')", "r = create({})"); + scratch.file("r/impl.bzl", "def make_struct(ctx): return struct(value='hello')"); + scratch.file( + "r/BUILD", + "load(':create.bzl', 'create')", + "load(':impl.bzl', 'make_struct')", + "r = create(make_struct)", + "r(name='r')"); + + getConfiguredTarget("//r:r"); + + ev.assertContainsError("Error in rule: Invalid rule class hasn't been exported by a bzl file"); + } }