bazel starlark transitions: grab build setting label from Starlark call stack in order to canonicalize.

There are 5 locations a user might write the label string of a build setting:
(1) on the command line
(2) in the transition definition inputs
(3) in the transition definition output
(4) in the transition impl function while reading the settings dict
(5) in the transition impl function return dict

We know that, depending on the situation, there are multiple ways to write "a build setting in the main repo of this project" - //myflag, @//myflag, @main_repo//my:flag.

In all 5 places listed above, bazel needs to recognize that the three different versions of //myflag above all map back to a single target. We need this for deduping purposes and consistency. 1 is taken care of in a grandparent CL to this CL. This Cl addresses locations 2-5.

To do this, whenever we are interpreting a user's input at one of the locations above, we covert that input string to the canonical string form of that build setting for our back end book keeping purposes. The logic to do this was modeled off of the `Label()` function logic. We also keep a map of canonicalized form to the user-given form in order to display messages (or key the dict in 4) to the user using the forms of the flags they used themselves.

work towards #11128

PiperOrigin-RevId: 351426279

Author: juliexxia

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

@@ -1762,6 +1762,7 @@ java_library(
     srcs = ["config/StarlarkDefinedConfigTransition.java"],
     deps = [
         ":config/transitions/configuration_transition",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/config",
@@ -2096,8 +2097,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/common/options",
         "//src/main/java/net/starlark/java/eval",
-        "//src/main/java/net/starlark/java/syntax",
-        "//third_party:error_prone_annotations",
         "//third_party:guava",
         "//third_party:jsr305",
     ],

src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java

@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
+import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.ValidationException;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
@@ -34,9 +35,7 @@
 import com.google.devtools.common.options.OptionDefinition;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
-import com.google.errorprone.annotations.FormatMethod;
 import java.lang.reflect.Field;
-import java.util.Collection;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -96,7 +95,6 @@ static Map<String, BuildOptions> applyAndValidate(
       if (transitions == null) {
         return null; // errors reported to handler
       }
-      validateFunctionOutputsMatchesDeclaredOutputs(transitions.values(), starlarkTransition);
 
       for (Map.Entry<String, Map<String, Object>> entry : transitions.entrySet()) {
         Map<String, Object> newValues = handleImplicitPlatformChange(entry.getValue());
@@ -113,17 +111,6 @@ static Map<String, BuildOptions> applyAndValidate(
     }
   }
 
-  private static final class ValidationException extends Exception {
-    ValidationException(String message) {
-      super(message);
-    }
-
-    @FormatMethod
-    static ValidationException format(String format, Object... args) {
-      return new ValidationException(String.format(format, args));
-    }
-  }
-
   /**
    * If the transition changes --cpu but not --platforms, clear out --platforms.
    *
@@ -166,33 +153,6 @@ private static void checkForDenylistedOptions(StarlarkDefinedConfigTransition tr
     }
   }
 
-  /**
-   * Validates that function outputs exactly the set of outputs it declares. More thorough checking
-   * (like type checking of output values) is done elsewhere because it requires loading. see {@link
-   * StarlarkTransition#validate}
-   */
-  private static void validateFunctionOutputsMatchesDeclaredOutputs(
-      Collection<Map<String, Object>> transitions,
-      StarlarkDefinedConfigTransition starlarkTransition)
-      throws ValidationException {
-    for (Map<String, Object> transition : transitions) {
-      LinkedHashSet<String> remainingOutputs =
-          Sets.newLinkedHashSet(starlarkTransition.getOutputs());
-      for (String outputKey : transition.keySet()) {
-        if (!remainingOutputs.remove(outputKey)) {
-          throw ValidationException.format(
-              "transition function returned undeclared output '%s'", outputKey);
-        }
-      }
-
-      if (!remainingOutputs.isEmpty()) {
-        throw ValidationException.format(
-            "transition outputs [%s] were not defined by transition function",
-            Joiner.on(", ").join(remainingOutputs));
-      }
-    }
-  }
-
   /** For all the options in the BuildOptions, build a map from option name to its information. */
   static ImmutableMap<String, OptionInfo> buildOptionInfo(BuildOptions buildOptions) {
     ImmutableMap.Builder<String, OptionInfo> builder = new ImmutableMap.Builder<>();
@@ -230,7 +190,10 @@ static Dict<String, Object> buildSettings(
       Map<String, OptionInfo> optionInfoMap,
       StarlarkDefinedConfigTransition starlarkTransition)
       throws ValidationException {
-    LinkedHashSet<String> remainingInputs = Sets.newLinkedHashSet(starlarkTransition.getInputs());
+    Map<String, String> inputsCanonicalizedToGiven =
+        starlarkTransition.getInputsCanonicalizedToGiven();
+    LinkedHashSet<String> remainingInputs =
+        Sets.newLinkedHashSet(inputsCanonicalizedToGiven.keySet());
 
     try (Mutability mutability = Mutability.create("build_settings")) {
       Dict<String, Object> dict = Dict.of(mutability);
@@ -261,11 +224,15 @@ static Dict<String, Object> buildSettings(
 
       // Add Starlark options
       for (Map.Entry<Label, Object> starlarkOption : buildOptions.getStarlarkOptions().entrySet()) {
-        if (!remainingInputs.remove(starlarkOption.getKey().toString())) {
+        String canonicalLabelForm = starlarkOption.getKey().getUnambiguousCanonicalForm();
+        if (!remainingInputs.remove(canonicalLabelForm)) {
           continue;
         }
+        // Convert the canonical form to the user requested form that they expect to see in this
+        // dict.
+        String userRequestedLabelForm = inputsCanonicalizedToGiven.get(canonicalLabelForm);
         try {
-          dict.putEntry(starlarkOption.getKey().toString(), starlarkOption.getValue());
+          dict.putEntry(userRequestedLabelForm, starlarkOption.getValue());
         } catch (EvalException ex) {
           throw new IllegalStateException(ex); // can't happen
         }

src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java

@@ -48,7 +48,9 @@ public void storeInThread(StarlarkThread thread) {
   }
 
   private final Phase phase;
-  private final String toolsRepository;
+  // Only necessary for loading phase threads.
+  @Nullable private final String toolsRepository;
+  // Only necessary for loading phase threads to construct configuration_field.
   @Nullable private final ImmutableMap<String, Class<?>> fragmentNameToClass;
   private final ImmutableMap<RepositoryName, RepositoryName> repoMapping;
   private final HashMap<String, Label> convertedLabelsInPackage;
@@ -57,9 +59,11 @@ public void storeInThread(StarlarkThread thread) {
 
   /**
    * @param phase the phase to which this Starlark thread belongs
-   * @param toolsRepository the name of the tools repository, such as "@bazel_tools"
+   * @param toolsRepository the name of the tools repository, such as "@bazel_tools" for loading
+   *     phase threads, null for other threads.
    * @param fragmentNameToClass a map from configuration fragment name to configuration fragment
-   *     class, such as "apple" to AppleConfiguration.class
+   *     class, such as "apple" to AppleConfiguration.class for loading phase threads, null for
+   *     other threads.
    * @param repoMapping a map from RepositoryName to RepositoryName to be used for external
    * @param convertedLabelsInPackage a mutable map from String to Label, used during package loading
    *     of a single package.
@@ -78,7 +82,7 @@ public void storeInThread(StarlarkThread thread) {
   // analysis threads?
   public BazelStarlarkContext(
       Phase phase,
-      String toolsRepository,
+      @Nullable String toolsRepository,
       @Nullable ImmutableMap<String, Class<?>> fragmentNameToClass,
       ImmutableMap<RepositoryName, RepositoryName> repoMapping,
       HashMap<String, Label> convertedLabelsInPackage,
@@ -99,6 +103,7 @@ public Phase getPhase() {
   }
 
   /** Returns the name of the tools repository, such as "@bazel_tools". */
+  @Nullable
   @Override
   public String getToolsRepository() {
     return toolsRepository;

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

@@ -16,10 +16,14 @@
 
 import static com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.COMMAND_LINE_OPTION_PREFIX;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.Settings;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.packages.BazelModuleContext;
+import com.google.devtools.build.lib.packages.BazelStarlarkContext;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.starlarkbuildapi.config.ConfigGlobalLibraryApi;
 import com.google.devtools.build.lib.starlarkbuildapi.config.ConfigurationTransitionApi;
@@ -28,11 +32,13 @@
 import java.util.Map;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
+import net.starlark.java.eval.Module;
 import net.starlark.java.eval.Sequence;
 import net.starlark.java.eval.Starlark;
 import net.starlark.java.eval.StarlarkCallable;
 import net.starlark.java.eval.StarlarkSemantics;
 import net.starlark.java.eval.StarlarkThread;
+import net.starlark.java.syntax.Location;
 
 /**
  * Implementation of {@link ConfigGlobalLibraryApi}.
@@ -59,8 +65,12 @@ public ConfigurationTransitionApi transition(
         outputsList,
         Settings.OUTPUTS,
         semantics.getBool(BuildLanguageOptions.EXPERIMENTAL_STARLARK_CONFIG_TRANSITIONS));
+    Label parentLabel =
+        BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).label();
+    Location location = thread.getCallerLocation();
+    BazelStarlarkContext context = BazelStarlarkContext.from(thread);
     return StarlarkDefinedConfigTransition.newRegularTransition(
-        implementation, inputsList, outputsList, semantics, thread);
+        implementation, inputsList, outputsList, semantics, parentLabel, location, context);
   }
 
   @Override
@@ -71,8 +81,13 @@ public ConfigurationTransitionApi analysisTestTransition(
     Map<String, Object> changedSettingsMap =
         Dict.cast(changedSettings, String.class, Object.class, "changed_settings dict");
     validateBuildSettingKeys(changedSettingsMap.keySet(), Settings.OUTPUTS, true);
+    ImmutableMap<RepositoryName, RepositoryName> repoMapping =
+        BazelStarlarkContext.from(thread).getRepoMapping();
+    Label parentLabel =
+        BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).label();
+    Location location = thread.getCallerLocation();
     return StarlarkDefinedConfigTransition.newAnalysisTestTransition(
-        changedSettingsMap, thread.getCallerLocation());
+        changedSettingsMap, repoMapping, parentLabel, location);
   }
 
   private void validateBuildSettingKeys(

src/main/java/com/google/devtools/build/lib/rules/config/ConfigGlobalLibrary.java

@@ -765,7 +765,7 @@ public void testInvalidNativeOptionInput() throws Exception {
     reporter.removeHandler(failFastHandler);
     getConfiguredTarget("//test/starlark:test");
     assertContainsEvent(
-        "transition inputs [//command_line_option:foop, //command_line_option:barp] "
+        "transition inputs [//command_line_option:barp, //command_line_option:foop] "
             + "do not correspond to valid settings");
   }
 

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

@@ -0,0 +1,165 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.analysis;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.testutil.Scratch;
+import java.util.Map;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ * Test of common logic between Starlark-defined transitions. Rule-transition- or
+ * attr-transition-specific logic should be tested in {@link StarlarkRuleTransitionProviderTest} and
+ * {@link StarlarkAttrTransitionProviderTest}.
+ */
+@RunWith(JUnit4.class)
+public class StarlarkTransitionTest extends BuildViewTestCase {
+  static void writeAllowlistFile(Scratch scratch) throws Exception {
+    scratch.file(
+        "tools/allowlists/function_transition_allowlist/BUILD",
+        "package_group(",
+        "    name = 'function_transition_allowlist',",
+        "    packages = [",
+        "        '//test/...',",
+        "    ],",
+        ")");
+  }
+
+  @Test
+  public void testDupeSettingsInInputsThrowsError() throws Exception {
+    scratch.file(
+        "test/defs.bzl",
+        "def _setting_impl(ctx):",
+        "  return []",
+        "string_flag = rule(",
+        "  implementation = _setting_impl,",
+        "  build_setting = config.string(flag=True),",
+        ")",
+        "def _transition_impl(settings, attr):",
+        "  return {'//test:formation': 'mesa'}",
+        "formation_transition = transition(",
+        "  implementation = _transition_impl,",
+        "  inputs = ['@//test:formation', '//test:formation'],", // duplicates here
+        "  outputs = ['//test:formation'],",
+        ")",
+        "def _impl(ctx):",
+        "  return []",
+        "state = rule(",
+        "  implementation = _impl,",
+        "  cfg = formation_transition,",
+        "  attrs = {",
+        "    '_allowlist_function_transition': attr.label(",
+        "        default = '//tools/allowlists/function_transition_allowlist',",
+        "    ),",
+        "  })");
+    scratch.file(
+        "test/BUILD",
+        "load('//test:defs.bzl', 'state', 'string_flag')",
+        "state(name = 'arizona')",
+        "string_flag(name = 'formation', build_setting_default = 'canyon')");
+
+    reporter.removeHandler(failFastHandler);
+    getConfiguredTarget("//test:arizona");
+    assertContainsEvent(
+        "Transition declares duplicate build setting '@//test:formation' in INPUTS");
+  }
+
+  @Test
+  public void testDupeSettingsInOutputsThrowsError() throws Exception {
+    scratch.file(
+        "test/defs.bzl",
+        "def _setting_impl(ctx):",
+        "  return []",
+        "string_flag = rule(",
+        "  implementation = _setting_impl,",
+        "  build_setting = config.string(flag=True),",
+        ")",
+        "def _transition_impl(settings, attr):",
+        "  return {'//test:formation': 'mesa'}",
+        "formation_transition = transition(",
+        "  implementation = _transition_impl,",
+        "  inputs = ['//test:formation'],",
+        "  outputs = ['@//test:formation', '//test:formation'],", // duplicates here
+        ")",
+        "def _impl(ctx):",
+        "  return []",
+        "state = rule(",
+        "  implementation = _impl,",
+        "  cfg = formation_transition,",
+        "  attrs = {",
+        "    '_allowlist_function_transition': attr.label(",
+        "        default = '//tools/allowlists/function_transition_allowlist',",
+        "    ),",
+        "  })");
+    scratch.file(
+        "test/BUILD",
+        "load('//test:defs.bzl', 'state', 'string_flag')",
+        "state(name = 'arizona')",
+        "string_flag(name = 'formation', build_setting_default = 'canyon')");
+
+    reporter.removeHandler(failFastHandler);
+    getConfiguredTarget("//test:arizona");
+    assertContainsEvent(
+        "Transition declares duplicate build setting '@//test:formation' in OUTPUTS");
+  }
+
+  @Test
+  public void testDifferentFormsOfFlagInInputsAndOutputs() throws Exception {
+    writeAllowlistFile(scratch);
+    scratch.file(
+        "test/defs.bzl",
+        "def _setting_impl(ctx):",
+        "  return []",
+        "string_flag = rule(",
+        "  implementation = _setting_impl,",
+        "  build_setting = config.string(flag=True),",
+        ")",
+        "def _transition_impl(settings, attr):",
+        "  return {",
+        "    '//test:formation': settings['@//test:formation']+'-transitioned',",
+        "  }",
+        "formation_transition = transition(",
+        "  implementation = _transition_impl,",
+        "  inputs = ['@//test:formation'],",
+        "  outputs = ['//test:formation'],",
+        ")",
+        "def _impl(ctx):",
+        "  return []",
+        "state = rule(",
+        "  implementation = _impl,",
+        "  cfg = formation_transition,",
+        "  attrs = {",
+        "    '_allowlist_function_transition': attr.label(",
+        "        default = '//tools/allowlists/function_transition_allowlist',",
+        "    ),",
+        "  })");
+    scratch.file(
+        "test/BUILD",
+        "load('//test:defs.bzl', 'state', 'string_flag')",
+        "state(name = 'arizona')",
+        "string_flag(name = 'formation', build_setting_default = 'canyon')");
+
+    Map<Label, Object> starlarkOptions =
+        getConfiguration(getConfiguredTarget("//test:arizona")).getOptions().getStarlarkOptions();
+    assertThat(starlarkOptions).hasSize(1);
+    assertThat(starlarkOptions.get(Label.parseAbsoluteUnchecked("//test:formation")))
+        .isEqualTo("canyon-transitioned");
+  }
+}