From 33439dc24284678f1a127665d9e6554b2bbe7966 Mon Sep 17 00:00:00 2001 From: janakr Date: Thu, 22 Mar 2018 13:44:05 -0700 Subject: 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 --- .../build/lib/analysis/skylark/SkylarkAttr.java | 65 ++++---- .../devtools/build/lib/packages/Attribute.java | 167 ++++++++++++++++----- .../devtools/build/lib/syntax/Environment.java | 43 +++++- 3 files changed, 199 insertions(+), 76 deletions(-) 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 a2fefdf7ca..d4b986bd86 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.cmdline.Label; 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.BuildType; 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 @@ public final class SkylarkAttr implements SkylarkValue { } } - private static Attribute.Builder createAttribute( + private static ImmutableAttributeFactory createAttributeFactory( Type type, SkylarkDict 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 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 @@ public final class SkylarkAttr implements SkylarkValue { 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 @@ public final class SkylarkAttr implements SkylarkValue { 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 @@ public final class SkylarkAttr implements SkylarkValue { throws EvalException { env.checkLoadingOrWorkspacePhase("attr.label", ast.getLocation()); try { - Attribute.Builder attribute = - createAttribute( + ImmutableAttributeFactory attribute = + createAttributeFactory( BuildType.LABEL, EvalUtils.optionMap( env, @@ -1102,8 +1119,8 @@ public final class SkylarkAttr implements SkylarkValue { 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 @@ public final class SkylarkAttr implements SkylarkValue { 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 @@ public final class SkylarkAttr implements SkylarkValue { + "rule or an " + "aspect." ) + @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 b7f2ca5f42..717b74bedc 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 @@ public final class Attribute implements Comparable { 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 condition; + private final ImmutableSet propertyFlags; + private final PredicateWithMessage allowedValues; + private final RequiredProviders requiredProviders; + private final ImmutableList> aspects; + + @AutoCodec.VisibleForSerialization + ImmutableAttributeFactory( + Type type, + ImmutableSet propertyFlags, + Object value, + ConfigurationTransition configTransition, + RuleClassNamePredicate allowedRuleClassesForLabels, + RuleClassNamePredicate allowedRuleClassesForLabelsWarning, + SplitTransitionProvider splitTransitionProvider, + FileTypeSet allowedFileTypesForLabels, + ValidityPredicate validityPredicate, + AttributeValueSource valueSource, + boolean valueSet, + Predicate condition, + PredicateWithMessage allowedValues, + RequiredProviders requiredProviders, + ImmutableList> 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 @@ public final class Attribute implements Comparable { 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 @@ public final class Attribute implements Comparable { 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 c882743e78..76b4f8b4f5 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.EventKind; 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 @@ public final class Environment implements Freezable { && 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 @@ public final class Environment implements Freezable { 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 thisDict = (SkylarkDict) value; + @SuppressWarnings("unchecked") + SkylarkDict otherDict = (SkylarkDict) 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( -- cgit v1.2.3