Simplify config_setting visibility checking.

Make select() keys normal attribute -> configured target prerequisite
dependencies. This automatically opts them into RuleContext.Builder's standard
visibility checking.

The main historic argument against this is interference with "manual
trimming". But that's an on-ice feature that needs fundamental reevaluation and
shouldn't block other code in the meantime.

This is a simplified alternative to
bazelbuild#12877, for bazelbuild#12669.

PiperOrigin-RevId: 354326088
Change-Id: Ifaafc563f22d401599540ead70e49db6a86a1995

src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java

@@ -578,14 +578,7 @@ private void resolveAttributes(
     Label ruleLabel = rule.getLabel();
     for (AttributeDependencyKind dependencyKind : getAttributes(rule, aspects)) {
       Attribute attribute = dependencyKind.getAttribute();
-      if (!attribute.getCondition().apply(attributeMap)
-          // Not only is resolving CONFIG_SETTING_DEPS_ATTRIBUTE deps here wasteful, since the only
-          // place they're used is in ConfiguredTargetFunction.getConfigConditions, but it actually
-          // breaks trimming as shown by
-          // FeatureFlagManualTrimmingTest#featureFlagInUnusedSelectBranchButNotInTransitiveConfigs_DoesNotError
-          // because it resolves a dep that trimming (correctly) doesn't account for because it's
-          // part of an unchosen select() branch.
-          || attribute.getName().equals(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE)) {
+      if (!attribute.getCondition().apply(attributeMap)) {
         continue;
       }
 

src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java

@@ -1817,10 +1817,6 @@ public RuleContext build() throws InvalidExecGroupException {
           ConfiguredAttributeMapper.of(target.getAssociatedRule(), configConditions.asProviders());
       checkAttributesNonEmpty(attributes);
       ListMultimap<String, ConfiguredTargetAndData> targetMap = createTargetMap();
-      Attribute configSettingAttr = attributes.getAttributeDefinition("$config_dependencies");
-      for (ConfiguredTargetAndData condition : configConditions.asConfiguredTargets().values()) {
-        validateDirectPrerequisite(configSettingAttr, condition);
-      }
       ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap =
           createFilesetEntryMap(target.getAssociatedRule(), configConditions.asProviders());
       if (rawExecProperties == null) {

src/main/java/com/google/devtools/build/lib/packages/RuleClass.java

@@ -287,28 +287,24 @@ public RuleErrorException(String message, Throwable cause) {
 
   /**
    * Name of the attribute that stores all {@link
-   * com.google.devtools.build.lib.rules.config.ConfigRuleClasses} labels this rule references (i.e.
-   * select() keys). This is specially populated in {@link #populateRuleAttributeValues}.
+   * com.google.devtools.build.lib.rules.config.ConfigRuleClasses} labels in a <code></code>
+   * select(). This is specially populated in {@link #populateRuleAttributeValues}.
    *
-   * <p>This isn't technically necessary for builds: select() keys are evaluated in {@link
-   * com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction#getConfigConditions} instead of
-   * normal dependency resolution because they're needed to determine other dependencies. So there's
-   * no intrinsic reason why we need an extra attribute to store them.
+   * <p>We don't technically need this attribute. This info is only needed for {@link
+   * com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction#getConfigConditions}, which
+   * could also retrieve it by iterating over every attribute with a <code>select()</code>
    *
-   * <p>There are three reasons why we still create this attribute:
+   * <p>We keep it for these reasons:
    *
    * <ol>
-   *   <li>Collecting them once in {@link #populateRuleAttributeValues} instead of multiple times in
-   *       ConfiguredTargetFunction saves extra looping over the rule's attributes.
+   *   <li>Collecting conditions once in {@link #populateRuleAttributeValues} instead of multiple
+   *       times in ConfiguredTargetFunction saves extra looping over the rule's attributes.
    *   <li>Query's dependency resolution has no equivalent of {@link
    *       com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction#getConfigConditions} and
    *       we need to make sure its coverage remains complete.
-   *   <li>Manual configuration trimming uses the normal dependency resolution process to work
-   *       correctly and config_setting keys are subject to this trimming.
+   *   <li>Storing it as an attribute guarantees <code>select()</code> conditions are validity
+   *       checked in {@link RuleContext.Builder#validateDirectPrerequisite}.
    * </ol>
-   *
-   * <p>It should be possible to clean up these issues if we decide we don't want an artificial
-   * attribute dependency. But care has to be taken to do that safely.
    */
   public static final String CONFIG_SETTING_DEPS_ATTRIBUTE = "$config_dependencies";