bazel packages: delete StructImpl.hasField
Essentially every use is followed by a call to getField,
so it is unnecessary and inefficient to make two calls,
and risks potential inconsistency.
Also delete getValueOrNull.
@VisibleForTesting isn't worth the paper it is printed on.
PiperOrigin-RevId: 284208235
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
index c61fd98..8247ec4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
@@ -347,7 +347,7 @@
StructImpl struct = (StructImpl) target;
oldStyleProviders = struct;
- if (struct.hasField("providers")) {
+ if (struct.getValue("providers") != null) {
Iterable<?> iterable = cast("providers", struct, Iterable.class, loc);
for (Object o : iterable) {
Info declaredProvider =
diff --git a/src/main/java/com/google/devtools/build/lib/packages/NativeInfo.java b/src/main/java/com/google/devtools/build/lib/packages/NativeInfo.java
index a05d9b9..867e18a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/NativeInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/NativeInfo.java
@@ -41,10 +41,14 @@
@Override
public Object getValue(String name) throws EvalException {
+ // Starlark field
Object x = values.get(name);
if (x != null) {
return x;
- } else if (hasField(name)) {
+ }
+
+ // @SkylarkCallable(structField=true) -- Java field
+ if (getFieldNames().contains(name)) {
try {
return CallUtils.getField(SEMANTICS, this, name);
} catch (InterruptedException exception) {
@@ -52,28 +56,32 @@
// exceptions, as they should be logicless field accessors. If this occurs, it's
// indicative of a bad NativeInfo implementation.
throw new IllegalStateException(
- String.format("Access of field %s was unexpectedly interrupted, but should be "
- + "uninterruptible. This is indicative of a bad provider implementation.", name));
+ String.format(
+ "Access of field %s was unexpectedly interrupted, but should be "
+ + "uninterruptible. This is indicative of a bad provider implementation.",
+ name));
}
- } else if (name.equals("to_json") || name.equals("to_proto")) {
- // to_json and to_proto should not be methods of struct or provider instances.
- // However, they are, for now, and it is important that they be consistently
- // returned by attribute lookup operations regardless of whether a field or method
- // is desired. TODO(adonovan): eliminate this hack.
- return new BuiltinCallable(this, name);
- } else {
- return null;
}
- }
- @Override
- public boolean hasField(String name) {
- return getFieldNames().contains(name);
+ // to_json and to_proto should not be methods of struct or provider instances.
+ // However, they are, for now, and it is important that they be consistently
+ // returned by attribute lookup operations regardless of whether a field or method
+ // is desired. TODO(adonovan): eliminate this hack.
+ if (name.equals("to_json") || name.equals("to_proto")) {
+ return new BuiltinCallable(this, name);
+ }
+
+ return null;
}
@Override
public ImmutableCollection<String> getFieldNames() {
if (fieldNames == null) {
+ // TODO(adonovan): the assignment to this.fieldNames is not thread safe!
+ // We cannot assume that build() is a constructor of an object all of
+ // whose fields are final (and thus subject to a write barrier).
+ //
+ // Also, consider using a lazy union of the two underlying sets.
fieldNames =
ImmutableSet.<String>builder()
.addAll(values.keySet())
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java
index 53f4796..df63ea5 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java
@@ -243,11 +243,6 @@
return map.size();
}
- /** Returns whether or not a field is mentioned in the layout. */
- public boolean hasField(String field) {
- return map.containsKey(field);
- }
-
/**
* Returns the index position associated with the given field, or null if the field is not
* mentioned by the layout.
@@ -292,11 +287,6 @@
}
@Override
- public boolean hasField(String name) {
- return values.containsKey(name);
- }
-
- @Override
public Object getValue(String name) {
return values.get(name);
}
@@ -365,12 +355,6 @@
}
@Override
- public boolean hasField(String name) {
- Integer index = layout.getFieldIndex(name);
- return index != null && values[index] != null;
- }
-
- @Override
public ImmutableCollection<String> getFieldNames() {
ImmutableSet.Builder<String> result = ImmutableSet.builder();
for (Map.Entry<String, Integer> entry : layout.entrySet()) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java b/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java
index 87a1a10..b03a5f0 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.packages;
-import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.base.Objects;
import com.google.common.base.Preconditions;
@@ -82,26 +81,6 @@
}
/**
- * Returns whether the given field name exists.
- *
- * <p>This conceptually extends the API for {@link ClassObject}.
- */
- public abstract boolean hasField(String name);
-
- /**
- * <p>Wraps {@link ClassObject#getValue(String)}, returning null in cases where
- * {@link EvalException} would have been thrown.
- */
- @VisibleForTesting
- public Object getValueOrNull(String name) {
- try {
- return getValue(name);
- } catch (EvalException e) {
- return null;
- }
- }
-
- /**
* Returns the result of {@link #getValue(String)}, cast as the given type, throwing {@link
* EvalException} if the cast fails.
*/
@@ -197,6 +176,14 @@
printer.append(")");
}
+ private Object getValueOrNull(String name) {
+ try {
+ return getValue(name);
+ } catch (EvalException e) {
+ return null;
+ }
+ }
+
@Override
public String toProto(Location loc) throws EvalException {
StringBuilder sb = new StringBuilder();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
index ea1bf79..6f7b470 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
@@ -57,6 +57,7 @@
import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
import com.google.devtools.build.lib.rules.cpp.Link.LinkingMode;
import com.google.devtools.build.lib.skylarkbuildapi.cpp.CcModuleApi;
+import com.google.devtools.build.lib.syntax.ClassObject;
import com.google.devtools.build.lib.syntax.Depset;
import com.google.devtools.build.lib.syntax.Dict;
import com.google.devtools.build.lib.syntax.EvalException;
@@ -882,17 +883,25 @@
/** Checks whether the {@link SkylarkInfo} is of the required type. */
private static void checkRightProviderType(SkylarkInfo provider, String type)
throws EvalException {
- String providerType = (String) provider.getValueOrNull("type_name");
+ String providerType = (String) getValueOrNull(provider, "type_name");
if (providerType == null) {
providerType = provider.getProvider().getPrintableName();
}
- if (!provider.hasField("type_name") || !provider.getValue("type_name").equals(type)) {
+ if (!type.equals(provider.getValue("type_name"))) {
throw new EvalException(
provider.getCreationLoc(),
String.format("Expected object of type '%s', received '%s'.", type, providerType));
}
}
+ private static Object getValueOrNull(ClassObject x, String name) {
+ try {
+ return x.getValue(name);
+ } catch (EvalException e) {
+ return null;
+ }
+ }
+
/** Creates a {@link Feature} from a {@link SkylarkInfo}. */
@VisibleForTesting
static Feature featureFromSkylark(SkylarkInfo featureStruct) throws EvalException {
@@ -939,8 +948,7 @@
ImmutableList<SkylarkInfo> requires =
getSkylarkProviderListFromSkylarkField(featureStruct, "requires");
for (SkylarkInfo featureSetStruct : requires) {
- if (!featureSetStruct.hasField("type_name")
- || !featureSetStruct.getValue("type_name").equals("feature_set")) {
+ if (!"feature_set".equals(featureSetStruct.getValue("type_name"))) { // getValue() may be null
throw new EvalException(
featureStruct.getCreationLoc(), "expected object of type 'feature_set'.");
}
@@ -1290,7 +1298,7 @@
private static <T> T getFieldFromSkylarkProvider(
SkylarkInfo provider, String fieldName, Class<T> clazz) throws EvalException {
- Object obj = provider.getValueOrNull(fieldName);
+ Object obj = provider.getValue(fieldName);
if (obj == null) {
throw new EvalException(
provider.getCreationLoc(), String.format("Missing mandatory field '%s'", fieldName));
@@ -1309,28 +1317,25 @@
/** Returns a list of strings from a field of a {@link SkylarkInfo}. */
private static ImmutableList<String> getStringListFromSkylarkProviderField(
SkylarkInfo provider, String fieldName) throws EvalException {
- return Sequence.castSkylarkListOrNoneToList(
- provider.getValueOrNull(fieldName), String.class, fieldName)
- .stream()
- .collect(ImmutableList.toImmutableList());
+ return ImmutableList.copyOf(
+ Sequence.castSkylarkListOrNoneToList(
+ getValueOrNull(provider, fieldName), String.class, fieldName));
}
/** Returns a set of strings from a field of a {@link SkylarkInfo}. */
private static ImmutableSet<String> getStringSetFromSkylarkProviderField(
SkylarkInfo provider, String fieldName) throws EvalException {
- return Sequence.castSkylarkListOrNoneToList(
- provider.getValueOrNull(fieldName), String.class, fieldName)
- .stream()
- .collect(ImmutableSet.toImmutableSet());
+ return ImmutableSet.copyOf(
+ Sequence.castSkylarkListOrNoneToList(
+ getValueOrNull(provider, fieldName), String.class, fieldName));
}
/** Returns a list of SkylarkInfo providers from a field of a {@link SkylarkInfo}. */
private static ImmutableList<SkylarkInfo> getSkylarkProviderListFromSkylarkField(
SkylarkInfo provider, String fieldName) throws EvalException {
- return Sequence.castSkylarkListOrNoneToList(
- provider.getValueOrNull(fieldName), SkylarkInfo.class, fieldName)
- .stream()
- .collect(ImmutableList.toImmutableList());
+ return ImmutableList.copyOf(
+ Sequence.castSkylarkListOrNoneToList(
+ getValueOrNull(provider, fieldName), SkylarkInfo.class, fieldName));
}
private static void getLegacyArtifactNamePatterns(
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java b/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java
index 522ecaa..96a2a07 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java
@@ -18,6 +18,7 @@
import javax.annotation.Nullable;
/** An interface for Skylark objects (such as structs) that have fields. */
+// TODO(adonovan): rename "HasFields".
public interface ClassObject {
/**
@@ -25,6 +26,7 @@
*
* @throws EvalException if a user-visible error occurs (other than non-existent field).
*/
+ // TODO(adonovan): rename "getField".
@Nullable
Object getValue(String name) throws EvalException;
@@ -33,6 +35,7 @@
*
* @throws EvalException if a user-visible error occurs
*/
+ // TODO(adonovan): change type to ImmutableSet. Users rely on O(1) lookup.
ImmutableCollection<String> getFieldNames() throws EvalException;
/**
