Skip to content

Commit bd1c3af

Browse files
tetrominocopybara-github
tetromino
authored and
copybara-github
committed
Emit function param's role in starlark_doc_extract proto output
... and clarify the order of parameters in the list. Note that we have two reasonable possibilities: * declaration order, e.g. foo(pos, *args, kwonly, **kwargs) * calling convention order, e.g. foo(pos, kwonly, *args, **kwargs) For backwards compatibility, and to avoid confusion since we do not emit the `*` marker between positional and keyword-only params, it makes more sense to use the latter. Users such as the Stardoc markdown renderer can easily transform it into declaration order if desired. Working towards bazelbuild/stardoc#225 PiperOrigin-RevId: 638390032 Change-Id: I201d89bc3451a98a02905722800c78cd3d09e5b7
1 parent 882a467 commit bd1c3af

File tree

8 files changed

+413
-46
lines changed

8 files changed

+413
-46
lines changed

‎src/main/java/com/google/devtools/build/lib/starlarkdocextract/StarlarkFunctionInfoExtractor.java

Lines changed: 36 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,19 @@
1414

1515
package com.google.devtools.build.lib.starlarkdocextract;
1616

17+
import static com.google.common.base.Preconditions.checkState;
18+
import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_KEYWORD_ONLY;
19+
import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_KWARGS;
20+
import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_ORDINARY;
21+
import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_VARARGS;
22+
1723
import com.google.common.collect.ImmutableList;
1824
import com.google.common.collect.Lists;
1925
import com.google.common.collect.Maps;
2026
import com.google.devtools.build.lib.cmdline.BazelModuleContext;
21-
import com.google.devtools.build.lib.cmdline.Label;
2227
import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionDeprecationInfo;
2328
import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamInfo;
29+
import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole;
2430
import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionReturnInfo;
2531
import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.OriginKey;
2632
import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.StarlarkFunctionInfo;
@@ -98,8 +104,12 @@ private StarlarkFunctionInfo extract(String functionName, StarlarkFunction fn)
98104
}
99105

