Remove proto_lang_toolchain rule's $(PLUGIN_OUT) placeholder and simplify ProtoCompileActionBuilder.

`$(OUT)` placeholder is replaced with `%s` so it can be directly used in addFormatted. This simplifies construction of proto compile action and makes it possible for Starlark version to have the same performance.

github shows no uses of the placeholder: https://github.com/search?q=%22%24%28PLUGIN_OUT%29%22&type=Repositories

PiperOrigin-RevId: 412286743

Author: comius

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java

@@ -42,9 +42,7 @@
 import net.starlark.java.eval.Starlark;
 import net.starlark.java.eval.StarlarkThread;
 
-/**
- * Utility functions for proto_library and proto aspect implementations.
- */
+/** Utility functions for proto_library and proto aspect implementations. */
 public class ProtoCommon {
   private ProtoCommon() {
     throw new UnsupportedOperationException();

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java

@@ -21,7 +21,6 @@
 import com.google.common.base.Preconditions;
 import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.AbstractAction;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.CommandLineItem;
@@ -36,17 +35,13 @@
 import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
-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.NestedSet;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
 import com.google.devtools.build.lib.util.OnDemandString;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.function.Consumer;
 import javax.annotation.Nullable;
 
@@ -150,48 +145,6 @@ public String toString() {
     }
   }
 
-  private static class OnDemandCommandLineExpansion extends OnDemandString {
-    // E.g., --java_out=%s
-    private final String template;
-    private final Map<String, ? extends CharSequence> variableValues;
-
-    OnDemandCommandLineExpansion(
-        String template, Map<String, ? extends CharSequence> variableValues) {
-      this.template = template;
-      this.variableValues = variableValues;
-    }
-
-    @Override
-    public String toString() {
-      try {
-        return TemplateExpander.expand(
-                template,
-                new TemplateContext() {
-                  @Override
-                  public String lookupVariable(String name) throws ExpansionException {
-                    CharSequence value = variableValues.get(name);
-                    if (value == null) {
-                      throw new ExpansionException(String.format("$(%s) not defined", name));
-                    }
-                    return value.toString();
-                  }
-
-                  @Override
-                  public String lookupFunction(String name, String param)
-                      throws ExpansionException {
-                    throw new ExpansionException(String.format("$(%s) not defined", name));
-                  }
-                })
-            .expansion();
-      } catch (ExpansionException e) {
-        // Squeelch. We don't throw this exception in the lookupMakeVariable implementation above,
-        // and we can't report it here anyway, because this code will typically execute in the
-        // Execution phase.
-      }
-      return template;
-    }
-  }
-
   /** Builds a ResourceSet based on the number of inputs. */
   public static class ProtoCompileResourceSetBuilder implements ResourceSetOrBuilder {
     @Override
@@ -367,7 +320,7 @@ private static ToolchainInvocation createDescriptorSetToolchain(
             // output size to become quadratic, so don't.
             // A rule that concatenates the artifacts from ctx.deps.proto.transitive_descriptor_sets
             // provides similar results.
-            "--descriptor_set_out=$(OUT)",
+            "--descriptor_set_out=%s",
             /* pluginExecutable= */ null,
             /* runtime= */ null,
             /* providedProtoSources= */ ImmutableList.of()),
@@ -492,8 +445,6 @@ public static boolean arePublicImportsStrict(RuleContext ruleContext) {
    * <ul>
    *   <li>Each toolchain contributes a command-line, formatted from its commandLine() method.
    *   <li>$(OUT) is replaced with the outReplacement field of ToolchainInvocation.
-   *   <li>$(PLUGIN_out) is replaced with PLUGIN_<key>_out where 'key' is the key of
-   *       toolchainInvocations. The key thus allows multiple plugins in one command-line.
    *   <li>If a toolchain's {@code plugin()} is non-null, we point at it by emitting
    *       --plugin=protoc-gen-PLUGIN_<key>=<location of plugin>.
    * </ul>
@@ -537,14 +488,15 @@ static CustomCommandLine createCommandLineFromToolchains(
 
       ProtoLangToolchainProvider toolchain = invocation.toolchain;
 
+      final String formatString = toolchain.outReplacementFormatFlag();
+      final CharSequence outReplacement = invocation.outReplacement;
       cmdLine.addLazyString(
-          new OnDemandCommandLineExpansion(
-              toolchain.commandLine(),
-              ImmutableMap.of(
-                  "OUT",
-                  invocation.outReplacement,
-                  "PLUGIN_OUT",
-                  String.format("PLUGIN_%s_out", invocation.name))));
+          new OnDemandString() {
+            @Override
+            public String toString() {
+              return String.format(formatString, outReplacement);
+            }
+          });
 
       if (toolchain.pluginExecutable() != null) {
         cmdLine.addFormatted(

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java

@@ -39,10 +39,19 @@ public ConfiguredTarget create(RuleContext ruleContext)
       providedProtoSources.addTransitive(protoInfo.getTransitiveSources());
     }
 
+    String flag = ruleContext.attributes().get("command_line", Type.STRING);
+
+    if (flag.contains("$(PLUGIN_OUT)")) {
+      ruleContext.attributeError("command_line", "Placeholder '$(PLUGIN_OUT)' is not supported.");
+      return null;
+    }
+
+    flag = flag.replace("$(OUT)", "%s");
+
     return new RuleConfiguredTargetBuilder(ruleContext)
         .addProvider(
             ProtoLangToolchainProvider.create(
-                ruleContext.attributes().get("command_line", Type.STRING),
+                flag,
                 ruleContext.getPrerequisite("plugin", FilesToRunProvider.class),
                 ruleContext.getPrerequisite("runtime"),
                 // We intentionally flatten the NestedSet here.

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainProvider.java

@@ -34,7 +34,7 @@
 @AutoValue
 @AutoCodec
 public abstract class ProtoLangToolchainProvider implements TransitiveInfoProvider {
-  public abstract String commandLine();
+  public abstract String outReplacementFormatFlag();
 
   @Nullable
   public abstract FilesToRunProvider pluginExecutable();
@@ -63,17 +63,21 @@ public NestedSet<Artifact> blacklistedProtos() {
 
   @AutoCodec.Instantiator
   public static ProtoLangToolchainProvider createForDeserialization(
-      String commandLine,
+      String outReplacementFormatFlag,
       FilesToRunProvider pluginExecutable,
       TransitiveInfoCollection runtime,
       ImmutableList<ProtoSource> providedProtoSources,
       NestedSet<Artifact> blacklistedProtos) {
     return new AutoValue_ProtoLangToolchainProvider(
-        commandLine, pluginExecutable, runtime, providedProtoSources, blacklistedProtos);
+        outReplacementFormatFlag,
+        pluginExecutable,
+        runtime,
+        providedProtoSources,
+        blacklistedProtos);
   }
 
   public static ProtoLangToolchainProvider create(
-      String commandLine,
+      String outReplacementFormatFlag,
       FilesToRunProvider pluginExecutable,
       TransitiveInfoCollection runtime,
       ImmutableList<ProtoSource> providedProtoSources) {
@@ -82,6 +86,10 @@ public static ProtoLangToolchainProvider create(
       blacklistedProtos.add(protoSource.getOriginalSourceFile());
     }
     return new AutoValue_ProtoLangToolchainProvider(
-        commandLine, pluginExecutable, runtime, providedProtoSources, blacklistedProtos.build());
+        outReplacementFormatFlag,
+        pluginExecutable,
+        runtime,
+        providedProtoSources,
+        blacklistedProtos.build());
   }
 }

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainRule.java

@@ -39,8 +39,6 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
           <li><code>$(OUT)</code> is LANG_proto_library-specific. The rules are expected to define
               how they interpret this variable. For Java, for example, $(OUT) will be replaced with
               the src-jar filename to create.</li>
-          <li><code>$(PLUGIN_out)</code> will be substituted to work with a
-              `--plugin=protoc-gen-PLUGIN` command line.</li>
         </ul>
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("command_line", Type.STRING).mandatory())
@@ -122,7 +120,7 @@ Specifies how a LANG_proto_library rule (e.g., <code>java_proto_library</code>)
 <pre class="code">
 proto_lang_toolchain(
     name = "javalite_toolchain",
-    command_line = "--$(PLUGIN_OUT)=shared,immutable:$(OUT)",
+    command_line = "--javalite_out=shared,immutable:$(OUT)",
     plugin = ":javalite_plugin",
     runtime = ":protobuf_lite",
 )

src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java

@@ -104,14 +104,14 @@ public void commandLine_basic() throws Exception {
 
     ProtoLangToolchainProvider toolchainNoPlugin =
         ProtoLangToolchainProvider.create(
-            "--java_out=param1,param2:$(OUT)",
+            "--java_out=param1,param2:%s",
             /* pluginExecutable= */ null,
             /* runtime= */ mock(TransitiveInfoCollection.class),
             /* providedProtoSources= */ ImmutableList.of());
 
     ProtoLangToolchainProvider toolchainWithPlugin =
         ProtoLangToolchainProvider.create(
-            "--$(PLUGIN_OUT)=param3,param4:$(OUT)",
+            "--PLUGIN_pluginName_out=param3,param4:%s",
             plugin,
             /* runtime= */ mock(TransitiveInfoCollection.class),
             /* providedProtoSources= */ ImmutableList.of());
@@ -174,7 +174,7 @@ public void commandline_derivedArtifact() throws Exception {
   public void commandLine_strictDeps() throws Exception {
     ProtoLangToolchainProvider toolchain =
         ProtoLangToolchainProvider.create(
-            "--java_out=param1,param2:$(OUT)",
+            "--java_out=param1,param2:%s",
             /* pluginExecutable= */ null,
             /* runtime= */ mock(TransitiveInfoCollection.class),
             /* providedProtoSources= */ ImmutableList.of());
@@ -212,7 +212,7 @@ public void commandLine_strictDeps() throws Exception {
   public void commandLine_exports() throws Exception {
     ProtoLangToolchainProvider toolchain =
         ProtoLangToolchainProvider.create(
-            "--java_out=param1,param2:$(OUT)",
+            "--java_out=param1,param2:%s",
             /* pluginExecutable= */ null,
             /* runtime= */ mock(TransitiveInfoCollection.class),
             /* providedProtoSources= */ ImmutableList.of());
@@ -282,7 +282,7 @@ public String toString() {
 
     ProtoLangToolchainProvider toolchain =
         ProtoLangToolchainProvider.create(
-            "--java_out=param1,param2:$(OUT)",
+            "--java_out=param1,param2:%s",
             /* pluginExecutable= */ null,
             /* runtime= */ mock(TransitiveInfoCollection.class),
             /* providedProtoSources= */ ImmutableList.of());

src/test/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainTest.java

@@ -40,7 +40,7 @@ public void setUp() throws Exception {
   }
 
   private void validateProtoLangToolchain(ProtoLangToolchainProvider toolchain) throws Exception {
-    assertThat(toolchain.commandLine()).isEqualTo("cmd-line");
+    assertThat(toolchain.outReplacementFormatFlag()).isEqualTo("cmd-line");
     assertThat(toolchain.pluginExecutable().getExecutable().getRootRelativePathString())
         .isEqualTo("third_party/x/plugin");