Reorder arguments to BuiltinFunction-s
By popular demand from other implementers, reorder BuiltinFunction arguments
so that mandatory named-only arguments come befor optional named-only arguments
rather than after. This will make Skylark internals slightly clearer and less
surprising, at the cost of eschewing a tiny optimization.
--
MOS_MIGRATED_REVID=89978554
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
index f1a6bd7..f1b148f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
@@ -230,8 +230,6 @@
int numPositionalParams = numMandatoryPositionalParams + numOptionalPositionalParams;
int numNamedOnlyParams = numMandatoryNamedOnlyParams + numOptionalNamedOnlyParams;
int numNamedParams = numPositionalParams + numNamedOnlyParams;
- int numOptionalParams = numOptionalPositionalParams + numOptionalNamedOnlyParams;
- int endOptionalParams = numMandatoryPositionalParams + numOptionalParams;
int kwParamIndex = names.size() - 1; // only valid if hasKwParam
// (1) handle positional arguments
@@ -271,9 +269,11 @@
throw new EvalException(loc, String.format(
"missing mandatory keyword arguments in call to %s", this));
}
- // Fill in defaults for missing optional parameters, that were conveniently grouped together.
+ // Fill in defaults for missing optional parameters, that were conveniently grouped together,
+ // thanks to the absence of mandatory named-only parameters as checked above.
if (defaultValues != null) {
int j = numPositionalArgs - numMandatoryPositionalParams;
+ int endOptionalParams = numPositionalParams + numOptionalNamedOnlyParams;
for (int i = numPositionalArgs; i < endOptionalParams; i++) {
arguments[i] = defaultValues.get(j++);
}
@@ -337,26 +337,31 @@
if (arguments[i] == null) {
throw new EvalException(loc, String.format(
"missing mandatory positional argument '%s' while calling %s",
- names.get(i), toString()));
+ names.get(i), this));
}
}
- for (int i = numNamedParams - numMandatoryNamedOnlyParams; i < numNamedParams; i++) {
+ int endMandatoryNamedOnlyParams = numPositionalParams + numMandatoryNamedOnlyParams;
+ for (int i = numPositionalParams; i < endMandatoryNamedOnlyParams; i++) {
if (arguments[i] == null) {
throw new EvalException(loc, String.format(
"missing mandatory named-only argument '%s' while calling %s",
- names.get(i), toString()));
+ names.get(i), this));
}
}
// Get defaults for those parameters that weren't passed.
if (defaultValues != null) {
- for (int j = numPositionalArgs > numMandatoryPositionalParams
- ? numPositionalArgs - numMandatoryPositionalParams : 0;
- j < numOptionalParams; j++) {
- int i = j + numMandatoryPositionalParams;
+ for (int i = Math.max(numPositionalArgs, numMandatoryPositionalParams);
+ i < numPositionalParams; i++) {
if (arguments[i] == null) {
- arguments[i] = defaultValues.get(j);
+ arguments[i] = defaultValues.get(i - numMandatoryPositionalParams);
+ }
+ }
+ int numMandatoryParams = numMandatoryPositionalParams + numMandatoryNamedOnlyParams;
+ for (int i = numMandatoryParams + numOptionalPositionalParams; i < numNamedParams; i++) {
+ if (arguments[i] == null) {
+ arguments[i] = defaultValues.get(i - numMandatoryParams);
}
}
}
@@ -442,11 +447,11 @@
* Render this object in the form of an equivalent Python function signature.
*/
public String toString() {
- StringBuffer sb = new StringBuffer();
+ StringBuilder sb = new StringBuilder();
sb.append(getName());
if (signature != null) {
sb.append('(');
- signature.toStringBuffer(sb);
+ signature.toStringBuilder(sb);
sb.append(')');
} // if unconfigured, don't even output parentheses
return sb.toString();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java b/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
index 0208cd7..23d8e8c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
@@ -38,20 +38,24 @@
* <p>To enable various optimizations in the argument processing routine,
* we sort arguments according the following constraints, enabling corresponding optimizations:
* <ol>
- * <li>the positional mandatories come just before the positional optionals,
+ * <li>The positional mandatories come just before the positional optionals,
* so they can be filled in one go.
- * <li>the optionals are grouped together, so we can iterate over them in one go.
- * <li>positionals come first, so it's easy to prepend extra positional arguments such as "self"
+ * <li>Positionals come first, so it's easy to prepend extra positional arguments such as "self"
* to an argument list, and we optimize for the common case of no key-only mandatory parameters.
* key-only parameters are thus grouped together.
* positional mandatory and key-only mandatory parameters are separate,
* but there no loop over a contiguous chunk of them, anyway.
- * <li>the named are all grouped together, with star and star_star rest arguments coming last.
+ * <li>The named are all grouped together, with star and star_star rest arguments coming last.
+ * <li>Mandatory arguments in each category (positional and named-only) come before the optional
+ * arguments, for the sake of slightly better clarity to human implementers. This eschews an
+ * optimization whereby grouping optionals together allows to iterate over them in one go instead
+ * of two; however, this relatively minor optimization only matters when keyword arguments are
+ * passed, at which point it is dwarfed by the slowness of keyword processing.
* </ol>
*
* <p>Parameters are thus sorted in the following obvious order:
* positional mandatory arguments (if any), positional optional arguments (if any),
- * key-only optional arguments (if any), key-only mandatory arguments (if any),
+ * key-only mandatory arguments (if any), key-only optional arguments (if any),
* then star argument (if any), then star_star argument (if any).
*/
public abstract class FunctionSignature implements Serializable {
@@ -138,14 +142,14 @@
public abstract ImmutableList<String> getNames();
/** append a representation of this signature to a string buffer. */
- public StringBuffer toStringBuffer(StringBuffer sb) {
- return WithValues.<Object, SkylarkType>create(this).toStringBuffer(sb);
+ public StringBuilder toStringBuilder(StringBuilder sb) {
+ return WithValues.<Object, SkylarkType>create(this).toStringBuilder(sb);
}
@Override
public String toString() {
- StringBuffer sb = new StringBuffer();
- toStringBuffer(sb);
+ StringBuilder sb = new StringBuilder();
+ toStringBuilder(sb);
return sb.toString();
}
@@ -228,9 +232,10 @@
ArrayList<String> params = new ArrayList<>();
ArrayList<V> defaults = new ArrayList<>();
ArrayList<T> types = new ArrayList<>();
- // mandatory named-only parameters are kept aside to be spliced after the optional ones.
- ArrayList<String> mandatoryNamedOnlyParams = new ArrayList<>();
- ArrayList<T> mandatoryNamedOnlyTypes = new ArrayList<>();
+ // optional named-only parameters are kept aside to be spliced after the mandatory ones.
+ ArrayList<String> optionalNamedOnlyParams = new ArrayList<>();
+ ArrayList<T> optionalNamedOnlyTypes = new ArrayList<>();
+ ArrayList<V> optionalNamedOnlyDefaultValues = new ArrayList<>();
boolean defaultRequired = false; // true after mandatory positionals and before star.
Set<String> paramNameSet = new HashSet<>(); // set of names, to avoid duplicates
@@ -261,35 +266,33 @@
star = name;
starType = type;
}
- } else if (hasStar && param.isMandatory()) {
- // mandatory named-only, added contiguously at the end, to simplify calls
- mandatoryNamedOnlyParams.add(name);
- mandatoryNamedOnlyTypes.add(type);
- mandatoryNamedOnly++;
+ } else if (hasStar && param.isOptional()) {
+ optionalNamedOnly++;
+ optionalNamedOnlyParams.add(name);
+ optionalNamedOnlyTypes.add(type);
+ optionalNamedOnlyDefaultValues.add(param.getDefaultValue());
} else {
params.add(name);
types.add(type);
- if (param.isMandatory()) {
- if (defaultRequired) {
+ if (param.isOptional()) {
+ optionalPositionals++;
+ defaults.add(param.getDefaultValue());
+ defaultRequired = true;
+ } else if (hasStar) {
+ mandatoryNamedOnly++;
+ } else if (defaultRequired) {
throw new SignatureException(
"a mandatory positional parameter must not follow an optional parameter",
param);
- }
+ } else {
mandatoryPositionals++;
- } else { // At this point, it's an optional parameter
- defaults.add(param.getDefaultValue());
- if (hasStar) {
- // named-only optional
- optionalNamedOnly++;
- } else {
- optionalPositionals++;
- defaultRequired = true;
- }
}
}
}
- params.addAll(mandatoryNamedOnlyParams);
- types.addAll(mandatoryNamedOnlyTypes);
+ params.addAll(optionalNamedOnlyParams);
+ types.addAll(optionalNamedOnlyTypes);
+ defaults.addAll(optionalNamedOnlyDefaultValues);
+
if (star != null) {
params.add(star);
types.add(starType);
@@ -312,7 +315,7 @@
/**
* Append a representation of this signature to a string buffer.
*/
- public StringBuffer toStringBuffer(final StringBuffer sb) {
+ public StringBuilder toStringBuilder(final StringBuilder sb) {
FunctionSignature signature = getSignature();
Shape shape = signature.getShape();
final ImmutableList<String> names = signature.getNames();
@@ -329,7 +332,7 @@
int namedOnly = mandatoryNamedOnly + optionalNamedOnly;
int named = positionals + namedOnly;
int args = named + (starArg ? 1 : 0) + (kwArg ? 1 : 0);
- int endOptionals = positionals + optionalNamedOnly;
+ int endMandatoryNamedOnly = positionals + mandatoryNamedOnly;
boolean hasStar = starArg || (namedOnly > 0);
int iStarArg = named;
int iKwArg = args - 1;
@@ -374,11 +377,11 @@
sb.append(names.get(iStarArg));
}
}
- for (; i < endOptionals; i++) {
- show.optional(i);
+ for (; i < endMandatoryNamedOnly; i++) {
+ show.mandatory(i);
}
for (; i < named; i++) {
- show.mandatory(i);
+ show.optional(i);
}
if (kwArg) {
show.comma();
@@ -391,8 +394,8 @@
@Override
public String toString() {
- StringBuffer sb = new StringBuffer();
- toStringBuffer(sb);
+ StringBuilder sb = new StringBuilder();
+ toStringBuilder(sb);
return sb.toString();
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java
index 6833622..9deb624 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java
@@ -80,6 +80,7 @@
"mixed(1, 2, 3)",
"mixed(1, 2, wiz=3, quux=4)",
"mixed(foo=1)",
+ "mixed(bar=2)",
"mixed(foo=1, bar=2)",
"mixed(bar=2, foo=1)",
"mixed(2, foo=1)",
@@ -111,6 +112,7 @@
"too many (3) positional arguments in call to mixed(foo, bar = ?)",
"unexpected keywords 'quux', 'wiz' in call to mixed(foo, bar = ?)",
"[1, null]",
+ "missing mandatory positional argument 'foo' while calling mixed(foo, bar = ?)",
"[1, 2]",
"[1, 2]",
"argument 'foo' passed both by position and by name in call to mixed(foo, bar = ?)",
@@ -119,17 +121,18 @@
@Test
public void testOnlyNamedArguments() throws Exception {
- checkBaseFunctions(true, "mixed(*, foo = ?, bar)",
- "missing mandatory keyword arguments in call to mixed(*, foo = ?, bar)",
- "mixed(*, foo = ?, bar) does not accept positional arguments, but got 1",
- "mixed(*, foo = ?, bar) does not accept positional arguments, but got 2",
- "mixed(*, foo = ?, bar) does not accept positional arguments, but got 3",
- "mixed(*, foo = ?, bar) does not accept positional arguments, but got 2",
- "missing mandatory named-only argument 'bar' while calling mixed(*, foo = ?, bar)",
+ checkBaseFunctions(true, "mixed(*, foo, bar = ?)",
+ "missing mandatory keyword arguments in call to mixed(*, foo, bar = ?)",
+ "mixed(*, foo, bar = ?) does not accept positional arguments, but got 1",
+ "mixed(*, foo, bar = ?) does not accept positional arguments, but got 2",
+ "mixed(*, foo, bar = ?) does not accept positional arguments, but got 3",
+ "mixed(*, foo, bar = ?) does not accept positional arguments, but got 2",
+ "[1, null]",
+ "missing mandatory named-only argument 'foo' while calling mixed(*, foo, bar = ?)",
"[1, 2]",
"[1, 2]",
- "mixed(*, foo = ?, bar) does not accept positional arguments, but got 1",
- "unexpected keyword 'wiz' in call to mixed(*, foo = ?, bar)");
+ "mixed(*, foo, bar = ?) does not accept positional arguments, but got 1",
+ "unexpected keyword 'wiz' in call to mixed(*, foo, bar = ?)");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
index 2eea35a..a55fc01 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
@@ -383,7 +383,7 @@
assertEquals("xyce", env.lookup("v2"));
assertEquals("xyzf", env.lookup("v3"));
UserDefinedFunction foo = (UserDefinedFunction) env.lookup("foo");
- assertEquals("foo(a, b = \"b\", *, d = \"d\", c)", foo.toString());
+ assertEquals("foo(a, b = \"b\", *, c, d = \"d\")", foo.toString());
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
index 683f2c7..9157a09 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
@@ -1018,9 +1018,9 @@
assertThat(statements.get(0)).isInstanceOf(FunctionDefStatement.class);
FunctionDefStatement stmt = (FunctionDefStatement) statements.get(0);
FunctionSignature sig = stmt.getArgs().getSignature();
- // Note the reordering of mandatory named-only at the end.
+ // Note the reordering of optional named-only at the end.
assertThat(sig.getNames()).isEqualTo(ImmutableList.<String>of(
- "a", "b1", "b2", "d", "c1", "c2"));
+ "a", "b1", "b2", "c1", "c2", "d"));
FunctionSignature.Shape shape = sig.getShape();
assertThat(shape.getMandatoryPositionals()).isEqualTo(1);
assertThat(shape.getOptionalPositionals()).isEqualTo(2);
