Switch GenRuleBase to use the new Expander API

This is a roll-forward of https://github.com/bazelbuild/bazel/commit/e8d32b7c922f65539b74357711d5ad6b70934115, which broke some genrules, but without
some cleanup changes which I'm submitting separately, and with a fix for the
bug.

The problem was that I switched from withExecLocations(labels) to
withExecLocations(); the original code was in CommandHelper, and the new
code in GenRuleBase, so this was not obvious.

Also, we didn't have test coverage for this case - note that the specified
labels are _added_ to the default map of labels, rather than replacing the
default map of labels. This only matters if the dependent rule provides a
GenRuleSourcesProvider, which only a single (Google-internal) rule does.

PiperOrigin-RevId: 177802902

2 files changed, 20 insertions, 37 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
index cb0abd7026..85dfc7a043 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
@@ -144,6 +144,10 @@ public final class CommandHelper {
     return toolsRunfilesSuppliers;
   }
 
+  public ImmutableMap<Label, ImmutableCollection<Artifact>> getLabelMap() {
+    return labelMap;
+  }
+
   // Returns the value in the specified corresponding to 'key', creating and
   // inserting an empty container if absent.  We use Map not Multimap because
   // we need to distinguish the cases of "empty value" and "absent key".
@@ -159,23 +163,10 @@ public final class CommandHelper {
   }
 
   /**
-   * Resolves a command, and expands known locations for $(location) variables. This method supports
-   * legacy heuristic label expansion, which replaces strings that look like labels with their
-   * corresponding file names. Use {@link #resolveCommandAndExpandLabels} instead.
-   */
-  public String resolveCommandAndHeuristicallyExpandLabels(
-      String command, @Nullable String attribute, boolean enableLegacyHeuristicLabelExpansion) {
-    command = resolveCommandAndExpandLabels(command, attribute, false);
-    if (enableLegacyHeuristicLabelExpansion) {
-      command = expandLabels(command, labelMap);
-    }
-    return command;
-  }
-
-  /**
    * Resolves a command, and expands known locations for $(location)
    * variables.
    */
+  @Deprecated // Only exists to support a legacy Skylark API.
   public String resolveCommandAndExpandLabels(
       String command, @Nullable String attribute, boolean allowDataInLabel) {
     LocationExpander expander;
@@ -200,7 +191,7 @@ public final class CommandHelper {
    * <p>If the expansion fails, an attribute error is reported and the original
    * expression is returned.
    */
-  private <T extends Iterable<Artifact>> String expandLabels(String expr, Map<Label, T> labelMap) {
+  public String expandLabelsHeuristically(String expr) {
     try {
       return LabelExpander.expand(expr, labelMap, ruleContext.getLabel());
     } catch (LabelExpander.NotUniqueExpansionException nuee) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
index ecd7e71d34..dc1ff59a8a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
@@ -141,17 +141,22 @@ public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
       return null;
     }
 
-    String baseCommand = commandHelper.resolveCommandAndHeuristicallyExpandLabels(
-        ruleContext.attributes().get("cmd", Type.STRING),
-        "cmd",
-        ruleContext.attributes().get("heuristic_label_expansion", Type.BOOLEAN));
+    String baseCommand = ruleContext.attributes().get("cmd", Type.STRING);
+    // Expand template variables and functions.
+    String command = ruleContext
+        .getExpander(new CommandResolverContext(ruleContext, resolvedSrcs, filesToBuild))
+        .withExecLocations(commandHelper.getLabelMap())
+        .expand("cmd", baseCommand);
 
-    // Adds the genrule environment setup script before the actual shell command
-    String command = String.format("source %s; %s",
-        ruleContext.getPrerequisiteArtifact("$genrule_setup", Mode.HOST).getExecPath(),
-        baseCommand);
+    // Heuristically expand things that look like labels.
+    if (ruleContext.attributes().get("heuristic_label_expansion", Type.BOOLEAN)) {
+      command = commandHelper.expandLabelsHeuristically(command);
+    }
 
-    command = resolveCommand(command, ruleContext, resolvedSrcs, filesToBuild);
+    // Add the genrule environment setup script before the actual shell command.
+    command = String.format("source %s; %s",
+        ruleContext.getPrerequisiteArtifact("$genrule_setup", Mode.HOST).getExecPath(),
+        command);
 
     String messageAttr = ruleContext.attributes().get("message", Type.STRING);
     String message = messageAttr.isEmpty() ? "Executing genrule" : messageAttr;
@@ -249,19 +254,6 @@ public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
   }
 
   /**
-   * Resolves any variables, including make and genrule-specific variables, in the command and
-   * returns the expanded command.
-   *
-   * <p>GenRule implementations may override this method to perform additional expansions.
-   */
-  protected String resolveCommand(String command, final RuleContext ruleContext,
-      final NestedSet<Artifact> resolvedSrcs, final NestedSet<Artifact> filesToBuild) {
-    return ruleContext
-        .getExpander(new CommandResolverContext(ruleContext, resolvedSrcs, filesToBuild))
-        .expand("cmd", command);
-  }
-
-  /**
    * Implementation of {@link ConfigurationMakeVariableContext} used to expand variables in a
    * genrule command string.
    */