Add Codec for NativeAspectClass. As a result, remove @AutoCodec from concrete subclasses. Improve debugging message on serialization failures.

Lot of test-side changes to make sure aspects are properly registered with the RuleClassProvider.

PiperOrigin-RevId: 185607202

9 files changed, 102 insertions, 48 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/NativeAspectClass.java b/src/main/java/com/google/devtools/build/lib/packages/NativeAspectClass.java
index 905862a8d7..ed6b77bbc2 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/NativeAspectClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/NativeAspectClass.java
@@ -14,6 +14,16 @@
 
 package com.google.devtools.build.lib.packages;
 
+import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext;
+import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
+import com.google.devtools.build.lib.skyframe.serialization.SerializationContext;
+import com.google.devtools.build.lib.skyframe.serialization.SerializationException;
+import com.google.devtools.build.lib.skyframe.serialization.strings.StringCodecs;
+import com.google.protobuf.CodedInputStream;
+import com.google.protobuf.CodedOutputStream;
+import java.io.IOException;
+
 /**
  * A class of aspects that are implemented natively in Bazel.
  *
@@ -21,6 +31,7 @@ package com.google.devtools.build.lib.packages;
  * aspect factory. All wrappers of the same class are equal.
  */
 public abstract class NativeAspectClass implements AspectClass {
+  public static final ObjectCodec<NativeAspectClass> CODEC = new Codec();
 
   @Override
   public String getName() {
@@ -29,4 +40,30 @@ public abstract class NativeAspectClass implements AspectClass {
 
   public abstract AspectDefinition getDefinition(AspectParameters aspectParameters);
 
+  private static class Codec implements ObjectCodec<NativeAspectClass> {
+    @Override
+    public Class<NativeAspectClass> getEncodedClass() {
+      return NativeAspectClass.class;
+    }
+
+    @Override
+    public void serialize(
+        SerializationContext context, NativeAspectClass obj, CodedOutputStream codedOut)
+        throws SerializationException, IOException {
+      RuleClassProvider ruleClassProvider = context.getDependency(RuleClassProvider.class);
+      NativeAspectClass storedAspect = ruleClassProvider.getNativeAspectClass(obj.getKey());
+      Preconditions.checkState(
+          obj == storedAspect, "Not stored right: %s %s %s", obj, storedAspect, ruleClassProvider);
+      StringCodecs.asciiOptimized().serialize(context, obj.getKey(), codedOut);
+    }
+
+    @Override
+    public NativeAspectClass deserialize(DeserializationContext context, CodedInputStream codedIn)
+        throws SerializationException, IOException {
+      String aspectKey = StringCodecs.asciiOptimized().deserialize(context, codedIn);
+      return Preconditions.checkNotNull(
+          context.getDependency(RuleClassProvider.class).getNativeAspectClass(aspectKey),
+          aspectKey);
+    }
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeAspect.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeAspect.java
index 00b82486fb..4048b2fc22 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeAspect.java
@@ -16,15 +16,10 @@ package com.google.devtools.build.lib.packages;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
 
 /** A natively-defined aspect that is may be referenced by skylark attribute definitions. */
-@AutoCodec(strategy = AutoCodec.Strategy.POLYMORPHIC)
 public abstract class SkylarkNativeAspect extends NativeAspectClass implements SkylarkAspect {
-  public static final ObjectCodec<SkylarkNativeAspect> CODEC = new SkylarkNativeAspect_AutoCodec();
-
   @Override
   public void repr(SkylarkPrinter printer) {
     printer.append("<native aspect>");
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidNeverlinkAspect.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidNeverlinkAspect.java
index 42858be571..ff56c613fe 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidNeverlinkAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidNeverlinkAspect.java
@@ -28,8 +28,6 @@ import com.google.devtools.build.lib.rules.java.JavaCommon;
 import com.google.devtools.build.lib.rules.java.JavaInfo;
 import com.google.devtools.build.lib.rules.java.JavaRuntimeJarProvider;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndTarget;
-import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -41,11 +39,7 @@ import java.util.List;
  * <p>One would think that using the compile time classpath would be enough, but alas, those are
  * ijars,
  */
-@AutoCodec
 public class AndroidNeverlinkAspect extends NativeAspectClass implements ConfiguredAspectFactory {
-  public static final ObjectCodec<AndroidNeverlinkAspect> CODEC =
-      new AndroidNeverlinkAspect_AutoCodec();
-
   public static final String NAME = "AndroidNeverlinkAspect";
   private static final ImmutableList<String> ATTRIBUTES =
       ImmutableList.of(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java b/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java
index 92eaa877f5..e8be797e4f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java
@@ -65,7 +65,6 @@ import com.google.devtools.build.lib.rules.java.proto.JavaProtoLibraryAspectProv
 import com.google.devtools.build.lib.rules.proto.ProtoLangToolchainProvider;
 import com.google.devtools.build.lib.rules.proto.ProtoSourcesProvider;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndTarget;
-import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -78,8 +77,6 @@ import java.util.Set;
 public final class DexArchiveAspect extends NativeAspectClass implements ConfiguredAspectFactory {
   public static final String NAME = "DexArchiveAspect";
 
-  public static final ObjectCodec<DexArchiveAspect> CODEC = new DexArchiveAspect_AutoCodec();
-
   /**
    * Function that returns a {@link Rule}'s {@code incremental_dexing} attribute for use by this
    * aspect. Must be provided when attaching this aspect to a target.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/J2ObjcAspect.java b/src/main/java/com/google/devtools/build/lib/rules/objc/J2ObjcAspect.java
index 5de8b4656a..984d145bc9 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/J2ObjcAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/J2ObjcAspect.java
@@ -73,17 +73,13 @@ import com.google.devtools.build.lib.rules.proto.ProtoSourcesProvider;
 import com.google.devtools.build.lib.rules.proto.ProtoSupportDataProvider;
 import com.google.devtools.build.lib.rules.proto.SupportData;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndTarget;
-import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.List;
 
 /** J2ObjC transpilation aspect for Java and proto rules. */
-@AutoCodec
 public class J2ObjcAspect extends NativeAspectClass implements ConfiguredAspectFactory {
   public static final String NAME = "J2ObjcAspect";
-  public static final ObjectCodec<J2ObjcAspect> CODEC = new J2ObjcAspect_AutoCodec();
 
   private final String toolsRepository;
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoAspect.java
index 9bd8e02294..010f2bcca3 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoAspect.java
@@ -30,19 +30,14 @@ import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.SkylarkNativeAspect;
 import com.google.devtools.build.lib.rules.proto.ProtoSourcesProvider;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndTarget;
-import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 
 /**
  * Aspect that gathers the proto dependencies of the attached rule target, and propagates the proto
  * values of its dependencies through the ObjcProtoProvider.
  */
-@AutoCodec
 public class ObjcProtoAspect extends SkylarkNativeAspect implements ConfiguredAspectFactory {
   public static final String NAME = "ObjcProtoAspect";
 
-  public static final ObjectCodec<ObjcProtoAspect> CODEC = new ObjcProtoAspect_AutoCodec();
-
   @Override
   public AspectDefinition getDefinition(AspectParameters aspectParameters) {
     return new AspectDefinition.Builder(this)
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
index e60bfebff6..a3cb103758 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
@@ -409,16 +409,25 @@ public class AspectTest extends AnalysisTestCase {
    * Rule definitions to be used in emptyAspectAttributesAreAvailableInRuleContext().
    */
   public static class EmptyAspectAttributesAreAvailableInRuleContext {
-    public static final MockRule TEST_RULE = () ->
-        MockRule.ancestor(TestAspects.BASE_RULE.getClass()).factory(DummyRuleFactory.class).define(
-            "testrule",
-            (builder, env) ->
-                builder
-                    .add(attr("foo", LABEL_LIST).legacyAllowAnyFileType()
-                        .aspect(new AspectWithEmptyLateBoundAttribute())));
+    public static final MockRule TEST_RULE =
+        () ->
+            MockRule.ancestor(TestAspects.BASE_RULE.getClass())
+                .factory(DummyRuleFactory.class)
+                .define(
+                    "testrule",
+                    (builder, env) ->
+                        builder.add(
+                            attr("foo", LABEL_LIST)
+                                .legacyAllowAnyFileType()
+                                .aspect(AspectWithEmptyLateBoundAttribute.INSTANCE)));
 
     public static class AspectWithEmptyLateBoundAttribute extends NativeAspectClass
       implements ConfiguredAspectFactory {
+      static final AspectWithEmptyLateBoundAttribute INSTANCE =
+          new AspectWithEmptyLateBoundAttribute();
+
+      private AspectWithEmptyLateBoundAttribute() {}
+
       @Override
       public AspectDefinition getDefinition(AspectParameters params) {
         return new AspectDefinition.Builder(this)
@@ -449,8 +458,13 @@ public class AspectTest extends AnalysisTestCase {
    */
   @Test
   public void emptyAspectAttributesAreAvailableInRuleContext() throws Exception {
-    setRulesAvailableInTests(TestAspects.BASE_RULE,
-        EmptyAspectAttributesAreAvailableInRuleContext.TEST_RULE);
+    setRulesAndAspectsAvailableInTests(
+        ImmutableList.of(
+            TestAspects.SIMPLE_ASPECT,
+            EmptyAspectAttributesAreAvailableInRuleContext.AspectWithEmptyLateBoundAttribute
+                .INSTANCE),
+        ImmutableList.of(
+            TestAspects.BASE_RULE, EmptyAspectAttributesAreAvailableInRuleContext.TEST_RULE));
     pkg("a",
         "testrule(name='a', foo=[':b'])",
         "testrule(name='b')");
@@ -462,19 +476,30 @@ public class AspectTest extends AnalysisTestCase {
    * Rule definitions to be used in extraActionsAreEmitted().
    */
   public static class ExtraActionsAreEmitted {
-    public static final MockRule TEST_RULE = () ->
-        MockRule.ancestor(TestAspects.BASE_RULE.getClass()).factory(DummyRuleFactory.class).define(
-            "testrule",
-            (builder, env) ->
-                builder
-                    .add(attr("foo", LABEL_LIST).legacyAllowAnyFileType()
-                        .aspect(new AspectThatRegistersAction()))
-                    .add(attr(":action_listener", LABEL_LIST)
-                        .cfg(HostTransition.INSTANCE)
-                        .value(ACTION_LISTENER)));
+    public static final MockRule TEST_RULE =
+        () ->
+            MockRule.ancestor(TestAspects.BASE_RULE.getClass())
+                .factory(DummyRuleFactory.class)
+                .define(
+                    "testrule",
+                    (builder, env) ->
+                        builder
+                            .add(
+                                attr("foo", LABEL_LIST)
+                                    .legacyAllowAnyFileType()
+                                    .aspect(AspectThatRegistersAction.INSTANCE))
+                            .add(
+                                attr(":action_listener", LABEL_LIST)
+                                    .cfg(HostTransition.INSTANCE)
+                                    .value(ACTION_LISTENER)));
 
     public static class AspectThatRegistersAction extends NativeAspectClass
       implements ConfiguredAspectFactory {
+
+      static final AspectThatRegistersAction INSTANCE = new AspectThatRegistersAction();
+
+      private AspectThatRegistersAction() {}
+
       @Override
       public AspectDefinition getDefinition(AspectParameters params) {
         return new AspectDefinition.Builder(this).build();
@@ -500,7 +525,10 @@ public class AspectTest extends AnalysisTestCase {
    */
   @Test
   public void extraActionsAreEmitted() throws Exception {
-    setRulesAvailableInTests(TestAspects.BASE_RULE, ExtraActionsAreEmitted.TEST_RULE);
+    setRulesAndAspectsAvailableInTests(
+        ImmutableList.of(
+            TestAspects.SIMPLE_ASPECT, ExtraActionsAreEmitted.AspectThatRegistersAction.INSTANCE),
+        ImmutableList.of(TestAspects.BASE_RULE, ExtraActionsAreEmitted.TEST_RULE));
     useConfiguration("--experimental_action_listener=//extra_actions:listener");
     scratch.file(
         "extra_actions/BUILD",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index fc9aa99a9a..279675f12c 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -500,8 +500,21 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
    * Also see {@link AnalysisTestCase#setRulesAndAspectsAvailableInTests(Iterable, Iterable)}.
    */
   protected void setRulesAvailableInTests(RuleDefinition... rules) throws Exception {
+    // Not all of these aspects are needed for all tests, but it makes it simple to offer them all.
     setRulesAndAspectsAvailableInTests(
-        ImmutableList.<NativeAspectClass>of(),
+        ImmutableList.of(
+            TestAspects.SIMPLE_ASPECT,
+            TestAspects.PARAMETRIZED_DEFINITION_ASPECT,
+            TestAspects.ASPECT_REQUIRING_PROVIDER,
+            TestAspects.FALSE_ADVERTISEMENT_ASPECT,
+            TestAspects.ALL_ATTRIBUTES_ASPECT,
+            TestAspects.ALL_ATTRIBUTES_WITH_TOOL_ASPECT,
+            TestAspects.BAR_PROVIDER_ASPECT,
+            TestAspects.EXTRA_ATTRIBUTE_ASPECT,
+            TestAspects.FOO_PROVIDER_ASPECT,
+            TestAspects.ASPECT_REQUIRING_PROVIDER_SETS,
+            TestAspects.WARNING_ASPECT,
+            TestAspects.ERROR_ASPECT),
         ImmutableList.copyOf(rules));
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
index 353f62f4a4..dcea00a3ac 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
@@ -444,12 +444,11 @@ public class TestAspects {
     }
   }
 
-  private static final ParametrizedDefinitionAspect PARAMETRIZED_DEFINITION_ASPECT =
+  static final ParametrizedDefinitionAspect PARAMETRIZED_DEFINITION_ASPECT =
       new ParametrizedDefinitionAspect();
 
-  private static final AspectRequiringProvider ASPECT_REQUIRING_PROVIDER =
-      new AspectRequiringProvider();
-  private static final AspectRequiringProviderSets ASPECT_REQUIRING_PROVIDER_SETS =
+  static final AspectRequiringProvider ASPECT_REQUIRING_PROVIDER = new AspectRequiringProvider();
+  static final AspectRequiringProviderSets ASPECT_REQUIRING_PROVIDER_SETS =
       new AspectRequiringProviderSets();
   private static final AspectDefinition ASPECT_REQUIRING_PROVIDER_DEFINITION =
       new AspectDefinition.Builder(ASPECT_REQUIRING_PROVIDER)