Skip to content

Commit

Permalink
Throw a rule error when a skylark rule implementation returns multipl…
Browse files Browse the repository at this point in the history 
…e 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
  • Loading branch information
@c-parsons
c-parsons authored and Copybara-Service committed Aug 15, 2018
1 parent c544257 commit 5a6fc8d
Show file tree
Hide file tree
Showing 6 changed files with 145 additions and 6 deletions.
26 changes: 20 additions & 6 deletions .../java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -299,7 +300,7 @@ private static void addProviders(
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)
Expand All @@ -319,12 +320,19 @@ private static void addProviders(
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
Expand All @@ -336,13 +344,19 @@ private static void addProviders(
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()
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1012,6 +1012,10 @@ public Tuple<Object> resolveCommand(
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.
Expand Down
16 changes: 16 additions & 0 deletions src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,21 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable
)
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",
Expand Down Expand Up @@ -376,6 +391,7 @@ public SkylarkSemantics toSkylarkSemantics() {
.incompatibleDepsetUnion(incompatibleDepsetUnion)
.incompatibleDisableDeprecatedAttrParams(incompatibleDisableDeprecatedAttrParams)
.incompatibleDisableObjcProviderResources(incompatibleDisableObjcProviderResources)
.incompatibleDisallowConflictingProviders(incompatibleDisallowConflictingProviders)
.incompatibleDisallowDataTransition(incompatibleDisallowDataTransition)
.incompatibleDisallowDictPlus(incompatibleDisallowDictPlus)
.incompatibleDisallowFileType(incompatibleDisallowFileType)
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ public abstract class SkylarkSemantics {

public abstract boolean incompatibleDisableObjcProviderResources();

public abstract boolean incompatibleDisallowConflictingProviders();

public abstract boolean incompatibleDisallowDataTransition();

public abstract boolean incompatibleDisallowDictPlus();
Expand Down Expand Up @@ -110,6 +112,7 @@ public static Builder builderWithDefaults() {
.incompatibleDepsetUnion(false)
.incompatibleDisableDeprecatedAttrParams(false)
.incompatibleDisableObjcProviderResources(false)
.incompatibleDisallowConflictingProviders(false)
.incompatibleDisallowDataTransition(false)
.incompatibleDisallowDictPlus(false)
.incompatibleDisallowFileType(false)
Expand Down Expand Up @@ -148,6 +151,8 @@ public abstract static class Builder {

public abstract Builder incompatibleDisableObjcProviderResources(boolean value);

public abstract Builder incompatibleDisallowConflictingProviders(boolean value);

public abstract Builder incompatibleDisallowDataTransition(boolean value);

public abstract Builder incompatibleDisallowDictPlus(boolean value);
Expand Down
2 changes: 2 additions & 0 deletions src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ private static SkylarkSemanticsOptions buildRandomOptions(Random rand) throws Ex
"--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(),
Expand Down Expand Up @@ -162,6 +163,7 @@ private static SkylarkSemantics buildRandomSemantics(Random rand) {
.incompatibleDepsetUnion(rand.nextBoolean())
.incompatibleDisableDeprecatedAttrParams(rand.nextBoolean())
.incompatibleDisableObjcProviderResources(rand.nextBoolean())
.incompatibleDisallowConflictingProviders(rand.nextBoolean())
.incompatibleDisallowDataTransition(rand.nextBoolean())
.incompatibleDisallowDictPlus(rand.nextBoolean())
.incompatibleDisallowFileType(rand.nextBoolean())
Expand Down
98 changes: 98 additions & 0 deletions src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1184,6 +1184,104 @@ public void rulesReturningDeclaredProvidersCompatMode() throws Exception {
assertThat(declaredProvider.getValue("x")).isEqualTo(1);
}

@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);
Expand Down

0 comments on commit 5a6fc8d

Please sign in to comment.