Check for duplicates labels in multi-select statements after analysis.
Checking for duplicates in labels in the load phase results can reulst in false
positives where there is no duplicate for any particular configuration but the
same file is referenced in multiple select statements.
For label lists without a select or those that only have a single select, the
check remains in the loading phase.
For label lists with multiple selects, the check occurs after configurations
are resolved and only checks for duplicate labels in the configurations used
within a build rather than all potential configurations.
RELNOTES: selects() no longer produce irrelevant duplicate label checks
PiperOrigin-RevId: 470241662
Change-Id: I626518b96a66cc1fd80b8d355dbfd69506c061a9
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 4ce591a..d7fa514 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -1671,8 +1671,24 @@
}
}
+ /**
+ * Same as {@link #build}, except without some attribute checks.
+ *
+ * <p>Don't use this function outside of testing. The use should be limited to cases where
+ * specifying ConfigConditions.EMPTY, which can cause a noMatchError when accessing attributes
+ * within attribute checking.
+ */
+ @VisibleForTesting
+ public RuleContext unsafeBuild() throws InvalidExecGroupException {
+ return build(false);
+ }
+
@VisibleForTesting
public RuleContext build() throws InvalidExecGroupException {
+ return build(true);
+ }
+
+ private RuleContext build(boolean attributeChecks) throws InvalidExecGroupException {
Preconditions.checkNotNull(ruleClassProvider);
Preconditions.checkNotNull(hostConfiguration);
Preconditions.checkNotNull(configurationFragmentPolicy);
@@ -1681,14 +1697,21 @@
Preconditions.checkNotNull(configConditions);
Preconditions.checkNotNull(mutability);
Preconditions.checkNotNull(visibility);
- AttributeMap attributes =
+ ConfiguredAttributeMapper attributes =
ConfiguredAttributeMapper.of(
target.getAssociatedRule(), configConditions.asProviders(), configuration);
- checkAttributesNonEmpty(attributes);
ListMultimap<String, ConfiguredTargetAndData> targetMap = createTargetMap();
+ // These checks can fail in BuildViewForTesting.getRuleContextForTesting as it specifies
+ // ConfigConditions.EMPTY, resulting in noMatchError accessing attributes without a default
+ // condition.
+ if (attributeChecks) {
+ checkAttributesNonEmpty(attributes);
+ checkAttributesForDuplicateLabels(attributes);
+ }
// This conditionally checks visibility on config_setting rules based on
// --config_setting_visibility_policy. This should be removed as soon as it's deemed safe
- // to unconditionally check visibility. See https://github.com/bazelbuild/bazel/issues/12669.
+ // to unconditionally check visibility. See
+ // https://github.com/bazelbuild/bazel/issues/12669.
if (target.getPackage().getConfigSettingVisibilityPolicy()
!= ConfigSettingVisibilityPolicy.LEGACY_OFF) {
Attribute configSettingAttr = attributes.getAttributeDefinition("$config_dependencies");
@@ -1740,6 +1763,20 @@
}
}
+ private void checkAttributesForDuplicateLabels(ConfiguredAttributeMapper attributes) {
+ for (String attributeName : attributes.getAttributeNames()) {
+ Attribute attr = attributes.getAttributeDefinition(attributeName);
+ if (attr.getType() != BuildType.LABEL_LIST) {
+ continue;
+ }
+
+ Set<Label> duplicates = attributes.checkForDuplicateLabels(attr);
+ for (Label label : duplicates) {
+ reporter.attributeError(attr.getName(), String.format("Label '%s' is duplicated", label));
+ }
+ }
+ }
+
@CanIgnoreReturnValue
public Builder setRuleClassProvider(ConfiguredRuleClassProvider ruleClassProvider) {
this.ruleClassProvider = ruleClassProvider;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
index 5c8fa74..5c7c568 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
@@ -31,7 +31,6 @@
import java.util.Collection;
import java.util.Deque;
import java.util.HashMap;
-import java.util.LinkedHashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
@@ -181,26 +180,16 @@
return checkForDuplicateLabels(selectors.get(0).getEntries().values());
}
- // Multiple selects concatenated together. It's expensive to iterate over every possible
- // permutation of values, so instead check for duplicates within a single select branch while
- // also collecting all labels for a cross-select duplicate check at the end. This is overly
- // strict, since this counts values present in mutually exclusive select branches. We can
- // presumably relax this if necessary, but doing so would incur some of the expense this code
- // path avoids.
+ // It's expensive to iterate over every possible permutation of values, so instead check for
+ // duplicates within a single select branch. Then, after analysis we will check for duplicates
+ // within only the used permutations.
ImmutableSet.Builder<Label> duplicates = null;
- List<Label> combinedLabels = new ArrayList<>(); // Labels that appear across all selectors.
for (Selector<List<Label>> selector : selectors) {
- // Labels within a single selector. It's okay for there to be duplicates as long as
- // they're in different selector paths (since only one path can actually get chosen).
- Set<Label> selectorLabels = new LinkedHashSet<>();
for (List<Label> labelsInSelectorValue : selector.getEntries().values()) {
// Duplicates within a single select branch are not okay.
duplicates = addDuplicateLabels(duplicates, labelsInSelectorValue);
- selectorLabels.addAll(labelsInSelectorValue);
}
- combinedLabels.addAll(selectorLabels);
}
- duplicates = addDuplicateLabels(duplicates, combinedLabels);
return duplicates == null ? ImmutableSet.of() : duplicates.build();
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BUILD b/src/main/java/com/google/devtools/build/lib/packages/BUILD
index 0b496c0..8a1b4aa 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/packages/BUILD
@@ -19,6 +19,7 @@
"//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/cmdline",
+ "//src/main/java/com/google/devtools/build/lib/collect",
"//third_party:guava",
"//third_party:jsr305",
],
diff --git a/src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java
index 290bea7..dbf0946 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java
@@ -13,16 +13,20 @@
// limitations under the License.
package com.google.devtools.build.lib.packages;
+import static com.google.common.base.Preconditions.checkArgument;
+
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.base.Predicates;
import com.google.common.base.Verify;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.collect.CollectionUtils;
import com.google.devtools.build.lib.packages.BuildType.Selector;
import com.google.devtools.build.lib.packages.BuildType.SelectorList;
import java.util.ArrayList;
@@ -31,6 +35,7 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import javax.annotation.Nullable;
/**
@@ -317,4 +322,18 @@
}
return false; // Every select() in this list chooses a path with value "None".
}
+
+ /** Returns the labels that appear multiple times in the same attribute value. */
+ public Set<Label> checkForDuplicateLabels(Attribute attribute) {
+ Type<List<Label>> attrType = BuildType.LABEL_LIST;
+ checkArgument(attribute.getType() == attrType, "Not a label list type: %s", attribute);
+ String attrName = attribute.getName();
+ SelectorList<List<Label>> selectorList = getSelectorList(attrName, attrType);
+ // already checked in RuleClass via AggregatingAttributeMapper.checkForDuplicateLabels
+ if (selectorList == null || selectorList.getSelectors().size() == 1) {
+ return ImmutableSet.of();
+ }
+ List<Label> labels = get(attrName, attrType);
+ return CollectionUtils.duplicatedElementsOf(labels);
+ }
}
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 3bacb5d..ee24a14 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
@@ -282,7 +282,8 @@
@Test
public void duplicatesAcrossMultipleSelects() throws Exception {
writeConfigRules();
- scratch.file("java/hello/BUILD",
+ scratch.file(
+ "java/hello/BUILD",
"java_binary(",
" name = 'hello',",
" srcs = select({",
@@ -290,15 +291,16 @@
" '//conditions:b': ['b.java'],",
" })",
" + select({",
- " '//conditions:c': ['c.java'],",
- " '//conditions:d': ['a.java'],",
+ " '//conditions:a': ['a.java'],",
+ " '//conditions:b': ['c.java'],",
" }))");
reporter.removeHandler(failFastHandler); // Expect errors.
useConfiguration("--foo=a");
getConfiguredTarget("//java/hello:hello");
assertContainsEvent(
- "Label '//java/hello:a.java' is duplicated in the 'srcs' attribute of rule 'hello'");
+ "in srcs attribute of java_binary rule //java/hello:hello: Label '//java/hello:a.java' is"
+ + " duplicated");
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
index e0ae47c..7ad1d4d 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -670,7 +670,7 @@
.setConfigConditions(ConfigConditions.EMPTY)
.setToolchainContexts(resolvedToolchainContext.build())
.setExecGroupCollectionBuilder(execGroupCollectionBuilder)
- .build();
+ .unsafeBuild();
}
/** Clears the analysis cache as in --discard_analysis_cache. */
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
index 95674ab..7483778 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
@@ -315,6 +315,218 @@
}
@Test
+ public void testDuplicatedDepsWithinSingleSelectConditionError() throws Exception {
+ RuleClass depsRuleClass =
+ newRuleClass(
+ "ruleDeps",
+ false,
+ false,
+ false,
+ false,
+ false,
+ false,
+ ImplicitOutputsFunction.NONE,
+ null,
+ DUMMY_CONFIGURED_TARGET_FACTORY,
+ PredicatesWithMessage.<Rule>alwaysTrue(),
+ PREFERRED_DEPENDENCY_PREDICATE,
+ AdvertisedProviderSet.EMPTY,
+ null,
+ ImmutableSet.of(),
+ true,
+ attr("list1", LABEL_LIST).mandatory().legacyAllowAnyFileType().build());
+
+ SelectorList selectorList1 =
+ SelectorList.of(
+ new SelectorValue(
+ ImmutableMap.of("//conditions:a", ImmutableList.of(":dup1", ":dup1")), ""));
+
+ // expect errors
+ reporter.removeHandler(failFastHandler);
+
+ Map<String, Object> attributeValues = new HashMap<>();
+ attributeValues.put("list1", selectorList1);
+ createRule(depsRuleClass, "depsRule", attributeValues, testRuleLocation, NO_STACK);
+
+ assertThat(eventCollector.count()).isSameInstanceAs(1);
+ assertDupError("//testpackage:dup1", "list1", "depsRule");
+ }
+
+ @Test
+ public void testDuplicatedDepsWithinConditionMultipleSelectsErrors() throws Exception {
+ RuleClass depsRuleClass =
+ newRuleClass(
+ "ruleDeps",
+ false,
+ false,
+ false,
+ false,
+ false,
+ false,
+ ImplicitOutputsFunction.NONE,
+ null,
+ DUMMY_CONFIGURED_TARGET_FACTORY,
+ PredicatesWithMessage.<Rule>alwaysTrue(),
+ PREFERRED_DEPENDENCY_PREDICATE,
+ AdvertisedProviderSet.EMPTY,
+ null,
+ ImmutableSet.of(),
+ true,
+ attr("list1", LABEL_LIST).mandatory().legacyAllowAnyFileType().build());
+
+ SelectorList selectorList1a =
+ SelectorList.of(
+ new SelectorValue(
+ ImmutableMap.of(
+ "//conditions:a", ImmutableList.of(":dup1", "dup1"),
+ "//conditions:b", ImmutableList.of(":nodup1")),
+ ""));
+ SelectorList selectorList1b =
+ SelectorList.of(
+ new SelectorValue(
+ ImmutableMap.of(
+ "//conditions:c", ImmutableList.of(":dup2", "dup2"),
+ "//conditions:d", ImmutableList.of(":nodup1")),
+ ""));
+ SelectorList selectorList1 = SelectorList.concat(selectorList1a, selectorList1b);
+
+ // expect errors
+ reporter.removeHandler(failFastHandler);
+
+ Map<String, Object> attributeValues = new HashMap<>();
+ attributeValues.put("list1", selectorList1);
+ createRule(depsRuleClass, "depsRule", attributeValues, testRuleLocation, NO_STACK);
+
+ assertThat(eventCollector.count()).isSameInstanceAs(2);
+ assertDupError("//testpackage:dup1", "list1", "depsRule");
+ assertDupError("//testpackage:dup2", "list1", "depsRule");
+ }
+
+ @Test
+ public void testSameDepAcrossMultipleSelectsNoDuplicateNoError() throws Exception {
+ RuleClass depsRuleClass =
+ newRuleClass(
+ "ruleDeps",
+ false,
+ false,
+ false,
+ false,
+ false,
+ false,
+ ImplicitOutputsFunction.NONE,
+ null,
+ DUMMY_CONFIGURED_TARGET_FACTORY,
+ PredicatesWithMessage.<Rule>alwaysTrue(),
+ PREFERRED_DEPENDENCY_PREDICATE,
+ AdvertisedProviderSet.EMPTY,
+ null,
+ ImmutableSet.of(),
+ true,
+ attr("list1", LABEL_LIST).mandatory().legacyAllowAnyFileType().build());
+
+ // ignore duplicatess across selects where values appear duplicated but are not
+ SelectorList selectorList1a =
+ SelectorList.of(
+ new SelectorValue(
+ ImmutableMap.of(
+ "//conditions:a", ImmutableList.of(":nodup1"),
+ "//conditions:b", ImmutableList.of(":nodup2")),
+ ""));
+ SelectorList selectorList1b =
+ SelectorList.of(
+ new SelectorValue(
+ ImmutableMap.of(
+ "//conditions:a", ImmutableList.of(":nodup2"),
+ "//conditions:b", ImmutableList.of(":nodup1")),
+ ""));
+ SelectorList selectorList1 = SelectorList.concat(selectorList1a, selectorList1b);
+
+ Map<String, Object> attributeValues = new HashMap<>();
+ attributeValues.put("list1", selectorList1);
+ createRule(depsRuleClass, "depsRule", attributeValues, testRuleLocation, NO_STACK);
+ }
+
+ @Test
+ public void testSameDepAcrossMultipleSelectsIsDuplicateNoError() throws Exception {
+ RuleClass depsRuleClass =
+ newRuleClass(
+ "ruleDeps",
+ false,
+ false,
+ false,
+ false,
+ false,
+ false,
+ ImplicitOutputsFunction.NONE,
+ null,
+ DUMMY_CONFIGURED_TARGET_FACTORY,
+ PredicatesWithMessage.<Rule>alwaysTrue(),
+ PREFERRED_DEPENDENCY_PREDICATE,
+ AdvertisedProviderSet.EMPTY,
+ null,
+ ImmutableSet.of(),
+ true,
+ attr("list1", LABEL_LIST).mandatory().legacyAllowAnyFileType().build());
+
+ // repetition of dup1 is identified at analysis time, not loading time
+ SelectorList selectorList1a =
+ SelectorList.of(
+ new SelectorValue(
+ ImmutableMap.of(
+ "//conditions:a", ImmutableList.of(":dup1"),
+ "//conditions:b", ImmutableList.of(":nodup1")),
+ ""));
+ SelectorList selectorList1b =
+ SelectorList.of(
+ new SelectorValue(
+ ImmutableMap.of(
+ "//conditions:a", ImmutableList.of(":dup1"),
+ "//conditions:b", ImmutableList.of(":nodup2")),
+ ""));
+ SelectorList selectorList1 = SelectorList.concat(selectorList1a, selectorList1b);
+
+ Map<String, Object> attributeValues = new HashMap<>();
+ attributeValues.put("list1", selectorList1);
+ createRule(depsRuleClass, "depsRule", attributeValues, testRuleLocation, NO_STACK);
+ }
+
+ @Test
+ public void testSameDepAcrossConditionsInSelectNoError() throws Exception {
+ RuleClass depsRuleClass =
+ newRuleClass(
+ "ruleDeps",
+ false,
+ false,
+ false,
+ false,
+ false,
+ false,
+ ImplicitOutputsFunction.NONE,
+ null,
+ DUMMY_CONFIGURED_TARGET_FACTORY,
+ PredicatesWithMessage.<Rule>alwaysTrue(),
+ PREFERRED_DEPENDENCY_PREDICATE,
+ AdvertisedProviderSet.EMPTY,
+ null,
+ ImmutableSet.of(),
+ true,
+ attr("list1", LABEL_LIST).mandatory().legacyAllowAnyFileType().build());
+
+ SelectorList selectorList1 =
+ SelectorList.of(
+ new SelectorValue(
+ ImmutableMap.of(
+ "//conditions:a", ImmutableList.of(":nodup1"),
+ "//conditions:b", ImmutableList.of(":nodup1")),
+ ""));
+
+ Map<String, Object> attributeValues = new HashMap<>();
+ attributeValues.put("list1", selectorList1);
+
+ createRule(depsRuleClass, "depsRule", attributeValues, testRuleLocation, NO_STACK);
+ }
+
+ @Test
public void testCreateRule() throws Exception {
RuleClass ruleClassA = createRuleClassA();
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
index 1e56ab1..9b66920 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
@@ -1452,6 +1452,107 @@
}
@Test
+ public void testLabelListNoDuplicatesNoError() throws Exception {
+ reporter.removeHandler(failFastHandler);
+ scratch.file("a.txt", "");
+ scratch.file(
+ "my_rule.bzl",
+ "def _impl(ctx):",
+ " return",
+ "my_rule = rule(",
+ " implementation = _impl,",
+ " attrs = {",
+ " 'label_list': attr.label_list(allow_files=True),",
+ " }",
+ ")");
+
+ scratch.file(
+ "BUILD",
+ "load('//:my_rule.bzl', 'my_rule')",
+ "my_rule(name='r',",
+ " label_list=[\"a.txt\"])");
+
+ invalidatePackages();
+ getConfiguredTarget("//:r");
+ assertNoEvents();
+ }
+
+ @Test
+ public void testLabelListNoDuplicatesNonOverlappingSelectsNoError() throws Exception {
+ reporter.removeHandler(failFastHandler);
+ scratch.file("a.txt", "");
+ scratch.file(
+ "my_rule.bzl",
+ "def _impl(ctx):",
+ " return",
+ "my_rule = rule(",
+ " implementation = _impl,",
+ " attrs = {",
+ " 'label_list': attr.label_list(allow_files=True),",
+ " }",
+ ")");
+
+ scratch.file(
+ "BUILD",
+ "load('//:my_rule.bzl', 'my_rule')",
+ "config_setting(",
+ " name = 'arm_cpu',",
+ " values = {'cpu': 'arm'},",
+ ")",
+ "my_rule(name='r',",
+ " label_list=select({",
+ " ':arm_cpu': [],",
+ " '//conditions:default': ['a.txt'],",
+ "}) + select({",
+ " ':arm_cpu': ['a.txt'],",
+ " '//conditions:default': [],",
+ "}),",
+ ")");
+
+ invalidatePackages();
+ getConfiguredTarget("//:r");
+ assertNoEvents();
+ }
+
+ @Test
+ public void testLabelListNoDuplicatesOverlappingSelectsHasError() throws Exception {
+ reporter.removeHandler(failFastHandler);
+ scratch.file("a.txt", "");
+ scratch.file(
+ "my_rule.bzl",
+ "def _impl(ctx):",
+ " return",
+ "my_rule = rule(",
+ " implementation = _impl,",
+ " attrs = {",
+ " 'label_list': attr.label_list(allow_files=True),",
+ " }",
+ ")");
+
+ scratch.file(
+ "BUILD",
+ "load('//:my_rule.bzl', 'my_rule')",
+ "config_setting(",
+ " name = 'arm_cpu',",
+ " values = {'cpu': 'arm'},",
+ ")",
+ "my_rule(name='r',",
+ " label_list=select({",
+ " ':arm_cpu': [],",
+ " '//conditions:default': ['a.txt'],",
+ "}) + select({",
+ " ':arm_cpu': ['a.txt'],",
+ " '//conditions:default': ['a.txt'],",
+ "}),",
+ ")");
+
+ invalidatePackages();
+ getConfiguredTarget("//:r");
+ assertContainsEvent(
+ "in label_list attribute of my_rule rule //:r: " + "Label \'//:a.txt\' is duplicated");
+ }
+
+ @Test
public void testLabelKeyedStringDictForbidsMissingAttributeWhenMandatoryIsTrue()
throws Exception {
reporter.removeHandler(failFastHandler);
