Throw a checked exception if there would be an illegal character in t…

…he output directory.

We do somewhat more granular checking so that we can error out with the most accurate possible message for the illegal character. This shouldn't have a big impact given the small number of BuildConfiguration/OutputDirectory objects created.

In some cases, this turns a crash into a silent modification of the top-level target, which seems equally bad: will not submit until this is resolved.

PiperOrigin-RevId: 342379102

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

@@ -1518,6 +1518,7 @@ java_library(
         ":config/transitive_option_details",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception",
         "//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/cmdline",
@@ -1531,6 +1532,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/java/net/starlark/java/annot",
+        "//src/main/protobuf:failure_details_java_proto",
         "//third_party:guava",
         "//third_party:jsr305",
     ],

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

@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.actions.BuildConfigurationEvent;
 import com.google.devtools.build.lib.actions.CommandLines.CommandLineLimits;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
+import com.google.devtools.build.lib.analysis.config.OutputDirectories.InvalidMnemonicException;
 import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId;
@@ -230,7 +231,8 @@ public BuildConfiguration(
       ImmutableSet<String> reservedActionMnemonics,
       ActionEnvironment actionEnvironment,
       String repositoryName,
-      boolean siblingRepositoryLayout) {
+      boolean siblingRepositoryLayout)
+      throws InvalidMnemonicException {
     this(
         directories,
         fragmentsMap,
@@ -252,7 +254,8 @@ public BuildConfiguration(
       ImmutableSet<String> reservedActionMnemonics,
       ActionEnvironment actionEnvironment,
       RepositoryName mainRepositoryName,
-      boolean siblingRepositoryLayout) {
+      boolean siblingRepositoryLayout)
+      throws InvalidMnemonicException {
     // this.directories = directories;
     this.fragments = makeFragmentsMap(fragmentsMap);
     this.fragmentClassSet = FragmentClassSet.of(this.fragments.keySet());
@@ -324,17 +327,20 @@ public BuildConfiguration clone(
     }
     BuildOptions options =
         buildOptions.trim(getOptionsClasses(fragmentsMap.keySet(), ruleClassProvider));
-    BuildConfiguration newConfig =
-        new BuildConfiguration(
-            getDirectories(),
-            fragmentsMap,
-            options,
-            BuildOptions.diffForReconstruction(defaultBuildOptions, options),
-            reservedActionMnemonics,
-            actionEnv,
-            mainRepositoryName.strippedName(),
-            siblingRepositoryLayout);
-    return newConfig;
+    try {
+      return new BuildConfiguration(
+          getDirectories(),
+          fragmentsMap,
+          options,
+          BuildOptions.diffForReconstruction(defaultBuildOptions, options),
+          reservedActionMnemonics,
+          actionEnv,
+          mainRepositoryName.strippedName(),
+          siblingRepositoryLayout);
+    } catch (InvalidMnemonicException e) {
+      throw new IllegalStateException(
+          "Invalid mnemonic unexpected when cloning: " + this + ", " + fragmentClasses, e);
+    }
   }
 
   /** Returns the config fragment options classes used by the given fragment types. */

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

@@ -15,16 +15,20 @@
 package com.google.devtools.build.lib.analysis.config;
 
 import com.google.common.base.Joiner;
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.server.FailureDetails.BuildConfiguration.Code;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.PathFragment.InvalidBaseNameException;
 import java.util.ArrayList;
+import java.util.Map;
 
 /**
  * Logic for figuring out what base directories to place outputs generated from a given
@@ -141,7 +145,8 @@ public ArtifactRoot getRoot(
       CoreOptions options,
       ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments,
       RepositoryName mainRepositoryName,
-      boolean siblingRepositoryLayout) {
+      boolean siblingRepositoryLayout)
+      throws InvalidMnemonicException {
     this.directories = directories;
     this.mnemonic = buildMnemonic(options, fragments);
     this.outputDirName =
@@ -167,16 +172,44 @@ public ArtifactRoot getRoot(
     this.mainRepository = mainRepositoryName;
   }
 
-  private String buildMnemonic(
-      CoreOptions options, ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments) {
+  private static String buildMnemonic(
+      CoreOptions options, ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments)
+      throws InvalidMnemonicException {
     // See explanation at declaration for outputRoots.
-    String platformSuffix = (options.platformSuffix != null) ? options.platformSuffix : "";
     ArrayList<String> nameParts = new ArrayList<>();
-    for (Fragment fragment : fragments.values()) {
-      nameParts.add(fragment.getOutputDirectoryName());
+    for (Map.Entry<Class<? extends Fragment>, Fragment> entry : fragments.entrySet()) {
+      String outputDirectoryName = entry.getValue().getOutputDirectoryName();
+      if (Strings.isNullOrEmpty(outputDirectoryName)) {
+        continue;
+      }
+      try {
+        PathFragment.checkSeparators(outputDirectoryName);
+      } catch (InvalidBaseNameException e) {
+        throw new InvalidMnemonicException(
+            "Output directory name '"
+                + outputDirectoryName
+                + "' specified by "
+                + entry.getKey().getSimpleName(),
+            e);
+      }
+      nameParts.add(outputDirectoryName);
+    }
+    String platformSuffix = (options.platformSuffix != null) ? options.platformSuffix : "";
+    try {
+      PathFragment.checkSeparators(platformSuffix);
+    } catch (InvalidBaseNameException e) {
+      throw new InvalidMnemonicException("Platform suffix '" + platformSuffix + "'", e);
     }
-    nameParts.add(options.compilationMode + platformSuffix);
+    String shortString = options.compilationMode + platformSuffix;
+    nameParts.add(shortString);
     if (options.transitionDirectoryNameFragment != null) {
+      try {
+        PathFragment.checkSeparators(options.transitionDirectoryNameFragment);
+      } catch (InvalidBaseNameException e) {
+        throw new InvalidMnemonicException(
+            "Transition directory name fragment '" + options.transitionDirectoryNameFragment + "'",
+            e);
+      }
       nameParts.add(options.transitionDirectoryNameFragment);
     }
     return Joiner.on('-').skipNulls().join(nameParts);
@@ -282,4 +315,14 @@ boolean mergeGenfilesDirectory() {
   BlazeDirectories getDirectories() {
     return directories;
   }
+
+  /** Indicates a failure to construct the mnemonic for an output directory. */
+  public static class InvalidMnemonicException extends InvalidConfigurationException {
+    InvalidMnemonicException(String message, InvalidBaseNameException e) {
+      super(
+          message + " is invalid as part of a path: " + e.getMessage(),
+          Code.INVALID_OUTPUT_DIRECTORY_MNEMONIC);
+    }
+  }
 }
+

src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java

@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
 import com.google.devtools.build.lib.analysis.config.Fragment;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
+import com.google.devtools.build.lib.analysis.config.OutputDirectories.InvalidMnemonicException;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.skyframe.SkyFunction;
@@ -105,17 +106,21 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     ActionEnvironment actionEnvironment =
       ruleClassProvider.getActionEnvironmentProvider().getActionEnvironment(options);
 
-    BuildConfiguration config =
-        new BuildConfiguration(
-            directories,
-            fragmentsMap,
-            options,
-            key.getOptionsDiff(),
-            ruleClassProvider.getReservedActionMnemonics(),
-            actionEnvironment,
-            workspaceNameValue.getName(),
-            starlarkSemantics.getBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT));
-    return new BuildConfigurationValue(config);
+    try {
+      return new BuildConfigurationValue(
+          new BuildConfiguration(
+              directories,
+              fragmentsMap,
+              options,
+              key.getOptionsDiff(),
+              ruleClassProvider.getReservedActionMnemonics(),
+              actionEnvironment,
+              workspaceNameValue.getName(),
+              starlarkSemantics.getBool(
+                  BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT)));
+    } catch (InvalidMnemonicException e) {
+      throw new BuildConfigurationFunctionException(e);
+    }
   }
 
   private Set<Fragment> getConfigurationFragments(BuildConfigurationValue.Key key)

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -2090,10 +2090,17 @@ public ConfigurationsResult getConfigurations(
               toConfigurationKey(platformMappingValue, depFragments, toOption);
           BuildConfigurationValue configValue =
               ((BuildConfigurationValue) configsResult.get(configKey));
-          // configValue will be null here if there was an exception thrown during configuration
-          // creation. This will be reported elsewhere.
           if (configValue != null) {
             builder.put(key, configValue.getConfiguration());
+          } else if (configsResult.errorMap().containsKey(configKey)) {
+            ErrorInfo configError = configsResult.getError(configKey);
+            if (configError.getException() instanceof InvalidConfigurationException) {
+              // Wrap underlying exception to make it clearer to developers which line of code
+              // actually threw exception.
+              InvalidConfigurationException underlying =
+                  (InvalidConfigurationException) configError.getException();
+              throw new InvalidConfigurationException(underlying.getDetailedExitCode(), underlying);
+            }
           }
         }
       }
