diff --git a/src/main/java/com/google/devtools/build/docgen/ApiExporter.java b/src/main/java/com/google/devtools/build/docgen/ApiExporter.java index a1a5e14a904ed9..15be4a77373017 100644 --- a/src/main/java/com/google/devtools/build/docgen/ApiExporter.java +++ b/src/main/java/com/google/devtools/build/docgen/ApiExporter.java @@ -335,6 +335,10 @@ private static Signature getSignature(StarlarkMethod annot) { ArrayList defaults = new ArrayList<>(); for (net.starlark.java.annot.Param param : annot.parameters()) { + // Ignore undocumented parameters + if (!param.documented()) { + continue; + } // Implicit * or *args parameter separates transition from positional to named. // f (..., *, ... ) or f(..., *args, ...) // TODO(adonovan): this logic looks fishy. Clean it up. diff --git a/src/main/java/com/google/devtools/build/docgen/starlark/StarlarkDocUtils.java b/src/main/java/com/google/devtools/build/docgen/starlark/StarlarkDocUtils.java index 7dbc2ce34b57ba..e24993c8d21166 100644 --- a/src/main/java/com/google/devtools/build/docgen/starlark/StarlarkDocUtils.java +++ b/src/main/java/com/google/devtools/build/docgen/starlark/StarlarkDocUtils.java @@ -44,7 +44,9 @@ static ImmutableList determineParams( Param extraKeywords) { ImmutableList.Builder paramsBuilder = ImmutableList.builder(); for (Param param : userSuppliedParams) { - paramsBuilder.add(new StarlarkParamDoc(methodDoc, param)); + if (param.documented()) { + paramsBuilder.add(new StarlarkParamDoc(methodDoc, param)); + } } if (!extraPositionals.name().isEmpty()) { paramsBuilder.add(new StarlarkParamDoc(methodDoc, extraPositionals)); diff --git a/src/main/java/com/google/devtools/build/docgen/starlark/StarlarkMethodDoc.java b/src/main/java/com/google/devtools/build/docgen/starlark/StarlarkMethodDoc.java index 24fa60486621a1..1851367ff00f1f 100644 --- a/src/main/java/com/google/devtools/build/docgen/starlark/StarlarkMethodDoc.java +++ b/src/main/java/com/google/devtools/build/docgen/starlark/StarlarkMethodDoc.java @@ -75,6 +75,9 @@ private String getParameterString(Method method) { boolean named = false; for (Param param : withoutSelfParam(annotation, method)) { + if (!param.documented()) { + continue; + } if (param.named() && !param.positional() && !named) { named = true; if (!argList.isEmpty()) { diff --git a/src/main/java/net/starlark/java/annot/Param.java b/src/main/java/net/starlark/java/annot/Param.java index 25fbd6d0902ac2..5bae876cd1ddaa 100644 --- a/src/main/java/net/starlark/java/annot/Param.java +++ b/src/main/java/net/starlark/java/annot/Param.java @@ -31,6 +31,15 @@ */ String doc() default ""; + /** + * Determines whether the parameter appears in generated documentation. Set this to false to + * suppress parameters whose use is intentionally restricted. + * + *

An undocumented parameter must be {@link #named} and may not be followed by positional + * parameters or {@code **kwargs}. + */ + boolean documented() default true; + /** * Default value for the parameter, written as a Starlark expression (e.g. "False", "True", "[]", * "None"). diff --git a/src/main/java/net/starlark/java/annot/processor/StarlarkMethodProcessor.java b/src/main/java/net/starlark/java/annot/processor/StarlarkMethodProcessor.java index 6cf1254d9357c4..96d8b8f6205753 100644 --- a/src/main/java/net/starlark/java/annot/processor/StarlarkMethodProcessor.java +++ b/src/main/java/net/starlark/java/annot/processor/StarlarkMethodProcessor.java @@ -245,6 +245,7 @@ private void checkParameters(ExecutableElement method, StarlarkMethod annot) { boolean allowPositionalNext = true; boolean allowPositionalOnlyNext = true; boolean allowNonDefaultPositionalNext = true; + boolean hasUndocumentedMethods = false; // Check @Param annotations match parameters. Param[] paramAnnots = annot.parameters(); @@ -274,7 +275,8 @@ private void checkParameters(ExecutableElement method, StarlarkMethod annot) { if (!paramAnnot.named() && !allowPositionalOnlyNext) { errorf( param, - "Positional-only parameter '%s' is specified after one or more named parameters", + "Positional-only parameter '%s' is specified after one or more named or undocumented" + + " parameters", paramAnnot.name()); } if (paramAnnot.defaultValue().isEmpty()) { // There is no default value. @@ -297,12 +299,21 @@ private void checkParameters(ExecutableElement method, StarlarkMethod annot) { errorf(param, "Parameter '%s' must be either positional or named", paramAnnot.name()); } } - if (paramAnnot.named()) { + if (!paramAnnot.documented()) { + hasUndocumentedMethods = true; + } + if (paramAnnot.named() || !paramAnnot.documented()) { // No positional-only parameters can come after this parameter. allowPositionalOnlyNext = false; } } + if (hasUndocumentedMethods && !annot.extraKeywords().name().isEmpty()) { + errorf( + method, + "Method '%s' has undocumented parameters but also allows extra keyword parameters", + annot.name()); + } checkSpecialParams(method, annot); } @@ -379,6 +390,12 @@ private void checkParameter(Element param, Param paramAnnot) { : "Parameter '%s' has valueWhenDisabled set, but is always enabled", paramAnnot.name()); } + + // Ensure positional arguments are documented. + if (!paramAnnot.documented() && paramAnnot.positional()) { + errorf( + param, "Parameter '%s' must be documented because it is positional.", paramAnnot.name()); + } } private static boolean hasPlusMinusPrefix(String s) { diff --git a/src/test/java/com/google/devtools/build/docgen/StarlarkDocumentationTest.java b/src/test/java/com/google/devtools/build/docgen/StarlarkDocumentationTest.java index 6f16737bf8c509..43f1ba575b9585 100644 --- a/src/test/java/com/google/devtools/build/docgen/StarlarkDocumentationTest.java +++ b/src/test/java/com/google/devtools/build/docgen/StarlarkDocumentationTest.java @@ -193,6 +193,30 @@ public Integer test(int a, int b, int c, int d, Sequence args, Dict kwa } } + /** MockClassI */ + @StarlarkBuiltin(name = "MockClassI", doc = "MockClassI") + private static class MockClassI implements StarlarkValue { + @StarlarkMethod( + name = "test", + doc = "MockClassI#test", + parameters = { + @Param(name = "a", named = false, positional = true), + @Param(name = "b", named = true, positional = true), + @Param(name = "c", named = true, positional = false), + @Param(name = "d", named = true, positional = false, defaultValue = "1"), + @Param( + name = "e", + named = true, + positional = false, + documented = false, + defaultValue = "2"), + }, + extraPositionals = @Param(name = "myArgs")) + public Integer test(int a, int b, int c, int d, int e, Sequence args) { + return 0; + } + } + /** * MockGlobalLibrary. While nothing directly depends on it, a test method in * StarlarkDocumentationTest checks all of the classes under a wide classpath and ensures this one @@ -387,6 +411,21 @@ public void testStarlarkCallableParametersAndArgsAndKwargs() throws Exception { assertThat(methodDoc.getParams()).hasSize(6); } + @Test + public void testStarlarkUndocumentedParameters() throws Exception { + Map objects = collect(MockClassI.class); + StarlarkBuiltinDoc moduleDoc = objects.get("MockClassI"); + assertThat(moduleDoc.getDocumentation()).isEqualTo("MockClassI"); + assertThat(moduleDoc.getMethods()).hasSize(1); + StarlarkMethodDoc methodDoc = moduleDoc.getMethods().iterator().next(); + assertThat(methodDoc.getDocumentation()).isEqualTo("MockClassI#test"); + assertThat(methodDoc.getSignature()) + .isEqualTo( + "int " + + "MockClassI.test(a, b, *, c, d=1, *myArgs)"); + assertThat(methodDoc.getParams()).hasSize(5); + } + @Test public void testStarlarkGlobalLibraryCallable() throws Exception { StarlarkBuiltinDoc topLevel = diff --git a/src/test/java/net/starlark/java/annot/processor/StarlarkMethodProcessorTest.java b/src/test/java/net/starlark/java/annot/processor/StarlarkMethodProcessorTest.java index 90e1afe4388226..44a9f993307c6d 100644 --- a/src/test/java/net/starlark/java/annot/processor/StarlarkMethodProcessorTest.java +++ b/src/test/java/net/starlark/java/annot/processor/StarlarkMethodProcessorTest.java @@ -198,7 +198,8 @@ public void testPositionalOnlyParamAfterNamed() throws Exception { .processedWith(new StarlarkMethodProcessor()) .failsToCompile() .withErrorContaining( - "Positional-only parameter 'two' is specified after one or more named parameters"); + "Positional-only parameter 'two' is specified after one or more named or undocumented" + + " parameters"); } @Test @@ -323,4 +324,24 @@ public void testSpecifiedGenericType() throws Exception { "parameter 'one' has generic type " + "net.starlark.java.eval.Sequence"); } + + @Test + public void testKwargsWithUndocumentedParam() throws Exception { + assertAbout(javaSource()) + .that(getFile("KwargsWithUndocumentedParams.java")) + .processedWith(new StarlarkMethodProcessor()) + .failsToCompile() + .withErrorContaining( + "Method 'undocumented_with_kwargs' has undocumented parameters but also allows extra" + + " keyword parameters"); + } + + @Test + public void testUndocumentedPositionalParam() throws Exception { + assertAbout(javaSource()) + .that(getFile("UndocumentedPositionalParam.java")) + .processedWith(new StarlarkMethodProcessor()) + .failsToCompile() + .withErrorContaining("Parameter 'one' must be documented because it is positional"); + } } diff --git a/src/test/java/net/starlark/java/annot/processor/testsources/KwargsWithUndocumentedParams.java b/src/test/java/net/starlark/java/annot/processor/testsources/KwargsWithUndocumentedParams.java new file mode 100644 index 00000000000000..8ca9315bb95ce2 --- /dev/null +++ b/src/test/java/net/starlark/java/annot/processor/testsources/KwargsWithUndocumentedParams.java @@ -0,0 +1,43 @@ +// Copyright 2020 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 net.starlark.java.annot.processor.testsources; + +import net.starlark.java.annot.Param; +import net.starlark.java.annot.StarlarkMethod; +import net.starlark.java.eval.Dict; +import net.starlark.java.eval.StarlarkValue; + +/** + * Test case for a StarlarkMethod method which specifies extraKeywords whilst also specifying an + * undocumented parameter. + */ +public class KwargsWithUndocumentedParams implements StarlarkValue { + + @StarlarkMethod( + name = "undocumented_with_kwargs", + documented = false, + parameters = { + @Param( + name = "one", + named = true, + positional = false, + documented = false, + defaultValue = "test") + }, + extraKeywords = @Param(name = "kwargs")) + public String threeArgMethod(String one, Dict kwargs) { + return "bar"; + } +} diff --git a/src/test/java/net/starlark/java/annot/processor/testsources/UndocumentedPositionalParam.java b/src/test/java/net/starlark/java/annot/processor/testsources/UndocumentedPositionalParam.java new file mode 100644 index 00000000000000..32ea664663970e --- /dev/null +++ b/src/test/java/net/starlark/java/annot/processor/testsources/UndocumentedPositionalParam.java @@ -0,0 +1,32 @@ +// Copyright 2020 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 net.starlark.java.annot.processor.testsources; + +import net.starlark.java.annot.Param; +import net.starlark.java.annot.StarlarkMethod; +import net.starlark.java.eval.Dict; +import net.starlark.java.eval.StarlarkValue; + +/** Test case for a StarlarkMethod method which specifies an undocumented positional parameter. */ +public class UndocumentedPositionalParam implements StarlarkValue { + + @StarlarkMethod( + name = "undocumented_positional", + documented = false, + parameters = {@Param(name = "one", documented = false)}) + public String threeArgMethod(String one, Dict kwargs) { + return "bar"; + } +}