diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java index 72520c89a31830..2ca2e7f32589d6 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java @@ -120,7 +120,7 @@ private static Attribute.Builder createAttribute( String name) throws EvalException { Attribute.Builder builder = Attribute.attr(name, type).starlarkDefined(); - doc.ifPresent(builder::setDoc); + doc.map(Starlark::trimDocString).ifPresent(builder::setDoc); Object defaultValue = arguments.get(DEFAULT_ARG); if (!Starlark.isNullOrNone(defaultValue)) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index 84ba8ba2b715b0..2f22c95c5d67f4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java @@ -34,6 +34,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Maps; import com.google.devtools.build.lib.analysis.Allowlist; import com.google.devtools.build.lib.analysis.BaseRuleClasses; import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; @@ -252,11 +253,15 @@ public static RuleClass getTestBaseRule(RuleDefinitionEnvironment env) { public Object provider(Object doc, Object fields, Object init, StarlarkThread thread) throws EvalException { StarlarkProvider.Builder builder = StarlarkProvider.builder(thread.getCallerLocation()); - Starlark.toJavaOptional(doc, String.class).ifPresent(builder::setDocumentation); + Starlark.toJavaOptional(doc, String.class) + .map(Starlark::trimDocString) + .ifPresent(builder::setDocumentation); if (fields instanceof Sequence) { builder.setSchema(Sequence.cast(fields, String.class, "fields")); } else if (fields instanceof Dict) { - builder.setSchema(Dict.cast(fields, String.class, String.class, "fields")); + builder.setSchema( + Maps.transformValues( + Dict.cast(fields, String.class, String.class, "fields"), Starlark::trimDocString)); } if (init == Starlark.NONE) { return builder.build(); @@ -495,7 +500,7 @@ public static StarlarkRuleFunction createRule( type, attributes, thread.getCallerLocation(), - Starlark.toJavaOptional(doc, String.class)); + Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString)); } private static void checkAttributeName(String name) throws EvalException { @@ -673,7 +678,7 @@ public StarlarkAspect aspect( return new StarlarkDefinedAspect( implementation, - Starlark.toJavaOptional(doc, String.class), + Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString), attrAspects.build(), attributes.build(), StarlarkAttrModule.buildProviderPredicate(requiredProvidersArg, "required_providers"), diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java index 91e2ecd56ae145..1bfdd5245e4f98 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java @@ -116,7 +116,9 @@ public StarlarkCallable repositoryRule( bzlModule.label(), bzlModule.bzlTransitiveDigest()); builder.setWorkspaceOnly(); return new RepositoryRuleFunction( - builder, implementation, Starlark.toJavaOptional(doc, String.class)); + builder, + implementation, + Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString)); } /** @@ -298,7 +300,7 @@ public Object moduleExtension( .setImplementation(implementation) .setTagClasses( ImmutableMap.copyOf(Dict.cast(tagClasses, String.class, TagClass.class, "tag_classes"))) - .setDoc(Starlark.toJavaOptional(doc, String.class)) + .setDoc(Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString)) .setDefiningBzlFileLabel( BzlInitThreadContext.fromOrFailFunction(thread, "module_extension").getBzlFile()) .setEnvVariables(ImmutableList.copyOf(Sequence.cast(environ, String.class, "environ"))) @@ -324,7 +326,7 @@ public TagClass tagClass( } return TagClass.create( attrBuilder.build(), - Starlark.toJavaOptional(doc, String.class), + Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString), thread.getCallerLocation()); } } diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkAttrModuleApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkAttrModuleApi.java index 76ac8c3ea1930c..e1f94ce67c9cf3 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkAttrModuleApi.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkAttrModuleApi.java @@ -21,6 +21,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Optional; import net.starlark.java.eval.Dict; import net.starlark.java.eval.EvalException; import net.starlark.java.eval.Module; @@ -30,9 +31,7 @@ import net.starlark.java.eval.StarlarkInt; import net.starlark.java.eval.StarlarkThread; -/** - * Fake implementation of {@link StarlarkAttrModuleApi}. - */ +/** Fake implementation of {@link StarlarkAttrModuleApi}. */ public class FakeStarlarkAttrModuleApi implements StarlarkAttrModuleApi { @Override @@ -44,11 +43,7 @@ public Descriptor intAttribute( StarlarkThread thread) throws EvalException { return new FakeDescriptor( - AttributeType.INT, - Starlark.toJavaOptional(doc, String.class), - mandatory, - ImmutableList.of(), - defaultInt); + AttributeType.INT, toTrimmedString(doc), mandatory, ImmutableList.of(), defaultInt); } @Override @@ -61,7 +56,7 @@ public Descriptor stringAttribute( throws EvalException { return new FakeDescriptor( AttributeType.STRING, - Starlark.toJavaOptional(doc, String.class), + toTrimmedString(doc), mandatory, ImmutableList.of(), defaultString != null ? "\"" + defaultString + "\"" : null); @@ -87,11 +82,7 @@ public Descriptor labelAttribute( allNameGroups = allProviderNameGroups(providers, thread); } return new FakeDescriptor( - AttributeType.LABEL, - Starlark.toJavaOptional(doc, String.class), - mandatory, - allNameGroups, - defaultO); + AttributeType.LABEL, toTrimmedString(doc), mandatory, allNameGroups, defaultO); } @Override @@ -100,7 +91,7 @@ public Descriptor stringListAttribute( throws EvalException { return new FakeDescriptor( AttributeType.STRING_LIST, - Starlark.toJavaOptional(doc, String.class), + toTrimmedString(doc), mandatory, ImmutableList.of(), defaultList); @@ -115,11 +106,7 @@ public Descriptor intListAttribute( StarlarkThread thread) throws EvalException { return new FakeDescriptor( - AttributeType.INT_LIST, - Starlark.toJavaOptional(doc, String.class), - mandatory, - ImmutableList.of(), - defaultList); + AttributeType.INT_LIST, toTrimmedString(doc), mandatory, ImmutableList.of(), defaultList); } @Override @@ -141,11 +128,7 @@ public Descriptor labelListAttribute( allNameGroups = allProviderNameGroups(providers, thread); } return new FakeDescriptor( - AttributeType.LABEL_LIST, - Starlark.toJavaOptional(doc, String.class), - mandatory, - allNameGroups, - defaultList); + AttributeType.LABEL_LIST, toTrimmedString(doc), mandatory, allNameGroups, defaultList); } @Override @@ -168,7 +151,7 @@ public Descriptor labelKeyedStringDictAttribute( } return new FakeDescriptor( AttributeType.LABEL_STRING_DICT, - Starlark.toJavaOptional(doc, String.class), + toTrimmedString(doc), mandatory, allNameGroups, defaultList); @@ -179,7 +162,7 @@ public Descriptor boolAttribute( Boolean defaultO, Object doc, Boolean mandatory, StarlarkThread thread) throws EvalException { return new FakeDescriptor( AttributeType.BOOLEAN, - Starlark.toJavaOptional(doc, String.class), + toTrimmedString(doc), mandatory, ImmutableList.of(), Boolean.TRUE.equals(defaultO) ? "True" : "False"); @@ -189,11 +172,7 @@ public Descriptor boolAttribute( public Descriptor outputAttribute(Object doc, Boolean mandatory, StarlarkThread thread) throws EvalException { return new FakeDescriptor( - AttributeType.OUTPUT, - Starlark.toJavaOptional(doc, String.class), - mandatory, - ImmutableList.of(), - ""); + AttributeType.OUTPUT, toTrimmedString(doc), mandatory, ImmutableList.of(), ""); } @Override @@ -201,11 +180,7 @@ public Descriptor outputListAttribute( Boolean allowEmpty, Object doc, Boolean mandatory, StarlarkThread thread) throws EvalException { return new FakeDescriptor( - AttributeType.OUTPUT_LIST, - Starlark.toJavaOptional(doc, String.class), - mandatory, - ImmutableList.of(), - ""); + AttributeType.OUTPUT_LIST, toTrimmedString(doc), mandatory, ImmutableList.of(), ""); } @Override @@ -213,11 +188,7 @@ public Descriptor stringDictAttribute( Boolean allowEmpty, Dict defaultO, Object doc, Boolean mandatory, StarlarkThread thread) throws EvalException { return new FakeDescriptor( - AttributeType.STRING_DICT, - Starlark.toJavaOptional(doc, String.class), - mandatory, - ImmutableList.of(), - defaultO); + AttributeType.STRING_DICT, toTrimmedString(doc), mandatory, ImmutableList.of(), defaultO); } @Override @@ -226,7 +197,7 @@ public Descriptor stringListDictAttribute( throws EvalException { return new FakeDescriptor( AttributeType.STRING_LIST_DICT, - Starlark.toJavaOptional(doc, String.class), + toTrimmedString(doc), mandatory, ImmutableList.of(), defaultO); @@ -236,11 +207,11 @@ public Descriptor stringListDictAttribute( public Descriptor licenseAttribute( Object defaultO, Object doc, Boolean mandatory, StarlarkThread thread) throws EvalException { return new FakeDescriptor( - AttributeType.STRING_LIST, - Starlark.toJavaOptional(doc, String.class), - mandatory, - ImmutableList.of(), - defaultO); + AttributeType.STRING_LIST, toTrimmedString(doc), mandatory, ImmutableList.of(), defaultO); + } + + private static Optional toTrimmedString(Object doc) { + return Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString); } @Override diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java index 1b8176e11711de..f3c3696e9bf34f 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java @@ -106,8 +106,7 @@ public Object provider(Object doc, Object fields, Object init, StarlarkThread th // fields is NONE, so there is no field information to add. } providerInfoList.add( - forProviderInfo( - fakeProvider, Starlark.toJavaOptional(doc, String.class), providerFieldInfos.build())); + forProviderInfo(fakeProvider, toTrimmedString(doc), providerFieldInfos.build())); if (init == Starlark.NONE) { return fakeProvider; } else { @@ -167,7 +166,7 @@ public StarlarkCallable rule( // Only the Builder is passed to RuleInfoWrapper as the rule name is not yet available. RuleInfo.Builder ruleInfo = RuleInfo.newBuilder().addAllAttribute(attrInfos); - Starlark.toJavaOptional(doc, String.class).ifPresent(ruleInfo::setDocString); + toTrimmedString(doc).ifPresent(ruleInfo::setDocString); Location loc = thread.getCallerLocation(); ruleInfoList.add(new RuleInfoWrapper(functionIdentifier, loc, ruleInfo)); @@ -231,7 +230,7 @@ public StarlarkAspectApi aspect( // Only the Builder is passed to AspectInfoWrapper as the aspect name is not yet available. AspectInfo.Builder aspectInfo = AspectInfo.newBuilder().addAllAttribute(attrInfos).addAllAspectAttribute(aspectAttrs); - Starlark.toJavaOptional(doc, String.class).ifPresent(aspectInfo::setDocString); + toTrimmedString(doc).ifPresent(aspectInfo::setDocString); aspectInfoList.add(new AspectInfoWrapper(fakeAspect, thread.getCallerLocation(), aspectInfo)); @@ -279,4 +278,8 @@ public int compare(AttributeInfo o1, AttributeInfo o2) { } } } + + private static Optional toTrimmedString(Object doc) { + return Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString); + } } diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java index 668d9adfb14c67..3ab502b98fd455 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java @@ -97,7 +97,9 @@ public StarlarkCallable repositoryRule( // Only the Builder is passed to RuleInfoWrapper as the rule name is not yet available. RuleInfo.Builder ruleInfo = RuleInfo.newBuilder().addAllAttribute(attrInfos); - Starlark.toJavaOptional(doc, String.class).ifPresent(ruleInfo::setDocString); + Starlark.toJavaOptional(doc, String.class) + .map(Starlark::trimDocString) + .ifPresent(ruleInfo::setDocString); Location loc = thread.getCallerLocation(); ruleInfoList.add(new RuleInfoWrapper(functionIdentifier, loc, ruleInfo)); return functionIdentifier; diff --git a/src/main/java/net/starlark/java/eval/Starlark.java b/src/main/java/net/starlark/java/eval/Starlark.java index 545fb2816da974..722f37df51d43e 100644 --- a/src/main/java/net/starlark/java/eval/Starlark.java +++ b/src/main/java/net/starlark/java/eval/Starlark.java @@ -13,8 +13,12 @@ // limitations under the License. package net.starlark.java.eval; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Strings.isNullOrEmpty; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static java.lang.Math.min; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -540,6 +544,113 @@ public static String formatWithList( return pr.toString(); } + /** + * Returns a Starlark doc string with each line trimmed and dedented to the minimal common + * indentation level (except for the first line, which is always fully trimmed), and with leading + * and trailing empty lines removed, following the PEP-257 algorithm. See + * https://peps.python.org/pep-0257/#handling-docstring-indentation + * + *

For whitespace trimming, we use the same definition of whitespace as the Starlark {@code + * string.strip} method. + * + *

Following PEP-257, we expand tabs in the doc string with tab size 8 before dedenting. + * Starlark does not use tabs for indentation, but Starlark string values may contain tabs, so we + * choose to expand them for consistency with Python. + * + *

The intent is to turn documentation strings like + * + * 

+   *     """Heading
+   *
+   *     Details paragraph
+   *     """
+   *
+ * + * and + * + * 
+   *     """
+   *     Heading
+   *
+   *     Details paragraph
+   *     """
+   *
+ * + * into the desired "Heading\n\nDetails paragraph" form, and avoid the risk of documentation + * processors interpreting indented parts of the original string as special formatting (e.g. code + * blocks in the case of Markdown). + */ + public static String trimDocString(String docString) { + ImmutableList lines = expandTabs(docString, 8).lines().collect(toImmutableList()); + if (lines.isEmpty()) { + return ""; + } + // First line is special: we fully strip it and ignore it for leading spaces calculation + String firstLineTrimmed = StringModule.INSTANCE.strip(lines.get(0), NONE); + Iterable subsequentLines = Iterables.skip(lines, 1); + int minLeadingSpaces = Integer.MAX_VALUE; + for (String line : subsequentLines) { + String strippedLeading = StringModule.INSTANCE.lstrip(line, NONE); + if (!strippedLeading.isEmpty()) { + int leadingSpaces = line.length() - strippedLeading.length(); + minLeadingSpaces = min(leadingSpaces, minLeadingSpaces); + } + } + if (minLeadingSpaces == Integer.MAX_VALUE) { + minLeadingSpaces = 0; + } + + StringBuilder result = new StringBuilder(); + result.append(firstLineTrimmed); + for (String line : subsequentLines) { + // Length check ensures we ignore leading empty lines + if (result.length() > 0) { + result.append("\n"); + } + if (line.length() > minLeadingSpaces) { + result.append(StringModule.INSTANCE.rstrip(line.substring(minLeadingSpaces), NONE)); + } + } + // Remove trailing empty lines + return StringModule.INSTANCE.rstrip(result.toString(), NONE); + } + + /** + * Expands tab characters to one or more spaces, producing the same indentation level at any given + * point on any given line as would be expected when rendering the string with a given tab size; a + * Java port of Python's {@code str.expandtabs}. + */ + static String expandTabs(String line, int tabSize) { + if (!line.contains("\t")) { + // Don't alloc in the fast case. + return line; + } + checkArgument(tabSize > 0); + StringBuilder result = new StringBuilder(); + int col = 0; + for (int i = 0; i < line.length(); i++) { + char c = line.charAt(i); + switch (c) { + case '\n': + case '\r': + result.append(c); + col = 0; + break; + case '\t': + int spaces = tabSize - col % tabSize; + for (int j = 0; j < spaces; j++) { + result.append(' '); + } + col += spaces; + break; + default: + result.append(c); + col++; + } + } + return result.toString(); + } + /** Returns a slice of a sequence as if by the Starlark operation {@code x[start:stop:step]}. */ public static Object slice( Mutability mu, Object x, Object startObj, Object stopObj, Object stepObj) @@ -972,7 +1083,10 @@ public static Object execFileProgram(Program prog, Module module, StarlarkThread int[] globalIndex = module.getIndicesOfGlobals(rfn.getGlobals()); if (module.getDocumentation() == null) { - module.setDocumentation(rfn.getDocumentation()); + String documentation = rfn.getDocumentation(); + if (documentation != null) { + module.setDocumentation(Starlark.trimDocString(documentation)); + } } StarlarkFunction toplevel = diff --git a/src/main/java/net/starlark/java/eval/StarlarkFunction.java b/src/main/java/net/starlark/java/eval/StarlarkFunction.java index 60f0af44390461..3136a3bc3e6de9 100644 --- a/src/main/java/net/starlark/java/eval/StarlarkFunction.java +++ b/src/main/java/net/starlark/java/eval/StarlarkFunction.java @@ -145,10 +145,14 @@ public String getName() { return rfn.getName(); } - /** Returns the value denoted by the function's doc string literal, or null if absent. */ + /** + * Returns the value denoted by the function's doc string literal (trimmed if necessary), or null + * if absent. + */ @Nullable public String getDocumentation() { - return rfn.getDocumentation(); + String documentation = rfn.getDocumentation(); + return documentation != null ? Starlark.trimDocString(documentation) : null; } public Module getModule() { diff --git a/src/main/java/net/starlark/java/eval/StringModule.java b/src/main/java/net/starlark/java/eval/StringModule.java index b4cdc137fd9a09..302dbc1312b50c 100644 --- a/src/main/java/net/starlark/java/eval/StringModule.java +++ b/src/main/java/net/starlark/java/eval/StringModule.java @@ -184,9 +184,11 @@ public String upper(String self) { *

Note that this differs from Python 2.7, which uses ctype.h#isspace(), and from * java.lang.Character#isWhitespace(), which does not recognize U+00A0. */ + // TODO(https://github.com/bazelbuild/starlark/issues/112): use the Unicode definition of + // whitespace, matching Python 3. private static final String LATIN1_WHITESPACE = ("\u0009" + "\n" + "\u000B" + "\u000C" + "\r" + "\u001C" + "\u001D" + "\u001E" + "\u001F" - + "\u0020" + "\u0085" + "\u00A0"); + + " " + "\u0085" + "\u00A0"); private static String stringLStrip(String self, String chars) { CharMatcher matcher = CharMatcher.anyOf(chars); diff --git a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java index 607ee5c3db8fd0..4c54059625c77c 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java @@ -824,7 +824,10 @@ public void repositoryRule() throws Exception { "", "my_repo_rule = repository_rule(", " implementation = _impl,", - " doc = 'My repository rule',", + " doc = '''My repository rule", + "", + " With details", + " ''',", " attrs = {", " 'a': attr.string(doc = 'My doc', default = 'foo'),", " 'b': attr.string(mandatory = True),", @@ -855,7 +858,7 @@ public void repositoryRule() throws Exception { .setRuleName("foo.repo_rule") .setOriginKey( OriginKey.newBuilder().setName("my_repo_rule").setFile("//:dep.bzl").build()) - .setDocString("My repository rule") + .setDocString("My repository rule\n\nWith details") .addAllAttribute(ModuleInfoExtractor.IMPLICIT_REPOSITORY_RULE_ATTRIBUTES) .addAttribute( AttributeInfo.newBuilder() @@ -878,7 +881,9 @@ public void moduleExtension() throws Exception { scratch.file( "dep.bzl", "_install = tag_class(", - " doc = 'Install',", + " doc = '''Install", + " ", + " With details''',", " attrs = {", " 'artifacts': attr.string_list(doc = 'Artifacts'),", " '_hidden': attr.bool(),", @@ -896,7 +901,9 @@ public void moduleExtension() throws Exception { " pass", "", "my_ext = module_extension(", - " doc = 'My extension',", + " doc = '''My extension", + "", + " With details''',", " tag_classes = {", " 'install': _install,", " 'artifact': _artifact,", @@ -925,12 +932,12 @@ public void moduleExtension() throws Exception { .containsExactly( ModuleExtensionInfo.newBuilder() .setExtensionName("foo.ext") - .setDocString("My extension") + .setDocString("My extension\n\nWith details") .setOriginKey(OriginKey.newBuilder().setFile("//:dep.bzl").build()) .addTagClass( ModuleExtensionTagClassInfo.newBuilder() .setTagName("install") - .setDocString("Install") + .setDocString("Install\n\nWith details") .addAttribute( AttributeInfo.newBuilder() .setName("artifacts") diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java index 67e17504c6c1b3..dbbfc96f4b5e73 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java @@ -870,6 +870,11 @@ public void testAttrDoc( Attribute documented = buildAttribute("documented", String.format("attr.%s(doc='foo')", attrType)); assertThat(documented.getDoc()).isEqualTo("foo"); + Attribute documentedNeedingDedent = + buildAttribute( + "documented", + String.format("attr.%s(doc='''foo\n\n More details.\n ''')", attrType)); + assertThat(documentedNeedingDedent.getDoc()).isEqualTo("foo\n\nMore details."); Attribute undocumented = buildAttribute("undocumented", String.format("attr.%s()", attrType)); assertThat(undocumented.getDoc()).isNull(); } @@ -899,11 +904,21 @@ public void testRuleDoc() throws Exception { ev, "def impl(ctx):", " return None", - "documented_rule = rule(impl, doc='My doc string')", + "documented_rule = rule(impl, doc = 'My doc string')", + "long_documented_rule = rule(", + " impl,", + " doc = '''Long doc", + "", + " With details", + "''',", + ")", "undocumented_rule = rule(impl)"); StarlarkRuleFunction documentedRule = (StarlarkRuleFunction) ev.lookup("documented_rule"); + StarlarkRuleFunction longDocumentedRule = + (StarlarkRuleFunction) ev.lookup("long_documented_rule"); StarlarkRuleFunction undocumentedRule = (StarlarkRuleFunction) ev.lookup("undocumented_rule"); assertThat(documentedRule.getDocumentation()).hasValue("My doc string"); + assertThat(longDocumentedRule.getDocumentation()).hasValue("Long doc\n\nWith details"); assertThat(undocumentedRule.getDocumentation()).isEmpty(); } @@ -1720,10 +1735,13 @@ public void declaredProviderDocumentation() throws Exception { evalAndExport( ev, "UndocumentedInfo = provider()", - "DocumentedInfo = provider(doc = 'My documented provider')", + "DocumentedInfo = provider(doc = '''", + " My documented provider", + "", + " Details''')", // Note fields below are not alphabetized "SchemafulWithoutDocsInfo = provider(fields = ['b', 'a'])", - "SchemafulWithDocsInfo = provider(fields = {'b': 'Field b', 'a': 'Field a'})"); + "SchemafulWithDocsInfo = provider(fields = {'b': 'Field b', 'a': 'Field\\n a'})"); StarlarkProvider undocumentedInfo = (StarlarkProvider) ev.lookup("UndocumentedInfo"); StarlarkProvider documentedInfo = (StarlarkProvider) ev.lookup("DocumentedInfo"); @@ -1732,11 +1750,11 @@ public void declaredProviderDocumentation() throws Exception { StarlarkProvider schemafulWithDocsInfo = (StarlarkProvider) ev.lookup("SchemafulWithDocsInfo"); assertThat(undocumentedInfo.getDocumentation()).isEmpty(); - assertThat(documentedInfo.getDocumentation()).hasValue("My documented provider"); + assertThat(documentedInfo.getDocumentation()).hasValue("My documented provider\n\nDetails"); assertThat(schemafulWithoutDocsInfo.getSchema()) .containsExactly("b", Optional.empty(), "a", Optional.empty()); assertThat(schemafulWithDocsInfo.getSchema()) - .containsExactly("b", Optional.of("Field b"), "a", Optional.of("Field a")); + .containsExactly("b", Optional.of("Field b"), "a", Optional.of("Field\na")); } @Test @@ -2199,10 +2217,20 @@ public void aspectDoc() throws Exception { "def _impl(target, ctx):", // " pass", "documented_aspect = aspect(_impl, doc='My doc string')", + "long_documented_aspect = aspect(", + " _impl,", + " doc='''", + " My doc string", + " ", + " With details''',", + ")", "undocumented_aspect = aspect(_impl)"); StarlarkDefinedAspect documentedAspect = (StarlarkDefinedAspect) ev.lookup("documented_aspect"); assertThat(documentedAspect.getDocumentation()).hasValue("My doc string"); + StarlarkDefinedAspect longDocumentedAspect = + (StarlarkDefinedAspect) ev.lookup("long_documented_aspect"); + assertThat(longDocumentedAspect.getDocumentation()).hasValue("My doc string\n\nWith details"); StarlarkDefinedAspect undocumentedAspect = (StarlarkDefinedAspect) ev.lookup("undocumented_aspect"); assertThat(undocumentedAspect.getDocumentation()).isEmpty(); diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/function_wrap_multiple_lines_test/golden.textproto b/src/test/java/com/google/devtools/build/skydoc/testdata/function_wrap_multiple_lines_test/golden.textproto index 20846fe4f06857..e3255e3c534f38 100644 --- a/src/test/java/com/google/devtools/build/skydoc/testdata/function_wrap_multiple_lines_test/golden.textproto +++ b/src/test/java/com/google/devtools/build/skydoc/testdata/function_wrap_multiple_lines_test/golden.textproto @@ -123,7 +123,7 @@ rule_info { } attribute { name: "deps" - doc_string: "\nThe dependencies to use. Defaults to the most recent ANTLR 3 release,\nbut if you need to use a different version, you can specify the\ndependencies here.\n" + doc_string: "The dependencies to use. Defaults to the most recent ANTLR 3 release,\nbut if you need to use a different version, you can specify the\ndependencies here." type: LABEL_LIST default_value: "[Label(\"@antlr3_runtimes//:tool\")]" } diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/misc_apis_test/golden.textproto b/src/test/java/com/google/devtools/build/skydoc/testdata/misc_apis_test/golden.textproto index ff7dbce0f141f4..3a7f865d521330 100644 --- a/src/test/java/com/google/devtools/build/skydoc/testdata/misc_apis_test/golden.textproto +++ b/src/test/java/com/google/devtools/build/skydoc/testdata/misc_apis_test/golden.textproto @@ -9,7 +9,7 @@ rule_info { } attribute { name: "deps" - doc_string: "\nA list of dependencies.\n" + doc_string: "A list of dependencies." type: LABEL_LIST provider_name_group { provider_name: "MyInfo" diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/provider_basic_test/golden.textproto b/src/test/java/com/google/devtools/build/skydoc/testdata/provider_basic_test/golden.textproto index 14c7b778a64460..0f82fdc11fb27a 100644 --- a/src/test/java/com/google/devtools/build/skydoc/testdata/provider_basic_test/golden.textproto +++ b/src/test/java/com/google/devtools/build/skydoc/testdata/provider_basic_test/golden.textproto @@ -15,7 +15,7 @@ provider_info { } provider_info { provider_name: "MyVeryDocumentedInfo" - doc_string: "\nA provider with some really neat documentation.\nLook on my works, ye mighty, and despair!\n" + doc_string: "A provider with some really neat documentation.\nLook on my works, ye mighty, and despair!" field_info { name: "favorite_food" doc_string: "A string representing my favorite food" diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/unknown_predefines_test/golden.textproto b/src/test/java/com/google/devtools/build/skydoc/testdata/unknown_predefines_test/golden.textproto index 00e84866d2bbb7..6936c857c0484a 100644 --- a/src/test/java/com/google/devtools/build/skydoc/testdata/unknown_predefines_test/golden.textproto +++ b/src/test/java/com/google/devtools/build/skydoc/testdata/unknown_predefines_test/golden.textproto @@ -38,4 +38,4 @@ func_info { mandatory: true } } -module_docstring: "\nTests that the documentation is generated even when unknown modules, calls, or providers are used.\n" +module_docstring: "Tests that the documentation is generated even when unknown modules, calls, or providers are used." diff --git a/src/test/java/net/starlark/java/eval/BUILD b/src/test/java/net/starlark/java/eval/BUILD index 82386955da80e7..3249dc9e1dea0b 100644 --- a/src/test/java/net/starlark/java/eval/BUILD +++ b/src/test/java/net/starlark/java/eval/BUILD @@ -24,6 +24,7 @@ java_test( "MutabilityTest.java", "PrinterTest.java", "StarlarkAnnotationsTest.java", + "StarlarkClassTest.java", "StarlarkEvaluationTest.java", "StarlarkFlagGuardingTest.java", "StarlarkListTest.java", diff --git a/src/test/java/net/starlark/java/eval/EvalTests.java b/src/test/java/net/starlark/java/eval/EvalTests.java index ab364c13d6e83a..63105613cf7b09 100644 --- a/src/test/java/net/starlark/java/eval/EvalTests.java +++ b/src/test/java/net/starlark/java/eval/EvalTests.java @@ -26,6 +26,7 @@ MethodLibraryTest.class, MutabilityTest.class, PrinterTest.class, + StarlarkClassTest.class, StarlarkEvaluationTest.class, StarlarkFlagGuardingTest.class, StarlarkAnnotationsTest.class, diff --git a/src/test/java/net/starlark/java/eval/EvaluationTest.java b/src/test/java/net/starlark/java/eval/EvaluationTest.java index 2f4c37fc8c8b7f..241110df864cf4 100644 --- a/src/test/java/net/starlark/java/eval/EvaluationTest.java +++ b/src/test/java/net/starlark/java/eval/EvaluationTest.java @@ -791,7 +791,8 @@ public void moduleWithDocString() throws Exception { assertThat(module.getDocumentation()).isNull(); ParserInput input = ParserInput.fromLines( - "\"\"\"Module doc header", // + "\"\"\"", + "Module doc header", // "", "Module doc details", "\"\"\"", @@ -805,7 +806,7 @@ public void moduleWithDocString() throws Exception { StarlarkThread thread = new StarlarkThread(mu, StarlarkSemantics.DEFAULT); Starlark.execFile(input, FileOptions.DEFAULT, module, thread); } - assertThat(module.getDocumentation()).isEqualTo("Module doc header\n\nModule doc details\n"); + assertThat(module.getDocumentation()).isEqualTo("Module doc header\n\nModule doc details"); } @Test diff --git a/src/test/java/net/starlark/java/eval/StarlarkClassTest.java b/src/test/java/net/starlark/java/eval/StarlarkClassTest.java new file mode 100644 index 00000000000000..6acb968a1cdeb9 --- /dev/null +++ b/src/test/java/net/starlark/java/eval/StarlarkClassTest.java @@ -0,0 +1,93 @@ +// Copyright 2023 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.eval; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for static utility methods in the {@link Starlark} class. */ +@RunWith(JUnit4.class) +public final class StarlarkClassTest { + + @Test + public void trimDocString() { + // See https://peps.python.org/pep-0257/#handling-docstring-indentation + // Single line + assertThat(Starlark.trimDocString("")).isEmpty(); + assertThat(Starlark.trimDocString(" ")).isEmpty(); + assertThat(Starlark.trimDocString("\t\t\t")).isEmpty(); + assertThat(Starlark.trimDocString("Hello world")).isEqualTo("Hello world"); + assertThat(Starlark.trimDocString(" Hello world ")).isEqualTo("Hello world"); + // First line is always fully trimmed, regardless of subsequent indentation levels. + assertThat(Starlark.trimDocString(" Hello\t\nworld")).isEqualTo("Hello\nworld"); + assertThat(Starlark.trimDocString(" Hello \n world")).isEqualTo("Hello\nworld"); + assertThat(Starlark.trimDocString(" Hello \n world")).isEqualTo("Hello\nworld"); + // Subsequent lines are dedented to their minimal indentation level and fully right-trimmed + assertThat(Starlark.trimDocString(" Hello \n world \n and \n good-bye ")) + .isEqualTo("Hello\n world\nand\n good-bye"); + // ... and the first line's indentation does not affect minimal indentation level computation. + assertThat(Starlark.trimDocString(" Hello\n world\n and\n good-bye")) + .isEqualTo("Hello\n world\n and\ngood-bye"); + // Blank lines are trimmed and do not affect indentation level computation + assertThat( + Starlark.trimDocString( + " Hello \n\n world \n\n\n \n and \n \n good-bye ")) + .isEqualTo("Hello\n\n world\n\n\n\nand\n\n good-bye"); + // Windows-style \r\n is simplified to \n + assertThat(Starlark.trimDocString("Hello\r\nworld")).isEqualTo("Hello\nworld"); + assertThat(Starlark.trimDocString("Hello\r\n\r\nworld")).isEqualTo("Hello\n\nworld"); + // Leading and trailing blank lines are removed + assertThat(Starlark.trimDocString("\n\n\n")).isEmpty(); + assertThat(Starlark.trimDocString("\r\n\r\n\r\n")).isEmpty(); + assertThat(Starlark.trimDocString("\n \n \n ")).isEmpty(); + assertThat(Starlark.trimDocString("\n \r\n \r\n ")).isEmpty(); + assertThat(Starlark.trimDocString("\n\r\nHello world\n\r\n")).isEqualTo("Hello world"); + assertThat(Starlark.trimDocString("\n\n \nHello\nworld\n\n \n\t")).isEqualTo("Hello\nworld"); + assertThat(Starlark.trimDocString("\n\n \t\nHello\n world\n \n")).isEqualTo("Hello\n world"); + // Tabs are expanded to size 8 (following Python convention) + assertThat(Starlark.trimDocString("Hello\tworld")).isEqualTo("Hello world"); + assertThat(Starlark.trimDocString("\n\tHello\n\t\tworld")).isEqualTo("Hello\n world"); + assertThat(Starlark.trimDocString("\n \tHello\n\t\tworld")).isEqualTo("Hello\n world"); + assertThat(Starlark.trimDocString("\n Hello\n\tworld")).isEqualTo("Hello\n world"); + } + + @Test + public void expandTabs() { + assertThat(Starlark.expandTabs("", 8)).isEmpty(); + assertThat(Starlark.expandTabs("Hello\nworld", 8)).isEqualTo("Hello\nworld"); + + assertThat(Starlark.expandTabs("\t", 1)).isEqualTo(" "); + assertThat(Starlark.expandTabs("\t", 2)).isEqualTo(" "); + assertThat(Starlark.expandTabs(" \t", 2)).isEqualTo(" "); + assertThat(Starlark.expandTabs("\t", 8)).isEqualTo(" "); + assertThat(Starlark.expandTabs(" \t", 8)).isEqualTo(" "); + + assertThat(Starlark.expandTabs("01\t012\t0123\t01234", 4)).isEqualTo("01 012 0123 01234"); + assertThat(Starlark.expandTabs("01\t012\t0123\t01234", 8)) + .isEqualTo("01 012 0123 01234"); + + assertThat(Starlark.expandTabs("01\t\n\t012\t0123\t01234", 4)) + .isEqualTo("01 \n 012 0123 01234"); + assertThat(Starlark.expandTabs("\r01\r\n\t012\r\n\t0123\t01234\n", 8)) + .isEqualTo("\r01\r\n 012\r\n 0123 01234\n"); + + assertThrows(IllegalArgumentException.class, () -> Starlark.expandTabs("\t", 0)); + assertThrows(IllegalArgumentException.class, () -> Starlark.expandTabs("\t", -1)); + } +} diff --git a/src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java b/src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java index 222276ad929945..791ad657afc789 100644 --- a/src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java +++ b/src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java @@ -20,7 +20,9 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.Iterator; import java.util.List; +import java.util.Locale; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -30,7 +32,7 @@ public final class DocstringUtils { private DocstringUtils() {} // uninstantiable /** - * Parses a docstring. + * Parses a trimmed docstring. * *

The format of the docstring is as follows * @@ -58,7 +60,12 @@ private DocstringUtils() {} // uninstantiable * """ * } * - * @param docstring a docstring of the format described above + *

We expect the docstring to already be trimmed and dedented to a minimal common indentation + * level by {@link Starlark#trimDocString} or an equivalent PEP-257 style trim() implementation; + * note that {@link StarlarkFunction#getDocumentation} returns a correctly trimmed and dedented + * doc string. + * + * @param doc a docstring of the format described above * @param parseErrors a list to which parsing error messages are written * @return the parsed docstring information */ @@ -166,64 +173,35 @@ public String getDescription() { } private static class DocstringParser { - private final String docstring; - /** Start offset of the current line. */ - private int startOfLineOffset = 0; - /** End offset of the current line. */ - private int endOfLineOffset = 0; - /** Current line number within the docstring. */ - private int lineNumber = 0; /** - * The indentation of the docstring literal in the source file. - * - *

Every line except the first one must be indented by at least that many spaces. + * Current line number within the docstring, 1-based; 0 indicates that parsing has not started; + * {@code lineNumber > lines.size()} indicates EOF. */ - private int baselineIndentation = 0; + private int lineNumber = 0; + /** Whether there was a blank line before the current line. */ private boolean blankLineBefore = false; + /** Whether we've seen a special section, e.g. 'Args:', already. */ private boolean specialSectionsStarted = false; - /** List of all parsed lines in the docstring so far, including all indentation. */ - private ArrayList originalLines = new ArrayList<>(); - /** - * The current line in the docstring with the baseline indentation removed. - * - *

If the indentation of a docstring line is less than the expected {@link - * #baselineIndentation}, only the existing indentation is stripped; none of the remaining - * characters are cut off. - */ + + /** List of all parsed lines in the docstring. */ + private final ImmutableList lines; + + /** Iterator over lines. */ + private final Iterator linesIter; + + /** The current line in the docstring. */ private String line = ""; + /** Errors that occurred so far. */ private final List errors = new ArrayList<>(); private DocstringParser(String docstring) { - this.docstring = docstring; - - // Infer the indentation level: - // the smallest amount of leading whitespace - // common to all non-blank lines except the first. - int indentation = Integer.MAX_VALUE; - boolean first = true; - for (String line : Splitter.on("\n").split(docstring)) { - // ignore first line - if (first) { - first = false; - continue; - } - // count leading spaces - int i; - for (i = 0; i < line.length() && line.charAt(i) == ' '; i++) {} - if (i != line.length()) { - indentation = Math.min(indentation, i); - } - } - if (indentation == Integer.MAX_VALUE) { - indentation = 0; - } - + this.lines = ImmutableList.copyOf(Splitter.on("\n").split(docstring)); + this.linesIter = lines.iterator(); + // Load the summary line nextLine(); - // the indentation is only relevant for the following lines, not the first one: - this.baselineIndentation = indentation; } /** @@ -232,69 +210,20 @@ private DocstringParser(String docstring) { * @return whether there are lines remaining to be parsed */ private boolean nextLine() { - if (startOfLineOffset >= docstring.length()) { - return false; - } - blankLineBefore = line.trim().isEmpty(); - startOfLineOffset = endOfLineOffset; - if (startOfLineOffset >= docstring.length()) { - // Previous line was the last; previous line had no trailing newline character. + if (linesIter.hasNext()) { + blankLineBefore = line.trim().isEmpty(); + line = linesIter.next(); + lineNumber++; + return true; + } else { line = ""; + lineNumber = lines.size() + 1; return false; } - // If not the first line, advance start past the newline character. In the case where there is - // no more content, then the previous line was the second-to-last line and this last line is - // empty. - if (docstring.charAt(startOfLineOffset) == '\n') { - startOfLineOffset += 1; - } - lineNumber++; - endOfLineOffset = docstring.indexOf('\n', startOfLineOffset); - if (endOfLineOffset < 0) { - endOfLineOffset = docstring.length(); - } - String originalLine = docstring.substring(startOfLineOffset, endOfLineOffset); - originalLines.add(originalLine); - int indentation = getIndentation(originalLine); - if (endOfLineOffset == docstring.length() && startOfLineOffset != 0) { - if (!originalLine.trim().isEmpty()) { - error("closing docstring quote should be on its own line, indented the same as the " - + "opening quote"); - } else if (indentation != baselineIndentation) { - error("closing docstring quote should be indented the same as the opening quote"); - } - } - if (originalLine.trim().isEmpty()) { - line = ""; - } else { - if (indentation < baselineIndentation) { - error( - "line indented too little (here: " - + indentation - + " spaces; expected: " - + baselineIndentation - + " spaces)"); - startOfLineOffset += indentation; - } else { - startOfLineOffset += baselineIndentation; - } - line = docstring.substring(startOfLineOffset, endOfLineOffset); - } - return true; - } - - /** - * Returns whether the current line is the last one in the docstring. - * - *

It is possible for both this function and {@link #eof} to return true if all content has - * been exhausted, or if the last line is empty. - */ - private boolean onLastLine() { - return endOfLineOffset >= docstring.length(); } private boolean eof() { - return startOfLineOffset >= docstring.length(); + return lineNumber > lines.size(); } private static int getIndentation(String line) { @@ -310,7 +239,7 @@ private void error(String message) { } private void error(int lineNumber, String message) { - errors.add(new DocstringParseError(message, lineNumber, originalLines.get(lineNumber - 1))); + errors.add(new DocstringParseError(message, lineNumber, lines.get(lineNumber - 1))); } private void parseArgumentSection( @@ -375,9 +304,7 @@ DocstringInfo parse() { + " \n\n" + "For more details, please have a look at the documentation."); } - if (!(onLastLine() && line.trim().isEmpty())) { - longDescriptionLines.add(line); - } + longDescriptionLines.add(line); nextLine(); } } @@ -399,7 +326,7 @@ private void checkSectionStart(boolean duplicateSection) { } private String checkForNonStandardDeprecation(String line) { - if (line.toLowerCase().startsWith("deprecated:") || line.contains("DEPRECATED")) { + if (line.toLowerCase(Locale.ROOT).startsWith("deprecated:") || line.contains("DEPRECATED")) { error( "use a 'Deprecated:' section for deprecations, similar to a 'Returns:' section:\n\n" + "Deprecated:\n" @@ -562,7 +489,10 @@ public String getMessage() { return message; } - /** Returns the line number in the containing Starlark file which contains this error. */ + /** + * Returns the line number (skipping leading blank lines, if any) in the original doc string + * which contains this error. + */ public int getLineNumber() { return lineNumber; }