Optionally simplify unconditional selects when storing rule attribute…

… values

If the --incompatible_simplify_unconditional_selects_in_rule_attrs option is
enabled, we simplify unconditional selects (in other words, selects with only
the //conditions:default branch), as well as concatenations of such selects,
into a non-select value. This non-select value will be reflected in various
introspection results, such as query output and native.existing_rules()
attribute values.

The motivation for this change is symbolic macros (#19922),
which promote non-select values of configurable macro attributes to
unconditional selects. If these unconditional selects propagate to saved rule
attribute values, they have the potential to make native.existing_rules()
useless since we do not (yet) have a build language API for introspecting
selects.

RELNOTES: Add the --incompatible_simplify_unconditional_selects_in_rule_attrs option to simplify configurable rule attributes which contain only unconditional selects; for example, if ["a"] + select("//conditions:default", ["b"]) is assigned to a rule attribute, it is stored as ["a", "b"].
PiperOrigin-RevId: 678803898
Change-Id: I989b6a2f0cc297b8740d6854e676210ee35f37d1

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java

@@ -66,6 +66,8 @@ public static Rule createRule(
                 Root.fromPath(directories.getWorkspace()),
                 LabelConstants.MODULE_DOT_BAZEL_FILE_NAME),
             semantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
+            semantics.getBool(
+                BuildLanguageOptions.INCOMPATIBLE_SIMPLIFY_UNCONDITIONAL_SELECTS_IN_RULE_ATTRS),
             basePackageId,
             repoMapping);
     BuildLangTypedAttributeValuesMap attributeValues =

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

@@ -175,20 +175,66 @@ public static boolean isLabelType(Type<?> type) {
 
   /**
    * Variation of {@link Type#convert} that supports selector expressions for configurable
-   * attributes* (i.e. "{ config1: 'value1_of_orig_type', config2: 'value2_of_orig_type; }"). If x
-   * is a selector expression, returns a {@link Selector} instance that contains key-mapped entries
+   * attributes (i.e. "{ config1: 'value1_of_orig_type', config2: 'value2_of_orig_type; }"). If x is
+   * a selector expression, returns a {@link SelectorList} instance that contains key-mapped entries
    * of the native type. Else, returns the native type directly.
    *
+   * <p>If {@code simplifyUnconditionalSelects} is true, then an unconditional select is simplified
+   * to the select's value converted to a native value; and a concatenation of unconditional selects
+   * (and direct values, if any) is simplified to a concatenation of the select's values and the
+   * direct values converted to native values. In other words, {@code ["//x"] +
+   * select("//conditions:default": ["//y"])} becomes {@code [Label("//x"), Label("//y")]}. If a
+   * concatenation contains a non-unconditional select, the concatenation is not simplified.
+   *
+   * <p>Returns null iff {@code simplifyUnconditionalSelects} is true, {@code x} is {@code
+   * select({"//conditions:default": None})}, and the {@code type.getDefaultValue()} is null.
+   *
    * <p>The caller is responsible for casting the returned value appropriately.
    */
-  static <T> Object selectableConvert(Type<T> type, Object x, Object what, LabelConverter context)
+  @Nullable
+  static <T> Object selectableConvert(
+      Type<T> type,
+      Object x,
+      Object what,
+      LabelConverter context,
+      boolean simplifyUnconditionalSelects)
       throws ConversionException {
-    if (x instanceof com.google.devtools.build.lib.packages.SelectorList) {
-      return new SelectorList<>(
-          ((com.google.devtools.build.lib.packages.SelectorList) x).getElements(),
-          what,
-          context,
-          type);
+    if (x instanceof com.google.devtools.build.lib.packages.SelectorList selectorList) {
+      List<Object> selectorListElements = selectorList.getElements();
+      if (!simplifyUnconditionalSelects) {
+        return new SelectorList<T>(selectorListElements, what, context, type);
+      }
+      if (selectorListElements.size() > 1 && type.concat(ImmutableList.of()) == null) {
+        throw new ConversionException(
+            String.format("type '%s' doesn't support select concatenation", type));
+      }
+      // Note: ArrayList, not ImmutableList, because we may insert a null into it; the default value
+      // of an unconditional Selector<T> is null if the SelectorValue value is None and the native
+      // type's default value is null.
+      ArrayList<T> values = new ArrayList<>(selectorListElements.size());
+      for (Object element : selectorListElements) {
+        if (element instanceof SelectorValue selectorValue) {
+          ImmutableMap<?, ?> dictionary = selectorValue.getDictionary();
+          if (dictionary.size() != 1) {
+            // Cannot simplify: selectorValue has multiple branches.
+            return new SelectorList<T>(selectorListElements, what, context, type);
+          }
+          Selector<T> selector =
+              new Selector<>(dictionary, what, context, type, selectorValue.getNoMatchError());
+          if (!selector.isUnconditional()) {
+            // Cannot simplify: the only branch is not the default condition.
+            return new SelectorList<T>(selectorListElements, what, context, type);
+          }
+          values.add(selector.getDefault());
+        } else {
+          values.add(type.convert(element, what, context));
+        }
+      }
+      if (values.size() == 1) {
+        return values.getFirst();
+      } else {
+        return type.concat(values);
+      }
     } else {
       return type.convert(x, what, context);
     }
@@ -199,27 +245,35 @@ static <T> Object selectableConvert(Type<T> type, Object x, Object what, LabelCo
    * BuildType#selectableConvert}. Canonicalizes the value's order if it is a {@link List} type and
    * {@code attr.isOrderIndependent()} returns {@code true}.
    *
+   * <p>Returns null iff {@code simplifyUnconditionalSelects} is true, {@code buildLangValue} is
+   * {@code select({"//conditions:default": None})}, and {@code attr.getType().getDefaultValue()} is
+   * null.
+   *
    * <p>Throws {@link ConversionException} if the conversion fails, or if {@code buildLangValue} is
    * a selector expression but {@code attr.isConfigurable()} is {@code false}.
    */
+  @Nullable
   public static Object convertFromBuildLangType(
       String ruleClass,
       Attribute attr,
       Object buildLangValue,
       LabelConverter labelConverter,
-      Interner<ImmutableList<?>> listInterner)
+      Interner<ImmutableList<?>> listInterner,
+      boolean simplifyUnconditionalSelects)
       throws ConversionException {
+    if ((buildLangValue instanceof com.google.devtools.build.lib.packages.SelectorList)
+        && !attr.isConfigurable()) {
+      throw new ConversionException(
+          String.format("attribute \"%s\" is not configurable", attr.getName()));
+    }
+
     Object converted =
         BuildType.selectableConvert(
             attr.getType(),
             buildLangValue,
             new AttributeConversionContext(attr.getName(), ruleClass),
-            labelConverter);
-
-    if ((converted instanceof SelectorList<?>) && !attr.isConfigurable()) {
-      throw new ConversionException(
-          String.format("attribute \"%s\" is not configurable", attr.getName()));
-    }
+            labelConverter,
+            simplifyUnconditionalSelects);
 
     if (converted instanceof List<?>) {
       if (attr.isOrderIndependent()) {

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

@@ -206,7 +206,9 @@ private static <T> void visitAttributeLabels(
               value,
               "macro attribute (internal)",
               // No string -> Label conversion is being done here.
-              /* context= */ null);
+              /* context= */ null,
+              // Macros always preserve selects as selects.
+              /* simplifyUnconditionalSelects= */ false);
     } catch (ConversionException e) {
       // TODO: #19922 - The fact that we have to do this seems like a signal that we should
       // transition to storing macro attribute values as native-typed attributes in the future.

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

@@ -887,6 +887,7 @@ public static Builder newPackageBuilder(
       Optional<String> associatedModuleName,
       Optional<String> associatedModuleVersion,
       boolean noImplicitFileExport,
+      boolean simplifyUnconditionalSelectsInRuleAttrs,
       RepositoryMapping repositoryMapping,
       RepositoryMapping mainRepositoryMapping,
       @Nullable Semaphore cpuBoundSemaphore,
@@ -918,6 +919,7 @@ public static Builder newPackageBuilder(
         SymbolGenerator.create(id),
         packageSettings.precomputeTransitiveLoads(),
         noImplicitFileExport,
+        simplifyUnconditionalSelectsInRuleAttrs,
         workspaceName,
         mainRepositoryMapping,
         cpuBoundSemaphore,
@@ -932,6 +934,7 @@ public static Builder newExternalPackageBuilder(
       String workspaceName,
       RepositoryMapping mainRepoMapping,
       boolean noImplicitFileExport,
+      boolean simplifyUnconditionalSelectsInRuleAttrs,
       PackageOverheadEstimator packageOverheadEstimator) {
     return new Builder(
         new Metadata(
@@ -948,6 +951,7 @@ public static Builder newExternalPackageBuilder(
         SymbolGenerator.create(workspaceFileKey),
         packageSettings.precomputeTransitiveLoads(),
         noImplicitFileExport,
+        simplifyUnconditionalSelectsInRuleAttrs,
         workspaceName,
         mainRepoMapping,
         /* cpuBoundSemaphore= */ null,
@@ -959,6 +963,7 @@ public static Builder newExternalPackageBuilder(
   public static Builder newExternalPackageBuilderForBzlmod(
       RootedPath moduleFilePath,
       boolean noImplicitFileExport,
+      boolean simplifyUnconditionalSelectsInRuleAttrs,
       PackageIdentifier basePackageId,
       RepositoryMapping repoMapping) {
     return new Builder(
@@ -975,6 +980,7 @@ public static Builder newExternalPackageBuilderForBzlmod(
             SymbolGenerator.create(basePackageId),
             PackageSettings.DEFAULTS.precomputeTransitiveLoads(),
             noImplicitFileExport,
+            simplifyUnconditionalSelectsInRuleAttrs,
             /* workspaceName= */ DUMMY_WORKSPACE_NAME_FOR_BZLMOD_PACKAGES,
             /* mainRepositoryMapping= */ null,
             /* cpuBoundSemaphore= */ null,
@@ -1053,6 +1059,7 @@ default boolean precomputeTransitiveLoads() {
 
     private final boolean precomputeTransitiveLoads;
     private final boolean noImplicitFileExport;
+    private final boolean simplifyUnconditionalSelectsInRuleAttrs;
 
     // The map from each repository to that repository's remappings map.
     // This is only used in the //external package, it is an empty map for all other packages.
@@ -1183,6 +1190,7 @@ private Builder(
         SymbolGenerator<?> symbolGenerator,
         boolean precomputeTransitiveLoads,
         boolean noImplicitFileExport,
+        boolean simplifyUnconditionalSelectsInRuleAttrs,
         String workspaceName,
         RepositoryMapping mainRepositoryMapping,
         @Nullable Semaphore cpuBoundSemaphore,
@@ -1208,6 +1216,7 @@ private Builder(
 
       this.precomputeTransitiveLoads = precomputeTransitiveLoads;
       this.noImplicitFileExport = noImplicitFileExport;
+      this.simplifyUnconditionalSelectsInRuleAttrs = simplifyUnconditionalSelectsInRuleAttrs;
       this.labelConverter =
           new LabelConverter(metadata.packageIdentifier(), metadata.repositoryMapping());
       if (metadata.getName().startsWith("javatests/")) {
@@ -1631,6 +1640,14 @@ public Globber getGlobber() {
       return globber;
     }
 
+    /**
+     * Returns true if values of conditional rule attributes which only contain unconditional
+     * selects should be simplified and stored as a non-select value.
+     */
+    public boolean simplifyUnconditionalSelectsInRuleAttrs() {
+      return this.simplifyUnconditionalSelectsInRuleAttrs;
+    }
+
     /**
      * Returns the innermost currently executing symbolic macro, or null if not in a symbolic macro.
      */

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

@@ -219,6 +219,8 @@ public Package.Builder newExternalPackageBuilder(
         workspaceName,
         mainRepoMapping,
         starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
+        starlarkSemantics.getBool(
+            BuildLanguageOptions.INCOMPATIBLE_SIMPLIFY_UNCONDITIONAL_SELECTS_IN_RULE_ATTRS),
         packageOverheadEstimator);
   }
 
@@ -247,6 +249,8 @@ public Package.Builder newPackageBuilder(
         associatedModuleName,
         associatedModuleVersion,
         starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
+        starlarkSemantics.getBool(
+            BuildLanguageOptions.INCOMPATIBLE_SIMPLIFY_UNCONDITIONAL_SELECTS_IN_RULE_ATTRS),
         repositoryMapping,
         mainRepositoryMapping,
         cpuBoundSemaphore,

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

@@ -2139,7 +2139,8 @@ private <T> void populateRuleAttributeValues(
             attributeValues,
             failOnUnknownAttributes,
             pkgBuilder.getListInterner(),
-            pkgBuilder.getLocalEventHandler());
+            pkgBuilder.getLocalEventHandler(),
+            pkgBuilder.simplifyUnconditionalSelectsInRuleAttrs());
     populateDefaultRuleAttributeValues(rule, pkgBuilder, definedAttrIndices);
     // Now that all attributes are bound to values, collect and store configurable attribute keys.
     populateConfigDependenciesAttribute(rule);
@@ -2162,7 +2163,8 @@ private <T> BitSet populateDefinedRuleAttributeValues(
       AttributeValues<T> attributeValues,
       boolean failOnUnknownAttributes,
       Interner<ImmutableList<?>> listInterner,
-      EventHandler eventHandler) {
+      EventHandler eventHandler,
+      boolean simplifyUnconditionalSelects) {
     BitSet definedAttrIndices = new BitSet();
     for (T attributeAccessor : attributeValues.getAttributeAccessors()) {
       String attributeName = attributeValues.getName(attributeAccessor);
@@ -2215,11 +2217,20 @@ private <T> BitSet populateDefinedRuleAttributeValues(
         try {
           nativeAttributeValue =
               BuildType.convertFromBuildLangType(
-                  rule.getRuleClass(), attr, attributeValue, labelConverter, listInterner);
+                  rule.getRuleClass(),
+                  attr,
+                  attributeValue,
+                  labelConverter,
+                  listInterner,
+                  simplifyUnconditionalSelects);
         } catch (ConversionException e) {
           rule.reportError(String.format("%s: %s", rule.getLabel(), e.getMessage()), eventHandler);
           continue;
         }
+        // Ignore select({"//conditions:default": None}) values for attr types with null default.
+        if (nativeAttributeValue == null) {
+          continue;
+        }
       } else {
         nativeAttributeValue = attributeValue;
       }

src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java

@@ -785,6 +785,19 @@ public final class BuildLanguageOptions extends OptionsBase {
               + " ctx.actions.run_shell.")
   public boolean incompatibleDisallowCtxResolveTools;
 
+  @Option(
+      name = "incompatible_simplify_unconditional_selects_in_rule_attrs",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      help =
+          "If true, simplify configurable rule attributes which contain only unconditional selects;"
+              + " for example, if [\"a\"] + select(\"//conditions:default\", [\"b\"]) is assigned"
+              + " to a rule attribute, it is stored as [\"a\", \"b\"]. This option does not affect"
+              + " attributes of symbolic macros or attribute default values.")
+  public boolean incompatibleSimplifyUnconditionalSelectsInRuleAttrs;
+
   /**
    * An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should
    * never accumulate a large number of these and being able to shortcut on object identity makes a
@@ -894,6 +907,9 @@ public StarlarkSemantics toStarlarkSemantics() {
             .setBool(
                 INCOMPATIBLE_STOP_EXPORTING_BUILD_FILE_PATH, incompatibleStopExportingBuildFilePath)
             .setBool(INCOMPATIBLE_DISALLOW_CTX_RESOLVE_TOOLS, incompatibleDisallowCtxResolveTools)
+            .setBool(
+                INCOMPATIBLE_SIMPLIFY_UNCONDITIONAL_SELECTS_IN_RULE_ATTRS,
+                incompatibleSimplifyUnconditionalSelectsInRuleAttrs)
             .build();
     return INTERNER.intern(semantics);
   }
@@ -995,6 +1011,8 @@ public StarlarkSemantics toStarlarkSemantics() {
       "-incompatible_stop_exporting_build_file_path";
   public static final String INCOMPATIBLE_DISALLOW_CTX_RESOLVE_TOOLS =
       "+incompatible_disallow_ctx_resolve_tools";
+  public static final String INCOMPATIBLE_SIMPLIFY_UNCONDITIONAL_SELECTS_IN_RULE_ATTRS =
+      "-incompatible_simplify_unconditional_selects_in_rule_attrs";
   // non-booleans
   public static final StarlarkSemantics.Key<String> EXPERIMENTAL_BUILTINS_BZL_PATH =
       new StarlarkSemantics.Key<>("experimental_builtins_bzl_path", "%bundled%");

src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java

@@ -2144,4 +2144,57 @@ public void stringListDictTypeConcatConfigurable() throws Exception {
     assertThat(attributes.get("string_list_dict_attr", Types.STRING_LIST_DICT))
         .containsExactly("a", Arrays.asList("a.out"), "b", Arrays.asList("b.out"));
   }
+
+  @Test
+  public void incompatibleSimplifyUnconditionalSelectsInRuleAttrs_checksForNonconfigurableAttrs()
+      throws Exception {
+    setBuildLanguageOptions("--incompatible_simplify_unconditional_selects_in_rule_attrs=true");
+    writeConfigRules();
+    scratch.file(
+        "foo/BUILD",
+        """
+        rule_with_output_attr(
+            name = "foo",
+            out = select({"//conditions:default": "default.out"}),
+        )
+        """);
+
+    reporter.removeHandler(failFastHandler); // Expect errors.
+    getConfiguredTarget("//foo");
+    assertContainsEvent("attribute \"out\" is not configurable");
+  }
+
+  @Test
+  public void incompatibleSimplifyUnconditionalSelectsInRuleAttrs_doesNotAffectConfiguredAttrValue()
+      throws Exception {
+    scratch.file(
+        "foo/BUILD",
+        """
+        cc_binary(
+            name = "foo",
+            srcs = select({"//conditions:default": ["foo.cc"]}),
+            link_extra_lib = select({"//conditions:default": None}),
+        )
+        """);
+    setBuildLanguageOptions("--incompatible_simplify_unconditional_selects_in_rule_attrs=false");
+    AttributeMap attributesFromUnsimplifiedSelects =
+        getMapperFromConfiguredTargetAndTarget(getConfiguredTargetAndData("//foo"));
+
+    assertThat(attributesFromUnsimplifiedSelects.get("srcs", BuildType.LABEL_LIST))
+        .containsExactly(Label.parseCanonicalUnchecked("//foo:foo.cc"));
+    assertThat(attributesFromUnsimplifiedSelects.get("link_extra_lib", BuildType.LABEL))
+        .isEqualTo(
+            attributesFromUnsimplifiedSelects
+                .getAttributeDefinition("link_extra_lib")
+                .getDefaultValueUnchecked());
+
+    setBuildLanguageOptions("--incompatible_simplify_unconditional_selects_in_rule_attrs=true");
+    AttributeMap attributesFromSimplifiedSelects =
+        getMapperFromConfiguredTargetAndTarget(getConfiguredTargetAndData("//foo"));
+
+    assertThat(attributesFromSimplifiedSelects.get("srcs", BuildType.LABEL_LIST))
+        .isEqualTo(attributesFromUnsimplifiedSelects.get("srcs", BuildType.LABEL_LIST));
+    assertThat(attributesFromSimplifiedSelects.get("link_extra_lib", BuildType.LABEL))
+        .isEqualTo(attributesFromUnsimplifiedSelects.get("link_extra_lib", BuildType.LABEL));
+  }
 }

src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java

@@ -148,6 +148,8 @@ private void setUpContextForRule(
             "runfiles",
             RepositoryMapping.ALWAYS_FALLBACK,
             starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
+            starlarkSemantics.getBool(
+                BuildLanguageOptions.INCOMPATIBLE_SIMPLIFY_UNCONDITIONAL_SELECTS_IN_RULE_ATTRS),
             PackageOverheadEstimator.NOOP_ESTIMATOR);
     ExtendedEventHandler listener = Mockito.mock(ExtendedEventHandler.class);
     Rule rule =

src/test/java/com/google/devtools/build/lib/packages/BUILD

@@ -73,6 +73,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/packages:exec_group",
         "//src/main/java/com/google/devtools/build/lib/packages:globber",
         "//src/main/java/com/google/devtools/build/lib/packages:package_specification",
+        "//src/main/java/com/google/devtools/build/lib/packages/semantics",
         "//src/main/java/com/google/devtools/build/lib/pkgcache",
         "//src/main/java/com/google/devtools/build/lib/runtime/commands",
         "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value",