Change CommandHelper to use TemplateExpander directly

This is a partial rollback of https://github.com/bazelbuild/bazel/commit/e8d32b7c922f65539b74357711d5ad6b70934115, only the CommandHelper change.

Progress on #2475.

PiperOrigin-RevId: 179413908

5 files changed, 72 insertions, 52 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 85dfc7a043..cd7a8f38b7 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
@@ -24,6 +24,9 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ExecutionRequirements;
 import com.google.devtools.build.lib.actions.RunfilesSupplier;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
+import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException;
+import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
+import com.google.devtools.build.lib.analysis.stringtemplate.TemplateExpander;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.syntax.SkylarkList;
@@ -163,24 +166,26 @@ public final class CommandHelper {
   }
 
   /**
-   * Resolves a command, and expands known locations for $(location)
-   * variables.
+   * 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;
-    if (allowDataInLabel) {
-      expander = LocationExpander.withExecPathsAndData(ruleContext, labelMap);
-    } else {
-      expander = LocationExpander.withExecPaths(ruleContext, labelMap);
-    }
-    if (attribute != null) {
-      command = expander.expandAttribute(attribute, command);
-    } else {
-      command = expander.expand(command);
+  public String expandForSkylark(
+      String command, @Nullable String attribute,
+      TemplateContext templateContext, boolean expandLocations) {
+    try {
+      if (expandLocations) {
+        templateContext = new LocationTemplateContext(
+            templateContext, ruleContext, labelMap, true, false);
+      }
+      return TemplateExpander.expand(command, templateContext);
+    } catch (ExpansionException e) {
+      if (attribute != null) {
+        ruleContext.attributeError(attribute, e.getMessage());
+      } else {
+        ruleContext.ruleError(e.getMessage());
+      }
+      return command;
     }
-    return command;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
index 20bb7b895a..7887eea3d2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
@@ -39,6 +39,7 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.FragmentCollection;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException;
+import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector;
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesProvider;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -1117,27 +1118,30 @@ public final class SkylarkRuleContext implements SkylarkValue {
       + "Additional variables may come from other places, such as configurations. Note that "
       + "this function is experimental.")
   public String expandMakeVariables(String attributeName, String command,
-      final Map<String, String> additionalSubstitutions) throws EvalException {
+      Map<String, String> additionalSubstitutions) throws EvalException {
     checkMutable("expand_make_variables");
-    ConfigurationMakeVariableContext makeVariableContext =
-        new ConfigurationMakeVariableContext(
-            // TODO(lberki): This should be removed. But only after either verifying that no one
-            // uses it or providing an alternative.
-            ruleContext.getMakeVariables(ImmutableList.of(":cc_toolchain")),
-            ruleContext.getRule().getPackage(),
-            ruleContext.getConfiguration()) {
-          @Override
-          public String lookupVariable(String variableName) throws ExpansionException {
-            if (additionalSubstitutions.containsKey(variableName)) {
-              return additionalSubstitutions.get(variableName);
-            } else {
-              return super.lookupVariable(variableName);
-            }
-          }
-        };
-    return ruleContext.getExpander(makeVariableContext).expand(attributeName, command);
+    TemplateContext templateContext = getConfigurationMakeVariableContext(additionalSubstitutions);
+    return ruleContext.getExpander(templateContext).expand(attributeName, command);
   }
 
+  TemplateContext getConfigurationMakeVariableContext(
+      final Map<String, String> additionalSubstitutions) {
+    return new ConfigurationMakeVariableContext(
+        // TODO(lberki): This should be removed. But only after either verifying that no one
+        // uses it or providing an alternative.
+        ruleContext.getMakeVariables(ImmutableList.of(":cc_toolchain")),
+        ruleContext.getRule().getPackage(),
+        ruleContext.getConfiguration()) {
+      @Override
+      public String lookupVariable(String variableName) throws ExpansionException {
+        if (additionalSubstitutions.containsKey(variableName)) {
+          return additionalSubstitutions.get(variableName);
+        } else {
+          return super.lookupVariable(variableName);
+        }
+      }
+    };
+  }
 
   FilesToRunProvider getExecutableRunfiles(Artifact executable) {
     return attributesCollection.getExecutableRunfilesMap().get(executable);
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 5a0ea681bd..eccd577659 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
@@ -25,6 +25,7 @@ import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.configuredtargets.AbstractConfiguredTarget;
+import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.skylarkinterface.Param;
@@ -709,15 +710,13 @@ public class SkylarkRuleImplementationFunctions {
                   ImmutableMap.copyOf(labelDict));
           String attribute =
               Type.STRING.convertOptional(attributeUnchecked, "attribute", ruleLabel);
-          if (expandLocations) {
-            command = helper.resolveCommandAndExpandLabels(
-                command, attribute, /*allowDataInLabel=*/false);
-          }
+          TemplateContext templateContext = TemplateContext.EMPTY;
           if (!EvalUtils.isNullOrNone(makeVariablesUnchecked)) {
             Map<String, String> makeVariables =
                 Type.STRING_DICT.convert(makeVariablesUnchecked, "make_variables", ruleLabel);
-            command = ctx.expandMakeVariables(attribute, command, makeVariables);
+            templateContext = ctx.getConfigurationMakeVariableContext(makeVariables);
           }
