Skip to content

Commit

Permalink
Fix incorrect rule class digest when creating rules through macros.
Browse files Browse the repository at this point in the history
In bazelbuild@9f2cab5, 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
  • Loading branch information
alexjski authored and aiuto committed Sep 25, 2020
1 parent 4ebea2c commit f1f9411
Show file tree
Hide file tree
Showing 2 changed files with 150 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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<Artifact> {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
}
}

0 comments on commit f1f9411

Please sign in to comment.