diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java index b737f85588943b..7d91c19e3d0e23 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java +++ b/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 = diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java index a291cd5fb75d4f..8f9f964cde4662 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java +++ b/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. * + *

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. + * + *

Returns null iff {@code simplifyUnconditionalSelects} is true, {@code x} is {@code + * select({"//conditions:default": None})}, and the {@code type.getDefaultValue()} is null. + * *

The caller is responsible for casting the returned value appropriately. */ - static Object selectableConvert(Type type, Object x, Object what, LabelConverter context) + @Nullable + static Object selectableConvert( + Type 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 selectorListElements = selectorList.getElements(); + if (!simplifyUnconditionalSelects) { + return new SelectorList(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 is null if the SelectorValue value is None and the native + // type's default value is null. + ArrayList 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(selectorListElements, what, context, type); + } + Selector 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(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 Object selectableConvert(Type 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}. * + *

Returns null iff {@code simplifyUnconditionalSelects} is true, {@code buildLangValue} is + * {@code select({"//conditions:default": None})}, and {@code attr.getType().getDefaultValue()} is + * null. + * *

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> listInterner) + Interner> 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()) { diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java b/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java index fa47339bfa444e..6591c38d7747d4 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java +++ b/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java @@ -206,7 +206,9 @@ private static 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. diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index 1955df7b67ed0b..e8b87acf562cf8 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java @@ -887,6 +887,7 @@ public static Builder newPackageBuilder( Optional associatedModuleName, Optional 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. */ diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java index ef0cfb6e35d633..3ce05f46d70fe6 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java +++ b/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, diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java index 10ef37134e7469..e1c5e20676edd9 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java @@ -2139,7 +2139,8 @@ private 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 BitSet populateDefinedRuleAttributeValues( AttributeValues attributeValues, boolean failOnUnknownAttributes, Interner> 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 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; } diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index fd494f176aa23e..65e734ff2d0710 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/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 EXPERIMENTAL_BUILTINS_BZL_PATH = new StarlarkSemantics.Key<>("experimental_builtins_bzl_path", "%bundled%"); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java index 12805eb0c101d4..d791045f9b07d9 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java +++ b/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)); + } } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java index 15a8b13fe723fc..05c30e32699610 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java +++ b/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 = diff --git a/src/test/java/com/google/devtools/build/lib/packages/BUILD b/src/test/java/com/google/devtools/build/lib/packages/BUILD index 4aa3eb213eeb78..d34c24441bcffd 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/BUILD +++ b/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", diff --git a/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java b/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java index 83edb0713ad97c..a9aa19126ca32f 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.packages; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; @@ -24,6 +25,8 @@ import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.packages.BuildType.Selector; import com.google.devtools.build.lib.packages.Type.ConversionException; +import java.lang.reflect.Field; +import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -489,7 +492,7 @@ public void testSelectorList_concatenate_invalidType() throws Exception { */ @SuppressWarnings({"unchecked", "TruthIncompatibleType"}) @Test - public void testSelectableConvert() throws Exception { + public void selectableConvert_basicUsage() throws Exception { Object nativeInput = Arrays.asList("//a:a1", "//a:a2"); Object selectableInput = SelectorList.of(new SelectorValue(ImmutableMap.of( @@ -500,13 +503,23 @@ public void testSelectableConvert() throws Exception { // Conversion to direct type: Object converted = - BuildType.selectableConvert(BuildType.LABEL_LIST, nativeInput, null, labelConverter); + BuildType.selectableConvert( + BuildType.LABEL_LIST, + nativeInput, + null, + labelConverter, + /* simplifyUnconditionalSelects= */ false); assertThat(converted instanceof List).isTrue(); assertThat((List