LocationExpander: always require options to be passed in

Also update CommandHelper to split the heuristic label expansion code path from
the normal code path.

Progress on #2475.

PiperOrigin-RevId: 170675702

5 files changed, 35 insertions, 63 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 330c017a1d..74d05a7edf 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
@@ -26,9 +26,7 @@ import com.google.devtools.build.lib.actions.RunfilesSupplier;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
-import com.google.devtools.build.lib.syntax.SkylarkDict;
 import com.google.devtools.build.lib.syntax.SkylarkList;
-import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -73,7 +71,7 @@ public final class CommandHelper {
    * This is similar to heuristic location expansion in LocationExpander
    * and should be kept in sync.
    */
-  private final SkylarkDict<Label, ImmutableCollection<Artifact>> labelMap;
+  private final ImmutableMap<Label, ImmutableCollection<Artifact>> labelMap;
 
   /**
    * The ruleContext this helper works on
@@ -135,7 +133,7 @@ public final class CommandHelper {
     for (Entry<Label, Collection<Artifact>> entry : tempLabelMap.entrySet()) {
       labelMapBuilder.put(entry.getKey(), ImmutableList.copyOf(entry.getValue()));
     }
-    this.labelMap = SkylarkDict.copyOf(null, labelMapBuilder.build());
+    this.labelMap = labelMapBuilder.build();
   }
 
   public SkylarkList<Artifact> getResolvedTools() {
@@ -161,38 +159,38 @@ public final class CommandHelper {
   }
 
   /**
-   * Resolves a command, and expands known locations for $(location)
-   * variables.
+   * 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 resolveCommandAndExpandLabels(
-      String command,
-      @Nullable String attribute,
-      Boolean supportLegacyExpansion,
-      Boolean allowDataInLabel) {
-    LocationExpander expander = new LocationExpander(
-        ruleContext, ImmutableMap.copyOf(labelMap), allowDataInLabel);
-    if (attribute != null) {
-      command = expander.expandAttribute(attribute, command);
-    } else {
-      command = expander.expand(command);
-    }
-    if (supportLegacyExpansion) {
+  public String resolveCommandAndHeuristicallyExpandLabels(
+      String command, @Nullable String attribute, boolean enableLegacyHeuristicLabelExpansion) {
+    command = resolveCommandAndExpandLabels(command, attribute, false);
+    if (enableLegacyHeuristicLabelExpansion) {
       command = expandLabels(command, labelMap);
     }
     return command;
   }
 
   /**
-   * Resolves the 'cmd' attribute, and expands known locations for $(location)
+   * Resolves a command, and expands known locations for $(location)
    * variables.
    */
   public String resolveCommandAndExpandLabels(
-      Boolean supportLegacyExpansion, Boolean allowDataInLabel) {
-    return resolveCommandAndExpandLabels(
-        ruleContext.attributes().get("cmd", Type.STRING),
-        "cmd",
-        supportLegacyExpansion,
-        allowDataInLabel);
+      String command, @Nullable String attribute, boolean allowDataInLabel) {
+    LocationExpander expander;
+    if (allowDataInLabel) {
+      expander = new LocationExpander(ruleContext, labelMap,
+          LocationExpander.Options.EXEC_PATHS, LocationExpander.Options.ALLOW_DATA);
+    } else {
+      expander = new LocationExpander(ruleContext, labelMap, LocationExpander.Options.EXEC_PATHS);
+    }
+    if (attribute != null) {
+      command = expander.expandAttribute(attribute, command);
+    } else {
+      command = expander.expand(command);
+    }
+    return command;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java b/src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java
index 46c1493b07..8df1a9f171 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java
@@ -47,6 +47,8 @@ import java.util.stream.Stream;
  *
  * and location will be substituted with //mypackage:myhelper executable output.
  * Note that //mypackage:myhelper should have just one output.
+ *
+ * <p>DO NOT USE DIRECTLY! Use RuleContext.getExpander() instead.
  */
 public class LocationExpander {
 
@@ -78,27 +80,6 @@ public class LocationExpander {
    *
    * @param ruleContext BUILD rule
    * @param labelMap A mapping of labels to build artifacts.
-   * @param allowDataAttributeEntriesInLabel set to true if the <code>data</code> attribute should
-   *        be used too.
-   */
-  public LocationExpander(
-      RuleContext ruleContext, ImmutableMap<Label, ImmutableCollection<Artifact>> labelMap,
-      boolean allowDataAttributeEntriesInLabel) {
-    this.ruleContext = ruleContext;
-    ImmutableSet.Builder<Options> builder = ImmutableSet.builder();
-    builder.add(Options.EXEC_PATHS);
-    if (allowDataAttributeEntriesInLabel) {
-      builder.add(Options.ALLOW_DATA);
-    }
-    this.options = builder.build();
-    this.labelMap = labelMap;
-  }
-
-  /**
-   * Creates location expander helper bound to specific target and with default location map.
-   *
-   * @param ruleContext BUILD rule
-   * @param labelMap A mapping of labels to build artifacts.
    * @param options the list of options, see {@link Options}
    */
   public LocationExpander(
@@ -115,17 +96,6 @@ public class LocationExpander {
    * @param ruleContext the BUILD rule's context
    * @param options the list of options, see {@link Options}.
    */
-  public LocationExpander(RuleContext ruleContext, ImmutableSet<Options> options) {
-    this.ruleContext = ruleContext;
-    this.options = options;
-  }
-
-  /**
-   * Creates location expander helper bound to specific target.
-   *
-   * @param ruleContext the BUILD rule's context
-   * @param options the list of options, see {@link Options}.
-   */
   public LocationExpander(RuleContext ruleContext, Options... options) {
     this.ruleContext = ruleContext;
     this.options = ImmutableSet.copyOf(options);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java
index 5d7ced8940..81e0ea52ac 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java
@@ -321,7 +321,7 @@ public class SkylarkRuleImplementationFunctions {
             return new LocationExpander(
                     ctx.getRuleContext(),
                     makeLabelMap(targets.getContents(TransitiveInfoCollection.class, "targets")),
-                    false)
+                    LocationExpander.Options.EXEC_PATHS)
                 .expand(input);
           } catch (IllegalStateException ise) {
             throw new EvalException(loc, ise);
@@ -711,7 +711,8 @@ public class SkylarkRuleImplementationFunctions {
           String attribute =
               Type.STRING.convertOptional(attributeUnchecked, "attribute", ruleLabel);
           if (expandLocations) {
-            command = helper.resolveCommandAndExpandLabels(command, attribute, false, false);
+            command = helper.resolveCommandAndExpandLabels(
+                command, attribute, /*allowDataInLabel=*/false);
           }
           if (!EvalUtils.isNullOrNone(makeVariablesUnchecked)) {
             Map<String, String> makeVariables =
diff --git a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionFactory.java b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionFactory.java
index b2d7247e14..47411e1a95 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionFactory.java
@@ -54,7 +54,8 @@ public final class ExtraActionFactory implements RuleConfiguredTargetFactory {
     List<String>outputTemplates =
         context.attributes().get("out_templates", Type.STRING_LIST);
 
-    String command = commandHelper.resolveCommandAndExpandLabels(false, true);
+    String command = commandHelper.resolveCommandAndExpandLabels(
+        context.attributes().get("cmd", Type.STRING), "cmd", /*allowDataInLabel=*/true);
     // This is a bit of a hack. We want to run the MakeVariableExpander first, so we expand $ on
     // variables that are expanded below with $$, which gets reverted to $ by the
     // MakeVariableExpander. This allows us to expand package-specific make variables in the
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 2a59bffbc7..213063fab5 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
@@ -153,8 +153,10 @@ public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
       return null;
     }
 
-    String baseCommand = commandHelper.resolveCommandAndExpandLabels(
-        ruleContext.attributes().get("heuristic_label_expansion", Type.BOOLEAN), false);
+    String baseCommand = commandHelper.resolveCommandAndHeuristicallyExpandLabels(
+        ruleContext.attributes().get("cmd", Type.STRING),
+        "cmd",
+        ruleContext.attributes().get("heuristic_label_expansion", Type.BOOLEAN));
 
     // Adds the genrule environment setup script before the actual shell command
     String command = String.format("source %s; %s",