Always restrict aspects to only access requested configuration fragments.

This completes the introduction of aspect configuration fragment enforcement
for static configuration builds; as of this change, it is no longer possible to
fall back to the base rule's set of requested configuration fragments.

This sort of fallback may become possible later, likely in a more controlled way.

--
MOS_MIGRATED_REVID=122638152

4 files changed, 8 insertions, 55 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
index 5e393412bb..368f373d27 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -308,19 +308,13 @@ public final class ConfiguredTargetFactory {
       BuildConfiguration aspectConfiguration,
       BuildConfiguration hostConfiguration)
       throws InterruptedException {
-    ConfigurationFragmentPolicy aspectPolicy =
-        aspect.getDefinition().getConfigurationFragmentPolicy();
-    ConfigurationFragmentPolicy rulePolicy =
-        ((Rule) associatedTarget.getTarget()).getRuleClassObject().getConfigurationFragmentPolicy();
     RuleContext.Builder builder = new RuleContext.Builder(env,
         associatedTarget.getTarget(),
         aspect.getAspectClass().getName(),
         aspectConfiguration,
         hostConfiguration,
         ruleClassProvider.getPrerequisiteValidator(),
-        // TODO(mstaib): When AspectDefinition can no longer have null ConfigurationFragmentPolicy,
-        // remove this conditional.
-        aspectPolicy != null ? aspectPolicy : rulePolicy);
+        aspect.getDefinition().getConfigurationFragmentPolicy());
     RuleContext ruleContext =
         builder
             .setVisibility(
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
index 4e3f08a777..606b17774d 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
@@ -125,13 +125,9 @@ public final class AspectDefinition {
   }
 
   /**
-   * Returns the set of configuration fragments required by this Aspect, or {@code null} if it has
-   * not set a configuration fragment policy, meaning it should inherit from the attached rule.
+   * Returns the set of configuration fragments required by this Aspect.
    */
-  @Nullable public ConfigurationFragmentPolicy getConfigurationFragmentPolicy() {
-    // TODO(mstaib): When all existing aspects properly set their configuration fragment policy,
-    // this method and the associated member should no longer be nullable.
-    // "inherit from the attached rule" should go away.
+  public ConfigurationFragmentPolicy getConfigurationFragmentPolicy() {
     return configurationFragmentPolicy;
   }
 
@@ -219,14 +215,6 @@ public final class AspectDefinition {
     private final Multimap<String, AspectClass> attributeAspects = LinkedHashMultimap.create();
     private final ConfigurationFragmentPolicy.Builder configurationFragmentPolicy =
         new ConfigurationFragmentPolicy.Builder();
-    // TODO(mstaib): When all existing aspects properly set their configuration fragment policy,
-    // remove this flag and the code that interacts with it.
-    /**
-     * True if the aspect definition has intentionally specified a configuration fragment policy by
-     * calling any of the methods which set up the policy, and thus needs the built AspectDefinition
-     * to retain the policy.
-     */
-    private boolean hasConfigurationFragmentPolicy = false;
 
     public Builder(String name) {
       this.name = name;
@@ -306,7 +294,6 @@ public final class AspectDefinition {
      * <p>The value is inherited by subclasses.
      */
     public Builder requiresConfigurationFragments(Class<?>... configurationFragments) {
-      hasConfigurationFragmentPolicy = true;
       configurationFragmentPolicy
           .requiresConfigurationFragments(ImmutableSet.copyOf(configurationFragments));
       return this;
@@ -319,7 +306,6 @@ public final class AspectDefinition {
      * <p>The value is inherited by subclasses.
      */
     public Builder requiresHostConfigurationFragments(Class<?>... configurationFragments) {
-      hasConfigurationFragmentPolicy = true;
       configurationFragmentPolicy
           .requiresHostConfigurationFragments(ImmutableSet.copyOf(configurationFragments));
       return this;
@@ -334,11 +320,6 @@ public final class AspectDefinition {
      */
     public Builder requiresConfigurationFragmentsBySkylarkModuleName(
         Collection<String> configurationFragmentNames) {
-      // This method is unconditionally called from Skylark code, so only consider the user to have
-      // specified a configuration policy if the collection actually has anything in it.
-      // TODO(mstaib): Stop caring about this as soon as all aspects have configuration policies.
-      hasConfigurationFragmentPolicy =
-          hasConfigurationFragmentPolicy || !configurationFragmentNames.isEmpty();
       configurationFragmentPolicy
           .requiresConfigurationFragmentsBySkylarkModuleName(configurationFragmentNames);
       return this;
@@ -353,11 +334,6 @@ public final class AspectDefinition {
      */
     public Builder requiresHostConfigurationFragmentsBySkylarkModuleName(
         Collection<String> configurationFragmentNames) {
-      // This method is unconditionally called from Skylark code, so only consider the user to have
-      // specified a configuration policy if the collection actually has anything in it.
-      // TODO(mstaib): Stop caring about this as soon as all aspects have configuration policies.
-      hasConfigurationFragmentPolicy =
-          hasConfigurationFragmentPolicy || !configurationFragmentNames.isEmpty();
       configurationFragmentPolicy
           .requiresHostConfigurationFragmentsBySkylarkModuleName(configurationFragmentNames);
       return this;
@@ -368,7 +344,6 @@ public final class AspectDefinition {
      * {@link #requiresConfigurationFragments}).
      */
     public Builder setMissingFragmentPolicy(MissingFragmentPolicy missingFragmentPolicy) {
-      hasConfigurationFragmentPolicy = true;
       configurationFragmentPolicy.setMissingFragmentPolicy(missingFragmentPolicy);
       return this;
     }
@@ -381,7 +356,7 @@ public final class AspectDefinition {
     public AspectDefinition build() {
       return new AspectDefinition(name, ImmutableSet.copyOf(requiredProviders),
           ImmutableMap.copyOf(attributes), ImmutableSetMultimap.copyOf(attributeAspects),
-          hasConfigurationFragmentPolicy ? configurationFragmentPolicy.build() : null);
+          configurationFragmentPolicy.build());
     }
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java
index bdb273e15d..4bf2c55102 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java
@@ -138,10 +138,10 @@ public class AspectDefinitionTest {
   }
 
   @Test
-  public void testNoConfigurationFragmentPolicySetup_ReturnsNull() throws Exception {
+  public void testNoConfigurationFragmentPolicySetup_HasNonNullPolicy() throws Exception {
     AspectDefinition noPolicy = new AspectDefinition.Builder("no_policy")
         .build();
-    assertThat(noPolicy.getConfigurationFragmentPolicy()).isNull();
+    assertThat(noPolicy.getConfigurationFragmentPolicy()).isNotNull();
   }
 
   @Test
@@ -205,12 +205,12 @@ public class AspectDefinitionTest {
   }
 
   @Test
-  public void testEmptySkylarkConfigurationFragmentPolicySetup_ReturnsNull() throws Exception {
+  public void testEmptySkylarkConfigurationFragmentPolicySetup_HasNonNullPolicy() throws Exception {
     AspectDefinition noPolicy = new AspectDefinition.Builder("no_policy")
         .requiresConfigurationFragmentsBySkylarkModuleName(ImmutableList.<String>of())
         .requiresHostConfigurationFragmentsBySkylarkModuleName(ImmutableList.<String>of())
         .build();
-    assertThat(noPolicy.getConfigurationFragmentPolicy()).isNull();
+    assertThat(noPolicy.getConfigurationFragmentPolicy()).isNotNull();
   }
 
   @SkylarkModule(name = "test_fragment", doc = "test fragment")
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
index 63c5d683aa..4e5d38582a 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -856,22 +856,6 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
             + "(for example: host_fragments = [\"cpp\"])");
   }
 
-  @Test
-  public void testAspectFragmentFallback() throws Exception {
-    // TODO(mstaib): Remove this test when rule fragment fallback is no longer permitted.
-    getConfiguredTargetForAspectFragment(
-        "ctx.fragments.cpp.compiler", "", "", "'cpp'", "");
-    assertNoEvents();
-  }
-
-  @Test
-  public void testAspectHostFragmentFallback() throws Exception {
-    // TODO(mstaib): Remove this test when rule fragment fallback is no longer permitted.
-    getConfiguredTargetForAspectFragment(
-        "ctx.host_fragments.cpp.compiler", "", "", "", "'cpp'");
-    assertNoEvents();
-  }
-
   private ConfiguredTarget getConfiguredTargetForAspectFragment(
       String fullFieldName,
       String fragments,