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

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<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 @@ 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.<String, Object>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 {
             + "<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 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<Attribute> {
     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 @@ public final class Attribute implements Comparable<Attribute> {
       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<Attribute> {
           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<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(