100106
private FunctionParamInfo forParam(
101-
String name, Optional<String> docString, @Nullable Object defaultValue) {
102-
FunctionParamInfo.Builder paramBuilder = FunctionParamInfo.newBuilder().setName(name);
107+
String name,
108+
Optional<String> docString,
109+
@Nullable Object defaultValue,
110+
FunctionParamRole role) {
111+
FunctionParamInfo.Builder paramBuilder =
112+
FunctionParamInfo.newBuilder().setName(name).setRole(role);
103113
docString.ifPresent(paramBuilder::setDocString);
104114
if (defaultValue == null) {
105115
paramBuilder.setMandatory(true);
@@ -110,38 +120,45 @@ private FunctionParamInfo forParam(
110120
}
111121

112122
/** Constructor to be used for *args or **kwargs. */
113-
public static FunctionParamInfo forSpecialParam(String name, String docString) {
114-
return FunctionParamInfo.newBuilder()
115-
.setName(name)
116-
.setDocString(docString)
117-
.setMandatory(false)
118-
.build();
123+
private static FunctionParamInfo forSpecialParam(
124+
String name, Optional<String> docString, FunctionParamRole role) {
125+
FunctionParamInfo.Builder paramBuilder =
126+
FunctionParamInfo.newBuilder().setName(name).setRole(role).setMandatory(false);
127+
docString.ifPresent(paramBuilder::setDocString);
128+
return paramBuilder.build();
119129
}
120130

121131
private ImmutableList<FunctionParamInfo> parameterInfos(
122132
StarlarkFunction fn, Map<String, String> parameterDoc) {
123-
List<String> names = fn.getParameterNames();
124-
int nparams = names.size();
125-
int kwargsIndex = fn.hasKwargs() ? --nparams : -1;
126-
int varargsIndex = fn.hasVarargs() ? --nparams : -1;
127-
// Inv: nparams is number of regular parameters.
133+
ImmutableList<String> names = fn.getParameterNames();
134+
int numOrdinaryParams = fn.getNumOrdinaryParameters();
135+
int numKeywordOnlyParams = fn.getNumKeywordOnlyParameters();
136+
int varargsIndex = fn.hasVarargs() ? numOrdinaryParams + numKeywordOnlyParams : -1;
137+
int kwargsIndex = fn.hasKwargs() ? names.size() - 1 : -1;
138+
checkState(varargsIndex == -1 || varargsIndex < names.size());
139+
checkState(kwargsIndex == -1 || varargsIndex == -1 || kwargsIndex == varargsIndex + 1);
128140

129141
ImmutableList.Builder<FunctionParamInfo> infos = ImmutableList.builder();
130142
for (int i = 0; i < names.size(); i++) {
131143
String name = names.get(i);
132144
FunctionParamInfo info;
133145
if (i == varargsIndex) {
134146
// *args
135-
String doc = parameterDoc.getOrDefault("*" + name, "");
136-
info = forSpecialParam(name, doc);
147+
Optional<String> doc = Optional.ofNullable(parameterDoc.get("*" + name));
148+
info = forSpecialParam(name, doc, PARAM_ROLE_VARARGS);
137149
} else if (i == kwargsIndex) {
138150
// **kwargs
139-
String doc = parameterDoc.getOrDefault("**" + name, "");
140-
info = forSpecialParam(name, doc);
151+
Optional<String> doc = Optional.ofNullable(parameterDoc.get("**" + name));
152+
info = forSpecialParam(name, doc, PARAM_ROLE_KWARGS);
141153
} else {
142154
// regular parameter
143155
Optional<String> doc = Optional.ofNullable(parameterDoc.get(name));
144-
info = forParam(name, doc, fn.getDefaultValue(i));
156+
info =
157+
forParam(
158+
name,
159+
doc,
160+
fn.getDefaultValue(i),
161+
i < numOrdinaryParams ? PARAM_ROLE_ORDINARY : PARAM_ROLE_KEYWORD_ONLY);
145162
}
146163
infos.add(info);
147164
}

‎src/main/java/net/starlark/java/eval/StarlarkFunction.java

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -114,13 +114,30 @@ public Object getDefaultValue(int i) {
114114
}
115115

116116
/**
117-
* Returns the names of this function's parameters. The residual {@code *args} and {@code
118-
* **kwargs} parameters, if any, are always last.
117+
* Returns the names of this function's parameters.
118+
*
119+
* <p>The first {@code getNumOrdinaryParameters()} parameters in the returned list are ordinary
120+
* (non-residual, non-keyword-only); the following {@code getNumKeywordOnlyParameters()} are
121+
* keyword-only; and the residual {@code *args} and {@code **kwargs} parameters, if any, are
122+
* always last.
119123
*/
120124
public ImmutableList<String> getParameterNames() {
121125
return rfn.getParameterNames();
122126
}
123127

128+
/** Returns the number of ordinary (non-residual, non-keyword-only) parameters. */
129+
public int getNumOrdinaryParameters() {
130+
return rfn.getParameters().size()
131+
- (rfn.hasKwargs() ? 1 : 0)
132+
- (rfn.hasVarargs() ? 1 : 0)
133+
- rfn.numKeywordOnlyParams();
134+
}
135+
136+
/** Returns the number of non-residual keyword-only parameters. */
137+
public int getNumKeywordOnlyParameters() {
138+
return rfn.numKeywordOnlyParams();
139+
}
140+
124141
/**
125142
* Reports whether this function has a residual positional arguments parameter, {@code def
126143
* f(*args)}.
@@ -247,27 +264,26 @@ private Object[] processArgs(Mutability mu, Object[] positional, Object[] named)
247264

248265
Object[] locals = new Object[rfn.getLocals().size()];
249266

250-
// nparams is the number of ordinary parameters.
251-
int nparams =
252-
rfn.getParameters().size() - (rfn.hasKwargs() ? 1 : 0) - (rfn.hasVarargs() ? 1 : 0);
267+
// numOrdinaryParams is the number of ordinary (non-residual, non-kwonly) parameters.
268+
int numOrdinaryParams = getNumOrdinaryParameters();
253269

254-
// numPositionalParams is the number of non-kwonly parameters.
255-
int numPositionalParams = nparams - rfn.numKeywordOnlyParams();
270+
// nparams is the number of all non-residual parameters.
271+
int nparams = numOrdinaryParams + getNumKeywordOnlyParameters();
256272

257273
// Too many positional args?
258274
int n = positional.length;
259-
if (n > numPositionalParams) {
275+
if (n > numOrdinaryParams) {
260276
if (!rfn.hasVarargs()) {
261-
if (numPositionalParams > 0) {
277+
if (numOrdinaryParams > 0) {
262278
throw Starlark.errorf(
263279
"%s() accepts no more than %d positional argument%s but got %d",
264-
getName(), numPositionalParams, plural(numPositionalParams), n);
280+
getName(), numOrdinaryParams, plural(numOrdinaryParams), n);
265281
} else {
266282
throw Starlark.errorf(
267283
"%s() does not accept positional arguments, but got %d", getName(), n);
268284
}
269285
}
270-
n = numPositionalParams;
286+
n = numOrdinaryParams;
271287
}
272288
// Inv: n is number of positional arguments that are not surplus.
273289

@@ -353,7 +369,7 @@ private Object[] processArgs(Mutability mu, Object[] positional, Object[] named)
353369
}
354370

355371
// missing
356-
if (i < numPositionalParams) {
372+
if (i < numOrdinaryParams) {
357373
if (missingPositional == null) {
358374
missingPositional = new ArrayList<>();
359375
}

‎src/main/protobuf/stardoc_output.proto

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,16 @@ message StarlarkFunctionInfo {
190190
// "foo.frobnicate".
191191
string function_name = 1;
192192

193-
// The parameters for the function.
193+
// The parameters for the function, in the following order:
194+
// - positional parameters
195+
// - keyword-only parameters
196+
// - residual varargs parameter (`*args`)
197+
// - residual keyword arguments parameter (`**kwargs`)
198+
// This order differs from the order in which parameters are listed in the
199+
// function's declaration (where positional parameters and keyword-only
200+
// parameters are separated either by `*` or `*args`). The declaration order
201+
// can be recovered by looking for the transition from ordinary/positional to
202+
// keyword-only.
194203
repeated FunctionParamInfo parameter = 2;
195204

196205
// The documented description of the function (if specified in the function's
@@ -210,9 +219,28 @@ message StarlarkFunctionInfo {
210219
OriginKey origin_key = 6;
211220
}
212221

222+
// Representation of the syntactic role of a given function parameter.
223+
enum FunctionParamRole {
224+
PARAM_ROLE_UNSPECIFIED = 0;
225+
// An ordinary parameter which may be used as a positional or by keyword.
226+
PARAM_ROLE_ORDINARY = 1;
227+
// A positional-only parameter; such parameters cannot be defined in pure
228+
// Starlark code, but exist in some natively-defined functions.
229+
PARAM_ROLE_POSITIONAL_ONLY = 2;
230+
// A keyword-only parameter, i.e. a non-vararg/kwarg parameter that follows
231+
// `*` or `*args` in the function's declaration.
232+
PARAM_ROLE_KEYWORD_ONLY = 3;
233+
// Residual varargs, typically `*args` in the function's declaration.
234+
PARAM_ROLE_VARARGS = 4;
235+
// Residual keyword arguments, typically `**kwargs` in the function's
236+
// declaration.
237+
PARAM_ROLE_KWARGS = 5;
238+
}
239+
213240
// Representation of a Starlark function parameter definition.
214241
message FunctionParamInfo {
215-
// The name of the parameter.
242+
// The name of the parameter. This does *not* include the `*` or `**` prefix
243+
// for varargs or residual keyword argument parameters.
216244
string name = 1;
217245

218246
// The documented description of the parameter (if specified in the function's
@@ -227,6 +255,9 @@ message FunctionParamInfo {
227255
// parameter. This might be false even if defaultValue is empty in the case of
228256
// special parameter such as *args and **kwargs"
229257
bool mandatory = 4;
258+
259+
// The parameter's syntactic role.
260+
FunctionParamRole role = 5;
230261
}
231262

232263
message FunctionReturnInfo {

‎src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import static com.google.common.truth.Truth.assertThat;
1818
import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
19+
import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_ORDINARY;
1920
import static org.junit.Assert.assertThrows;
2021

2122
import com.google.devtools.build.lib.actions.Action;
@@ -525,14 +526,22 @@ def return_lambda():
525526
StarlarkFunctionInfo.newBuilder()
526527
.setFunctionName("exported_nested")
527528
.setDocString("My nested function")
528-
.addParameter(FunctionParamInfo.newBuilder().setName("x").setMandatory(true))
529+
.addParameter(
530+
FunctionParamInfo.newBuilder()
531+
.setName("x")
532+
.setRole(PARAM_ROLE_ORDINARY)
533+
.setMandatory(true))
529534
.setOriginKey(
530535
// OriginKey.name for nested functions is explicitly unset
531536
OriginKey.newBuilder().setFile("//:origin.bzl"))
532537
.build(),
533538
StarlarkFunctionInfo.newBuilder()
534539
.setFunctionName("exported_lambda")
535-
.addParameter(FunctionParamInfo.newBuilder().setName("y").setMandatory(true))
540+
.addParameter(
541+
FunctionParamInfo.newBuilder()
542+
.setName("y")
543+
.setRole(PARAM_ROLE_ORDINARY)
544+
.setMandatory(true))
536545
.setOriginKey(
537546
// OriginKey.name for lambdas is explicitly unset
538547
OriginKey.newBuilder().setFile("//:origin.bzl"))

‎src/test/java/com/google/devtools/build/lib/starlarkdocextract/BUILD

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,24 @@ java_test(
2626
],
2727
)
2828

29+
java_test(
30+
name = "StarlarkFunctionInfoExtractorTest",
31+
size = "small",
32+
srcs = ["StarlarkFunctionInfoExtractorTest.java"],
33+
deps = [
34+
"//src/main/java/com/google/devtools/build/lib/cmdline",
35+
"//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
36+
"//src/main/java/com/google/devtools/build/lib/starlarkdocextract:labelrenderer",
37+
"//src/main/java/com/google/devtools/build/lib/starlarkdocextract:starlarkfunctioninfoextractor",
38+
"//src/main/java/net/starlark/java/eval",
39+
"//src/main/java/net/starlark/java/syntax",
40+
"//src/main/protobuf:stardoc_output_java_proto",
41+
"//src/test/java/com/google/devtools/build/lib/starlark/util",
42+
"//third_party:junit4",
43+
"//third_party:truth",
44+
],
45+
)
46+
2947
java_test(
3048
name = "ModuleInfoExtractorTest",
3149
srcs = ["ModuleInfoExtractorTest.java"],

‎src/test/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractorTest.java

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@
1717
import static com.google.common.truth.Truth.assertThat;
1818
import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
1919
import static com.google.devtools.build.lib.skyframe.BzlLoadValue.keyForBuild;
20+
import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_KWARGS;
21+
import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_ORDINARY;
22+
import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_VARARGS;
2023

2124
import com.google.common.collect.ImmutableList;
2225
import com.google.common.collect.ImmutableMap;
@@ -301,16 +304,23 @@ def my_func(documented, undocumented, has_default = {"foo": "bar"}, *args, **kwa
301304
.containsExactly(
302305
FunctionParamInfo.newBuilder()
303306
.setName("documented")
307+
.setRole(PARAM_ROLE_ORDINARY)
304308
.setDocString("Documented param")
305309
.setMandatory(true)
306310
.build(),
307-
FunctionParamInfo.newBuilder().setName("undocumented").setMandatory(true).build(),
311+
FunctionParamInfo.newBuilder()
312+
.setName("undocumented")
313+
.setRole(PARAM_ROLE_ORDINARY)
314+
.setMandatory(true)
315+
.build(),
308316
FunctionParamInfo.newBuilder()
309317
.setName("has_default")
318+
.setRole(PARAM_ROLE_ORDINARY)
310319
.setDefaultValue("{\"foo\": \"bar\"}")
311320
.build(),
312-
FunctionParamInfo.newBuilder().setName("args").build(),
313-
FunctionParamInfo.newBuilder().setName("kwargs").build());
321+
FunctionParamInfo.newBuilder().setName("args").setRole(PARAM_ROLE_VARARGS).build(),
322+
FunctionParamInfo.newBuilder().setName("kwargs").setRole(PARAM_ROLE_KWARGS).build())
323+
.inOrder();
314324
}
315325

316326
@Test
@@ -394,7 +404,11 @@ public void unexportedLambdaFunction() throws Exception {
394404
// Note that origin key name is unset
395405
.setOriginKey(OriginKey.newBuilder().setFile(fakeLabelString))
396406
.setFunctionName("s.lambda_function")
397-
.addParameter(FunctionParamInfo.newBuilder().setName("x").setMandatory(true))
407+
.addParameter(
408+
FunctionParamInfo.newBuilder()
409+
.setName("x")
410+
.setRole(PARAM_ROLE_ORDINARY)
411+
.setMandatory(true))
398412
.build());
399413
}
400414

@@ -421,7 +435,11 @@ def multiply(x):
421435
.setOriginKey(OriginKey.newBuilder().setFile(fakeLabelString))
422436
.setFunctionName("s.generated")
423437
.setDocString("Multiplies x by constant y")
424-
.addParameter(FunctionParamInfo.newBuilder().setName("x").setMandatory(true))
438+
.addParameter(
439+
FunctionParamInfo.newBuilder()
440+
.setName("x")
441+
.setRole(PARAM_ROLE_ORDINARY)
442+
.setMandatory(true))
425443
.build());
426444
}
427445

@@ -515,12 +533,14 @@ def _my_info_init(x_value, y_value = 0):
515533
.addParameter(
516534
FunctionParamInfo.newBuilder()
517535
.setName("x_value")
536+
.setRole(PARAM_ROLE_ORDINARY)
518537
.setDocString("my x value")
519538
.setMandatory(true)
520539
.build())
521540
.addParameter(
522541
FunctionParamInfo.newBuilder()
523542
.setName("y_value")
543+
.setRole(PARAM_ROLE_ORDINARY)
524544
.setDocString("my y value")
525545
.setDefaultValue("0")
526546
.build())

0 commit comments

Comments
 (0)