Lift all subrule attributes to a rule that uses it.
We just add the attributes defined on the subrules to the rule class as if they were defined on the rule itself (renaming them to avoid conflict). This seems to be the simplest way to leverage all the existing rule machinery to resolve dependencies *and* get the right results for `bazel query`
A downside of doing it this way, is that it makes these attributes bona fide attributes of the rule. So if we want to ensure these attributes are not accessible via the rule context, it will require performing some extra checks. I haven't done that here and I'm not sure if that is worth it. The subrule proposal is meant to help make the code modular and have better abstractions. If one wishes to go the extra mile to violate that, :shrugs:
PiperOrigin-RevId: 563703792
Change-Id: Ifdb0893208e3693a5cde473f0cfeb3d16583d402
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 903b01b..47e52e7 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
@@ -395,7 +395,7 @@
Object buildSetting,
Object cfg,
Object execGroups,
- Sequence<?> subrules)
+ Sequence<?> subrulesUnchecked)
throws EvalException {
// analysis_test=true implies test=true.
@@ -541,14 +541,12 @@
parseExecCompatibleWith(execCompatibleWith, labelConverter));
}
- builder.setSubrules(
- Sequence.cast(subrules, StarlarkSubruleApi.class, "subrules").getImmutableList());
-
return new StarlarkRuleFunction(
builder,
type,
attributes,
initializer,
+ Sequence.cast(subrulesUnchecked, StarlarkSubrule.class, "subrules").getImmutableList(),
loc,
Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString));
}
@@ -761,6 +759,7 @@
private final RuleClassType type;
private ImmutableList<Pair<String, StarlarkAttrModule.Descriptor>> attributes;
@Nullable private final StarlarkFunction initializer;
+ private ImmutableList<StarlarkSubrule> subrules;
private final Location definitionLocation;
@Nullable private final String documentation;
private Label starlarkLabel;
@@ -774,6 +773,7 @@
RuleClassType type,
ImmutableList<Pair<String, StarlarkAttrModule.Descriptor>> attributes,
@Nullable StarlarkFunction initializer,
+ ImmutableList<StarlarkSubrule> subrules,
Location definitionLocation,
Optional<String> documentation) {
this.builder = builder;
@@ -785,6 +785,7 @@
this.type = type;
this.attributes = attributes;
this.initializer = initializer;
+ this.subrules = subrules;
this.definitionLocation = definitionLocation;
this.documentation = documentation.orElse(null);
}
@@ -932,6 +933,27 @@
ruleClassName);
return;
}
+
+ // the set of subrules is stored in the rule class, primarily for validating that a rule class
+ // declared the subrule when using it.
+ builder.setSubrules(subrules);
+ // lift the subrule attributes to the rule class as if they were declared there, this lets us
+ // exploit dependency resolution for "free"
+ ImmutableList<Pair<String, Descriptor>> subruleAttributes;
+ try {
+ subruleAttributes = StarlarkSubrule.discoverAttributesForRule(subrules);
+ } catch (EvalException e) {
+ errorf(handler, "%s", e.getMessage());
+ return;
+ }
+ if (!subruleAttributes.isEmpty()) {
+ attributes =
+ ImmutableList.<Pair<String, Descriptor>>builder()
+ .addAll(attributes)
+ .addAll(subruleAttributes)
+ .build();
+ }
+
// Thus far, we only know if we have a rule transition. While iterating through attributes,
// check if we have an attribute transition.
boolean hasStarlarkDefinedTransition = builder.hasStarlarkRuleTransition();
@@ -1034,6 +1056,7 @@
this.builder = null;
this.attributes = null;
+ this.subrules = null;
}
@FormatMethod
@@ -1124,6 +1147,7 @@
for (Entry<String, Descriptor> attr : attrs.entrySet()) {
// TODO: b/293304174 - only permit label/label-list typed attributes
// TODO: b/293304174 - add support for late bound defaults (will require declaring fragments)
+ // TODO: b/293304174 - do not permit split transitions?
String attrName = attr.getKey();
Descriptor descriptor = attr.getValue();
checkAttributeName(attrName);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubrule.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubrule.java
index 3208997..b510538 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubrule.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubrule.java
@@ -14,7 +14,8 @@
package com.google.devtools.build.lib.analysis.starlark;
-import com.google.common.annotations.VisibleForTesting;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@@ -32,7 +33,7 @@
import com.google.devtools.build.lib.packages.StarlarkExportable;
import com.google.devtools.build.lib.starlarkbuildapi.StarlarkActionFactoryApi;
import com.google.devtools.build.lib.starlarkbuildapi.StarlarkSubruleApi;
-import java.util.Map.Entry;
+import com.google.devtools.build.lib.util.Pair;
import javax.annotation.Nullable;
import net.starlark.java.annot.StarlarkBuiltin;
import net.starlark.java.annot.StarlarkMethod;
@@ -46,31 +47,28 @@
import net.starlark.java.eval.StarlarkThread;
import net.starlark.java.eval.Tuple;
-/** Represents a subrule which can be invoked in a Starlark rule's implementation function. */
+/**
+ * Represents a subrule which can be invoked in a Starlark rule's implementation function.
+ *
+ * <p>The basic mechanism used is that a rule class declared a dependency on a set of subrules. The
+ * (implicit) attributes of the subrule are lifted to the rule class, and thus, behave as if they
+ * were directly declared on the rule class itself. The rule class also holds a reference to the set
+ * of subrules. The latter is only used for validating that a rule invoking a subrule declared that
+ * subrule as a dependency.
+ */
public class StarlarkSubrule implements StarlarkExportable, StarlarkCallable, StarlarkSubruleApi {
// TODO(hvd) this class is a WIP, will be implemented over many commits
private final StarlarkFunction implementation;
- private final ImmutableList<SubruleAttribute> attributes;
// following fields are set on export
@Nullable private String exportedName = null;
+ private ImmutableList<SubruleAttribute> attributes;
public StarlarkSubrule(
StarlarkFunction implementation, ImmutableMap<String, Descriptor> attributes) {
this.implementation = implementation;
- this.attributes = createSubruleAttributeList(attributes);
- }
-
- private ImmutableList<SubruleAttribute> createSubruleAttributeList(
- ImmutableMap<String, Descriptor> attributes) {
- ImmutableList.Builder<SubruleAttribute> builder = ImmutableList.builder();
- for (Entry<String, Descriptor> attr : attributes.entrySet()) {
- String attrName = attr.getKey();
- Descriptor descriptor = attr.getValue();
- builder.add(new SubruleAttribute(attrName, descriptor));
- }
- return builder.build();
+ this.attributes = SubruleAttribute.from(attributes);
}
@Override
@@ -90,9 +88,7 @@
@Override
public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwargs)
throws EvalException, InterruptedException {
- if (!isExported()) {
- throw Starlark.errorf("Invalid subrule hasn't been exported by a bzl file");
- }
+ checkExported();
StarlarkRuleContext ruleContext =
BazelRuleAnalysisThreadContext.fromOrFail(thread, getName())
.getRuleContext()
@@ -111,8 +107,7 @@
if (kwargs.containsKey(attr.attrName)) {
throw Starlark.errorf("got invalid named argument: '%s'", attr.attrName);
}
- // TODO: b/293304174 - fetch value from rule context
- namedArgs.put(attr.attrName, attr.descriptor.build(attr.attrName).getDefaultValue(null));
+ namedArgs.put(attr.attrName, ruleContext.getAttr().getValue(attr.ruleAttrName));
}
try {
ruleContext.setLockedForSubrule(true);
@@ -137,9 +132,24 @@
}
}
- @VisibleForTesting
- ImmutableList<SubruleAttribute> getAttributes() {
- return attributes;
+ /**
+ * Returns the collection of attributes to be lifted to a rule that uses this {@code subrule}.
+ *
+ * @throws EvalException if this subrule is unexported
+ */
+ private ImmutableList<Pair<String, Descriptor>> attributesForRule() throws EvalException {
+ checkExported();
+ ImmutableList.Builder<Pair<String, Descriptor>> builder = ImmutableList.builder();
+ for (SubruleAttribute attr : attributes) {
+ builder.add(Pair.of(attr.ruleAttrName, attr.descriptor));
+ }
+ return builder.build();
+ }
+
+ private void checkExported() throws EvalException {
+ if (!isExported()) {
+ throw Starlark.errorf("Invalid subrule hasn't been exported by a bzl file");
+ }
}
@Override
@@ -151,6 +161,29 @@
public void export(EventHandler handler, Label extensionLabel, String exportedName) {
Preconditions.checkState(!isExported());
this.exportedName = exportedName;
+ this.attributes = SubruleAttribute.transformOnExport(attributes, extensionLabel, exportedName);
+ }
+
+ /**
+ * Returns all attributes to be lifted from the given subrules to a rule
+ *
+ * <p>Attributes are discovered transitively (if a subrule depends on another subrule) and those
+ * from common, transitive dependencies are de-duped.
+ *
+ * @throws EvalException if any of the given subrules are unexported
+ */
+ static ImmutableList<Pair<String, Descriptor>> discoverAttributesForRule(
+ ImmutableList<StarlarkSubrule> subrules) throws EvalException {
+ ImmutableSet.Builder<StarlarkSubrule> uniqueSubrules = ImmutableSet.builder();
+ for (StarlarkSubrule subrule : subrules) {
+ // TODO: b/293304174 - use all transitive subrules once subrules can depend on other subrules
+ uniqueSubrules.add(subrule);
+ }
+ ImmutableList.Builder<Pair<String, Descriptor>> attributes = ImmutableList.builder();
+ for (StarlarkSubrule subrule : uniqueSubrules.build()) {
+ attributes.addAll(subrule.attributesForRule());
+ }
+ return attributes.build();
}
/**
@@ -265,15 +298,42 @@
}
}
- @VisibleForTesting
- static class SubruleAttribute {
+ private static class SubruleAttribute {
- final String attrName;
- final Descriptor descriptor;
+ private final String attrName;
+ private final Descriptor descriptor;
- private SubruleAttribute(String attrName, Descriptor descriptor) {
+ /**
+ * This is the attribute name when lifted to a rule, see {@link #copyWithRuleAttributeName} and
+ * is set only after the subrule is exported
+ */
+ @Nullable private final String ruleAttrName;
+
+ private SubruleAttribute(
+ String attrName, Descriptor descriptor, @Nullable String ruleAttrName) {
this.attrName = attrName;
this.descriptor = descriptor;
+ this.ruleAttrName = ruleAttrName;
+ }
+
+ private static ImmutableList<SubruleAttribute> from(
+ ImmutableMap<String, Descriptor> attributes) {
+ return attributes.entrySet().stream()
+ .map(e -> new SubruleAttribute(e.getKey(), e.getValue(), null))
+ .collect(toImmutableList());
+ }
+
+ private static ImmutableList<SubruleAttribute> transformOnExport(
+ ImmutableList<SubruleAttribute> attributes, Label label, String exportedName) {
+ return attributes.stream()
+ .map(s -> s.copyWithRuleAttributeName(label, exportedName))
+ .collect(toImmutableList());
+ }
+
+ private SubruleAttribute copyWithRuleAttributeName(Label label, String exportedName) {
+ // _foo -> //pkg:label%my_subrule%_foo
+ String ruleAttrName = label + "%" + exportedName + "%" + attrName;
+ return new SubruleAttribute(attrName, descriptor, ruleAttrName);
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java
index b6168dc..e8983a8 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java
@@ -220,6 +220,7 @@
builder.requiredAspectClasses(requiredAspectsClasses.build());
builder.execCompatibleWith(execCompatibleWith);
builder.execGroups(execGroups);
+ // TODO: b/293304174 - lift subrule attributes
builder.subrules(subrules);
return builder.build();
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
index 2655f2e..64f1c02 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
@@ -489,6 +489,7 @@
"//src/main/java/net/starlark/java/eval",
"//src/test/java/com/google/devtools/build/lib/analysis/util",
"//src/test/java/com/google/devtools/build/lib/starlark/util",
+ "//third_party:guava",
"//third_party:junit4",
"//third_party:truth",
],
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java b/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java
index 5096b53..ebe63c4 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;
+import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
@@ -29,6 +30,7 @@
import com.google.devtools.build.lib.starlark.util.BazelEvaluationTestCase;
import com.google.devtools.build.lib.starlarkbuildapi.StarlarkSubruleApi;
import net.starlark.java.eval.BuiltinFunction;
+import net.starlark.java.eval.Sequence;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -418,6 +420,41 @@
}
@Test
+ // this is not the correct behavior and will be inverted in a followup
+ public void testSubruleAttrs_visibleInRuleCtx() throws Exception {
+ scratch.file("default/BUILD", "genrule(name = 'default', outs = ['a'], cmd = '')");
+ scratch.file(
+ "subrule_testing/myrule.bzl",
+ "def _subrule_impl(ctx):",
+ " return ",
+ "_my_subrule = subrule(",
+ " implementation = _subrule_impl,",
+ " attrs = {'_foo' : attr.label(default = '//default')},",
+ ")",
+ "MyInfo=provider()",
+ "def _rule_impl(ctx):",
+ " res = dir(ctx.attr)",
+ " return MyInfo(result = res)",
+ "",
+ "my_rule = rule(implementation = _rule_impl, subrules = [_my_subrule])");
+ scratch.file(
+ "subrule_testing/BUILD",
+ //
+ "load('myrule.bzl', 'my_rule')",
+ "my_rule(name = 'foo')");
+
+ ImmutableList<String> attributes =
+ Sequence.cast(
+ getProvider("//subrule_testing:foo", "//subrule_testing:myrule.bzl", "MyInfo")
+ .getValue("result"),
+ String.class,
+ "")
+ .getImmutableList();
+
+ assertThat(attributes).contains("//subrule_testing:myrule.bzl%_my_subrule%_foo");
+ }
+
+ @Test
public void testSubruleAttrs_overridingImplicitAttributeValueFails() throws Exception {
scratch.file(
"subrule_testing/myrule.bzl",
@@ -478,6 +515,42 @@
}
@Test
+ public void testSubruleAttrs_implicitLabelDepsAreResolvedToTargets() throws Exception {
+ scratch.file(
+ "some/pkg/BUILD",
+ //
+ "genrule(name = 'tool', cmd = '', outs = ['tool.exe'])");
+ scratch.file(
+ "subrule_testing/myrule.bzl",
+ "def _subrule_impl(ctx, _tool):",
+ " return _tool",
+ "_my_subrule = subrule(",
+ " implementation = _subrule_impl,",
+ " attrs = {'_tool' : attr.label(default = '//some/pkg:tool')},",
+ ")",
+ "",
+ "MyInfo = provider()",
+ "def _rule_impl(ctx):",
+ " res = _my_subrule()",
+ " return MyInfo(result = res)",
+ "",
+ "my_rule = rule(implementation = _rule_impl, subrules = [_my_subrule])");
+ scratch.file(
+ "subrule_testing/BUILD",
+ //
+ "load('myrule.bzl', 'my_rule')",
+ "my_rule(name = 'foo')");
+
+ StructImpl provider =
+ getProvider("//subrule_testing:foo", "//subrule_testing:myrule.bzl", "MyInfo");
+
+ assertThat(provider).isNotNull();
+ Object value = provider.getValue("result");
+ assertThat(value).isInstanceOf(ConfiguredTarget.class);
+ assertThat(((ConfiguredTarget) value).getLabel().toString()).isEqualTo("//some/pkg:tool");
+ }
+
+ @Test
public void testSubruleInstantiation_outsideAllowlist_failsWithPrivateAPIError()
throws Exception {
evOutsideAllowlist.checkEvalErrorContains(
diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD
index cab3d59..b977b64 100644
--- a/src/test/shell/integration/BUILD
+++ b/src/test/shell/integration/BUILD
@@ -877,6 +877,15 @@
tags = ["no_windows"],
)
+sh_test(
+ name = "subrules_test",
+ srcs = ["subrules_test.sh"],
+ data = [
+ ":test-deps",
+ "@bazel_tools//tools/bash/runfiles",
+ ],
+)
+
########################################################################
# Test suites.
diff --git a/src/test/shell/integration/subrules_test.sh b/src/test/shell/integration/subrules_test.sh
new file mode 100755
index 0000000..20d3e0a
--- /dev/null
+++ b/src/test/shell/integration/subrules_test.sh
@@ -0,0 +1,129 @@
+#!/bin/bash
+#
+# Copyright 2023 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# An end-to-end test for Bazel's *experimental* subrule API.
+
+# --- begin runfiles.bash initialization ---
+set -euo pipefail
+if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
+ if [[ -f "$0.runfiles_manifest" ]]; then
+ export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest"
+ elif [[ -f "$0.runfiles/MANIFEST" ]]; then
+ export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST"
+ elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then
+ export RUNFILES_DIR="$0.runfiles"
+ fi
+fi
+if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then
+ source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash"
+elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
+ source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \
+ "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)"
+else
+ echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash"
+ exit 1
+fi
+# --- end runfiles.bash initialization ---
+
+source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \
+ || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
+
+case "$(uname -s | tr [:upper:] [:lower:])" in
+msys*|mingw*|cygwin*)
+ declare -r is_windows=true
+ ;;
+*)
+ declare -r is_windows=false
+ ;;
+esac
+
+if "$is_windows"; then
+ export MSYS_NO_PATHCONV=1
+ export MSYS2_ARG_CONV_EXCL="*"
+fi
+
+function set_up() {
+ mkdir -p subrule_testing
+ cat > subrule_testing/rule.bzl <<EOF
+def _subrule_impl(ctx, _foo, _bar):
+ pass
+
+my_subrule = subrule(
+ implementation = _subrule_impl,
+ attrs = {
+ "_foo": attr.label(default = "//some:label"),
+ "_bar": attr.int(default = 12345),
+ },
+)
+
+def _rule_impl(ctx):
+ my_subrule()
+ return []
+
+my_rule = rule(
+ implementation = _rule_impl,
+ attrs = {
+ "_foo": attr.label(),
+ },
+ subrules = [my_subrule],
+)
+EOF
+ cat > subrule_testing/BUILD <<EOF
+load(":rule.bzl", "my_rule")
+my_rule(name = 'foo')
+EOF
+
+ mkdir -p some
+ cat > some/BUILD <<EOF
+package(default_visibility = ["//visibility:public"])
+genrule(name = 'label', cmd = '', outs = ['0.out'])
+genrule(name = 'label_1', cmd = '', outs = ['1.out'])
+genrule(name = 'label_2', cmd = '', outs = ['2.out'])
+EOF
+}
+
+function test_query_xml_outputs_subrule_implicit_deps() {
+ bazel query --output xml //subrule_testing:foo &> $TEST_log || fail "query failed"
+ expect_log '<rule-input name="//some:label"/>'
+}
+
+function test_query_xml_outputs_subrule_attributes() {
+ bazel query --output xml --xml:default_values //subrule_testing:foo &> $TEST_log || fail "query failed"
+ expect_log '<label name="//subrule_testing:rule.bzl%my_subrule%_foo" value="//some:label"/>'
+ expect_log '<int name="//subrule_testing:rule.bzl%my_subrule%_bar" value="12345"/>'
+}
+
+# native.existing_rules skips all implicit attributes so this is trivially true
+function test_subrule_attributes_are_not_visible_to_native.existing_rules() {
+ cat > subrule_testing/helper.bzl <<EOF
+def print_rules():
+ rules = native.existing_rules()
+ for name, rule in rules.items():
+ for attr, value in rule.items():
+ print('rule:', name + ', attr:', attr + ', value:', value)
+EOF
+ cat > subrule_testing/BUILD <<EOF
+load(":rule.bzl", "my_rule")
+load(":helper.bzl", "print_rules")
+my_rule(name = 'foo')
+print_rules()
+EOF
+ bazel build --nobuild //subrule_testing:foo &> $TEST_log || fail "analysis failed"
+ expect_log 'rule: foo, attr: kind, value: my_rule'
+ expect_not_log '_foo'
+}
+
+run_suite "Tests for subrules"
