Enforce that command line options in transition() functions begin wit…

…h //command_line_option.

By enforcing label-like syntax on these options, this will make it easier to migrate these functions to use actual labels when this is implemented.

Progress toward #5574 and #6237

RELNOTES: None.
PiperOrigin-RevId: 218734648

src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionSplitTransitionProvider.java

@@ -51,6 +51,9 @@
  * Currently the implementation ignores the attributes provided by the containing function.
  */
 public class FunctionSplitTransitionProvider implements SplitTransitionProvider {
+
+  private static final String COMMAND_LINE_OPTION_PREFIX = "//command_line_option:";
+
   private final BaseFunction transitionFunction;
   private final SkylarkSemantics semantics;
   private final EventHandler eventHandler;
@@ -106,6 +109,26 @@ public final List<BuildOptions> split(BuildOptions buildOptions) {
       return splitBuildOptions.build();
     }
 
+    /**
+     * Given a label-like string representing a command line option, returns the command line
+     * option string that it represents. This is a temporary measure to support command line
+     * options with strings that look "label-like", so that migrating users using this
+     * experimental syntax is easier later.
+     *
+     * @throws EvalException if the given string is not a valid format to represent to
+     *     a command line option
+     */
+    private String commandLineOptionLabelToOption(String label) throws EvalException {
+      if (label.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
+        return label.substring(COMMAND_LINE_OPTION_PREFIX.length());
+      } else {
+        throw new EvalException(transitionFunction.getLocation(),
+            String.format("Option key '%s' is of invalid form. "
+                   + "Expected command line option to begin with %s",
+                label, COMMAND_LINE_OPTION_PREFIX));
+      }
+    }
+
     /**
      * For all the options in the BuildOptions, build a map from option name to its information.
      */
@@ -147,14 +170,15 @@ private SkylarkDict<String, Object> buildSettings(BuildOptions buildOptions,
 
         for (Map.Entry<String, OptionInfo> entry : optionInfoMap.entrySet()) {
           String optionName = entry.getKey();
+          String optionKey = COMMAND_LINE_OPTION_PREFIX + optionName;
           OptionInfo optionInfo = entry.getValue();
 
           try {
             Field field = optionInfo.getDefinition().getField();
             FragmentOptions options = buildOptions.get(optionInfo.getOptionClass());
             Object optionValue = field.get(options);
 
-            dict.put(optionName, optionValue, null, mutability);
+            dict.put(optionKey, optionValue, null, mutability);
           } catch (IllegalAccessException | EvalException e) {
             // These exceptions should not happen, but if they do, throw a RuntimeException.
             throw new RuntimeException(e);
@@ -243,7 +267,11 @@ private void applyTransition(BuildOptions buildOptions, Map<String, Object> tran
         Map<String, OptionInfo> optionInfoMap)
         throws EvalException {
       for (Map.Entry<String, Object> entry : transition.entrySet()) {
-        String optionName = entry.getKey();
+        String optionKey = entry.getKey();
+
+        // TODO(juliexxia): Handle keys which correspond to build_setting target labels instead
+        // of assuming every key is for a command line option.
+        String optionName = commandLineOptionLabelToOption(optionKey);
         Object optionValue = entry.getValue();
 
         try {

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

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.analysis;
 
 import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
 
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ListMultimap;
@@ -50,8 +51,8 @@ private void writeBasicTestFiles() throws Exception {
         "test/skylark/my_rule.bzl",
         "def transition_func(settings):",
         "  return {",
-        "      't0': {'cpu': 'k8'},",
-        "      't1': {'cpu': 'armeabi-v7a'},",
+        "      't0': {'//command_line_option:cpu': 'k8'},",
+        "      't1': {'//command_line_option:cpu': 'armeabi-v7a'},",
         "  }",
         "my_transition = transition(implementation = transition_func, inputs = [], outputs = [])",
         "def impl(ctx): ",
@@ -188,9 +189,9 @@ private void writeReadSettingsTestFiles() throws Exception {
         "test/skylark/my_rule.bzl",
         "def transition_func(settings):",
         "  transitions = {}",
-        "  for cpu in settings['fat_apk_cpu']:",
+        "  for cpu in settings['//command_line_option:fat_apk_cpu']:",
         "    transitions[cpu] = {",
-        "      'cpu': cpu,",
+        "      '//command_line_option:cpu': cpu,",
         "    }",
         "  return transitions",
         "my_transition = transition(implementation = transition_func, inputs = [], outputs = [])",
@@ -241,9 +242,9 @@ private void writeOptionConversionTestFiles() throws Exception {
         "test/skylark/my_rule.bzl",
         "def transition_func(settings):",
         "  return {",
-        "    'cpu': 'armeabi-v7a',",
-        "    'dynamic_mode': 'off',",
-        "    'crosstool_top': '//android/crosstool:everything',",
+        "    '//command_line_option:cpu': 'armeabi-v7a',",
+        "    '//command_line_option:dynamic_mode': 'off',",
+        "    '//command_line_option:crosstool_top': '//android/crosstool:everything',",
         "  }",
         "my_transition = transition(implementation = transition_func, inputs = [], outputs = [])",
         "def impl(ctx): ",
@@ -278,6 +279,81 @@ public void testOptionConversionCpu() throws Exception {
     assertThat(getConfiguration(splitDep.get("armeabi-v7a")).getCpu()).isEqualTo("armeabi-v7a");
   }
 
+  @Test
+  public void testInvalidOptionKey() throws Exception {
+    setSkylarkSemanticsOptions("--experimental_starlark_config_transitions=true");
+    writeWhitelistFile();
+
+    scratch.file(
+        "test/skylark/my_rule.bzl",
+        "def transition_func(settings):",
+        "  return {'cpu': 'k8'}",
+        "my_transition = transition(implementation = transition_func, inputs = [], outputs = [])",
+        "def impl(ctx): ",
+        "  return []",
+        "my_rule = rule(",
+        "  implementation = impl,",
+        "  attrs = {",
+        "    'dep':  attr.label(cfg = my_transition),",
+        "    '_whitelist_function_transition': attr.label(",
+        "        default = '//tools/whitelists/function_transition_whitelist',",
+        "    ),",
+        "  })");
+
+    scratch.file(
+        "test/skylark/BUILD",
+        "load('//test/skylark:my_rule.bzl', 'my_rule')",
+        "my_rule(name = 'test', dep = ':main1')",
+        "cc_binary(name = 'main1', srcs = ['main1.c'])");
+
+    try {
+      getConfiguredTarget("//test/skylark:test");
+      fail("Expected failure");
+    } catch (IllegalStateException expected) {
+      // TODO(bazel-team): Register a failure event instead of throwing a RuntimeException.
+      assertThat(expected).hasCauseThat().hasCauseThat().hasMessageThat()
+          .contains("Option key 'cpu' is of invalid form. Expected command line option to "
+              + "begin with //command_line_option:");
+    }
+  }
+
+  @Test
+  public void testInvalidOptionValue() throws Exception {
+    setSkylarkSemanticsOptions("--experimental_starlark_config_transitions=true");
+    writeWhitelistFile();
+
+    scratch.file(
+        "test/skylark/my_rule.bzl",
+        "def transition_func(settings):",
+        "  return {'//command_line_option:cpu': 1}",
+        "my_transition = transition(implementation = transition_func, inputs = [], outputs = [])",
+        "def impl(ctx): ",
+        "  return []",
+        "my_rule = rule(",
+        "  implementation = impl,",
+        "  attrs = {",
+        "    'dep':  attr.label(cfg = my_transition),",
+        "    '_whitelist_function_transition': attr.label(",
+        "        default = '//tools/whitelists/function_transition_whitelist',",
+        "    ),",
+        "  })");
+
+    scratch.file(
+        "test/skylark/BUILD",
+        "load('//test/skylark:my_rule.bzl', 'my_rule')",
+        "my_rule(name = 'test', dep = ':main1')",
+        "cc_binary(name = 'main1', srcs = ['main1.c'])");
+
+    try {
+      getConfiguredTarget("//test/skylark:test");
+      fail("Expected failure");
+    } catch (IllegalStateException expected) {
+      // TODO(bazel-team): Register a failure event instead of throwing a RuntimeException.
+      assertThat(expected).hasCauseThat().hasCauseThat().hasMessageThat()
+          .contains("Invalid value type for option 'cpu'");
+    }
+  }
+
   @Test
   public void testOptionConversionDynamicMode() throws Exception {
     // TODO(waltl): check that dynamic_mode is parsed properly.

src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java

@@ -2209,7 +2209,7 @@ public void testAnalysisTestTransitionOnAnalysisTest() throws Exception {
         "  return [MyInfo(strict_java_deps = ctx.fragments.java.strict_java_deps)]",
         "",
         "def transition_func(settings):",
-        "  return { 'strict_java_deps' : 'WARN' }",
+        "  return { '//command_line_option:strict_java_deps' : 'WARN' }",
         "my_transition = transition(",
         "    implementation = transition_func,",
         "    for_analysis_testing=True,",