Allow Label instances as keys in select (bazelbuild#14755)

When a macro specifies a label string as a key in a select, this label
is resolved relative to the site of use rather than the .bzl file the
macro is defined in. The resolution will lead to incorrect results if
the repository that uses the macro has a different repo mapping, e.g.
because it is created by another Bazel module.

This can be solved by allowing macros to specify label instances created
with the `Label` constructor instead of label strings everywhere, which
previously was not possible in select.

This commit also updates the docs for Label, select and macros.

Fixes bazelbuild#14259.

Closes bazelbuild#14447.

PiperOrigin-RevId: 417386977
(cherry picked from commit 69f8b17)

Co-authored-by: Fabian Meumertzheim <[email protected]>

Author: brentleyjones

site/docs/skylark/macros.md

@@ -143,6 +143,38 @@ macro), use the function [native.package_name()](lib/native.html#package_name).
 Note that `native` can only be used in `.bzl` files, and not in `WORKSPACE` or
 `BUILD` files.
 
+## Label resolution in macros
+
+Since macros are evaluated in the [loading phase](concepts.md#evaluation-model),
+label strings such as `"//foo:bar"` that occur in a macro are interpreted
+relative to the `BUILD` file in which the macro is used rather than relative to
+the `.bzl` file in which it is defined. This behavior is generally undesirable
+for macros that are meant to be used in other repositories, e.g. because they
+are part of a published Starlark ruleset.
+
+To get the same behavior as for Starlark rules, wrap the label strings with the
+[`Label`](lib/Label.html#Label) constructor:
+
+```python
+# @my_ruleset//rules:defs.bzl
+def my_cc_wrapper(name, deps = [], **kwargs):
+  native.cc_library(
+    name = name,
+    deps = deps + select({
+      # Due to the use of Label, this label is resolved within @my_ruleset,
+      # regardless of its site of use.
+      Label("//config:needs_foo"): [
+        # Due to the use of Label, this label will resolve to the correct target
+        # even if the canonical name of @dep_of_my_ruleset should be different
+        # in the main workspace, e.g. due to repo mappings.
+        Label("@dep_of_my_ruleset//tools:foo"),
+      ],
+      "//conditions:default": [],
+    }),
+    **kwargs,
+  )
+```
+
 ## Debugging
 
 *   `bazel query --output=build //my/path:all` will show you how the `BUILD` file

src/main/java/com/google/devtools/build/lib/packages/SelectorList.java

@@ -16,6 +16,7 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.Depset;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import java.util.Arrays;
@@ -87,9 +88,9 @@ public static Object select(Dict<?, ?> dict, String noMatchError) throws EvalExc
               + " to match");
     }
     for (Object key : dict.keySet()) {
-      if (!(key instanceof String)) {
+      if (!(key instanceof String || key instanceof Label)) {
         throw Starlark.errorf(
-            "select: got %s for dict key, want a label string", Starlark.type(key));
+            "select: got %s for dict key, want a Label or label string", Starlark.type(key));
       }
     }
     return SelectorList.of(new SelectorValue(dict, noMatchError));

src/main/java/com/google/devtools/build/lib/packages/StarlarkLibrary.java

@@ -330,8 +330,10 @@ public Depset depset(
               name = "x",
               positional = true,
               doc =
-                  "A dict that maps configuration conditions to values. Each key is a label string"
-                      + " that identifies a config_setting instance."),
+                  "A dict that maps configuration conditions to values. Each key is a "
+                      + "<a href=\"$BE_ROOT/../skylark/lib/Label.html\">Label</a> or a label string"
+                      + " that identifies a config_setting, constraint_setting, or constraint_value"
+                      + " instance."),
           @Param(
               name = "no_match_error",
               defaultValue = "''",

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java

@@ -579,10 +579,11 @@ StarlarkAspectApi aspect(
   @StarlarkMethod(
       name = "Label",
       doc =
-          "Creates a Label referring to a BUILD target. Use this function only when you want to"
-              + " give a default value for the label attributes. The argument must refer to an"
-              + " absolute label. The repo part of the label (or its absence) is interpreted in the"
-              + " context of the repo where this Label() call appears. Example: <br><pre"
+          "Creates a Label referring to a BUILD target. Use this function when you want to give a"
+              + " default value for the label attributes of a rule or when referring to a target"
+              + " via an absolute label from a macro. The argument must refer to an absolute label."
+              + " The repo part of the label (or its absence) is interpreted in the context of the"
+              + " repo where this Label() call appears. Example: <br><pre"
               + " class=language-python>Label(\"//tools:default\")</pre>",
       parameters = {
         @Param(name = "label_string", doc = "the label string."),

src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java

@@ -427,7 +427,8 @@ public void configKeyTypeChecking_Int() throws Exception {
         "    name = 'int_key',",
         "    srcs = select({123: ['a.java']})",
         ")");
-    assertTargetError("//java/foo:int_key", "select: got int for dict key, want a label string");
+    assertTargetError(
+        "//java/foo:int_key", "select: got int for dict key, want a Label or label string");
   }
 
   @Test
@@ -439,7 +440,8 @@ public void configKeyTypeChecking_Bool() throws Exception {
         "    name = 'bool_key',",
         "    srcs = select({True: ['a.java']})",
         ")");
-    assertTargetError("//java/foo:bool_key", "select: got bool for dict key, want a label string");
+    assertTargetError(
+        "//java/foo:bool_key", "select: got bool for dict key, want a Label or label string");
   }
 
   @Test
@@ -452,7 +454,7 @@ public void configKeyTypeChecking_None() throws Exception {
         "    srcs = select({None: ['a.java']})",
         ")");
     assertTargetError(
-        "//java/foo:none_key", "select: got NoneType for dict key, want a label string");
+        "//java/foo:none_key", "select: got NoneType for dict key, want a Label or label string");
   }
 
   @Test
