Report starlark error if multiple targets with the same label are use…

…d in `expand_location` `targets` list Fixes: #16664 PiperOrigin-RevId: 576172446 Change-Id: I22641ee4b0c34dc4818962ece58f786708a45da0
bazelbuild · Oct 24, 2023 · 3db362c · 3db362c
1 parent 0acc817
commit 3db362c
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 7 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java
@@ -1206,15 +1206,18 @@ private static Map<Label, Iterable<Artifact>> checkLabelDict(Map<?, ?> labelDict
    * @return Immutable map with immutable collections as values
    */
   public static ImmutableMap<Label, ImmutableCollection<Artifact>> makeLabelMap(
-      Iterable<TransitiveInfoCollection> knownLabels) {
-    ImmutableMap.Builder<Label, ImmutableCollection<Artifact>> builder = ImmutableMap.builder();
-
+      Iterable<TransitiveInfoCollection> knownLabels) throws EvalException {
+    var targetsMap = new LinkedHashMap<Label, ImmutableCollection<Artifact>>();
     for (TransitiveInfoCollection current : knownLabels) {
-      builder.put(
-          AliasProvider.getDependencyLabel(current),
-          current.getProvider(FileProvider.class).getFilesToBuild().toList());
+      Label label = AliasProvider.getDependencyLabel(current);
+      if (targetsMap.containsKey(label)) {
+        throw Starlark.errorf(
+            "Label %s is found more than once in 'targets' list.", Starlark.repr(label.toString()));
+      }
+
+      targetsMap.put(label, current.getProvider(FileProvider.class).getFilesToBuild().toList());
     }
 
-    return builder.buildOrThrow();
+    return ImmutableMap.copyOf(targetsMap);
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java
@@ -884,6 +884,65 @@ public void aspectSkippingOrphanArtifactsWithLocation() throws Exception {
     assertThat(result).isEqualTo("simple/afile");
   }
 
+  @Test
+  public void expandLocationFailsForTargetsWithSameLabel() throws Exception {
+    scratch.overwriteFile(
+        "tools/allowlists/function_transition_allowlist/BUILD",
+        "package_group(",
+        "    name = 'function_transition_allowlist',",
+        "    packages = [",
+        "        '//a/...',",
+        "    ],",
+        ")");
+    scratch.file(
+        "a/defs.bzl",
+        "def _transition_impl(settings, attr):",
+        "    return {",
+        "        'opt': {'//command_line_option:compilation_mode': 'opt'},",
+        "        'dbg': {'//command_line_option:compilation_mode': 'dbg'},",
+        "    }",
+        "split_transition = transition(",
+        "    implementation = _transition_impl,",
+        "    inputs = [],",
+        "    outputs = ['//command_line_option:compilation_mode'])",
+        "def _split_deps_rule_impl(ctx):",
+        "    pass",
+        "split_deps_rule = rule(",
+        "    implementation = _split_deps_rule_impl,",
+        "    attrs = {",
+        "        'my_dep': attr.label(cfg = split_transition),",
+        "        '_allowlist_function_transition': attr.label(",
+        "            default = '//tools/allowlists/function_transition_allowlist')",
+        "    })",
+        "",
+        "def _print_expanded_location_impl(target, ctx):",
+        "    return struct(result=ctx.expand_location('$(location //a:lib)',"
+            + " [ctx.rule.attr.my_dep[0], ctx.rule.attr.my_dep[1]]))",
+        "",
+        "print_expanded_location = aspect(",
+        "    implementation = _print_expanded_location_impl,",
+        ")");
+    scratch.file(
+        "a/BUILD",
+        "load('//a:defs.bzl', 'split_deps_rule')",
+        "cc_library(name = 'lib', srcs = ['lib.cc'])",
+        "split_deps_rule(",
+        "    name = 'a',",
+        "    my_dep = ':lib')");
+
+    reporter.removeHandler(failFastHandler);
+
+    try {
+      AnalysisResult analysisResult =
+          update(ImmutableList.of("//a:defs.bzl%print_expanded_location"), "//a");
+      assertThat(keepGoing()).isTrue();
+      assertThat(analysisResult.hasError()).isTrue();
+    } catch (ViewCreationFailedException e) {
+      // expect to fail.
+    }
+    assertContainsEvent("Label \"//a:lib\" is found more than once in 'targets' list.");
+  }
+
   @Test
   public void topLevelAspectIsNotAnAspect() throws Exception {
     scratch.file("test/aspect.bzl", "MyAspect = 4");