Trim and dedent all Starlark doc strings following PEP-257

Documentation processors often require doc strings to be dedented. For example,
Markdown interprets indented blocks as code. This means before handing doc
strings to any form of API doc processor (and ideally - when storing the doc
string), we want to dedent them to a minimal indentation level and trim blank
lines.

In the Python world, the standard algorithm for doing so is specified in
PEP-257.

Until now, we dedented multiline function doc strings in DocstringUtils
using an algorithm which differed from PEP-257 in a number of corner cases.
Meanwhile, all other docs (doc strings for rules, attributes, providers etc.)
were not trimmed/dedented at all, despite often containing multiple lines.

To fix, we introduce Starlark.trimDocString and use it comprehensively on
all doc strings. Whenever possible, we store the docstring in its trimmed
form. The one exception is function docstrings, because they are stored at
parse time, not eval time; we have to trim them in the accessor method.

This change allows us to massively simplify the DocstringUtils parser, since
it no longer needs to mix low-level string munging for dedenting/trimming
with the task of finding args, returns, and deprecation stanzas.

A more comprehensive alternative to #13403
Fixes #13402

PiperOrigin-RevId: 552859517
Change-Id: I225f064c7b38f2fdbf78242d5b4597ec545518d4

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)) {

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"),

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());
   }
 }

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,35 +172,23 @@ 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
   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
   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<String> toTrimmedString(Object doc) {
+    return Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString);
   }
 
   @Override

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<String> toTrimmedString(Object doc) {
+    return Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString);
+  }
 }

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;