[8.0.1] Add and flip --incompatible_locations_prefers_executable (#…

…24874)

Work towards #11820
Fixes #20038
Fixes #23200
Fixes #24613

RELNOTES: Extra targets provided to `ctx.expand_location` now expand to
their executable (if any) instead of resulting in an error if they
provide a number of files different from one.

RELNOTES[INC]: The `--incompatible_locations_prefers_executable` flag
has been added and enabled, which makes it so that `ctx.expand_location`
expands `$(locations :x)` to the executable of an extra target `:x` if
it provides one and the number of files provided by it is not one.

Closes #24690.

PiperOrigin-RevId: 713453768
Change-Id: I0d6e052bc70deea029554ab722feb544f9597a23 
(cherry picked from commit 457d248)

Fixes #24646

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

@@ -1016,7 +1016,11 @@ public String expandLocation(
     checkMutable("expand_location");
     try {
       ImmutableMap<Label, ImmutableCollection<Artifact>> labelMap =
-          makeLabelMap(Sequence.cast(targets, TransitiveInfoCollection.class, "targets"));
+          makeLabelMap(
+              Sequence.cast(targets, TransitiveInfoCollection.class, "targets"),
+              thread
+                  .getSemantics()
+                  .getBool(BuildLanguageOptions.INCOMPATIBLE_LOCATIONS_PREFERS_EXECUTABLE));
       LocationExpander expander;
       if (!shortPaths) {
         expander = LocationExpander.withExecPaths(ruleContext, labelMap);
@@ -1239,10 +1243,13 @@ private static Map<Label, Iterable<Artifact>> checkLabelDict(Map<?, ?> labelDict
    * Builds a map: Label -> List of files from the given labels
    *
    * @param knownLabels List of known labels
+   * @param locationsPrefersExecutable whether to prefer an executable over a list of files that
+   *     isn't a singleton when requesting a plural location function
    * @return Immutable map with immutable collections as values
    */
   public static ImmutableMap<Label, ImmutableCollection<Artifact>> makeLabelMap(
-      Iterable<TransitiveInfoCollection> knownLabels) throws EvalException {
+      Iterable<TransitiveInfoCollection> knownLabels, boolean locationsPrefersExecutable)
+      throws EvalException {
     var targetsMap = new LinkedHashMap<Label, ImmutableCollection<Artifact>>();
     for (TransitiveInfoCollection current : knownLabels) {
       Label label = AliasProvider.getDependencyLabel(current);
@@ -1251,7 +1258,22 @@ public static ImmutableMap<Label, ImmutableCollection<Artifact>> makeLabelMap(
             "Label %s is found more than once in 'targets' list.", Starlark.repr(label.toString()));
       }
 
-      targetsMap.put(label, current.getProvider(FileProvider.class).getFilesToBuild().toList());
+      var filesToRunProvider = current.getProvider(FilesToRunProvider.class);
+      ImmutableCollection<Artifact> expansion;
+      // Only use the executable if requesting the files to build via a singleton location function
+      // would fail to ensure backwards compatibility (this function used to always return the
+      // files to build). The case of a plural location function is potentially breaking, but gated
+      // behind locationsPrefersExecutable.
+      // Avoid flattening a nested set if the first branch is taken.
+      if (filesToRunProvider != null
+          && filesToRunProvider.getExecutable() != null
+          && locationsPrefersExecutable
+          && !current.getProvider(FileProvider.class).getFilesToBuild().isSingleton()) {
+        expansion = ImmutableList.of(filesToRunProvider.getExecutable());
+      } else {
+        expansion = current.getProvider(FileProvider.class).getFilesToBuild().toList();
+      }
+      targetsMap.put(label, expansion);
     }
 
     return ImmutableMap.copyOf(targetsMap);

src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java

@@ -817,6 +817,18 @@ public final class BuildLanguageOptions extends OptionsBase {
               + " attributes of symbolic macros or attribute default values.")
   public boolean incompatibleSimplifyUnconditionalSelectsInRuleAttrs;
 
+  @Option(
+      name = "incompatible_locations_prefers_executable",
+      defaultValue = "true",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      help =
+          "Whether a target that provides an executable expands to the executable rather than the"
+              + " files in <code>DefaultInfo.files</code> under $(locations ...) expansion if the"
+              + " number of files is not 1.")
+  public boolean incompatibleLocationsPrefersExecutable;
+
   /**
    * An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should
    * never accumulate a large number of these and being able to shortcut on object identity makes a
@@ -930,6 +942,8 @@ public StarlarkSemantics toStarlarkSemantics() {
             .setBool(
                 INCOMPATIBLE_SIMPLIFY_UNCONDITIONAL_SELECTS_IN_RULE_ATTRS,
                 incompatibleSimplifyUnconditionalSelectsInRuleAttrs)
+            .setBool(
+                INCOMPATIBLE_LOCATIONS_PREFERS_EXECUTABLE, incompatibleLocationsPrefersExecutable)
             .build();
     return INTERNER.intern(semantics);
   }
@@ -1037,6 +1051,8 @@ public StarlarkSemantics toStarlarkSemantics() {
       "+incompatible_disallow_ctx_resolve_tools";
   public static final String INCOMPATIBLE_SIMPLIFY_UNCONDITIONAL_SELECTS_IN_RULE_ATTRS =
       "+incompatible_simplify_unconditional_selects_in_rule_attrs";
+  public static final String INCOMPATIBLE_LOCATIONS_PREFERS_EXECUTABLE =
+      "+incompatible_locations_prefers_executable";
   // non-booleans
   public static final StarlarkSemantics.Key<String> EXPERIMENTAL_BUILTINS_BZL_PATH =
       new StarlarkSemantics.Key<>("experimental_builtins_bzl_path", "%bundled%");

src/main/java/com/google/devtools/build/lib/rules/objc/BUILD

@@ -34,6 +34,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//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/packages/semantics",
         "//src/main/java/com/google/devtools/build/lib/rules/apple",
         "//src/main/java/com/google/devtools/build/lib/rules/cpp",
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",

src/main/java/com/google/devtools/build/lib/rules/objc/ObjcStarlarkInternal.java

@@ -24,6 +24,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext;
 import com.google.devtools.build.lib.packages.Types;
+import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.rules.apple.AppleConfiguration;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
 import java.util.List;
@@ -128,7 +129,10 @@ public Sequence<String> expandAndTokenize(
                             starlarkRuleContext.getRuleContext().getPrerequisites("srcs"),
                             starlarkRuleContext.getRuleContext().getPrerequisites("non_arc_srcs"),
                             starlarkRuleContext.getRuleContext().getPrerequisites("hdrs"),
-                            starlarkRuleContext.getRuleContext().getPrerequisites("data")))))
+                            starlarkRuleContext.getRuleContext().getPrerequisites("data"))),
+                    starlarkRuleContext
+                        .getStarlarkSemantics()
+                        .getBool(BuildLanguageOptions.INCOMPATIBLE_LOCATIONS_PREFERS_EXECUTABLE)))
             .withDataExecLocations();
     ImmutableList<String> expandedFlags =
         expander.tokenized(attributeName, Sequence.cast(flags, String.class, attributeName));

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

@@ -448,7 +448,13 @@ String expandMakeVariables(
             },
             defaultValue = "[]",
             named = true,
-            doc = "List of targets for additional lookup information."),
+            doc =
+                "List of targets for additional lookup information. These are expanded as follows:"
+                    + " A target with a single file in <code>DefaultInfo.files</code> expands to"
+                    + " that file. Other targets expand to their"
+                    + " <code>DefaultInfo.executable</code> file if set and if"
+                    + " <code>--incompatible_locations_prefers_executable</code> is enabled,"
+                    + " otherwise they expand to <code>DefaultInfo.files</code>."),
         @Param(
             name = "short_paths",
             named = true,

src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java

@@ -143,6 +143,7 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep
         "--incompatible_do_not_split_linking_cmdline=" + rand.nextBoolean(),
         "--incompatible_enable_deprecated_label_apis=" + rand.nextBoolean(),
         "--incompatible_java_common_parameters=" + rand.nextBoolean(),
+        "--incompatible_locations_prefers_executable=" + rand.nextBoolean(),
         "--incompatible_merge_fixed_and_default_shell_env=" + rand.nextBoolean(),
         "--incompatible_no_attr_license=" + rand.nextBoolean(),
         "--incompatible_no_implicit_file_export=" + rand.nextBoolean(),
@@ -195,6 +196,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
         .setBool(BuildLanguageOptions.INCOMPATIBLE_DO_NOT_SPLIT_LINKING_CMDLINE, rand.nextBoolean())
         .setBool(BuildLanguageOptions.INCOMPATIBLE_ENABLE_DEPRECATED_LABEL_APIS, rand.nextBoolean())
         .setBool(BuildLanguageOptions.INCOMPATIBLE_JAVA_COMMON_PARAMETERS, rand.nextBoolean())
+        .setBool(BuildLanguageOptions.INCOMPATIBLE_LOCATIONS_PREFERS_EXECUTABLE, rand.nextBoolean())
         .setBool(
             BuildLanguageOptions.INCOMPATIBLE_MERGE_FIXED_AND_DEFAULT_SHELL_ENV, rand.nextBoolean())
         .setBool(BuildLanguageOptions.INCOMPATIBLE_NO_ATTR_LICENSE, rand.nextBoolean())

src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java

@@ -46,6 +46,7 @@
 import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.Runfiles;
+import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
@@ -69,6 +70,8 @@
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.util.OsUtils;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import java.util.Arrays;
 import java.util.Comparator;
 import java.util.HashMap;
@@ -91,10 +94,9 @@
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
 
 /** Tests for Starlark functions relating to rule implementation. */
-@RunWith(JUnit4.class)
+@RunWith(TestParameterInjector.class)
 public final class StarlarkRuleImplementationFunctionsTest extends BuildViewTestCase {
 
   private final BazelEvaluationTestCase ev = new BazelEvaluationTestCase();
@@ -703,6 +705,134 @@ public void testExpandLocationWithDollarSignsAndCurlys() throws Exception {
         .isEqualTo("${abc} $(echo) $$ $");
   }
 
+  @Test
+  public void testExpandedLocationWithSingleFileDifferentFromExecutable(
+      @TestParameter boolean locationsPrefersExecutable) throws Exception {
+    setBuildLanguageOptions(
+        "--incompatible_locations_prefers_executable=" + locationsPrefersExecutable);
+
+    scratch.file(
+        "test/defs.bzl",
+        "def _my_binary_impl(ctx):",
+        "  executable = ctx.actions.declare_file(ctx.attr.name + '_executable')",
+        "  ctx.actions.write(executable, '', is_executable = True)",
+        "  file = ctx.actions.declare_file(ctx.attr.name + '_file')",
+        "  ctx.actions.write(file, '')",
+        "  return [DefaultInfo(executable = executable, files = depset([file]))]",
+        "my_binary = rule(",
+        "    implementation = _my_binary_impl,",
+        "    executable = True,",
+        ")",
+        "def _expand_location_rule_impl(ctx):",
+        "  expansions = []",
+        "  for data in ctx.attr.data:",
+        "    expansions.append(",
+        "        ctx.expand_location('$(location ' + str(data.label) + ')', ctx.attr.data),",
+        "    )",
+        "    expansions.append(",
+        "        ctx.expand_location('$(locations ' + str(data.label) + ')', ctx.attr.data)",
+        "    )",
+        "  file = ctx.actions.declare_file(ctx.attr.name)",
+        "  ctx.actions.write(file, '\\n'.join(expansions))",
+        "  return [DefaultInfo(files = depset([file]))]",
+        "expand_location_rule = rule(",
+        "    implementation = _expand_location_rule_impl,",
+        "    attrs = {",
+        "       'data': attr.label_list(),",
+        "    },",
+        ")");
+
+    scratch.file(
+        "test/BUILD",
+        "load('//test:defs.bzl', 'expand_location_rule', 'my_binary')",
+        "my_binary(name = 'main')",
+        "expand_location_rule(",
+        "  name = 'expand',",
+        "  data = [':main'],",
+        ")");
+
+    TransitiveInfoCollection expandTarget = getConfiguredTarget("//test:expand");
+    Artifact artifact =
+        Iterables.getOnlyElement(
+            expandTarget.getProvider(FileProvider.class).getFilesToBuild().toList());
+    FileWriteAction action = (FileWriteAction) getGeneratingAction(artifact);
+    assertThat(action.getFileContents())
+        .matches(
+            """
+            ^\\S*/bin/test/main_file
+            \\S*/bin/test/main_file$\
+            """);
+  }
+
+  @Test
+  public void testExpandedLocationsWithMultipleFilesAndExecutable(
+      @TestParameter boolean locationsPrefersExecutable) throws Exception {
+    setBuildLanguageOptions(
+        "--incompatible_locations_prefers_executable=" + locationsPrefersExecutable);
+
+    scratch.file(
+        "test/defs.bzl",
+        "def _my_binary_impl(ctx):",
+        "  executable = ctx.actions.declare_file(ctx.attr.name + '_executable')",
+        "  ctx.actions.write(executable, '', is_executable = True)",
+        "  file1 = ctx.actions.declare_file(ctx.attr.name + '_file1')",
+        "  file2 = ctx.actions.declare_file(ctx.attr.name + '_file2')",
+        "  ctx.actions.write(file1, '')",
+        "  ctx.actions.write(file2, '')",
+        "  return [DefaultInfo(executable = executable, files = depset([file1, file2]))]",
+        "my_binary = rule(",
+        "    implementation = _my_binary_impl,",
+        "    executable = True,",
+        ")",
+        "def _expand_location_rule_impl(ctx):",
+        "  expansions = []",
+        "  for data in ctx.attr.data:",
+        "    expansions.append(",
+        "        ctx.expand_location('$(location ' + str(data.label) + ')', ctx.attr.data),",
+        "    )",
+        "    expansions.append(",
+        "        ctx.expand_location('$(locations ' + str(data.label) + ')', ctx.attr.data)",
+        "    )",
+        "  file = ctx.actions.declare_file(ctx.attr.name)",
+        "  ctx.actions.write(file, '\\n'.join(expansions))",
+        "  return [DefaultInfo(files = depset([file]))]",
+        "expand_location_rule = rule(",
+        "    implementation = _expand_location_rule_impl,",
+        "    attrs = {",
+        "       'data': attr.label_list(),",
+        "    },",
+        ")");
+
+    scratch.file(
+        "test/BUILD",
+        "load('//test:defs.bzl', 'expand_location_rule', 'my_binary')",
+        "my_binary(name = 'main')",
+        "expand_location_rule(",
+        "  name = 'expand',",
+        "  data = [':main'],",
+        ")");
+
+    reporter.removeHandler(failFastHandler);
+    TransitiveInfoCollection expandTarget = getConfiguredTarget("//test:expand");
+    if (locationsPrefersExecutable) {
+      Artifact artifact =
+          Iterables.getOnlyElement(
+              expandTarget.getProvider(FileProvider.class).getFilesToBuild().toList());
+      FileWriteAction action = (FileWriteAction) getGeneratingAction(artifact);
+      assertThat(action.getFileContents())
+          .matches(
+              """
+              ^\\S*/bin/test/main_executable
+              \\S*/bin/test/main_executable$\
+              """);
+    } else {
+      assertContainsEvent(
+          "label '//test:main' in $(location) expression expands to more than one file");
+      assertContainsEvent("/bin/test/main_file1,");
+      assertContainsEvent("/bin/test/main_file2]");
+    }
+  }
+
   /**
    * Invokes ctx.expand_location() with the given parameters and checks whether this led to the
    * expected result