Tag Attribute, SkylarkDefinedAspect and some other Skylark classes with

@AutoCodec. Remove Environment from SkylarkDefinedAspect.

Add a note to BuiltInFunction that we shouldn't need to serialize it because
all instances should be constant.

PiperOrigin-RevId: 188763259

6 files changed, 115 insertions, 56 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
index c689435365..fd60a1835b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
@@ -70,6 +70,8 @@ import com.google.devtools.build.lib.packages.SkylarkExportable;
 import com.google.devtools.build.lib.packages.SkylarkProvider;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.packages.TestSize;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
 import com.google.devtools.build.lib.skylarkinterface.Param;
 import com.google.devtools.build.lib.skylarkinterface.ParamType;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
@@ -844,12 +846,16 @@ public class SkylarkRuleClassFunctions {
               ImmutableSet.copyOf(fragments.getContents(String.class, "fragments")),
               HostTransition.INSTANCE,
               ImmutableSet.copyOf(hostFragments.getContents(String.class, "host_fragments")),
-              collectToolchainLabels(toolchains, ast),
-              funcallEnv);
+              collectToolchainLabels(toolchains, ast));
         }
       };
 
-  /** The implementation for the magic function "rule" that creates Skylark rule classes */
+  /**
+   * The implementation for the magic function "rule" that creates Skylark rule classes.
+   *
+   * <p>Exactly one of {@link #builder} or {@link #ruleClass} is null except inside {@link #export}.
+   */
+  @AutoCodec
   public static final class SkylarkRuleFunction extends BaseFunction
       implements SkylarkExportable, RuleFunction {
     private RuleClass.Builder builder;
@@ -872,6 +878,28 @@ public class SkylarkRuleClassFunctions {
       this.definitionLocation = definitionLocation;
     }
 
+    /** This is for post-export reconstruction for serialization. */
+    @VisibleForSerialization
+    @AutoCodec.Instantiator
+    SkylarkRuleFunction(
+        RuleClass ruleClass,
+        RuleClassType type,
+        Location definitionLocation,
+        Label skylarkLabel
+    ) {
+      super("rule", FunctionSignature.KWARGS);
+      Preconditions.checkNotNull(
+          ruleClass,
+          "RuleClass must be non-null as this SkylarkRuleFunction should have been exported.");
+      Preconditions.checkNotNull(
+          skylarkLabel,
+          "Label must be non-null as this SkylarkRuleFunction should have been exported.");
+      this.ruleClass = ruleClass;
+      this.type = type;
+      this.definitionLocation = definitionLocation;
+      this.skylarkLabel = skylarkLabel;
+    }
+
     @Override
     @SuppressWarnings("unchecked") // the magic hidden $pkg_context variable is guaranteed
     // to be a PackageContext
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 5623847960..045520aa6d 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
@@ -37,6 +37,7 @@ import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassNamePredicate;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
 import com.google.devtools.build.lib.syntax.ClassObject;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.EvalUtils;
@@ -62,23 +63,22 @@ import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
 
 /**
- * Metadata of a rule attribute. Contains the attribute name and type, and an
- * default value to be used if none is provided in a rule declaration in a BUILD
- * file. Attributes are immutable, and may be shared by more than one rule (for
- * example, <code>foo_binary</code> and <code>foo_library</code> may share many
- * attributes in common).
+ * Metadata of a rule attribute. Contains the attribute name and type, and an default value to be
+ * used if none is provided in a rule declaration in a BUILD file. Attributes are immutable, and may
+ * be shared by more than one rule (for example, <code>foo_binary</code> and <code>foo_library
+ * </code> may share many attributes in common).
  */
 @Immutable
+@AutoCodec
 public final class Attribute implements Comparable<Attribute> {
 
   public static final RuleClassNamePredicate ANY_RULE = RuleClassNamePredicate.unspecified();
 
   public static final RuleClassNamePredicate NO_RULE = RuleClassNamePredicate.only();
 
-  /**
-   * Wraps the information necessary to construct an Aspect.
-   */
-  private abstract static class RuleAspect<C extends AspectClass> {
+  /** Wraps the information necessary to construct an Aspect. */
+  @VisibleForSerialization
+  abstract static class RuleAspect<C extends AspectClass> {
     protected final C aspectClass;
     protected final Function<Rule, AspectParameters> parametersExtractor;
 
@@ -115,7 +115,8 @@ public final class Attribute implements Comparable<Attribute> {
     }
   }
 
-  private static class SkylarkRuleAspect extends RuleAspect<SkylarkAspectClass> {
+  @AutoCodec
+  static class SkylarkRuleAspect extends RuleAspect<SkylarkAspectClass> {
     private final SkylarkDefinedAspect aspect;
 
     public SkylarkRuleAspect(SkylarkDefinedAspect aspect) {
@@ -150,7 +151,8 @@ public final class Attribute implements Comparable<Attribute> {
     }
   }
 
-  private enum PropertyFlag {
+  @VisibleForSerialization
+  enum PropertyFlag {
     MANDATORY,
     EXECUTABLE,
     UNDOCUMENTED,
@@ -258,6 +260,7 @@ public final class Attribute implements Comparable<Attribute> {
     String checkValid(Rule from, Rule to);
   }
 
+  @AutoCodec
   public static final ValidityPredicate ANY_EDGE =
       new ValidityPredicate() {
         @Override
@@ -297,9 +300,8 @@ public final class Attribute implements Comparable<Attribute> {
     }
   }
 
-  /**
-   * A predicate class to check if the value of the attribute comes from a predefined set.
-   */
+  /** A predicate class to check if the value of the attribute comes from a predefined set. */
+  @AutoCodec
   public static class AllowedValueSet implements PredicateWithMessage<Object> {
 
     private final Set<Object> allowedValues;
@@ -314,6 +316,12 @@ public final class Attribute implements Comparable<Attribute> {
       allowedValues = ImmutableSet.copyOf(values);
     }
 
+    @AutoCodec.Instantiator
+    @VisibleForSerialization
+    AllowedValueSet(Set<Object> allowedValues) {
+      this.allowedValues = allowedValues;
+    }
+
     @Override
     public boolean apply(Object input) {
       return allowedValues.contains(input);
@@ -1839,7 +1847,8 @@ public final class Attribute implements Comparable<Attribute> {
    * @param configTransition the configuration transition for this attribute (which must be of type
    *     LABEL, LABEL_LIST, NODEP_LABEL or NODEP_LABEL_LIST).
    */
-  private Attribute(
+  @VisibleForSerialization
+  Attribute(
       String name,
       Type<?> type,
       Set<PropertyFlag> propertyFlags,
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PredicatesWithMessage.java b/src/main/java/com/google/devtools/build/lib/packages/PredicatesWithMessage.java
index b0c8edab36..cfec2c78cc 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PredicatesWithMessage.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PredicatesWithMessage.java
@@ -13,26 +13,30 @@
 // limitations under the License.
 package com.google.devtools.build.lib.packages;
 
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
+
 /**
  * A helper class for PredicateWithMessage with default predicates.
  */
 public abstract class PredicatesWithMessage implements PredicateWithMessage<Object> {
 
-  private static final PredicateWithMessage<?> ALWAYS_TRUE = new PredicateWithMessage<Object>() {
-    @Override
-    public boolean apply(Object input) {
-      return true;
-    }
+  @AutoCodec @VisibleForSerialization
+  static final PredicateWithMessage<?> ALWAYS_TRUE =
+      new PredicateWithMessage<Object>() {
+        @Override
+        public boolean apply(Object input) {
+          return true;
+        }
 
-    @Override
-    public String getErrorReason(Object param) {
-      throw new UnsupportedOperationException();
-    }
-  };
+        @Override
+        public String getErrorReason(Object param) {
+          throw new UnsupportedOperationException();
+        }
+      };
 
   @SuppressWarnings("unchecked")
   public static <T> PredicateWithMessage<T> alwaysTrue() {
     return (PredicateWithMessage<T>) ALWAYS_TRUE;
   }
-
 }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RequiredProviders.java b/src/main/java/com/google/devtools/build/lib/packages/RequiredProviders.java
index 6e5060de4a..1551d70f35 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RequiredProviders.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RequiredProviders.java
@@ -18,27 +18,28 @@ import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
 import java.util.function.Function;
 import java.util.function.Predicate;
 import javax.annotation.Nullable;
 
 /**
- * Represents a constraint on a set of providers required by a dependency (of a rule
- * or an aspect).
+ * Represents a constraint on a set of providers required by a dependency (of a rule or an aspect).
+ *
+ * <p>Currently we support three kinds of constraints:
  *
- * Currently we support three kinds of constraints:
  * <ul>
- *   <li>accept any dependency.</li>
- *   <li>accept no dependency (used for aspects-on-aspects to indicate
- *   that an aspect never wants to see any other aspect applied to a target.</li>
- *   <li>accept a dependency that provides all providers from one of several sets of providers.
- *       It just so happens that in all current usages these sets are either all
- *       native providers or all Skylark providers, so this is the only use case this
- *       class currently supports.
- *   </li>
+ *   <li>accept any dependency.
+ *   <li>accept no dependency (used for aspects-on-aspects to indicate that an aspect never wants to
+ *       see any other aspect applied to a target.
+ *   <li>accept a dependency that provides all providers from one of several sets of providers. It
+ *       just so happens that in all current usages these sets are either all native providers or
+ *       all Skylark providers, so this is the only use case this class currently supports.
  * </ul>
  */
 @Immutable
+@AutoCodec
 public final class RequiredProviders {
   /** A constraint: either ANY, NONE, or RESTRICTED */
   private final Constraint constraint;
@@ -62,10 +63,9 @@ public final class RequiredProviders {
     return getDescription();
   }
 
-  /**
-   * Represents one of the constraints as desctibed in {@link RequiredProviders}
-   */
-  private enum Constraint {
+  /** Represents one of the constraints as desctibed in {@link RequiredProviders} */
+  @VisibleForSerialization
+  enum Constraint {
     /** Accept any dependency */
     ANY {
       @Override
@@ -272,8 +272,8 @@ public final class RequiredProviders {
     return constraint.equals(Constraint.ANY);
   }
 
-
-  private RequiredProviders(
+  @VisibleForSerialization
+  RequiredProviders(
       Constraint constraint,
       ImmutableList<ImmutableSet<Class<?>>> nativeProviders,
       ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> skylarkProviders) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkDefinedAspect.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkDefinedAspect.java
index 35d38c6510..14ec9d646e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkDefinedAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkDefinedAspect.java
@@ -21,15 +21,17 @@ import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
 import com.google.devtools.build.lib.cmdline.Label;
 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.VisibleForSerialization;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
 import com.google.devtools.build.lib.syntax.BaseFunction;
-import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Type;
 import java.util.Arrays;
 import java.util.List;
 
 /** A Skylark value that is a result of an 'aspect(..)' function call. */
+@AutoCodec
 public class SkylarkDefinedAspect implements SkylarkExportable, SkylarkAspect {
   private final BaseFunction implementation;
   private final ImmutableList<String> attributeAspects;
@@ -42,7 +44,6 @@ public class SkylarkDefinedAspect implements SkylarkExportable, SkylarkAspect {
   private final ImmutableSet<String> hostFragments;
   private final ImmutableList<Label> requiredToolchains;
 
-  private final Environment funcallEnv;
   private SkylarkAspectClass aspectClass;
 
   public SkylarkDefinedAspect(
@@ -56,8 +57,7 @@ public class SkylarkDefinedAspect implements SkylarkExportable, SkylarkAspect {
       // The host transition is in lib.analysis, so we can't reference it directly here.
       ConfigurationTransition hostTransition,
       ImmutableSet<String> hostFragments,
-      ImmutableList<Label> requiredToolchains,
-      Environment funcallEnv) {
+      ImmutableList<Label> requiredToolchains) {
     this.implementation = implementation;
     this.attributeAspects = attributeAspects;
     this.attributes = attributes;
@@ -68,7 +68,27 @@ public class SkylarkDefinedAspect implements SkylarkExportable, SkylarkAspect {
     this.hostTransition = hostTransition;
     this.hostFragments = hostFragments;
     this.requiredToolchains = requiredToolchains;
-    this.funcallEnv = funcallEnv;
+  }
+
+  /** Constructor for post export reconstruction for serialization. */
+  @VisibleForSerialization
+  @AutoCodec.Instantiator
+  SkylarkDefinedAspect(
+      BaseFunction implementation,
+      ImmutableList<String> attributeAspects,
+      ImmutableList<Attribute> attributes,
+      ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> requiredAspectProviders,
+      ImmutableSet<SkylarkProviderIdentifier> provides,
+      ImmutableSet<String> paramAttributes,
+      ImmutableSet<String> fragments,
+      // The host transition is in lib.analysis, so we can't reference it directly here.
+      ConfigurationTransition hostTransition,
+      ImmutableSet<String> hostFragments,
+      ImmutableList<Label> requiredToolchains,
+      SkylarkAspectClass aspectClass) {
+    this(implementation, attributeAspects, attributes, requiredAspectProviders, provides,
+        paramAttributes, fragments, hostTransition, hostFragments, requiredToolchains);
+    this.aspectClass = aspectClass;
   }
 
   public BaseFunction getImplementation() {
@@ -79,10 +99,6 @@ public class SkylarkDefinedAspect implements SkylarkExportable, SkylarkAspect {
     return attributeAspects;
   }
 
-  public Environment getFuncallEnv() {
-    return funcallEnv;
-  }
-
   public ImmutableList<Attribute> getAttributes() {
     return attributes;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
index f91d4269c5..d623e0f661 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
@@ -30,7 +30,9 @@ import java.util.NoSuchElementException;
 import javax.annotation.Nullable;
 
 /**
- * A class for Skylark functions provided as builtins by the Skylark implementation
+ * A class for Skylark functions provided as builtins by the Skylark implementation. Instances of
+ * this class do not need to be serializable because they should effectively be treated as
+ * constants.
  */
 public class BuiltinFunction extends BaseFunction {