@@ -2211,17 +2218,14 @@ EvaluationResult<SkyValue> evaluateSkyKeys(
     try {
       result =
           callUninterruptibly(
-              new Callable<EvaluationResult<SkyValue>>() {
-                @Override
-                public EvaluationResult<SkyValue> call() throws Exception {
-                  synchronized (valueLookupLock) {
-                    try {
-                      skyframeBuildView.enableAnalysis(true);
-                      return evaluate(
-                          skyKeys, keepGoing, /*numThreads=*/ DEFAULT_THREAD_COUNT, eventHandler);
-                    } finally {
-                      skyframeBuildView.enableAnalysis(false);
-                    }
+              () -> {
+                synchronized (valueLookupLock) {
+                  try {
+                    skyframeBuildView.enableAnalysis(true);
+                    return evaluate(
+                        skyKeys, keepGoing, /*numThreads=*/ DEFAULT_THREAD_COUNT, eventHandler);
+                  } finally {
+                    skyframeBuildView.enableAnalysis(false);
                   }
                 }
               });

src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java

@@ -740,18 +740,31 @@ private static void checkBaseName(String baseName) {
     if (baseName.equals(".") || baseName.equals("..")) {
       throw new IllegalArgumentException("baseName must not be '" + baseName + "'");
     }
+    try {
+      checkSeparators(baseName);
+    } catch (InvalidBaseNameException e) {
+      throw new IllegalArgumentException("baseName " + e.getMessage() + ": '" + baseName + "'", e);
+    }
+  }
+
+  public static void checkSeparators(String baseName) throws InvalidBaseNameException {
     if (baseName.indexOf(SEPARATOR_CHAR) != -1) {
-      throw new IllegalArgumentException(
-          "baseName must not contain " + SEPARATOR_CHAR + ": '" + baseName + "'");
+      throw new InvalidBaseNameException("must not contain " + SEPARATOR_CHAR);
     }
     if (ADDITIONAL_SEPARATOR_CHAR != 0) {
       if (baseName.indexOf(ADDITIONAL_SEPARATOR_CHAR) != -1) {
-        throw new IllegalArgumentException(
-            "baseName must not contain " + ADDITIONAL_SEPARATOR_CHAR + ": '" + baseName + "'");
+        throw new InvalidBaseNameException("must not contain " + ADDITIONAL_SEPARATOR_CHAR);
       }
     }
   }
 
+  /** Indicates that a path fragment's base name had invalid characters. */
+  public static class InvalidBaseNameException extends Exception {
+    private InvalidBaseNameException(String message) {
+      super(message);
+    }
+  }
+
   @SuppressWarnings("unused") // found by CLASSPATH-scanning magic
   private static class Codec implements ObjectCodec<PathFragment> {
     @Override

src/main/protobuf/failure_details.proto

@@ -565,6 +565,11 @@ message BuildConfiguration {
         [(metadata) = { exit_code: 2 }];
     CYCLE = 9 [(metadata) = { exit_code: 2 }];
     CONFLICTING_CONFIGURATIONS = 10 [(metadata) = { exit_code: 2 }];
+    // This can come from either an invalid user-specified option or a
+    // configuration transition. There's no sure-fire way to distinguish the two
+    // possibilities in Bazel, so we go with the more straightforward
+    // command-line error exit code 2.
+    INVALID_OUTPUT_DIRECTORY_MNEMONIC = 11 [(metadata) = { exit_code: 2 }];
   }
 
   Code code = 1;

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

@@ -1467,4 +1467,44 @@ public void starlarkPatchTransitionRequiredFragments() throws Exception {
     assertThat(ruleTransition.requiresOptionFragments(ct.getConfiguration().getOptions()))
         .containsExactly("CppOptions");
   }
+
+  /**
+   * Unit test for an invalid output directory from a mnemonic via a dep transition. Integration
+   * test for top-level transition in //src/test/shell/integration:starlark_configurations_test#
+   * test_invalid_mnemonic_from_transition_top_level. Has to be an integration test because the
+   * error is emitted in BuildTool.
+   */
+  @Test
+  public void invalidMnemonicFromDepTransition() throws Exception {
+    writeAllowlistFile();
+    scratch.file(
+        "test/transitions.bzl",
+        "def _impl(settings, attr):",
+        "  return {'//command_line_option:cpu': '//bad:cpu'}",
+        "my_transition = transition(implementation = _impl, inputs = [],",
+        "  outputs = ['//command_line_option:cpu'])");
+    scratch.file(
+        "test/rules.bzl",
+        "load('//test:transitions.bzl', 'my_transition')",
+        "def _impl(ctx):",
+        "  return []",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  cfg = my_transition,",
+        "  attrs = {",
+        "    '_allowlist_function_transition': attr.label(",
+        "        default = '//tools/allowlists/function_transition_allowlist',",
+        "    ),",
+        "  })");
+    scratch.file(
+        "test/BUILD",
+        "load('//test:rules.bzl', 'my_rule')",
+        "my_rule(name = 'bottom')",
+        "genrule(name = 'test', srcs = [':bottom'], outs = ['out'], cmd = 'touch $@')");
+    reporter.removeHandler(failFastHandler);
+    assertThat(getConfiguredTarget("//test:test")).isNull();
+    assertContainsEvent(
+        "Output directory name '//bad:cpu' specified by CppConfiguration is invalid as part of a "
+            + "path: must not contain /");
+  }
 }

src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java

@@ -21,6 +21,7 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.analysis.config.OutputDirectories.InvalidMnemonicException;
 import com.google.devtools.build.lib.analysis.util.ConfigurationTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
@@ -349,6 +350,24 @@ private ImmutableList<BuildConfiguration> getTestConfigurations() throws Excepti
             "#a=pounda"));
   }
 
+  @Test
+  public void throwsOnBadMnemonic() {
+    InvalidMnemonicException e =
+        assertThrows(InvalidMnemonicException.class, () -> create("--cpu=//bad/cpu"));
+    assertThat(e)
+        .hasMessageThat()
+        .isEqualTo(
+            "Output directory name '//bad/cpu' specified by CppConfiguration is invalid as part of"
+                + " a path: must not contain /");
+    e =
+        assertThrows(
+            InvalidMnemonicException.class, () -> create("--platform_suffix=//bad/suffix"));
+    assertThat(e)
+        .hasMessageThat()
+        .isEqualTo(
+            "Platform suffix '//bad/suffix' is invalid as part of a path: must not contain /");
+  }
+
   @Test
   public void testCodec() throws Exception {
     // Unnecessary ImmutableList.copyOf apparently necessary to choose non-varargs constructor.