Throw a rule error when a skylark rule implementation returns multiple providers of the same type.
This error-throwing functionality is tied to semantic flag --incompatible_disallow_conflicting_providers
Partial fix for #5902. (That issue will still track migration).
RELNOTES: A rule error will be thrown if a Skylark rule implementation function returns multiple providers of the same type. Try the `--incompatible_disallow_conflicting_providers` flag to ensure your code is forward-compatible.
PiperOrigin-RevId: 208884695
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
index 3b150c2..e0eafd6 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
@@ -36,6 +36,7 @@
import com.google.devtools.build.lib.packages.AdvertisedProviderSet;
import com.google.devtools.build.lib.packages.Info;
import com.google.devtools.build.lib.packages.NativeProvider;
+import com.google.devtools.build.lib.packages.Provider;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
@@ -57,8 +58,8 @@
import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.build.lib.util.FileType;
import com.google.devtools.build.lib.util.FileTypeSet;
-import java.util.ArrayList;
import java.util.Collections;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
@@ -299,7 +300,7 @@
throws EvalException {
Info oldStyleProviders = StructProvider.STRUCT.createEmpty(loc);
- ArrayList<Info> declaredProviders = new ArrayList<>();
+ Map<Provider.Key, Info> declaredProviders = new LinkedHashMap<>();
if (target instanceof Info) {
// Either an old-style struct or a single declared provider (not in a list)
@@ -319,12 +320,19 @@
Info.class,
loc,
"The value of 'providers' should be a sequence of declared providers");
- declaredProviders.add(declaredProvider);
+ Provider.Key providerKey = declaredProvider.getProvider().getKey();
+ if (declaredProviders.put(providerKey, declaredProvider) != null) {
+ if (context.getSkylarkSemantics().incompatibleDisallowConflictingProviders()) {
+ context.getRuleContext()
+ .ruleError("Multiple conflicting returned providers with key " + providerKey);
+ }
+ }
}
}
} else {
+ Provider.Key providerKey = struct.getProvider().getKey();
// Single declared provider
- declaredProviders.add(struct);
+ declaredProviders.put(providerKey, struct);
}
} else if (target instanceof Iterable) {
// Sequence of declared providers
@@ -336,13 +344,19 @@
loc,
"A return value of a rule implementation function should be "
+ "a sequence of declared providers");
- declaredProviders.add(declaredProvider);
+ Provider.Key providerKey = declaredProvider.getProvider().getKey();
+ if (declaredProviders.put(providerKey, declaredProvider) != null) {
+ if (context.getSkylarkSemantics().incompatibleDisallowConflictingProviders()) {
+ context.getRuleContext()
+ .ruleError("Multiple conflicting returned providers with key " + providerKey);
+ }
+ }
}
}
boolean defaultProviderProvidedExplicitly = false;
- for (Info declaredProvider : declaredProviders) {
+ for (Info declaredProvider : declaredProviders.values()) {
if (declaredProvider
.getProvider()
.getKey()
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
index 5dec148..6fda353 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
@@ -1012,6 +1012,10 @@
helper.getToolsRunfilesSuppliers());
}
+ public SkylarkSemantics getSkylarkSemantics() {
+ return skylarkSemantics;
+ }
+
/**
* Ensures the given {@link Map} has keys that have {@link Label} type and values that have either
* {@link Iterable} or {@link SkylarkNestedSet} type, and raises {@link EvalException} otherwise.
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
index 5ef70d3..126078d 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
@@ -161,6 +161,21 @@
public boolean incompatibleDisableObjcProviderResources;
@Option(
+ name = "incompatible_disallow_conflicting_providers",
+ defaultValue = "false",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+ metadataTags = {
+ OptionMetadataTag.INCOMPATIBLE_CHANGE,
+ OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+ },
+ help = "If set to true, disallow rule implementation functions from returning multiple "
+ + "instances of the same type of provider. (If false, only the last in the list will be "
+ + "used.)"
+ )
+ public boolean incompatibleDisallowConflictingProviders;
+
+ @Option(
name = "incompatible_disallow_data_transition",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
@@ -376,6 +391,7 @@
.incompatibleDepsetUnion(incompatibleDepsetUnion)
.incompatibleDisableDeprecatedAttrParams(incompatibleDisableDeprecatedAttrParams)
.incompatibleDisableObjcProviderResources(incompatibleDisableObjcProviderResources)
+ .incompatibleDisallowConflictingProviders(incompatibleDisallowConflictingProviders)
.incompatibleDisallowDataTransition(incompatibleDisallowDataTransition)
.incompatibleDisallowDictPlus(incompatibleDisallowDictPlus)
.incompatibleDisallowFileType(incompatibleDisallowFileType)
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
index 85f6d4d..ac13aad 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
@@ -57,6 +57,8 @@
public abstract boolean incompatibleDisableObjcProviderResources();
+ public abstract boolean incompatibleDisallowConflictingProviders();
+
public abstract boolean incompatibleDisallowDataTransition();
public abstract boolean incompatibleDisallowDictPlus();
@@ -110,6 +112,7 @@
.incompatibleDepsetUnion(false)
.incompatibleDisableDeprecatedAttrParams(false)
.incompatibleDisableObjcProviderResources(false)
+ .incompatibleDisallowConflictingProviders(false)
.incompatibleDisallowDataTransition(false)
.incompatibleDisallowDictPlus(false)
.incompatibleDisallowFileType(false)
@@ -148,6 +151,8 @@
public abstract Builder incompatibleDisableObjcProviderResources(boolean value);
+ public abstract Builder incompatibleDisallowConflictingProviders(boolean value);
+
public abstract Builder incompatibleDisallowDataTransition(boolean value);
public abstract Builder incompatibleDisallowDictPlus(boolean value);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
index 5bc1109..3dd878c 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
@@ -129,6 +129,7 @@
"--incompatible_depset_union=" + rand.nextBoolean(),
"--incompatible_disable_deprecated_attr_params=" + rand.nextBoolean(),
"--incompatible_disable_objc_provider_resources=" + rand.nextBoolean(),
+ "--incompatible_disallow_conflicting_providers=" + rand.nextBoolean(),
"--incompatible_disallow_data_transition=" + rand.nextBoolean(),
"--incompatible_disallow_dict_plus=" + rand.nextBoolean(),
"--incompatible_disallow_filetype=" + rand.nextBoolean(),
@@ -162,6 +163,7 @@
.incompatibleDepsetUnion(rand.nextBoolean())
.incompatibleDisableDeprecatedAttrParams(rand.nextBoolean())
.incompatibleDisableObjcProviderResources(rand.nextBoolean())
+ .incompatibleDisallowConflictingProviders(rand.nextBoolean())
.incompatibleDisallowDataTransition(rand.nextBoolean())
.incompatibleDisallowDictPlus(rand.nextBoolean())
.incompatibleDisallowFileType(rand.nextBoolean())
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index 1834735..17f115d 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -1185,6 +1185,104 @@
}
@Test
+ public void testConflictingProviderKeys_fromStruct_allowed() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_disallow_conflicting_providers=false");
+ scratch.file(
+ "test/extension.bzl",
+ "my_provider = provider()",
+ "other_provider = provider()",
+ "def _impl(ctx):",
+ " return struct(providers = [my_provider(x = 1), other_provider(), my_provider(x = 2)])",
+ "my_rule = rule(_impl)"
+ );
+
+ scratch.file(
+ "test/BUILD",
+ "load(':extension.bzl', 'my_rule')",
+ "my_rule(name = 'r')"
+ );
+
+ ConfiguredTarget configuredTarget = getConfiguredTarget("//test:r");
+ Provider.Key key =
+ new SkylarkProvider.SkylarkKey(
+ Label.create(configuredTarget.getLabel().getPackageIdentifier(), "extension.bzl"),
+ "my_provider");
+ Info declaredProvider = configuredTarget.get(key);
+ assertThat(declaredProvider).isNotNull();
+ assertThat(declaredProvider.getProvider().getKey()).isEqualTo(key);
+ assertThat(declaredProvider.getValue("x")).isEqualTo(2);
+ }
+
+ @Test
+ public void testConflictingProviderKeys_fromStruct_disallowed() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_disallow_conflicting_providers=true");
+ scratch.file(
+ "test/extension.bzl",
+ "my_provider = provider()",
+ "other_provider = provider()",
+ "def _impl(ctx):",
+ " return struct(providers = [my_provider(x = 1), other_provider(), my_provider(x = 2)])",
+ "my_rule = rule(_impl)"
+ );
+
+ checkError(
+ "test",
+ "r",
+ "Multiple conflicting returned providers with key my_provider",
+ "load(':extension.bzl', 'my_rule')",
+ "my_rule(name = 'r')");
+ }
+
+ @Test
+ public void testConflictingProviderKeys_fromIterable_allowed() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_disallow_conflicting_providers=false");
+ scratch.file(
+ "test/extension.bzl",
+ "my_provider = provider()",
+ "other_provider = provider()",
+ "def _impl(ctx):",
+ " return [my_provider(x = 1), other_provider(), my_provider(x = 2)]",
+ "my_rule = rule(_impl)"
+ );
+
+ scratch.file(
+ "test/BUILD",
+ "load(':extension.bzl', 'my_rule')",
+ "my_rule(name = 'r')"
+ );
+
+ ConfiguredTarget configuredTarget = getConfiguredTarget("//test:r");
+ Provider.Key key =
+ new SkylarkProvider.SkylarkKey(
+ Label.create(configuredTarget.getLabel().getPackageIdentifier(), "extension.bzl"),
+ "my_provider");
+ Info declaredProvider = configuredTarget.get(key);
+ assertThat(declaredProvider).isNotNull();
+ assertThat(declaredProvider.getProvider().getKey()).isEqualTo(key);
+ assertThat(declaredProvider.getValue("x")).isEqualTo(2);
+ }
+
+ @Test
+ public void testConflictingProviderKeys_fromIterable_disallowed() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_disallow_conflicting_providers=true");
+ scratch.file(
+ "test/extension.bzl",
+ "my_provider = provider()",
+ "other_provider = provider()",
+ "def _impl(ctx):",
+ " return [my_provider(x = 1), other_provider(), my_provider(x = 2)]",
+ "my_rule = rule(_impl)"
+ );
+
+ checkError(
+ "test",
+ "r",
+ "Multiple conflicting returned providers with key my_provider",
+ "load(':extension.bzl', 'my_rule')",
+ "my_rule(name = 'r')");
+ }
+
+ @Test
public void testRecursionDetection() throws Exception {
reporter.removeHandler(failFastHandler);
scratch.file(
