Throw a rule error when a skylark rule implementation returns multipl…

…e providers of the same type. RELNOTES: A rule error is now thrown if a Skylark rule implementation function returns multiple providers of the same type. PiperOrigin-RevId: 206371385
jin · Jul 27, 2018 · be88b85 · be88b85
1 parent b6bf51d
commit be88b85
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 6 deletions.
diff --git a/.../java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java b/.../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;
@@ -290,7 +291,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)
@@ -310,12 +311,17 @@ 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) {
+              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
@@ -327,13 +333,17 @@ 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) {
+          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/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -1184,6 +1184,25 @@ public void rulesReturningDeclaredProvidersCompatMode() throws Exception {
     assertThat(declaredProvider.getValue("x")).isEqualTo(1);
   }
 
+  @Test
+  public void testConflictingProviderKeys() throws Exception {
+    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()])",
+        "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);