Use an immutable Attribute factory in objects that are persisted to Skyframe, rather than the potentially mutable builder, and @AutoCodec SkylarkAttr.Descriptor.
PiperOrigin-RevId: 190118565
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java
index a2fefdf..d4b986b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java
@@ -26,6 +26,7 @@
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
+import com.google.devtools.build.lib.packages.Attribute.ImmutableAttributeFactory;
import com.google.devtools.build.lib.packages.Attribute.SkylarkComputedDefaultTemplate;
import com.google.devtools.build.lib.packages.Attribute.SplitTransitionProvider;
import com.google.devtools.build.lib.packages.AttributeValueSource;
@@ -33,6 +34,7 @@
import com.google.devtools.build.lib.packages.Provider;
import com.google.devtools.build.lib.packages.SkylarkAspect;
import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skylarkinterface.Param;
import com.google.devtools.build.lib.skylarkinterface.ParamType;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
@@ -171,12 +173,22 @@
}
}
- private static Attribute.Builder<?> createAttribute(
+ private static ImmutableAttributeFactory createAttributeFactory(
Type<?> type, SkylarkDict<String, Object> arguments, FuncallExpression ast, Environment env)
throws EvalException {
// We use an empty name now so that we can set it later.
// This trick makes sense only in the context of Skylark (builtin rules should not use it).
- return createAttribute(type, arguments, ast, env, "");
+ return createAttributeFactory(type, arguments, ast, env, "");
+ }
+
+ private static ImmutableAttributeFactory createAttributeFactory(
+ Type<?> type,
+ SkylarkDict<String, Object> arguments,
+ FuncallExpression ast,
+ Environment env,
+ String name)
+ throws EvalException {
+ return createAttribute(type, arguments, ast, env, name).buildPartial();
}
private static Attribute.Builder<?> createAttribute(
@@ -412,7 +424,7 @@
Environment env)
throws EvalException {
try {
- return new Descriptor(name, createAttribute(type, kwargs, ast, env));
+ return new Descriptor(name, createAttributeFactory(type, kwargs, ast, env));
} catch (ConversionException e) {
throw new EvalException(ast.getLocation(), e.getMessage());
}
@@ -444,8 +456,13 @@
String whyNotConfigurableReason =
Preconditions.checkNotNull(maybeGetNonConfigurableReason(type), type);
try {
+ // We use an empty name now so that we can set it later.
+ // This trick makes sense only in the context of Skylark (builtin rules should not use it).
return new Descriptor(
- name, createAttribute(type, kwargs, ast, env).nonconfigurable(whyNotConfigurableReason));
+ name,
+ createAttribute(type, kwargs, ast, env, "")
+ .nonconfigurable(whyNotConfigurableReason)
+ .buildPartial());
} catch (ConversionException e) {
throw new EvalException(ast.getLocation(), e.getMessage());
}
@@ -768,8 +785,8 @@
throws EvalException {
env.checkLoadingOrWorkspacePhase("attr.label", ast.getLocation());
try {
- Attribute.Builder<?> attribute =
- createAttribute(
+ ImmutableAttributeFactory attribute =
+ createAttributeFactory(
BuildType.LABEL,
EvalUtils.<String, Object>optionMap(
env,
@@ -1102,8 +1119,8 @@
ASPECTS_ARG,
aspects);
try {
- Attribute.Builder<?> attribute =
- createAttribute(BuildType.LABEL_LIST, kwargs, ast, env, "label_list");
+ ImmutableAttributeFactory attribute =
+ createAttributeFactory(BuildType.LABEL_LIST, kwargs, ast, env, "label_list");
return new Descriptor(getName(), attribute);
} catch (EvalException e) {
throw new EvalException(ast.getLocation(), e.getMessage(), e);
@@ -1263,8 +1280,8 @@
ASPECTS_ARG,
aspects);
try {
- Attribute.Builder<?> attribute =
- createAttribute(
+ ImmutableAttributeFactory attribute =
+ createAttributeFactory(
BuildType.LABEL_KEYED_STRING_DICT, kwargs, ast, env, "label_keyed_string_dict");
return new Descriptor(this.getName(), attribute);
} catch (EvalException e) {
@@ -1681,39 +1698,27 @@
+ "<a href=\"globals.html#rule\">rule</a> or an "
+ "<a href=\"globals.html#aspect\">aspect</a>."
)
+ @AutoCodec
public static final class Descriptor implements SkylarkValue {
- private final Attribute.Builder<?> attributeBuilder;
-
- /**
- * This lock guards {@code attributeBuilder} field.
- *
- * {@link Attribute.Builder} class is not thread-safe for concurrent modification.
- */
- private final Object lock = new Object();
-
+ private final ImmutableAttributeFactory attributeFactory;
private final String name;
- public Descriptor(String name, Attribute.Builder<?> attributeBuilder) {
- this.attributeBuilder = attributeBuilder;
+ @AutoCodec.VisibleForSerialization
+ Descriptor(String name, ImmutableAttributeFactory attributeFactory) {
+ this.attributeFactory = Preconditions.checkNotNull(attributeFactory);
this.name = name;
}
public boolean hasDefault() {
- synchronized (lock) {
- return attributeBuilder.isValueSet();
+ return attributeFactory.isValueSet();
}
- }
public AttributeValueSource getValueSource() {
- synchronized (lock) {
- return attributeBuilder.getValueSource();
- }
+ return attributeFactory.getValueSource();
}
public Attribute build(String name) {
- synchronized (lock) {
- return attributeBuilder.build(name);
- }
+ return attributeFactory.build(name);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
index b7f2ca5..717b74b 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -361,6 +361,108 @@
return new Builder<>(name, type);
}
+ /** A factory to generate {@link Attribute} instances. */
+ @AutoCodec
+ public static class ImmutableAttributeFactory {
+ private final Type<?> type;
+ private final ConfigurationTransition configTransition;
+ private final RuleClassNamePredicate allowedRuleClassesForLabels;
+ private final RuleClassNamePredicate allowedRuleClassesForLabelsWarning;
+ private final SplitTransitionProvider splitTransitionProvider;
+ private final FileTypeSet allowedFileTypesForLabels;
+ private final ValidityPredicate validityPredicate;
+ private final Object value;
+ private final AttributeValueSource valueSource;
+ private final boolean valueSet;
+ private final Predicate<AttributeMap> condition;
+ private final ImmutableSet<PropertyFlag> propertyFlags;
+ private final PredicateWithMessage<Object> allowedValues;
+ private final RequiredProviders requiredProviders;
+ private final ImmutableList<RuleAspect<?>> aspects;
+
+ @AutoCodec.VisibleForSerialization
+ ImmutableAttributeFactory(
+ Type<?> type,
+ ImmutableSet<PropertyFlag> propertyFlags,
+ Object value,
+ ConfigurationTransition configTransition,
+ RuleClassNamePredicate allowedRuleClassesForLabels,
+ RuleClassNamePredicate allowedRuleClassesForLabelsWarning,
+ SplitTransitionProvider splitTransitionProvider,
+ FileTypeSet allowedFileTypesForLabels,
+ ValidityPredicate validityPredicate,
+ AttributeValueSource valueSource,
+ boolean valueSet,
+ Predicate<AttributeMap> condition,
+ PredicateWithMessage<Object> allowedValues,
+ RequiredProviders requiredProviders,
+ ImmutableList<RuleAspect<?>> aspects) {
+ this.type = type;
+ this.configTransition = configTransition;
+ this.allowedRuleClassesForLabels = allowedRuleClassesForLabels;
+ this.allowedRuleClassesForLabelsWarning = allowedRuleClassesForLabelsWarning;
+ this.splitTransitionProvider = splitTransitionProvider;
+ this.allowedFileTypesForLabels = allowedFileTypesForLabels;
+ this.validityPredicate = validityPredicate;
+ this.value = value;
+ this.valueSource = valueSource;
+ this.valueSet = valueSet;
+ this.condition = condition;
+ this.propertyFlags = propertyFlags;
+ this.allowedValues = allowedValues;
+ this.requiredProviders = requiredProviders;
+ this.aspects = aspects;
+ }
+
+ public AttributeValueSource getValueSource() {
+ return valueSource;
+ }
+
+ public boolean isValueSet() {
+ return valueSet;
+ }
+
+ public Attribute build(String name) {
+ Preconditions.checkState(!name.isEmpty(), "name has not been set");
+ if (valueSource == AttributeValueSource.LATE_BOUND) {
+ Preconditions.checkState(isLateBound(name));
+ }
+ // TODO(bazel-team): Set the default to be no file type, then remove this check, and also
+ // remove all allowedFileTypes() calls without parameters.
+
+ // do not modify this.allowedFileTypesForLabels, instead create a copy.
+ FileTypeSet allowedFileTypesForLabels = this.allowedFileTypesForLabels;
+ if (type.getLabelClass() == LabelClass.DEPENDENCY) {
+ if (isPrivateAttribute(name) && allowedFileTypesForLabels == null) {
+ allowedFileTypesForLabels = FileTypeSet.ANY_FILE;
+ }
+ Preconditions.checkNotNull(
+ allowedFileTypesForLabels, "allowedFileTypesForLabels not set for %s", name);
+ } else if (type.getLabelClass() == LabelClass.OUTPUT) {
+ // TODO(bazel-team): Set the default to no file type and make explicit calls instead.
+ if (allowedFileTypesForLabels == null) {
+ allowedFileTypesForLabels = FileTypeSet.ANY_FILE;
+ }
+ }
+
+ return new Attribute(
+ name,
+ type,
+ propertyFlags,
+ value,
+ configTransition,
+ splitTransitionProvider,
+ allowedRuleClassesForLabels,
+ allowedRuleClassesForLabelsWarning,
+ allowedFileTypesForLabels,
+ validityPredicate,
+ condition,
+ allowedValues,
+ requiredProviders,
+ aspects);
+ }
+ }
+
/**
* A fluent builder for the {@code Attribute} instances.
*
@@ -1026,44 +1128,8 @@
return setPropertyFlag(PropertyFlag.NONCONFIGURABLE, "nonconfigurable");
}
- /**
- * Creates the attribute. Uses name, type, optionality, configuration type
- * and the default value configured by the builder.
- */
- public Attribute build() {
- return build(this.name);
- }
-
- /**
- * Creates the attribute. Uses type, optionality, configuration type
- * and the default value configured by the builder. Use the name
- * passed as an argument. This function is used by Skylark where the
- * name is provided only when we build. We don't want to modify the
- * builder, as it is shared in a multithreaded environment.
- */
- public Attribute build(String name) {
- Preconditions.checkState(!name.isEmpty(), "name has not been set");
- if (valueSource == AttributeValueSource.LATE_BOUND) {
- Preconditions.checkState(isLateBound(name));
- }
- // TODO(bazel-team): Set the default to be no file type, then remove this check, and also
- // remove all allowedFileTypes() calls without parameters.
-
- // do not modify this.allowedFileTypesForLabels, instead create a copy.
- FileTypeSet allowedFileTypesForLabels = this.allowedFileTypesForLabels;
- if (type.getLabelClass() == LabelClass.DEPENDENCY) {
- if (isPrivateAttribute(name) && allowedFileTypesForLabels == null) {
- allowedFileTypesForLabels = FileTypeSet.ANY_FILE;
- }
- Preconditions.checkNotNull(
- allowedFileTypesForLabels, "allowedFileTypesForLabels not set for %s", name);
- } else if (type.getLabelClass() == LabelClass.OUTPUT) {
- // TODO(bazel-team): Set the default to no file type and make explicit calls instead.
- if (allowedFileTypesForLabels == null) {
- allowedFileTypesForLabels = FileTypeSet.ANY_FILE;
- }
- }
-
+ /** Returns an {@link ImmutableAttributeFactory} that can be invoked to create attributes. */
+ public ImmutableAttributeFactory buildPartial() {
Preconditions.checkState(
!allowedRuleClassesForLabels.consideredOverlapping(allowedRuleClassesForLabelsWarning),
"allowedRuleClasses %s and allowedRuleClassesWithWarning %s "
@@ -1071,22 +1137,41 @@
allowedRuleClassesForLabels,
allowedRuleClassesForLabelsWarning);
- return new Attribute(
- name,
+ return new ImmutableAttributeFactory(
type,
Sets.immutableEnumSet(propertyFlags),
valueSet ? value : type.getDefaultValue(),
configTransition,
- splitTransitionProvider,
allowedRuleClassesForLabels,
allowedRuleClassesForLabelsWarning,
+ splitTransitionProvider,
allowedFileTypesForLabels,
validityPredicate,
+ valueSource,
+ valueSet,
condition,
allowedValues,
requiredProvidersBuilder.build(),
ImmutableList.copyOf(aspects.values()));
}
+
+ /**
+ * Creates the attribute. Uses name, type, optionality, configuration type and the default value
+ * configured by the builder.
+ */
+ public Attribute build() {
+ return build(this.name);
+ }
+
+ /**
+ * Creates the attribute. Uses type, optionality, configuration type and the default value
+ * configured by the builder. Use the name passed as an argument. This function is used by
+ * Skylark where the name is provided only when we build. We don't want to modify the builder,
+ * as it is shared in a multithreaded environment.
+ */
+ public Attribute build(String name) {
+ return buildPartial().build(name);
+ }
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
index c882743..76b4f8b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
@@ -26,6 +26,7 @@
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.Memoization;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
import com.google.devtools.build.lib.syntax.Mutability.Freezable;
import com.google.devtools.build.lib.syntax.Mutability.MutabilityException;
import com.google.devtools.build.lib.util.Fingerprint;
@@ -526,6 +527,14 @@
&& bindings.equals(other.getBindings());
}
+ private static boolean skylarkObjectsProbablyEqual(Object obj1, Object obj2) {
+ // TODO(b/76154791): check this more carefully.
+ return obj1.equals(obj2)
+ || (obj1 instanceof SkylarkValue
+ && obj2 instanceof SkylarkValue
+ && Printer.repr(obj1).equals(Printer.repr(obj2)));
+ }
+
/**
* Throws {@link IllegalStateException} if this {@link Extension} is not equal to {@code obj}.
*
@@ -560,11 +569,35 @@
if (value.equals(otherValue)) {
continue;
}
- if (value instanceof SkylarkNestedSet
- && otherValue instanceof SkylarkNestedSet
- && (((SkylarkNestedSet) value)
- .toCollection()
- .equals(((SkylarkNestedSet) otherValue).toCollection()))) {
+ if (value instanceof SkylarkNestedSet) {
+ if (otherValue instanceof SkylarkNestedSet
+ && ((SkylarkNestedSet) value)
+ .toCollection()
+ .equals(((SkylarkNestedSet) otherValue).toCollection())) {
+ continue;
+ }
+ } else if (value instanceof SkylarkDict) {
+ if (otherValue instanceof SkylarkDict) {
+ @SuppressWarnings("unchecked")
+ SkylarkDict<Object, Object> thisDict = (SkylarkDict<Object, Object>) value;
+ @SuppressWarnings("unchecked")
+ SkylarkDict<Object, Object> otherDict = (SkylarkDict<Object, Object>) otherValue;
+ if (thisDict.size() == otherDict.size()
+ && thisDict.keySet().equals(otherDict.keySet())) {
+ boolean foundProblem = false;
+ for (Object key : thisDict.keySet()) {
+ if (!skylarkObjectsProbablyEqual(
+ Preconditions.checkNotNull(thisDict.get(key), key),
+ Preconditions.checkNotNull(otherDict.get(key), key))) {
+ foundProblem = true;
+ }
+ }
+ if (!foundProblem) {
+ continue;
+ }
+ }
+ }
+ } else if (skylarkObjectsProbablyEqual(value, otherValue)) {
continue;
}
badEntries.add(