+          command = helper.expandForSkylark(command, attribute, templateContext, expandLocations);
           List<Artifact> inputs = new ArrayList<>();
           inputs.addAll(helper.getResolvedTools());
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java b/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java
index dc6d27598d..179af8746a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java
@@ -18,6 +18,17 @@ package com.google.devtools.build.lib.analysis.stringtemplate;
  * each template variable and function.
  */
 public interface TemplateContext {
+  public static final TemplateContext EMPTY = new TemplateContext() {
+    @Override
+    public String lookupVariable(String name) throws ExpansionException {
+      throw new ExpansionException(String.format("$(%s) not defined", name));
+    }
+
+    @Override
+    public String lookupFunction(String name, String param) throws ExpansionException {
+      throw new ExpansionException(String.format("$(%s) not defined", name));
+    }
+  };
 
   /**
    * Returns the expansion of the specified template variable.
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ExpanderIntegrationTest.java
index 1db06a8883..382ee60bc5 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/ExpanderIntegrationTest.java
@@ -16,15 +16,16 @@ package com.google.devtools.build.lib.analysis;
 
 import static com.google.common.truth.Truth.assertThat;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-/** Integration tests for {@link LocationExpander}. */
+/** Integration tests for {@link Expander}. */
 @RunWith(JUnit4.class)
-public class LocationExpanderIntegrationTest extends BuildViewTestCase {
+public class ExpanderIntegrationTest extends BuildViewTestCase {
 
   @Before
   public void createFiles() throws Exception {
@@ -40,10 +41,10 @@ public class LocationExpanderIntegrationTest extends BuildViewTestCase {
         "  deps = [':files'])");
   }
 
-  private LocationExpander makeExpander(String label) throws Exception {
+  private Expander makeExpander(String label) throws Exception {
     ConfiguredTarget target = getConfiguredTarget(label);
     RuleContext ruleContext = getRuleContext(target);
-    return LocationExpander.withRunfilesPaths(ruleContext);
+    return ruleContext.getExpander().withExecLocations(ImmutableMap.of());
   }
 
   @Test
@@ -57,9 +58,9 @@ public class LocationExpanderIntegrationTest extends BuildViewTestCase {
         "sh_library(name='lib',",
         "  deps = [':files'])");
 
-    LocationExpander expander = makeExpander("//spaces:lib");
+    Expander expander = makeExpander("//spaces:lib");
     String input = "foo $(locations :files) bar";
-    String result = expander.expand(input);
+    String result = expander.expand(null, input);
 
     assertThat(result).isEqualTo("foo 'spaces/file with space A' 'spaces/file with space B' bar");
   }
@@ -71,14 +72,14 @@ public class LocationExpanderIntegrationTest extends BuildViewTestCase {
         "genrule(name='foo', outs=['foo.txt'], cmd='never executed')",
         "sh_library(name='lib', srcs=[':foo'])");
 
-    LocationExpander expander = makeExpander("//expansion:lib");
-    assertThat(expander.expand("foo $(execpath :foo) bar"))
+    Expander expander = makeExpander("//expansion:lib");
+    assertThat(expander.expand("<attribute>", "foo $(execpath :foo) bar"))
         .matches("foo .*-out/.*/expansion/foo\\.txt bar");
-    assertThat(expander.expand("foo $(execpaths :foo) bar"))
+    assertThat(expander.expand("<attribute>", "foo $(execpaths :foo) bar"))
         .matches("foo .*-out/.*/expansion/foo\\.txt bar");
-    assertThat(expander.expand("foo $(rootpath :foo) bar"))
+    assertThat(expander.expand("<attribute>", "foo $(rootpath :foo) bar"))
         .matches("foo expansion/foo.txt bar");
-    assertThat(expander.expand("foo $(rootpaths :foo) bar"))
+    assertThat(expander.expand("<attribute>", "foo $(rootpaths :foo) bar"))
         .matches("foo expansion/foo.txt bar");
   }
 
@@ -89,10 +90,10 @@ public class LocationExpanderIntegrationTest extends BuildViewTestCase {
         "genrule(name='foo', outs=['foo.txt', 'bar.txt'], cmd='never executed')",
         "sh_library(name='lib', srcs=[':foo'])");
 
-    LocationExpander expander = makeExpander("//expansion:lib");
-    assertThat(expander.expand("foo $(execpaths :foo) bar"))
+    Expander expander = makeExpander("//expansion:lib");
+    assertThat(expander.expand("<attribute>", "foo $(execpaths :foo) bar"))
         .matches("foo .*-out/.*/expansion/bar\\.txt .*-out/.*/expansion/foo\\.txt bar");
-    assertThat(expander.expand("foo $(rootpaths :foo) bar"))
+    assertThat(expander.expand("<attribute>", "foo $(rootpaths :foo) bar"))
         .matches("foo expansion/bar.txt expansion/foo.txt bar");
   }
 }