From f1f941194ce04b96328154241fcfad0392b5cb38 Mon Sep 17 00:00:00 2001 From: ajurkowski Date: Fri, 11 Sep 2020 15:09:17 -0700 Subject: [PATCH] Fix incorrect rule class digest when creating rules through macros. In https://github.com/bazelbuild/bazel/commit/9f2cab5d84678e492d8f5848525633f1c73f0994, I made a change of how we are getting th digest for rule classes. We used to get them from a `StarlarkThread` and we are getting them from the inner Starlark function on the call stack. In a simple case, that is correct since that inner call is the call to the `rule` function, however when we have a rule class created thrugh a macro, it will not account for `.bzl` files which pass values to it. Please consider an example of: `def.bzl`: ``` def create_rule_class(function): return rule(implementation=function) ``` We will include the transitive digest of `def.bzl`, but not of the callers to `create_rule_class`. This means that if `function` is defined outside of the transitively loaded moduesl of `def.bzl`, the rule class definition environment digest will not account for changes to `function` itself. As a result of this, the hash is incorrect since it will produce the same value for different implementations. Change the digest collection to be based on the outermost Starlark function call instead. This will take the callers into the macros into account and, consequently, include `.bzl` file with passed `function` implementation in the digest. Please note that for the simple case of declaring a rule, the innermost and outermost frame is the same, hence that case has the same semanticss. PiperOrigin-RevId: 331231462 --- .../starlark/StarlarkRuleClassFunctions.java | 32 ++++- .../StarlarkRuleClassFunctionsTest.java | 121 ++++++++++++++++++ 2 files changed, 150 insertions(+), 3 deletions(-) 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"); + } }