Make a rule's `Location` consistent regardless of whether `generator_*` attributes are explicitly set.
The logic that detects whether `generator_name` or `generator_function` are already set dates back to python pre-processing and is no longer relevant, but it's still being used by rules that explicitly set one or both of these attributes. Remove this check to ensure that the location is determined consistently: from the outermost frame of the call stack.
Deriving the location directly from the call stack opens up the opportunity to avoid storing the `<toplevel>` frame in `CallStack`, since it is the same as the location.
Explicitly set `generator_*` attributes seems to happen in practice when attributes are copied from `native.existing_rule`. I will look into whether this can be cleaned up or hand it off if there's a volunteer.
RELNOTES: The location of rules that explicitly specify `generator_name` and/or `generator_function` attributes (typically because they are incidentally copied from `native.existing_rule()`) is now the top-level call in the `BUILD` file, which is consistent with rules that do not explicitly specify these attributes.
PiperOrigin-RevId: 521878142
Change-Id: Ie8fb7fa9c4bef9137f6e7d7a5c50fb049024a585
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
index c6d2fec..80881c1 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
@@ -98,21 +98,18 @@
ruleClass + " cannot be in the WORKSPACE file " + "(used by " + label + ")");
}
- AttributesAndLocation generator =
- generatorAttributesForMacros(pkgBuilder, attributeValues, callstack);
+ // TODO(b/273330483): The location doesn't need to be treated separately from the call stack.
+ Location location = callstack.get(0).location;
+
+ BuildLangTypedAttributeValuesMap attributes =
+ generatorAttributesForMacros(pkgBuilder, attributeValues, location, callstack);
// The raw stack is of the form [<toplevel>@BUILD:1, macro@lib.bzl:1, cc_library@<builtin>].
// Pop the innermost frame for the rule, since it's obvious.
callstack = callstack.subList(0, callstack.size() - 1); // pop
try {
- return ruleClass.createRule(
- pkgBuilder,
- label,
- generator.attributes,
- eventHandler,
- generator.location, // see b/23974287 for rationale
- callstack);
+ return ruleClass.createRule(pkgBuilder, label, attributes, eventHandler, location, callstack);
} catch (LabelSyntaxException | CannotPrecomputeDefaultsException e) {
throw new RuleFactory.InvalidRuleException(ruleClass + " " + e.getMessage());
}
@@ -160,17 +157,6 @@
}
}
- /** A pair of attributes and location. */
- private static final class AttributesAndLocation {
- final BuildLangTypedAttributeValuesMap attributes;
- final Location location;
-
- AttributesAndLocation(BuildLangTypedAttributeValuesMap attributes, Location location) {
- this.attributes = attributes;
- this.location = location;
- }
- }
-
/**
* A wrapper around an map of named attribute values that specifies whether the map's values are
* of "build-language" or of "native" types.
@@ -236,57 +222,42 @@
}
/**
- * If the rule was created by a macro, this method sets the appropriate values for the attributes
- * generator_{name, function, location} and returns all attributes.
+ * If the rule was created by a macro, sets the appropriate value for the generator_name attribute
+ * and returns all attributes.
*
* <p>Otherwise, it returns the given attributes without any changes.
*/
- private static AttributesAndLocation generatorAttributesForMacros(
+ private static BuildLangTypedAttributeValuesMap generatorAttributesForMacros(
Package.Builder pkgBuilder,
BuildLangTypedAttributeValuesMap args,
+ Location location,
ImmutableList<CallStackEntry> stack) {
- // For a callstack [BUILD <toplevel>, .bzl <function>, <rule>],
- // location is that of the caller of 'rule' (the .bzl function).
- Location location = stack.size() < 2 ? Location.BUILTIN : stack.get(stack.size() - 2).location;
-
- boolean hasName = args.containsAttributeNamed("generator_name");
- boolean hasFunc = args.containsAttributeNamed("generator_function");
- // TODO(bazel-team): resolve cases in our code where hasName && !hasFunc, or hasFunc && !hasName
- if (hasName || hasFunc) {
- return new AttributesAndLocation(args, location);
- }
-
- // The "generator" of a rule is the function (sometimes called "macro")
- // outermost in the call stack.
- // The stack must contain at least two entries:
+ // The "generator" of a rule is the function (sometimes called "macro") outermost in the call
+ // stack. For rules with generators, the stack must contain at least two entries:
// 0: the outermost function (e.g. a BUILD file),
// 1: the function called by it (e.g. a "macro" in a .bzl file).
- // optionally followed by other Starlark or built-in functions,
- // and finally the rule instantiation function.
+ // optionally followed by other Starlark or built-in functions, and finally the rule
+ // instantiation function.
if (stack.size() < 2 || !stack.get(1).location.file().endsWith(".bzl")) {
- return new AttributesAndLocation(args, location); // macro is not a Starlark function
+ return args; // Not instantiated by a Starlark macro.
}
- Location generatorLocation = stack.get(0).location; // location of call to generator
- ImmutableMap.Builder<String, Object> builder = ImmutableMap.builder();
- for (Map.Entry<String, Object> attributeAccessor : args.getAttributeAccessors()) {
- String attributeName = args.getName(attributeAccessor);
- builder.put(attributeName, args.getValue(attributeAccessor));
+
+ if (args.containsAttributeNamed("generator_name")) {
+ // generator_name is explicitly set. Return early to avoid a map key conflict.
+ // TODO(b/274802222): Should this be prohibited?
+ return args;
}
- String generatorName = pkgBuilder.getGeneratorNameByLocation(generatorLocation);
+
+ ImmutableMap.Builder<String, Object> builder =
+ ImmutableMap.builderWithExpectedSize(args.attributeValues.size() + 1);
+ builder.putAll(args.attributeValues);
+
+ String generatorName = pkgBuilder.getGeneratorNameByLocation(location);
if (generatorName == null) {
generatorName = (String) args.getAttributeValue("name");
}
builder.put("generator_name", generatorName);
- try {
- args = new BuildLangTypedAttributeValuesMap(builder.buildOrThrow());
- } catch (IllegalArgumentException unused) {
- // We just fall back to the default case and swallow any messages.
- }
-
- // TODO(adonovan): is it appropriate to use generatorLocation as the rule's main location?
- // Or would 'location' (the immediate call) be more informative? When there are errors, the
- // location of the toplevel call of the generator may be quite unrelated to the error message.
- return new AttributesAndLocation(args, generatorLocation);
+ return new BuildLangTypedAttributeValuesMap(builder.buildOrThrow());
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
index 8429bde..4608f3a 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
@@ -32,6 +32,8 @@
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.RootedPath;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
@@ -40,9 +42,8 @@
import net.starlark.java.syntax.Location;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-@RunWith(JUnit4.class)
+@RunWith(TestParameterInjector.class)
public final class RuleFactoryTest extends PackageLoadingTestCase {
private final ConfiguredRuleClassProvider provider = TestRuleClassProvider.getRuleClassProvider();
@@ -70,7 +71,7 @@
}
@Test
- public void testCreateRule() throws Exception {
+ public void testCreateRule(@TestParameter boolean explicitlySetGeneratorAttrs) throws Exception {
Path myPkgPath = scratch.resolve("/workspace/mypkg/BUILD");
Package.Builder pkgBuilder = newBuilder(PackageIdentifier.createInMainRepo("mypkg"), myPkgPath);
@@ -78,6 +79,12 @@
attributeValues.put("name", "foo");
attributeValues.put("alwayslink", true);
+ // TODO(b/274802222): Should this be prohibited?
+ if (explicitlySetGeneratorAttrs) {
+ attributeValues.put("generator_name", "fake_generator_name");
+ attributeValues.put("generator_function", "fake_generator_function");
+ }
+
RuleClass ruleClass = provider.getRuleClassMap().get("cc_library");
Rule rule =
RuleFactory.createAndAddRule(
