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);
+ }
}
