Remove type checking requirement from AttributeMap.has.
This overrides the traditional has(String name, Type<>T> type)
with has(String name) and removes the type check outright from
isConfigurable.
Ideally we'd remove the old version in this same change. But there
are enough uses of it that that's not a risk-free change and
is safer as followup changes.
--
PiperOrigin-RevId: 147513593
MOS_MIGRATED_REVID=147513593
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectAwareAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectAwareAttributeMapper.java
index 6d95f55..db7d6f0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AspectAwareAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectAwareAttributeMapper.java
@@ -86,12 +86,8 @@
}
@Override
- public <T> boolean isConfigurable(String attributeName, Type<T> type) {
- if (ruleAttributes.has(attributeName, type)) {
- return ruleAttributes.isConfigurable(attributeName, type);
- }
- // Any scenario aside from a "select(...)" in a BUILD file is not configurable.
- return false;
+ public boolean isConfigurable(String attributeName) {
+ return ruleAttributes.isConfigurable(attributeName);
}
@Override
@@ -154,7 +150,16 @@
}
@Override
- public boolean has(String attrName, Type<?> type) {
+ public boolean has(String attrName) {
+ if (ruleAttributes.has(attrName)) {
+ return true;
+ } else {
+ return aspectAttributes.containsKey(attrName);
+ }
+ }
+
+ @Override
+ public <T> boolean has(String attrName, Type<T> type) {
if (ruleAttributes.has(attrName, type)) {
return true;
} else {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
index a7b55b2..7837317 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
@@ -166,8 +166,9 @@
}
@Override
- public final <T> boolean isConfigurable(String attributeName, Type<T> type) {
- return getSelectorList(attributeName, type) != null;
+ public final boolean isConfigurable(String attributeName) {
+ Attribute attrDef = getAttributeDefinition(attributeName);
+ return attrDef == null ? false : getSelectorList(attributeName, attrDef.getType()) != null;
}
public static <T> boolean isConfigurable(Rule rule, String attributeName, Type<T> type) {
@@ -244,8 +245,13 @@
}
@Override
- public boolean has(String attrName, Type<?> type) {
+ public boolean has(String attrName) {
Attribute attribute = ruleClass.getAttributeByNameMaybe(attrName);
- return attribute != null && attribute.getType() == type;
+ return attribute != null;
+ }
+
+ @Override
+ public <T> boolean has(String attrName, Type<T> type) {
+ return getAttributeType(attrName) == type;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
index 17f8dad..e3a390e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
@@ -16,7 +16,6 @@
import com.google.common.base.Verify;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableList.Builder;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
@@ -460,8 +459,8 @@
}
@Override
- public <T> boolean isConfigurable(String attributeName, Type<T> type) {
- return owner.isConfigurable(attributeName, type);
+ public boolean isConfigurable(String attributeName) {
+ return owner.isConfigurable(attributeName);
}
@Override
@@ -525,7 +524,12 @@
}
@Override
- public boolean has(String attrName, Type<?> type) {
+ public boolean has(String attrName) {
+ return owner.has(attrName);
+ }
+
+ @Override
+ public <T> boolean has(String attrName, Type<T> type) {
return owner.has(attrName, type);
}
};
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java b/src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java
index 3e62f29..a573bf0 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java
@@ -39,6 +39,18 @@
Label getLabel();
/**
+ * Returns true if an attribute with the given name exists.
+ */
+ boolean has(String attrName);
+
+ /**
+ * Returns true if an attribute with the given name exists with the given type.
+ *
+ * <p>Don't use this version unless you really care about the type.
+ */
+ <T> boolean has(String attrName, Type<T> type);
+
+ /**
* Returns the value of the named rule attribute, which must be of the given type. This may
* be null (for example, for an attribute with no default value that isn't explicitly set in
* the rule - see {@link Type#getDefaultValue}).
@@ -51,9 +63,9 @@
/**
* Returns true if the given attribute is configurable for this rule instance, false
- * otherwise.
+ * if it isn't configurable or doesn't exist.
*/
- <T> boolean isConfigurable(String attributeName, Type<T> type);
+ boolean isConfigurable(String attributeName);
/**
* Returns the names of all attributes covered by this map.
@@ -115,9 +127,4 @@
String getPackageDefaultDeprecation();
ImmutableList<String> getPackageDefaultCopts();
-
- /**
- * @return true if an attribute with the given name and type is present
- */
- boolean has(String attrName, Type<?> type);
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/DelegatingAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/DelegatingAttributeMapper.java
index e57476c..3f0d2b0 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/DelegatingAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/DelegatingAttributeMapper.java
@@ -46,8 +46,8 @@
}
@Override
- public <T> boolean isConfigurable(String attributeName, Type<T> type) {
- return delegate.isConfigurable(attributeName, type);
+ public boolean isConfigurable(String attributeName) {
+ return delegate.isConfigurable(attributeName);
}
@Override
@@ -98,7 +98,12 @@
}
@Override
- public boolean has(String attrName, Type<?> type) {
+ public boolean has(String attrName) {
+ return delegate.has(attrName);
+ }
+
+ @Override
+ public <T> boolean has(String attrName, Type<T> type) {
return delegate.has(attrName, type);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/ImplicitOutputsFunction.java b/src/main/java/com/google/devtools/build/lib/packages/ImplicitOutputsFunction.java
index 89356a0..a2f0b6e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/ImplicitOutputsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/ImplicitOutputsFunction.java
@@ -89,7 +89,7 @@
Type<?> attrType = map.getAttributeType(attrName);
// Don't include configurable attributes: we don't know which value they might take
// since we don't yet have a build configuration.
- if (!map.isConfigurable(attrName, attrType)) {
+ if (!map.isConfigurable(attrName)) {
Object value = map.get(attrName, attrType);
attrValues.put(attrName, value == null ? Runtime.NONE : value);
}
@@ -291,7 +291,9 @@
}
Type<?> attrType = rule.getAttributeType(attrName);
- if (attrType == null) { return Collections.emptySet(); }
+ if (attrType == null) {
+ return Collections.emptySet();
+ }
// String attributes and lists are easy.
if (Type.STRING == attrType) {
return singleton(rule.get(attrName, Type.STRING));
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java
index cbe2702..eedaae0 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java
@@ -21,10 +21,8 @@
import com.google.devtools.build.lib.packages.BuildType.SelectorList;
import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.build.lib.util.Preconditions;
-
import java.util.Collection;
import java.util.List;
-
import javax.annotation.Nullable;
/**
@@ -69,7 +67,7 @@
@Nullable
public <T> Collection<T> getMergedValues(String attributeName, Type<List<T>> type) {
Preconditions.checkState(type instanceof Type.ListType);
- if (!isConfigurable(attributeName, type)) {
+ if (!isConfigurable(attributeName)) {
return get(attributeName, type);
}
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java b/src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java
index ac54a67..dd2bcbd 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java
@@ -168,8 +168,8 @@
for (Rule rule : orderedRuleList) {
RawAttributeMapper attributes = RawAttributeMapper.of(rule);
// We don't know which path to follow for configurable attributes, so skip them.
- if (attributes.isConfigurable("deps", BuildType.LABEL_LIST)
- || attributes.isConfigurable("srcs", BuildType.LABEL_LIST)) {
+ if (attributes.isConfigurable("deps")
+ || attributes.isConfigurable("srcs")) {
continue;
}
RuleClass ruleClass = rule.getRuleClassObject();
diff --git a/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java
index 81f2dd1..d20880c 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java
@@ -424,7 +424,7 @@
if ("name".equals(attr.getName())) {
continue;
}
- if (attributeMap.isConfigurable(attr.getName(), attr.getType())) {
+ if (attributeMap.isConfigurable(attr.getName())) {
continue; // TODO(bazel-team): handle configurable attributes.
}
PossibleAttributeValues values = getPossibleAttributeValues(rule, attr);
@@ -728,7 +728,7 @@
AggregatingAttributeMapper attributeMap = AggregatingAttributeMapper.of(rule);
if (attr.getType().equals(BuildType.LABEL_LIST)
- && attributeMap.isConfigurable(attr.getName(), attr.getType())) {
+ && attributeMap.isConfigurable(attr.getName())) {
// TODO(gregce): Expand this to all collection types (we don't do this for scalars because
// there's currently no syntax for expressing multiple scalar values). This unfortunately
// isn't trivial because Bazel's label visitation logic includes special methods built
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
index 30ef96e..76b57ef 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
@@ -128,9 +128,9 @@
.enableCcNativeLibrariesProvider()
.enableCompileProviders()
.enableInterfaceSharedObjects()
- // Generate .a and .so outputs even without object files to fulfill the rule class contract
- // wrt. implicit output files, if the contract says so. Behavior here differs between Bazel
- // and Blaze.
+ // Generate .a and .so outputs even without object files to fulfill the rule class
+ // contract wrt. implicit output files, if the contract says so. Behavior here differs
+ // between Bazel and Blaze.
.setGenerateLinkActionsIfEmpty(
ruleContext.getRule().getImplicitOutputsFunction() != ImplicitOutputsFunction.NONE)
.setLinkType(linkType)
@@ -188,7 +188,7 @@
ruleContext.registerAction(new FailAction(ruleContext.getActionOwner(),
ImmutableList.of(solibArtifact), "Toolchain does not support dynamic linking"));
} else if (!createDynamicLibrary
- && ruleContext.attributes().isConfigurable("srcs", BuildType.LABEL_LIST)) {
+ && ruleContext.attributes().isConfigurable("srcs")) {
// If "srcs" is configurable, the .so output is always declared because the logic that
// determines implicit outs doesn't know which value of "srcs" will ultimately get chosen.
// Here, where we *do* have the correct value, it may not contain any source files to
@@ -375,7 +375,7 @@
* name of a genrule that generates a source file.
*/
public static boolean appearsToHaveObjectFiles(AttributeMap rule) {
- if ((rule instanceof RawAttributeMapper) && rule.isConfigurable("srcs", BuildType.LABEL_LIST)) {
+ if ((rule instanceof RawAttributeMapper) && rule.isConfigurable("srcs")) {
// Since this method gets called by loading phase logic (e.g. the cc_library implicit outputs
// function), the attribute mapper may not be able to resolve configurable attributes. When
// that's the case, there's no way to know which value a configurable "srcs" will take, so
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JvmConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/java/JvmConfigurationLoader.java
index e82f20a..7ed45de 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JvmConfigurationLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JvmConfigurationLoader.java
@@ -122,7 +122,7 @@
NoSuchTargetException {
RawAttributeMapper javaHomeAttributes = RawAttributeMapper.of((Rule) javaHomeTarget);
- if (javaHomeAttributes.isConfigurable("srcs", BuildType.LABEL_LIST)) {
+ if (javaHomeAttributes.isConfigurable("srcs")) {
throw new InvalidConfigurationException(
String.format(
"\"srcs\" in %s is configurable. JAVABASE targets don't support configurable"
@@ -170,7 +170,7 @@
if ((jvmTarget instanceof Rule) && "filegroup".equals(((Rule) jvmTarget).getRuleClass())) {
RawAttributeMapper jvmTargetAttributes = RawAttributeMapper.of((Rule) jvmTarget);
- if (jvmTargetAttributes.isConfigurable("path", Type.STRING)) {
+ if (jvmTargetAttributes.isConfigurable("path")) {
throw new InvalidConfigurationException(
String.format(
"\"path\" in %s is configurable. JVM targets don't support configurable"
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectAwareAttributeMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectAwareAttributeMapperTest.java
index 9ab7b76..5cbaf47 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AspectAwareAttributeMapperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectAwareAttributeMapperTest.java
@@ -22,9 +22,7 @@
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.Rule;
-import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.build.lib.util.FileTypeSet;
-
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -104,8 +102,8 @@
@Test
public void isConfigurable() throws Exception {
- assertThat(mapper.isConfigurable("linkstatic", Type.BOOLEAN)).isTrue();
- assertThat(mapper.isConfigurable("fromaspect", BuildType.LABEL_LIST)).isFalse();
+ assertThat(mapper.isConfigurable("linkstatic")).isTrue();
+ assertThat(mapper.isConfigurable("fromaspect")).isFalse();
}
@Test
@@ -128,8 +126,8 @@
@Test
public void has() throws Exception {
- assertThat(mapper.has("srcs", BuildType.LABEL_LIST)).isTrue();
- assertThat(mapper.has("fromaspect", BuildType.LABEL)).isTrue();
+ assertThat(mapper.has("srcs")).isTrue();
+ assertThat(mapper.has("fromaspect")).isTrue();
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/select/RawAttributeMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/select/RawAttributeMapperTest.java
index 16ebc6c..04fda2f 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/select/RawAttributeMapperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/select/RawAttributeMapperTest.java
@@ -27,14 +27,12 @@
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.RawAttributeMapper;
import com.google.devtools.build.lib.packages.Rule;
-
+import java.util.List;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-import java.util.List;
-
/**
* Unit tests for {@link RawAttributeMapper}.
*/
@@ -89,8 +87,8 @@
@Test
public void testConfigurabilityCheck() throws Exception {
RawAttributeMapper rawMapper = RawAttributeMapper.of(setupGenRule());
- assertFalse(rawMapper.isConfigurable("data", BuildType.LABEL_LIST));
- assertTrue(rawMapper.isConfigurable("srcs", BuildType.LABEL_LIST));
+ assertFalse(rawMapper.isConfigurable("data"));
+ assertTrue(rawMapper.isConfigurable("srcs"));
}
/**