@@ -1526,4 +1528,49 @@ public void publicVisibilityConfigSetting_defaultIsPrivate() throws Exception {
     assertThat(getConfiguredTarget("//a:binary")).isNotNull();
     assertNoEvents();
   }
+
+  @Test
+  public void selectWithLabelKeysInMacro() throws Exception {
+    writeConfigRules();
+    scratch.file("java/BUILD");
+    scratch.file(
+        "java/macros.bzl",
+        "def my_java_binary(name, deps = [], **kwargs):",
+        "    native.java_binary(",
+        "        name = name,",
+        "        deps = select({",
+        "            Label('//conditions:a'): [Label('//java/foo:a')],",
+        "            '//conditions:b': [Label('//java/foo:b')],",
+        "        }) + select({",
+        "            '//conditions:a': [Label('//java/foo:a2')],",
+        "            Label('//conditions:b'): [Label('//java/foo:b2')],",
+        "        }),",
+        "        **kwargs,",
+        "    )");
+    scratch.file(
+        "java/foo/BUILD",
+        "load('//java:macros.bzl', 'my_java_binary')",
+        "my_java_binary(",
+        "    name = 'binary',",
+        "    srcs = ['binary.java'],",
+        ")",
+        "java_library(",
+        "    name = 'a',",
+        "    srcs = ['a.java'])",
+        "java_library(",
+        "    name = 'b',",
+        "    srcs = ['b.java'])",
+        "java_library(",
+        "    name = 'a2',",
+        "    srcs = ['a2.java'])",
+        "java_library(",
+        "    name = 'b2',",
+        "    srcs = ['b2.java'])");
+
+    checkRule(
+        "//java/foo:binary",
+        "--foo=b",
+        /*expected:*/ ImmutableList.of("bin java/foo/libb.jar", "bin java/foo/libb2.jar"),
+        /*not expected:*/ ImmutableList.of("bin java/foo/liba.jar", "bin java/foo/liba2.jar"));
+  }
 }