From beafd7ef1245511a74d12962cecfbeddc05e0d46 Mon Sep 17 00:00:00 2001 From: tomlu Date: Thu, 5 Apr 2018 15:03:19 -0700 Subject: Split Args#add into three methods. Args#add(value, *, arg, format) Args#add_all(value, *, arg, map_each, format_each, before_each, omit_if_empty, uniquify) Args#add_joined(value, *, arg, join_with, map_each, format_each, format_joined, omit_if_empty, uniquify) The old Args#add remains backwards compatible, but we add a flag to disable this compatibility mode. RELNOTES: None PiperOrigin-RevId: 191804482 --- .../lib/analysis/skylark/SkylarkActionFactory.java | 398 ++++++++++++++++++--- .../analysis/skylark/SkylarkCustomCommandLine.java | 342 +++++++++++++----- .../build/lib/packages/SkylarkSemanticsCodec.java | 2 + .../lib/packages/SkylarkSemanticsOptions.java | 16 + .../devtools/build/lib/syntax/BaseFunction.java | 18 +- .../google/devtools/build/lib/syntax/Printer.java | 9 + .../build/lib/syntax/SkylarkSemantics.java | 5 + 7 files changed, 644 insertions(+), 146 deletions(-) (limited to 'src/main/java/com/google/devtools') 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 89319e92f5..7a3a79d116 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 @@ -50,6 +50,7 @@ import com.google.devtools.build.lib.syntax.BaseFunction; import com.google.devtools.build.lib.syntax.Environment; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.EvalUtils; +import com.google.devtools.build.lib.syntax.FunctionSignature.Shape; import com.google.devtools.build.lib.syntax.Mutability; import com.google.devtools.build.lib.syntax.Runtime; import com.google.devtools.build.lib.syntax.Runtime.NoneType; @@ -880,6 +881,7 @@ public class SkylarkActionFactory implements SkylarkValue { ) static class Args extends SkylarkMutable { private final Mutability mutability; + private final SkylarkSemantics skylarkSemantics; private final SkylarkCustomCommandLine.Builder commandLine; private ParameterFileType parameterFileType = ParameterFileType.SHELL_QUOTED; private String flagFormatString; @@ -891,13 +893,18 @@ public class SkylarkActionFactory implements SkylarkValue { parameters = { @Param( name = "value", - type = Object.class, + named = true, doc = "The object to add to the argument list. " - + "If the object is scalar, the object's string representation is added. " - + "If it's a list or " - + "depset, " - + "each element's string representation is added." + + "The object's string representation is added. " + ), + @Param( + name = "arg_name", + type = String.class, + named = true, + defaultValue = "None", + noneable = true, + doc = "The argument name as a string to add before the value." ), @Param( name = "format", @@ -909,7 +916,7 @@ public class SkylarkActionFactory implements SkylarkValue { doc = "A format string used to format the object(s). " + "The format string is as per pattern % tuple. " - + "Limitations: only %d %s %r %% are supported." + + "Limitations: only %s and %% are supported." ), @Param( name = "before_each", @@ -920,7 +927,9 @@ public class SkylarkActionFactory implements SkylarkValue { noneable = true, doc = "Each object in the list is prepended by this string. " - + "Only supported for vector arguments." + + "Only supported for vector arguments. " + + "Deprecated. Please use add_all" + + " or add_joined instead." ), @Param( name = "join_with", @@ -931,7 +940,9 @@ public class SkylarkActionFactory implements SkylarkValue { noneable = true, doc = "Each object in the list is joined with this string. " - + "Only supported for vector arguments." + + "Only supported for vector arguments. " + + "Deprecated. Please use add_all" + + " or add_joined instead." ), @Param( name = "map_fn", @@ -943,36 +954,328 @@ public class SkylarkActionFactory implements SkylarkValue { doc = "The passed objects are passed through a map function. " + "For vector args the function is given a list and is expected to " - + "return a list of the same length as the input." + + "return a list of the same length as the input. " + + "Deprecated. Please use add_all" + + " or add_joined instead." ) }, useLocation = true ) public NoneType addArgument( Object value, + Object argName, Object format, Object beforeEach, Object joinWith, Object mapFn, Location loc) throws EvalException { - if (this.isImmutable()) { + if (isImmutable()) { throw new EvalException(null, "cannot modify frozen value"); } + if (argName != Runtime.NONE) { + commandLine.add(argName); + } if (value instanceof SkylarkNestedSet || value instanceof SkylarkList) { - addVectorArg(value, format, beforeEach, joinWith, mapFn, loc); + if (skylarkSemantics.incompatibleDisallowOldStyleArgsAdd()) { + throw new EvalException( + loc, + "Args#add no longer accepts vectorized arguments when " + + "--incompatible_disallow_old_style_args_add is set. " + + "Please use Args#add_all or Args#add_joined."); + } + addVectorArg( + value, + null /* argName */, + mapFn != Runtime.NONE ? (BaseFunction) mapFn : null, + null /* mapEach */, + format != Runtime.NONE ? (String) format : null, + beforeEach != Runtime.NONE ? (String) beforeEach : null, + joinWith != Runtime.NONE ? (String) joinWith : null, + null /* formatJoined */, + false /* omitIfEmpty */, + false /* uniquify */, + null /* terminateWith */, + loc); } else { - addScalarArg(value, format, beforeEach, joinWith, mapFn, loc); + if (mapFn != Runtime.NONE && skylarkSemantics.incompatibleDisallowOldStyleArgsAdd()) { + throw new EvalException( + loc, + "Args#add no longer accepts map_fn when" + + "--incompatible_disallow_old_style_args_add is set. " + + "Please eagerly map the value."); + } + if (beforeEach != Runtime.NONE) { + throw new EvalException(null, "'before_each' is not supported for scalar arguments"); + } + if (joinWith != Runtime.NONE) { + throw new EvalException(null, "'join_with' is not supported for scalar arguments"); + } + addScalarArg( + value, + format != Runtime.NONE ? (String) format : null, + mapFn != Runtime.NONE ? (BaseFunction) mapFn : null, + loc); } return Runtime.NONE; } - private void addVectorArg( - Object value, Object format, Object beforeEach, Object joinWith, Object mapFn, Location loc) + @SkylarkCallable( + name = "add_all", + doc = "Adds an vector argument to be dynamically expanded at evaluation time.", + parameters = { + @Param( + name = "values", + allowedTypes = { + @ParamType(type = SkylarkList.class), + @ParamType(type = SkylarkNestedSet.class), + }, + named = true, + doc = "The sequence to add. Each element's string representation is added." + ), + @Param( + name = "arg_name", + type = String.class, + named = true, + defaultValue = "None", + noneable = true, + doc = "The argument name as a string to add before the values." + ), + @Param( + name = "map_each", + type = BaseFunction.class, + named = true, + positional = false, + defaultValue = "None", + noneable = true, + doc = + "Each object is passed through a map function " + + "prior to formatting and joining. " + + "The function is given a single element and is expected to return a string, " + + "a list of strings, or None (in which case the return value is ignored)." + ), + @Param( + name = "format_each", + type = String.class, + named = true, + positional = false, + defaultValue = "None", + noneable = true, + doc = + "A format string used to format each object in the list. " + + "The format string is as per pattern % tuple. " + + "Limitations: only %s and %% are supported. " + ), + @Param( + name = "before_each", + type = String.class, + named = true, + positional = false, + defaultValue = "None", + noneable = true, + doc = + "Each object in the list is prepended by this string. " + + "This happens after any mapping and formatting." + ), + @Param( + name = "omit_if_empty", + type = Boolean.class, + named = true, + positional = false, + defaultValue = "True", + doc = + "Omits the arg_name and terminate_with (if passed) if the values end up empty, " + + "either because empty values was passed, " + + "or because map_each filtered out the values." + ), + @Param( + name = "uniquify", + type = Boolean.class, + named = true, + positional = false, + defaultValue = "False", + doc = + "Omits non-unique values. Order is preserved. " + + "This is usually not needed as depsets already omit duplicates." + ), + @Param( + name = "terminate_with", + type = String.class, + named = true, + positional = false, + defaultValue = "None", + noneable = true, + doc = "Adds a terminating argument last, after all other arguments have been added." + ), + }, + useLocation = true + ) + public NoneType addAll( + Object value, + Object argName, + Object mapEach, + Object formatEach, + Object beforeEach, + Boolean omitIfEmpty, + Boolean uniquify, + Object terminateWith, + Location loc) + throws EvalException { + if (isImmutable()) { + throw new EvalException(null, "cannot modify frozen value"); + } + addVectorArg( + value, + argName != Runtime.NONE ? (String) argName : null, + null /* mapAll */, + mapEach != Runtime.NONE ? (BaseFunction) mapEach : null, + formatEach != Runtime.NONE ? (String) formatEach : null, + beforeEach != Runtime.NONE ? (String) beforeEach : null, + null /* joinWith */, + null /* formatJoined */, + omitIfEmpty, + uniquify, + terminateWith != Runtime.NONE ? (String) terminateWith : null, + loc); + return Runtime.NONE; + } + + @SkylarkCallable( + name = "add_joined", + doc = + "Adds a vector argument to be dynamically expanded at evaluation time " + + "and joined with a string.", + parameters = { + @Param( + name = "values", + allowedTypes = { + @ParamType(type = SkylarkList.class), + @ParamType(type = SkylarkNestedSet.class), + }, + named = true, + doc = "The sequence to add. Each element's string representation is joined and added." + ), + @Param( + name = "arg_name", + type = String.class, + named = true, + defaultValue = "None", + noneable = true, + doc = "The argument name as a string to add before the values." + ), + @Param( + name = "join_with", + type = String.class, + named = true, + positional = false, + doc = "Each object in the supplied list is joined with this string. " + ), + @Param( + name = "map_each", + type = BaseFunction.class, + named = true, + positional = false, + defaultValue = "None", + noneable = true, + doc = + "Each object is passed through a map function " + + "prior to formatting and joining. " + + "The function is given a single element and is expected to return a string, " + + "a list of strings, or None (in which case the return value is ignored)." + ), + @Param( + name = "format_each", + type = String.class, + named = true, + positional = false, + defaultValue = "None", + noneable = true, + doc = + "A format string used to format each object in the list prior to joining. " + + "The format string is as per pattern % tuple. " + + "Limitations: only %s and %% are supported. " + + "Only supported for vector arguments." + ), + @Param( + name = "format_joined", + type = String.class, + named = true, + positional = false, + defaultValue = "None", + noneable = true, + doc = + "A format string used to format the final joined string. " + + "The format string is as per pattern % tuple. " + + "Limitations: only %s and %% are supported." + ), + @Param( + name = "omit_if_empty", + type = Boolean.class, + named = true, + positional = false, + defaultValue = "True", + doc = + "Omits the joined value when the passed objects are empty. " + + "In case 'arg_name' was passed it is also omitted. " + + "If false, the joined string is always added, even if it is the empty string." + ), + @Param( + name = "uniquify", + type = Boolean.class, + named = true, + positional = false, + defaultValue = "False", + doc = + "Omits non-unique values. Order is preserved. " + + "This is usually not needed as depsets already omit duplicates." + ) + }, + useLocation = true + ) + public NoneType addJoined( + Object value, + Object argName, + String joinWith, + Object mapEach, + Object formatEach, + Object formatJoined, + Boolean omitIfEmpty, + Boolean uniquify, + Location loc) throws EvalException { - if (beforeEach != Runtime.NONE && joinWith != Runtime.NONE) { - throw new EvalException(null, "cannot pass both 'before_each' and 'join_with'"); + if (isImmutable()) { + throw new EvalException(null, "cannot modify frozen value"); } + addVectorArg( + value, + argName != Runtime.NONE ? (String) argName : null, + null /* mapAll */, + mapEach != Runtime.NONE ? (BaseFunction) mapEach : null, + formatEach != Runtime.NONE ? (String) formatEach : null, + null /* beforeEach */, + joinWith, + formatJoined != Runtime.NONE ? (String) formatJoined : null, + omitIfEmpty, + uniquify, + null /* terminateWith */, + loc); + return Runtime.NONE; + } + + private void addVectorArg( + Object value, + String argName, + BaseFunction mapAll, + BaseFunction mapEach, + String formatEach, + String beforeEach, + String joinWith, + String formatJoined, + boolean omitIfEmpty, + boolean uniquify, + String terminateWith, + Location loc) + throws EvalException { SkylarkCustomCommandLine.VectorArg.Builder vectorArg; if (value instanceof SkylarkNestedSet) { NestedSet nestedSet = ((SkylarkNestedSet) value).getSet(Object.class); @@ -981,45 +1284,47 @@ public class SkylarkActionFactory implements SkylarkValue { SkylarkList skylarkList = (SkylarkList) value; vectorArg = new SkylarkCustomCommandLine.VectorArg.Builder(skylarkList); } - vectorArg.setLocation(loc); - if (format != Runtime.NONE) { - vectorArg.setFormat((String) format); - } - if (beforeEach != Runtime.NONE) { - vectorArg.setBeforeEach((String) beforeEach); - } - if (joinWith != Runtime.NONE) { - vectorArg.setJoinWith((String) joinWith); - } - if (mapFn != Runtime.NONE) { - vectorArg.setMapFn((BaseFunction) mapFn); + if (mapEach != null) { + validateMapEach(mapEach, loc); } + vectorArg + .setLocation(loc) + .setArgName(argName) + .setMapAll(mapAll) + .setFormatEach(formatEach) + .setBeforeEach(beforeEach) + .setJoinWith(joinWith) + .setFormatJoined(formatJoined) + .omitIfEmpty(omitIfEmpty) + .uniquify(uniquify) + .setTerminateWith(terminateWith) + .setMapEach(mapEach); commandLine.add(vectorArg); } - private void addScalarArg( - Object value, Object format, Object beforeEach, Object joinWith, Object mapFn, Location loc) + private void validateMapEach(BaseFunction mapEach, Location loc) throws EvalException { + Shape shape = mapEach.getSignature().getSignature().getShape(); + boolean valid = + shape.getMandatoryPositionals() == 1 + && shape.getOptionalPositionals() == 0 + && shape.getMandatoryNamedOnly() == 0 + && shape.getOptionalPositionals() == 0; + if (!valid) { + throw new EvalException( + loc, "map_each must be a function that accepts a single positional argument"); + } + } + + private void addScalarArg(Object value, String format, BaseFunction mapFn, Location loc) throws EvalException { if (!EvalUtils.isImmutable(value)) { throw new EvalException(null, "arg must be an immutable type"); } - if (beforeEach != Runtime.NONE) { - throw new EvalException(null, "'before_each' is not supported for scalar arguments"); - } - if (joinWith != Runtime.NONE) { - throw new EvalException(null, "'join_with' is not supported for scalar arguments"); - } - if (format == Runtime.NONE && mapFn == Runtime.NONE) { + if (format == null && mapFn == null) { commandLine.add(value); } else { - ScalarArg.Builder scalarArg = new ScalarArg.Builder(value); - scalarArg.setLocation(loc); - if (format != Runtime.NONE) { - scalarArg.setFormat((String) format); - } - if (mapFn != Runtime.NONE) { - scalarArg.setMapFn((BaseFunction) mapFn); - } + ScalarArg.Builder scalarArg = + new ScalarArg.Builder(value).setLocation(loc).setFormat(format).setMapFn(mapFn); commandLine.add(scalarArg); } } @@ -1104,6 +1409,7 @@ public class SkylarkActionFactory implements SkylarkValue { private Args(@Nullable Mutability mutability, SkylarkSemantics skylarkSemantics) { this.mutability = mutability != null ? mutability : Mutability.IMMUTABLE; + this.skylarkSemantics = skylarkSemantics; this.commandLine = new SkylarkCustomCommandLine.Builder(skylarkSemantics); } @@ -1128,7 +1434,7 @@ public class SkylarkActionFactory implements SkylarkValue { useEnvironment = true ) public Args args(Environment env) { - return new Args(env.mutability(), env.getSemantics()); + return new Args(env.mutability(), skylarkSemantics); } @Override 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 ded956388b..3fbb5dbaa8 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 @@ -15,10 +15,10 @@ package com.google.devtools.build.lib.analysis.skylark; import com.google.common.base.Joiner; import com.google.common.base.Objects; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Interner; -import com.google.common.collect.Lists; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLineItem; @@ -32,9 +32,11 @@ import com.google.devtools.build.lib.syntax.Environment; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.Mutability; import com.google.devtools.build.lib.syntax.Printer; +import com.google.devtools.build.lib.syntax.Runtime; import com.google.devtools.build.lib.syntax.SkylarkList; import com.google.devtools.build.lib.syntax.SkylarkSemantics; import java.util.ArrayList; +import java.util.HashSet; import java.util.IllegalFormatException; import java.util.List; import javax.annotation.Nullable; @@ -52,54 +54,55 @@ class SkylarkCustomCommandLine extends CommandLine { static final class VectorArg { private static Interner interner = BlazeInterners.newStrongInterner(); - private final boolean isNestedSet; - private final boolean hasFormat; - private final boolean hasBeforeEach; - private final boolean hasJoinWith; - private final boolean hasMapFn; - private final boolean hasLocation; - - private VectorArg( - boolean isNestedSet, - boolean hasFormat, - boolean hasBeforeEach, - boolean hasJoinWith, - boolean hasMapFn, - boolean hasLocation) { - this.isNestedSet = isNestedSet; - this.hasFormat = hasFormat; - this.hasBeforeEach = hasBeforeEach; - this.hasJoinWith = hasJoinWith; - this.hasMapFn = hasMapFn; - this.hasLocation = hasLocation; + private static final int IS_NESTED_SET = 1; + private static final int HAS_LOCATION = 1 << 1; + private static final int HAS_ARG_NAME = 1 << 2; + private static final int HAS_MAP_ALL = 1 << 3; + private static final int HAS_MAP_EACH = 1 << 4; + private static final int HAS_FORMAT_EACH = 1 << 5; + private static final int HAS_BEFORE_EACH = 1 << 6; + private static final int HAS_JOIN_WITH = 1 << 7; + private static final int HAS_FORMAT_JOINED = 1 << 8; + private static final int OMIT_IF_EMPTY = 1 << 9; + private static final int UNIQUIFY = 1 << 10; + private static final int HAS_TERMINATE_WITH = 1 << 11; + + private final int features; + + private VectorArg(int features) { + this.features = features; } @AutoCodec.VisibleForSerialization @AutoCodec.Instantiator - static VectorArg create( - boolean isNestedSet, - boolean hasFormat, - boolean hasBeforeEach, - boolean hasJoinWith, - boolean hasMapFn, - boolean hasLocation) { - return interner.intern( - new VectorArg(isNestedSet, hasFormat, hasBeforeEach, hasJoinWith, hasMapFn, hasLocation)); + static VectorArg create(int features) { + return interner.intern(new VectorArg(features)); } private static void push(ImmutableList.Builder arguments, Builder arg) { - boolean wantsLocation = arg.format != null || arg.mapFn != null; - boolean hasLocation = arg.location != null && wantsLocation; - VectorArg vectorArg = - VectorArg.create( - arg.nestedSet != null, - arg.format != null, - arg.beforeEach != null, - arg.joinWith != null, - arg.mapFn != null, - hasLocation); + int features = 0; + features |= arg.nestedSet != null ? IS_NESTED_SET : 0; + features |= arg.argName != null ? HAS_ARG_NAME : 0; + features |= arg.mapAll != null ? HAS_MAP_ALL : 0; + features |= arg.mapEach != null ? HAS_MAP_EACH : 0; + features |= arg.formatEach != null ? HAS_FORMAT_EACH : 0; + features |= arg.beforeEach != null ? HAS_BEFORE_EACH : 0; + features |= arg.joinWith != null ? HAS_JOIN_WITH : 0; + features |= arg.formatJoined != null ? HAS_FORMAT_JOINED : 0; + features |= arg.omitIfEmpty ? OMIT_IF_EMPTY : 0; + features |= arg.uniquify ? UNIQUIFY : 0; + features |= arg.terminateWith != null ? HAS_TERMINATE_WITH : 0; + boolean hasLocation = + arg.location != null + && (features & (HAS_FORMAT_EACH | HAS_FORMAT_JOINED | HAS_MAP_ALL | HAS_MAP_EACH)) + != 0; + features |= hasLocation ? HAS_LOCATION : 0; + Preconditions.checkState( + (features & (HAS_MAP_ALL | HAS_MAP_EACH)) != (HAS_MAP_ALL | HAS_MAP_EACH), + "Cannot use both map_all and map_each"); + VectorArg vectorArg = VectorArg.create(features); arguments.add(vectorArg); - if (vectorArg.isNestedSet) { + if (arg.nestedSet != null) { arguments.add(arg.nestedSet); } else { ImmutableList list = arg.list.getImmutableList(); @@ -112,18 +115,30 @@ class SkylarkCustomCommandLine extends CommandLine { if (hasLocation) { arguments.add(arg.location); } - if (vectorArg.hasMapFn) { - arguments.add(arg.mapFn); + if (arg.argName != null) { + arguments.add(arg.argName); } - if (vectorArg.hasFormat) { - arguments.add(arg.format); + if (arg.mapAll != null) { + arguments.add(arg.mapAll); } - if (vectorArg.hasBeforeEach) { + if (arg.mapEach != null) { + arguments.add(arg.mapEach); + } + if (arg.formatEach != null) { + arguments.add(arg.formatEach); + } + if (arg.beforeEach != null) { arguments.add(arg.beforeEach); } - if (vectorArg.hasJoinWith) { + if (arg.joinWith != null) { arguments.add(arg.joinWith); } + if (arg.formatJoined != null) { + arguments.add(arg.formatJoined); + } + if (arg.terminateWith != null) { + arguments.add(arg.terminateWith); + } } private int eval( @@ -132,23 +147,26 @@ class SkylarkCustomCommandLine extends CommandLine { ImmutableList.Builder builder, SkylarkSemantics skylarkSemantics) throws CommandLineExpansionException { - final List mutatedValues; - final int count; - if (isNestedSet) { - NestedSet nestedSet = (NestedSet) arguments.get(argi++); - mutatedValues = Lists.newArrayList(nestedSet); - count = mutatedValues.size(); + final List originalValues; + if ((features & IS_NESTED_SET) != 0) { + NestedSet nestedSet = (NestedSet) arguments.get(argi++); + originalValues = nestedSet.toList(); } else { - count = (Integer) arguments.get(argi++); - mutatedValues = new ArrayList<>(count); - for (int i = 0; i < count; ++i) { - mutatedValues.add(arguments.get(argi++)); - } - } - final Location location = hasLocation ? (Location) arguments.get(argi++) : null; - if (hasMapFn) { + int count = (Integer) arguments.get(argi++); + originalValues = arguments.subList(argi, argi + count); + argi += count; + } + List stringValues; + final Location location = + ((features & HAS_LOCATION) != 0) ? (Location) arguments.get(argi++) : null; + final String argName = + ((features & HAS_ARG_NAME) != 0) ? (String) arguments.get(argi++) : null; + if ((features & HAS_MAP_EACH) != 0) { + BaseFunction mapEach = (BaseFunction) arguments.get(argi++); + stringValues = applyMapEach(mapEach, originalValues, location, skylarkSemantics); + } else if ((features & HAS_MAP_ALL) != 0) { BaseFunction mapFn = (BaseFunction) arguments.get(argi++); - Object result = applyMapFn(mapFn, mutatedValues, location, skylarkSemantics); + Object result = applyMapFn(mapFn, originalValues, location, skylarkSemantics); if (!(result instanceof List)) { throw new CommandLineExpansionException( errorMessage( @@ -157,45 +175,90 @@ class SkylarkCustomCommandLine extends CommandLine { null)); } List resultAsList = (List) result; - if (resultAsList.size() != count) { + if (resultAsList.size() != originalValues.size()) { throw new CommandLineExpansionException( errorMessage( String.format( "map_fn must return a list of the same length as the input. " + "Found list of length %d, expected %d.", - resultAsList.size(), count), + resultAsList.size(), originalValues.size()), location, null)); } - mutatedValues.clear(); - mutatedValues.addAll(resultAsList); + int count = resultAsList.size(); + stringValues = new ArrayList<>(count); + // map_fn contract doesn't guarantee that the values returned are strings, + // so convert here + for (int i = 0; i < count; ++i) { + stringValues.add(CommandLineItem.expandToCommandLine(resultAsList.get(i))); + } + } else { + int count = originalValues.size(); + stringValues = new ArrayList<>(originalValues.size()); + for (int i = 0; i < count; ++i) { + stringValues.add(CommandLineItem.expandToCommandLine(originalValues.get(i))); + } } - for (int i = 0; i < count; ++i) { - mutatedValues.set(i, CommandLineItem.expandToCommandLine(mutatedValues.get(i))); + // It's safe to uniquify at this stage, any transformations after this + // will ensure continued uniqueness of the values + if ((features & UNIQUIFY) != 0) { + HashSet seen = new HashSet<>(stringValues.size()); + int count = stringValues.size(); + int addIndex = 0; + for (int i = 0; i < count; ++i) { + String val = stringValues.get(i); + if (seen.add(val)) { + stringValues.set(addIndex++, val); + } + } + stringValues = stringValues.subList(0, addIndex); } - if (hasFormat) { + boolean isEmptyAndShouldOmit = stringValues.isEmpty() && (features & OMIT_IF_EMPTY) != 0; + if (argName != null && !isEmptyAndShouldOmit) { + builder.add(argName); + } + if ((features & HAS_FORMAT_EACH) != 0) { String formatStr = (String) arguments.get(argi++); - Formatter formatter = new Formatter(formatStr, location); + Formatter formatter = new Formatter(formatStr, skylarkSemantics, location); try { + int count = stringValues.size(); for (int i = 0; i < count; ++i) { - mutatedValues.set(i, formatter.format(mutatedValues.get(i))); + stringValues.set(i, formatter.format(stringValues.get(i))); } } catch (IllegalFormatException e) { throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, null)); } } - if (hasBeforeEach) { + if ((features & HAS_BEFORE_EACH) != 0) { String beforeEach = (String) arguments.get(argi++); + int count = stringValues.size(); for (int i = 0; i < count; ++i) { builder.add(beforeEach); - builder.add((String) mutatedValues.get(i)); + builder.add(stringValues.get(i)); } - } else if (hasJoinWith) { + } else if ((features & HAS_JOIN_WITH) != 0) { String joinWith = (String) arguments.get(argi++); - builder.add(Joiner.on(joinWith).join(mutatedValues)); + String formatJoined = + ((features & HAS_FORMAT_JOINED) != 0) ? (String) arguments.get(argi++) : null; + if (!isEmptyAndShouldOmit) { + String result = Joiner.on(joinWith).join(stringValues); + if (formatJoined != null) { + Formatter formatter = new Formatter(formatJoined, skylarkSemantics, location); + try { + result = formatter.format(result); + } catch (IllegalFormatException e) { + throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, null)); + } + } + builder.add(result); + } } else { - for (int i = 0; i < count; ++i) { - builder.add((String) mutatedValues.get(i)); + builder.addAll(stringValues); + } + if ((features & HAS_TERMINATE_WITH) != 0) { + String terminateWith = (String) arguments.get(argi++); + if (!isEmptyAndShouldOmit) { + builder.add(terminateWith); } } return argi; @@ -204,18 +267,24 @@ class SkylarkCustomCommandLine extends CommandLine { static class Builder { @Nullable private final SkylarkList list; @Nullable private final NestedSet nestedSet; - private String format; + private Location location; + public String argName; + private BaseFunction mapAll; + private BaseFunction mapEach; + private String formatEach; private String beforeEach; private String joinWith; - private Location location; - private BaseFunction mapFn; + private String formatJoined; + private boolean omitIfEmpty; + private boolean uniquify; + private String terminateWith; - public Builder(SkylarkList list) { + Builder(SkylarkList list) { this.list = list; this.nestedSet = null; } - public Builder(NestedSet nestedSet) { + Builder(NestedSet nestedSet) { this.list = null; this.nestedSet = nestedSet; } @@ -225,8 +294,23 @@ class SkylarkCustomCommandLine extends CommandLine { return this; } - Builder setFormat(String format) { - this.format = format; + Builder setArgName(String argName) { + this.argName = argName; + return this; + } + + Builder setMapAll(BaseFunction mapAll) { + this.mapAll = mapAll; + return this; + } + + Builder setMapEach(BaseFunction mapEach) { + this.mapEach = mapEach; + return this; + } + + Builder setFormatEach(String format) { + this.formatEach = format; return this; } @@ -235,13 +319,28 @@ class SkylarkCustomCommandLine extends CommandLine { return this; } - public Builder setJoinWith(String joinWith) { + Builder setJoinWith(String joinWith) { this.joinWith = joinWith; return this; } - public Builder setMapFn(BaseFunction mapFn) { - this.mapFn = mapFn; + Builder setFormatJoined(String formatJoined) { + this.formatJoined = formatJoined; + return this; + } + + Builder omitIfEmpty(boolean omitIfEmpty) { + this.omitIfEmpty = omitIfEmpty; + return this; + } + + Builder uniquify(boolean uniquify) { + this.uniquify = uniquify; + return this; + } + + Builder setTerminateWith(String terminateWith) { + this.terminateWith = terminateWith; return this; } } @@ -255,18 +354,12 @@ class SkylarkCustomCommandLine extends CommandLine { return false; } VectorArg vectorArg = (VectorArg) o; - return isNestedSet == vectorArg.isNestedSet - && hasFormat == vectorArg.hasFormat - && hasBeforeEach == vectorArg.hasBeforeEach - && hasJoinWith == vectorArg.hasJoinWith - && hasMapFn == vectorArg.hasMapFn - && hasLocation == vectorArg.hasLocation; + return features == vectorArg.features; } @Override public int hashCode() { - return Objects.hashCode( - isNestedSet, hasFormat, hasBeforeEach, hasJoinWith, hasMapFn, hasLocation); + return Objects.hashCode(features); } } @@ -322,7 +415,7 @@ class SkylarkCustomCommandLine extends CommandLine { object = CommandLineItem.expandToCommandLine(object); if (hasFormat) { String formatStr = (String) arguments.get(argi++); - Formatter formatter = new Formatter(formatStr, location); + Formatter formatter = new Formatter(formatStr, skylarkSemantics, location); object = formatter.format(object); } builder.add((String) object); @@ -349,7 +442,7 @@ class SkylarkCustomCommandLine extends CommandLine { return this; } - public Builder setMapFn(BaseFunction mapFn) { + Builder setMapFn(BaseFunction mapFn) { this.mapFn = mapFn; return this; } @@ -425,11 +518,13 @@ class SkylarkCustomCommandLine extends CommandLine { private static class Formatter { private final String formatStr; + private final SkylarkSemantics skylarkSemantics; @Nullable private final Location location; private final ArrayList args; - public Formatter(String formatStr, Location location) { + public Formatter(String formatStr, SkylarkSemantics skylarkSemantics, Location location) { this.formatStr = formatStr; + this.skylarkSemantics = skylarkSemantics; this.location = location; this.args = new ArrayList<>(1); // Reused arg list to reduce GC this.args.add(null); @@ -438,7 +533,9 @@ class SkylarkCustomCommandLine extends CommandLine { String format(Object object) throws CommandLineExpansionException { try { args.set(0, object); - return Printer.getPrinter().formatWithList(formatStr, args).toString(); + return Printer.getPrinter(skylarkSemantics.incompatibleDisallowOldStyleArgsAdd()) + .formatWithList(formatStr, args) + .toString(); } catch (IllegalFormatException e) { throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, null)); } @@ -465,6 +562,53 @@ class SkylarkCustomCommandLine extends CommandLine { } } + private static ArrayList applyMapEach( + BaseFunction mapFn, + List originalValues, + Location location, + SkylarkSemantics skylarkSemantics) + throws CommandLineExpansionException { + try (Mutability mutability = Mutability.create("map_each")) { + Environment env = + Environment.builder(mutability) + .setSemantics(skylarkSemantics) + // TODO(b/77140311): Error if we issue print statements + .setEventHandler(NullEventHandler.INSTANCE) + .build(); + Object[] args = new Object[1]; + int count = originalValues.size(); + ArrayList stringValues = new ArrayList<>(count); + for (int i = 0; i < count; ++i) { + args[0] = originalValues.get(i); + Object ret = mapFn.callWithArgArray(args, null, env, location); + if (ret instanceof String) { + stringValues.add((String) ret); + } else if (ret instanceof SkylarkList) { + for (Object val : ((SkylarkList) ret)) { + if (!(val instanceof String)) { + throw new CommandLineExpansionException( + "Expected map_each to return string, None, or list of strings, " + + "found list containing " + + val.getClass().getSimpleName()); + } + stringValues.add((String) val); + } + } else if (ret != Runtime.NONE) { + throw new CommandLineExpansionException( + "Expected map_each to return string, None, or list of strings, found " + + ret.getClass().getSimpleName()); + } + } + return stringValues; + } catch (EvalException e) { + throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, e.getCause())); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new CommandLineExpansionException( + errorMessage("Thread was interrupted", location, null)); + } + } + private static String errorMessage( String message, @Nullable Location location, @Nullable Throwable cause) { return LINE_JOINER.join( diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java index 21d5c49aad..43975c1f56 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java @@ -49,6 +49,7 @@ public final class SkylarkSemanticsCodec implements ObjectCodec names = signature.getSignature().getNames(); int length = types.size(); for (int i = 0; i < length; i++) { Object value = arguments[i]; SkylarkType type = types.get(i); if (value != null && type != null && !type.contains(value)) { + List names = signature.getSignature().getNames(); throw new EvalException(loc, String.format("expected %s for '%s' while calling %s but got %s instead: %s", type, names.get(i), getName(), EvalUtils.getDataTypeName(value, true), value)); @@ -437,6 +437,22 @@ public abstract class BaseFunction implements SkylarkValue { Location loc = ast == null ? Location.BUILTIN : ast.getLocation(); Object[] arguments = processArguments(args, kwargs, loc, env); + return callWithArgArray(arguments, ast, env, location); + } + + /** + * The outer calling convention to a BaseFunction. This function expects all arguments to have + * been resolved into positional ones. + * + * @param ast the expression for this function's definition + * @param env the Environment in the function is called + * @return the value resulting from evaluating the function with the given arguments + * @throws EvalException-s containing source information. + */ + public Object callWithArgArray( + Object[] arguments, @Nullable FuncallExpression ast, Environment env, Location loc) + throws EvalException, InterruptedException { + Preconditions.checkState(isConfigured(), "Function %s was not configured", getName()); canonicalizeArguments(arguments, loc); try { diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Printer.java b/src/main/java/com/google/devtools/build/lib/syntax/Printer.java index 52a466b759..697248a8fb 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Printer.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Printer.java @@ -67,6 +67,15 @@ public class Printer { * @return new {@link BasePrinter} */ public static BasePrinter getPrinter() { + return new BasePrinter(new StringBuilder()); + } + + /** + * Creates an instance of {@link BasePrinter} with an empty buffer. + * + * @param simplifiedFormatStrings if true, format strings will allow only %s and %% + */ + public static BasePrinter getPrinter(boolean simplifiedFormatStrings) { return getPrinter(new StringBuilder()); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java index 46f3f1ae94..ea90d37d86 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java @@ -51,6 +51,8 @@ public abstract class SkylarkSemantics { public abstract boolean incompatibleDisallowDictPlus(); + public abstract boolean incompatibleDisallowOldStyleArgsAdd(); + public abstract boolean incompatibleDisallowThreeArgVardef(); public abstract boolean incompatibleDisallowToplevelIfStatement(); @@ -88,6 +90,7 @@ public abstract class SkylarkSemantics { .incompatibleDisableGlobTracking(true) .incompatibleDisableObjcProviderResources(false) .incompatibleDisallowDictPlus(false) + .incompatibleDisallowOldStyleArgsAdd(false) .incompatibleDisallowThreeArgVardef(false) .incompatibleDisallowToplevelIfStatement(true) .incompatibleNewActionsApi(false) @@ -115,6 +118,8 @@ public abstract class SkylarkSemantics { public abstract Builder incompatibleDisallowDictPlus(boolean value); + public abstract Builder incompatibleDisallowOldStyleArgsAdd(boolean value); + public abstract Builder incompatibleDisallowThreeArgVardef(boolean value); public abstract Builder incompatibleDisallowToplevelIfStatement(boolean value); -- cgit v1.2.3