Fix aspect attributes targeting PackageGroup.

A NullPointerException occured on such attributes while aspect was visiting target rule, because target rule had no such attribute. RELNOTES: None. PiperOrigin-RevId: 315272194
bazelbuild · Jun 8, 2020 · 4186fc3 · 4186fc3
1 parent 0f0ca6e
commit 4186fc3
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 2 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java b/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java
@@ -107,9 +107,12 @@ private void validateDirectPrerequisiteVisibility(
     }
 
     if (prerequisiteTarget instanceof PackageGroup) {
+      Attribute configuredAttribute = RawAttributeMapper.of(rule).getAttributeDefinition(attrName);
+      if (configuredAttribute == null) { // handles aspects
+        configuredAttribute = attribute;
+      }
       boolean containsPackageSpecificationProvider =
-          RawAttributeMapper.of(rule)
-              .getAttributeDefinition(attrName)
+          configuredAttribute
               .getRequiredProviders()
               .getDescription()
               .contains("PackageSpecificationProvider");

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
@@ -326,6 +326,17 @@ public void aspectDependenciesDontShowDeprecationWarnings() throws Exception {
     assertContainsEventWithFrequency("bad aspect", 0);
   }
 
+  @Test
+  public void aspectDependsOnPackageGroup() throws Exception {
+    setRulesAvailableInTests(
+        TestAspects.BASE_RULE, TestAspects.PACKAGE_GROUP_ATTRIBUTE_ASPECT_RULE);
+    pkg("extra", "package_group(name='extra')");
+    pkg("a", "rule_with_package_group_deps_aspect(name='a', foo=[':b'])", "base(name='b')");
+
+    getConfiguredTarget("//a:a");
+    assertContainsEventWithFrequency("bad aspect", 0);
+  }
+
   @Test
   public void aspectWithComputedAttribute() throws Exception {
     setRulesAvailableInTests(TestAspects.BASE_RULE, TestAspects.COMPUTED_ATTRIBUTE_ASPECT_RULE);

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
@@ -623,6 +623,7 @@ protected void setRulesAvailableInTests(RuleDefinition... rules) throws Exceptio
             TestAspects.ALL_ATTRIBUTES_WITH_TOOL_ASPECT,
             TestAspects.BAR_PROVIDER_ASPECT,
             TestAspects.EXTRA_ATTRIBUTE_ASPECT,
+            TestAspects.PACKAGE_GROUP_ATTRIBUTE_ASPECT,
             TestAspects.COMPUTED_ATTRIBUTE_ASPECT,
             TestAspects.FOO_PROVIDER_ASPECT,
             TestAspects.ASPECT_REQUIRING_PROVIDER_SETS,

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD
@@ -63,6 +63,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:extra_action_artifacts_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:inconsistent_aspect_order_exception",
+        "//src/main/java/com/google/devtools/build/lib/analysis:package_specification_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:resolved_toolchain_context",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
         "//src/main/java/com/google/devtools/build/lib/analysis:server_directories",

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
@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.PackageSpecificationProvider;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory;
 import com.google.devtools.build.lib.analysis.RuleContext;
@@ -338,6 +339,24 @@ public AspectDefinition getDefinition(AspectParameters aspectParameters) {
     }
   }
 
+  /** An aspect that defines its own implicit attribute, requiring PackageSpecificationProvider. */
+  public static class PackageGroupAttributeAspect extends BaseAspect {
+    @Override
+    public AspectDefinition getDefinition(AspectParameters aspectParameters) {
+      return PACKAGE_GROUP_ATTRIBUTE_ASPECT_DEFINITION;
+    }
+  }
+
+  public static final PackageGroupAttributeAspect PACKAGE_GROUP_ATTRIBUTE_ASPECT =
+      new PackageGroupAttributeAspect();
+  private static final AspectDefinition PACKAGE_GROUP_ATTRIBUTE_ASPECT_DEFINITION =
+      new AspectDefinition.Builder(PACKAGE_GROUP_ATTRIBUTE_ASPECT)
+          .add(
+              attr("$dep", LABEL)
+                  .value(Label.parseAbsoluteUnchecked("//extra:extra"))
+                  .mandatoryNativeProviders(ImmutableList.of(PackageSpecificationProvider.class)))
+          .build();
+
   public static final ComputedAttributeAspect COMPUTED_ATTRIBUTE_ASPECT =
       new ComputedAttributeAspect();
   private static final AspectDefinition COMPUTED_ATTRIBUTE_ASPECT_DEFINITION =
@@ -674,6 +693,17 @@ public ConfiguredAspect create(
           attr("foo", LABEL_LIST).allowedFileTypes(FileTypeSet.ANY_FILE)
               .aspect(EXTRA_ATTRIBUTE_ASPECT));
 
+  /** A rule that defines an {@link PackageGroupAttributeAspect} on one of its attributes. */
+  public static final MockRule PACKAGE_GROUP_ATTRIBUTE_ASPECT_RULE =
+      () ->
+          MockRule.ancestor(BASE_RULE.getClass())
+              .factory(DummyRuleFactory.class)
+              .define(
+                  "rule_with_package_group_deps_aspect",
+                  attr("foo", LABEL_LIST)
+                      .allowedFileTypes(FileTypeSet.ANY_FILE)
+                      .aspect(PACKAGE_GROUP_ATTRIBUTE_ASPECT));
+
   /** A rule that defines an {@link ComputedAttributeAspect} on one of its attributes. */
   public static final MockRule COMPUTED_ATTRIBUTE_ASPECT_RULE =
       () ->