diff options
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib')
5 files changed, 163 insertions, 32 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatter.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatter.java new file mode 100644 index 0000000000..462fe8278c --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatter.java @@ -0,0 +1,102 @@ +// Copyright 2018 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.actions; + +/** + * Implementation of a formatter that supports only a single '%s' + * + * <p>This implementation is used in command line item expansions that use formatting. We use a + * custom implementation to improve performance and avoid GC. + */ +public class CommandLineItemSimpleFormatter { + + /** + * Returns true if the format string contains a single '%s'. + * + * <p>%% escape sequences are supported. + */ + public static boolean isValid(String formatStr) { + return onlyOccurrence(formatStr, formatStr.length()) != -1; + } + + /** + * Returns the equivalent result of <code>String.format(formatStr, subject)</code>, under the + * assumption that the format string contains a single %s. + * + * <p>Use {@link #isValid} to validate the format string. + * + * @throws IllegalArgumentException if the format string is invalid. + */ + public static String format(String formatStr, String subject) { + int n = formatStr.length(); + int idx = onlyOccurrence(formatStr, n); + if (idx < 0) { + throw new IllegalArgumentException( + "Expected format string with single '%s', found: " + formatStr); + } + return new StringBuilder(n + subject.length() - 2) + .append(formatStr, 0, idx) + .append(subject) + .append(formatStr, idx + 2, n) + .toString(); + } + + /* + * Returns the index of the only occurrence of %s. Skips any %%. Returns -1 if {@code formatStr} + * is malformed. + */ + private static int onlyOccurrence(String formatStr, int n) { + int idx = nextOccurrence(formatStr, n, 0); + if (idx < 0) { + return -1; + } + // Only one occurrence please + if (nextOccurrence(formatStr, n, idx + 2) != -1) { + return -1; + } + return idx; + } + + /** + * Returns next occurrence of %s. Skips any %%. + * + * @return + * <li>[0-n]: Index of next %s + * <li>-1: No %s found until end of string + * <li>-2: Illegal sequence found, eg. %f + */ + private static int nextOccurrence(String formatStr, int n, int idx) { + while (idx < n) { + idx = formatStr.indexOf('%', idx); + if (idx == -1) { + break; + } + if ((idx + 1) < n) { + char c = formatStr.charAt(idx + 1); + if (c == 's') { + return idx; + } + if (c != '%') { + // Illegal sequence found + return -2; + } + idx += 2; + } else { + // Terminating '%' found, illegal + return -2; + } + } + return -1; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java index 1fa50a6628..6f04a0eb6f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineItem; +import com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.concurrent.BlazeInterners; @@ -239,7 +240,7 @@ public final class CustomCommandLine extends CommandLine { return new Builder().each(values); } - /** Each argument is formatted via {@link String#format}. */ + /** Each argument is formatted via {@link CommandLineItemSimpleFormatter#format}. */ public static Builder format(@CompileTimeConstant String formatEach) { return new Builder().format(formatEach); } @@ -259,7 +260,7 @@ public final class CustomCommandLine extends CommandLine { private String beforeEach; private String joinWith; - /** Each argument is formatted via {@link String#format}. */ + /** Each argument is formatted via {@link CommandLineItemSimpleFormatter#format}. */ public Builder format(@CompileTimeConstant String formatEach) { Preconditions.checkNotNull(formatEach); this.formatEach = formatEach; @@ -402,7 +403,8 @@ public final class CustomCommandLine extends CommandLine { if (hasFormatEach) { String formatStr = (String) arguments.get(argi++); for (int i = 0; i < count; ++i) { - mutatedValues.set(i, String.format(formatStr, mutatedValues.get(i))); + mutatedValues.set( + i, CommandLineItemSimpleFormatter.format(formatStr, mutatedValues.get(i))); } } if (hasBeforeEach) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index 9dbe7be02a..8f9e18aa97 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -39,6 +39,7 @@ import com.google.devtools.build.lib.actions.BaseSpawn; import com.google.devtools.build.lib.actions.CommandAction; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; +import com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter; import com.google.devtools.build.lib.actions.CompositeRunfilesSupplier; import com.google.devtools.build.lib.actions.EmptyRunfilesSupplier; import com.google.devtools.build.lib.actions.ExecException; @@ -1384,9 +1385,9 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie } else if (value instanceof Artifact) { Artifact paramFile = (Artifact) value; String flagFormatString = (String) values[++i]; - // TODO(bazel-team): Should probably use something that recognizes %% escapes. Possibly - // SkylarkPrinter#formatWithList, though that's kinda heavyweight for this usage. - result.add(flagFormatString.replaceFirst("%s", paramFile.getExecPathString())); + result.add( + CommandLineItemSimpleFormatter.format( + flagFormatString, paramFile.getExecPathString())); } else if (value instanceof CommandLine) { CommandLine commandLine = (CommandLine) value; if (artifactExpander != null) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java index 691a1e23a9..a3878a6300 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.CommandLine; +import com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter; import com.google.devtools.build.lib.actions.ParamFileInfo; import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.actions.RunfilesSupplier; @@ -1144,6 +1145,7 @@ public class SkylarkActionFactory implements SkylarkValue { false /* uniquify */, null /* terminateWith */, loc); + } else { if (mapFn != Runtime.NONE && skylarkSemantics.incompatibleDisallowOldStyleArgsAdd()) { throw new EvalException( @@ -1254,7 +1256,8 @@ public class SkylarkActionFactory implements SkylarkValue { noneable = true, doc = "An optional format string pattern, applied to each string returned by the " - + "<code>map_each</code> function." + + "<code>map_each</code> function. " + + "The format string must have exactly one '%s' placeholder." ), @Param( name = "before_each", @@ -1423,7 +1426,9 @@ public class SkylarkActionFactory implements SkylarkValue { positional = false, defaultValue = "None", noneable = true, - doc = "An optional format string pattern applied to the joined string." + doc = + "An optional format string pattern applied to the joined string. " + + "The format string must have exactly one '%s' placeholder." ), @Param( name = "omit_if_empty", @@ -1511,9 +1516,9 @@ public class SkylarkActionFactory implements SkylarkValue { SkylarkList skylarkList = (SkylarkList) value; vectorArg = new SkylarkCustomCommandLine.VectorArg.Builder(skylarkList); } - if (mapEach != null) { - validateMapEach(mapEach, loc); - } + validateMapEach(mapEach, loc); + validateFormatString("format_each", formatEach); + validateFormatString("format_joined", formatJoined); vectorArg .setLocation(loc) .setArgName(argName) @@ -1549,7 +1554,11 @@ public class SkylarkActionFactory implements SkylarkValue { } } - private void validateMapEach(BaseFunction mapEach, Location loc) throws EvalException { + private void validateMapEach(@Nullable BaseFunction mapEach, Location loc) + throws EvalException { + if (mapEach == null) { + return; + } Shape shape = mapEach.getSignature().getSignature().getShape(); boolean valid = shape.getMandatoryPositionals() == 1 @@ -1562,11 +1571,22 @@ public class SkylarkActionFactory implements SkylarkValue { } } - private void addScalarArg(Object value, String format, BaseFunction mapFn, Location loc) + private void validateFormatString(String argumentName, @Nullable String formatStr) throws EvalException { - if (!EvalUtils.isImmutable(value)) { - throw new EvalException(null, "arg must be an immutable type"); + if (formatStr != null + && skylarkSemantics.incompatibleDisallowOldStyleArgsAdd() + && !CommandLineItemSimpleFormatter.isValid(formatStr)) { + throw new EvalException( + null, + String.format( + "Invalid value for parameter \"%s\": Expected string with a single \"%%s\"", + argumentName)); } + } + + private void addScalarArg(Object value, String format, BaseFunction mapFn, Location loc) + throws EvalException { + validateFormatString("format", format); if (format == null && mapFn == null) { commandLine.add(value); } else { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java index 2512907e18..a67223a98e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java @@ -23,6 +23,7 @@ import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLineItem; +import com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.events.Location; @@ -245,11 +246,11 @@ public class SkylarkCustomCommandLine extends CommandLine { } if ((features & HAS_FORMAT_EACH) != 0) { String formatStr = (String) arguments.get(argi++); - Formatter formatter = new Formatter(formatStr, skylarkSemantics, location); + Formatter formatter = Formatter.get(location, skylarkSemantics); try { int count = stringValues.size(); for (int i = 0; i < count; ++i) { - stringValues.set(i, formatter.format(stringValues.get(i))); + stringValues.set(i, formatter.format(formatStr, stringValues.get(i))); } } catch (IllegalFormatException e) { throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, null)); @@ -269,9 +270,9 @@ public class SkylarkCustomCommandLine extends CommandLine { if (!isEmptyAndShouldOmit) { String result = Joiner.on(joinWith).join(stringValues); if (formatJoined != null) { - Formatter formatter = new Formatter(formatJoined, skylarkSemantics, location); + Formatter formatter = Formatter.get(location, skylarkSemantics); try { - result = formatter.format(result); + result = formatter.format(formatJoined, result); } catch (IllegalFormatException e) { throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, null)); } @@ -538,8 +539,8 @@ public class SkylarkCustomCommandLine extends CommandLine { String stringValue = CommandLineItem.expandToCommandLine(object); if (hasFormat) { String formatStr = (String) arguments.get(argi++); - Formatter formatter = new Formatter(formatStr, skylarkSemantics, location); - stringValue = formatter.format(stringValue); + Formatter formatter = Formatter.get(location, skylarkSemantics); + stringValue = formatter.format(formatStr, stringValue); } builder.add(stringValue); return argi; @@ -696,26 +697,31 @@ public class SkylarkCustomCommandLine extends CommandLine { } } - private static class Formatter { - private final String formatStr; - private final SkylarkSemantics skylarkSemantics; + private interface Formatter { + String format(String formatStr, String subject) throws CommandLineExpansionException; + + static Formatter get(Location location, SkylarkSemantics skylarkSemantics) { + return skylarkSemantics.incompatibleDisallowOldStyleArgsAdd() + ? CommandLineItemSimpleFormatter::format + : new LegacyFormatter(location); + } + } + + private static class LegacyFormatter implements Formatter { @Nullable private final Location location; private final ArrayList<Object> args; - public Formatter(String formatStr, SkylarkSemantics skylarkSemantics, Location location) { - this.formatStr = formatStr; - this.skylarkSemantics = skylarkSemantics; + public LegacyFormatter(Location location) { this.location = location; this.args = new ArrayList<>(1); // Reused arg list to reduce GC this.args.add(null); } - String format(Object object) throws CommandLineExpansionException { + @Override + public String format(String formatStr, String subject) throws CommandLineExpansionException { try { - args.set(0, object); - SkylarkPrinter printer = - skylarkSemantics.incompatibleDisallowOldStyleArgsAdd() - ? Printer.getSimplifiedPrinter() : Printer.getPrinter(); + args.set(0, subject); + SkylarkPrinter printer = Printer.getPrinter(); return printer.formatWithList(formatStr, args).toString(); } catch (IllegalFormatException e) { throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, null)); |