Dynamic configs: also include fragments needed to resolve the rule's late-bound attributes

To be clear: late-bound attributes introduce two new classes of fragments. First is
those needed by Attribute.LateBoundDefault to figure out what the attribute's value is.
Second is whatever's needed by the actual dep (for label-typed attributes) and its deps.

This change only adds the first. The second is harder since we can't determine
it until we're already in the analysis phase.

--
MOS_MIGRATED_REVID=129117755

1 files changed, 41 insertions, 17 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
index 828a5e5b99..81e53e0429 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
@@ -17,8 +17,8 @@ import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
-import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
 import com.google.devtools.build.lib.analysis.config.ConfigRuleClasses.ConfigSettingRule;
+import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
@@ -43,7 +43,6 @@ import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.ValueOrException2;
 import com.google.devtools.common.options.Option;
-
 import java.lang.reflect.Field;
 import java.util.Collection;
 import java.util.HashMap;
@@ -52,7 +51,6 @@ import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
-
 import javax.annotation.Nullable;
 
 /**
@@ -183,39 +181,65 @@ public class TransitiveTargetFunction
     Target target = targetAndErrorIfAny.getTarget();
     NoSuchTargetException errorLoadingTarget = targetAndErrorIfAny.getErrorLoadingTarget();
 
-    // Get configuration fragments directly required by this target.
+    // Get configuration fragments directly required by this rule.
     if (target instanceof Rule) {
+      Rule rule = (Rule) target;
+
+      // Declared by the rule class:
       ConfigurationFragmentPolicy configurationFragmentPolicy =
-          target.getAssociatedRule().getRuleClassObject().getConfigurationFragmentPolicy();
+          rule.getRuleClassObject().getConfigurationFragmentPolicy();
       for (ConfigurationFragmentFactory factory : ruleClassProvider.getConfigurationFragments()) {
         Class<? extends Fragment> fragment = factory.creates();
         // isLegalConfigurationFragment considers both natively declared fragments and Skylark
         // (named) fragments.
-        if (configurationFragmentPolicy.isLegalConfigurationFragment(fragment)
-            && !builder.getConfigFragmentsFromDeps().contains(fragment)) {
-          builder.getTransitiveConfigFragments().add(
-              fragment.asSubclass(BuildConfiguration.Fragment.class));
+        if (configurationFragmentPolicy.isLegalConfigurationFragment(fragment)) {
+          addFragmentIfNew(builder, fragment.asSubclass(BuildConfiguration.Fragment.class));
+        }
+      }
+
+      // Declared by late-bound attributes:
+      for (Attribute attr : rule.getAttributes()) {
+        if (attr.isLateBound()) {
+          addFragmentsIfNew(builder,
+              attr.getLateBoundDefault().getRequiredConfigurationFragments());
         }
       }
 
       // config_setting rules have values like {"some_flag": "some_value"} that need the
-      // corresponding fragments in their configurations to properly resolve.
-      Rule rule = (Rule) target;
+      // corresponding fragments in their configurations to properly resolve:
       if (rule.getRuleClass().equals(ConfigSettingRule.RULE_NAME)) {
-        builder.getTransitiveConfigFragments().addAll(
+        addFragmentsIfNew(builder,
             ConfigSettingRule.requiresConfigurationFragments(rule, optionsToFragmentMap));
       }
 
-      Class<? extends Fragment> universalFragment =
-          ruleClassProvider.getUniversalFragment().asSubclass(BuildConfiguration.Fragment.class);
-      if (!builder.getConfigFragmentsFromDeps().contains(universalFragment)) {
-        builder.getTransitiveConfigFragments().add(universalFragment);
-      }
+      // Fragments to unconditionally include:
+      addFragmentIfNew(builder,
+          ruleClassProvider.getUniversalFragment().asSubclass(BuildConfiguration.Fragment.class));
     }
 
     return builder.build(errorLoadingTarget);
   }
 
+  private void addFragmentIfNew(TransitiveTargetValueBuilder builder,
+      Class<? extends Fragment> fragment) {
+    // This only checks that the deps don't already use this fragment, not the parent rule itself.
+    // So duplicates are still possible. We can further optimize if needed.
+    if (!builder.getConfigFragmentsFromDeps().contains(fragment)) {
+      builder.getTransitiveConfigFragments().add(fragment);
+    }
+  }
+
+  private void addFragmentsIfNew(TransitiveTargetValueBuilder builder, Iterable<?> fragments) {
+    // We take Iterable<?> instead of Iterable<Class<?>> or Iterable<Class<? extends Fragment>>
+    // because both of the latter are passed as actual parameters and there's no way to consistently
+    // cast to one of them. In actuality, all values are Class<? extends Fragment>, but the values
+    // coming from Attribute.java don't have access to the Fragment symbol since Attribute is built
+    // in a different library.
+    for (Object fragment : fragments) {
+      addFragmentIfNew(builder, (Class<? extends Fragment>) fragment);
+    }
+  }
+
   @Override
   protected Collection<Label> getAspectLabels(Rule fromRule, Attribute attr, Label toLabel,
       ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal,