Fix a ridiculous bug with Starlark-defined transitions on label-list-typed flags

Thanks to @fmeum for spotting this. When we have a Starlark-defined transition on a label-list-typed flag (for example, --platforms), we need to somehow stringify the "label list" object we get in Starlark, so that it might get parsed back into a list of labels by the flag parsing mechanism. This stringification is done using Label#getUnambiguousCanonicalForm (see https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java;l=329;drc=1d4cecf9eab592b1e949185fa1cc850bfe42058d), which does *not* respect the --enable_bzlmod flag, and hence does not use the canonical label literal syntax. This means that the label gets stringified as "@foo//bar", which in turns means that when it gets parsed back using the flag parsing mechanism, it might trigger a strict deps violation.

The fix is simply to have Label#getUnambiguousCanonicalForm always use the canonical label literal syntax. This method isn't exposed to users so it doesn't change any visible behavior, so doing this only improves correctness.

Work towards bazelbuild#15916

PiperOrigin-RevId: 470203401
Change-Id: I664e15422cf74f2ce427924d8d79e3dee41c07f6

Author: Wyverald

src/main/java/com/google/devtools/build/lib/cmdline/Label.java

@@ -390,7 +390,9 @@ public String getName() {
   /**
    * Renders this label in canonical form.
    *
-   * <p>invariant: {@code parseAbsolute(x.toString(), false).equals(x)}
+   * <p>invariant: {@code parseCanonical(x.toString()).equals(x)}. Note that using {@link
+   * #parseWithPackageContext} or {@link #parseWithRepoContext} on the returned string might not
+   * yield the same label! For that, use {@link #getUnambiguousCanonicalForm()}.
    */
   @Override
   public String toString() {
@@ -400,18 +402,23 @@ public String toString() {
   /**
    * Renders this label in canonical form.
    *
-   * <p>invariant: {@code parseAbsolute(x.getCanonicalForm(), false).equals(x)}
+   * <p>invariant: {@code parseCanonical(x.getCanonicalForm()).equals(x)}. Note that using {@link
+   * #parseWithPackageContext} or {@link #parseWithRepoContext} on the returned string might not
+   * yield the same label! For that, use {@link #getUnambiguousCanonicalForm()}.
    */
   public String getCanonicalForm() {
     return packageIdentifier.getCanonicalForm() + ":" + name;
   }
 
+  /**
+   * Returns an absolutely unambiguous canonical form for this label. Parsing this string in any
+   * environment should yield the same label (as in {@code
+   * Label.parse*(x.getUnambiguousCanonicalForm(), ...).equals(x)}).
+   */
   public String getUnambiguousCanonicalForm() {
-    return packageIdentifier.getRepository().getNameWithAt()
-        + "//"
-        + packageIdentifier.getPackageFragment()
-        + ":"
-        + name;
+    return String.format(
+        "@@%s//%s:%s",
+        packageIdentifier.getRepository().getName(), packageIdentifier.getPackageFragment(), name);
   }
 
   /** Return the name of the repository label refers to without the leading `at` symbol. */
@@ -638,11 +645,17 @@ public void str(Printer printer, StarlarkSemantics semantics) {
       // If Bzlmod is enabled, we use canonical label literal syntax here and prepend an extra '@'.
       // So the result looks like "@@//foo:bar" for the main repo and "@@foo~1.0//bar:quux" for
       // other repos.
-      printer.append('@');
+      printer.append(getUnambiguousCanonicalForm());
+      return;
     }
     // If Bzlmod is not enabled, we just use a single '@'.
     // So the result looks like "@//foo:bar" for the main repo and "@foo//bar:quux" for other repos.
-    printer.append(getUnambiguousCanonicalForm());
+    printer.append(
+        String.format(
+            "@%s//%s:%s",
+            packageIdentifier.getRepository().getName(),
+            packageIdentifier.getPackageFragment(),
+            name));
   }
 
   @Override

src/test/java/com/google/devtools/build/lib/analysis/BUILD

@@ -121,6 +121,8 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:view_creation_failed_exception",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform",
         "//src/main/java/com/google/devtools/build/lib/analysis/stringtemplate",
+        "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution_impl",
+        "//src/main/java/com/google/devtools/build/lib/bazel/repository:repository_options",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
         "//src/main/java/com/google/devtools/build/lib/causes",
@@ -143,6 +145,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:build_configuration",
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
         "//src/main/java/com/google/devtools/build/lib/skyframe:toolchain_context_key",
         "//src/main/java/com/google/devtools/build/lib/skyframe:toolchain_exception",
@@ -170,6 +173,7 @@ java_library(
         "//src/test/java/com/google/devtools/build/lib/actions/util",
         "//src/test/java/com/google/devtools/build/lib/analysis/testing",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
+        "//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util",
         "//src/test/java/com/google/devtools/build/lib/exec/util",
         "//src/test/java/com/google/devtools/build/lib/packages:testutil",
         "//src/test/java/com/google/devtools/build/lib/rules/platform:testutil",

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

@@ -14,7 +14,9 @@
 package com.google.devtools.build.lib.analysis;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.bazel.bzlmod.BzlmodTestUtil.createModuleKey;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
@@ -23,18 +25,25 @@
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.analysis.util.DummyTestFragment;
 import com.google.devtools.build.lib.analysis.util.DummyTestFragment.DummyTestOptions;
+import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleResolutionFunction;
+import com.google.devtools.build.lib.bazel.bzlmod.FakeRegistry;
+import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileFunction;
+import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.CheckDirectDepsMode;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleTransitionData;
 import com.google.devtools.build.lib.rules.cpp.CppOptions;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
+import com.google.devtools.build.lib.skyframe.PrecomputedValue;
+import com.google.devtools.build.lib.skyframe.PrecomputedValue.Injected;
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
 import com.google.devtools.build.lib.vfs.ModifiedFileSet;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import com.google.testing.junit.testparameterinjector.TestParameters;
+import java.io.IOException;
 import java.util.List;
 import java.util.Map;
 import java.util.regex.Pattern;
@@ -44,6 +53,23 @@
 /** Tests for StarlarkRuleTransitionProvider. */
 @RunWith(TestParameterInjector.class)
 public final class StarlarkRuleTransitionProviderTest extends BuildViewTestCase {
+  private FakeRegistry registry;
+
+  @Override
+  protected ImmutableList<Injected> extraPrecomputedValues() {
+    try {
+      registry =
+          FakeRegistry.DEFAULT_FACTORY.newFakeRegistry(scratch.dir("modules").getPathString());
+    } catch (IOException e) {
+      throw new IllegalStateException(e);
+    }
+    return ImmutableList.of(
+        PrecomputedValue.injected(
+            ModuleFileFunction.REGISTRIES, ImmutableList.of(registry.getUrl())),
+        PrecomputedValue.injected(ModuleFileFunction.IGNORE_DEV_DEPS, false),
+        PrecomputedValue.injected(
+            BazelModuleResolutionFunction.CHECK_DIRECT_DEPENDENCIES, CheckDirectDepsMode.WARNING));
+  }
 
   @Override
   protected ConfiguredRuleClassProvider createRuleClassProvider() {
@@ -1272,6 +1298,50 @@ public void successfulTypeConversionOfNativeListOption() throws Exception {
     assertNoEvents();
   }
 
+  @Test
+  public void successfulTypeConversionOfNativeListOption_unambiguousLabels() throws Exception {
+    setBuildLanguageOptions("--enable_bzlmod", "--incompatible_unambiguous_label_stringification");
+
+    scratch.overwriteFile("MODULE.bazel", "bazel_dep(name='rules_x',version='1.0')");
+    registry.addModule(createModuleKey("rules_x", "1.0"), "module(name='rules_x', version='1.0')");
+    scratch.file("modules/rules_x~1.0/WORKSPACE");
+    scratch.file("modules/rules_x~1.0/BUILD");
+    scratch.file(
+        "modules/rules_x~1.0/defs.bzl",
+        "def _tr_impl(settings, attr):",
+        "  return {'//command_line_option:platforms': [Label('@@//test:my_platform')]}",
+        "my_transition = transition(implementation = _tr_impl, inputs = [],",
+        "  outputs = ['//command_line_option:platforms'])",
+        "def _impl(ctx):",
+        "  pass",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  cfg = my_transition,",
+        "  attrs = {",
+        "    '_allowlist_function_transition': attr.label(",
+        "        default = '@@//tools/allowlists/function_transition_allowlist',",
+        "    ),",
+        "  })");
+
+    scratch.overwriteFile(
+        "tools/allowlists/function_transition_allowlist/BUILD",
+        "package_group(",
+        "    name = 'function_transition_allowlist',",
+        "    packages = [",
+        "        '//...',",
+        "    ],",
+        ")");
+
+    scratch.file(
+        "test/BUILD",
+        "load('@rules_x//:defs.bzl', 'my_rule')",
+        "platform(name = 'my_platform')",
+        "my_rule(name = 'test')");
+
+    getConfiguredTarget("//test");
+    assertNoEvents();
+  }
+
   // Regression test for b/170729565
   @Test
   public void testSetBooleanNativeOptionWithStarlarkBoolean() throws Exception {

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

@@ -78,7 +78,7 @@ public void testDupeSettingsInInputsThrowsError() throws Exception {
     reporter.removeHandler(failFastHandler);
     getConfiguredTarget("//test:arizona");
     assertContainsEvent(
-        "Transition declares duplicate build setting '@//test:formation' in INPUTS");
+        "Transition declares duplicate build setting '@@//test:formation' in INPUTS");
   }
 
   @Test
@@ -117,7 +117,7 @@ public void testDupeSettingsInOutputsThrowsError() throws Exception {
     reporter.removeHandler(failFastHandler);
     getConfiguredTarget("//test:arizona");
     assertContainsEvent(
-        "Transition declares duplicate build setting '@//test:formation' in OUTPUTS");
+        "Transition declares duplicate build setting '@@//test:formation' in OUTPUTS");
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/rules/LabelBuildSettingTest.java

@@ -325,7 +325,6 @@ public void transitionOutput_otherRepo() throws Exception {
         "load('//test:rules.bzl', 'rule_with_transition')",
         "label_flag(name = 'my_flag1', build_setting_default = ':first_rule')",
         "label_flag(name = 'my_flag2', build_setting_default = ':first_rule')",
-        "label_flag(name = 'my_flag3', build_setting_default = ':first_rule')",
         "filegroup(name = 'first_rule')",
         "rule_with_transition(name = 'buildme')");
     assertThat(getConfiguredTarget("//test:buildme")).isNotNull();