Skylint: report missing documentation for a function's return value

In addition to checking the function parameter documentation,
skylint now also checks whether the return value is documented in a
'Returns:' section in the docstring.

The same restrictions as for parameters apply:
- Private functions need no documentation
- If a function has a single line docstring, it need not document the
  return value.

In addition, I improved the docstring parsing:

- Previously, the beginning and end of a section (e.g. 'Args:',
  'Returns:') were determined by line breaks. Now, they're determined by
  indentation and missing line breaks are reported. This change should
  make the docstring parser more robust.
- Additional indentation is not warned against anymore.
  There are many situations where it makes sense, like example code.

Both of these changes were motivated by the results of the linter on
Skylark files "in the wild".

RELNOTES: none
PiperOrigin-RevId: 168660248

2 files changed, 200 insertions, 68 deletions

diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringChecker.java
index 53e0e8d621..915b10ea83 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringChecker.java
@@ -15,12 +15,12 @@
 package com.google.devtools.skylark.skylint;
 
 import static com.google.devtools.skylark.skylint.DocstringUtils.extractDocstring;
-import static com.google.devtools.skylark.skylint.DocstringUtils.parseDocString;
 
 import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.Expression;
 import com.google.devtools.build.lib.syntax.FunctionDefStatement;
 import com.google.devtools.build.lib.syntax.Parameter;
+import com.google.devtools.build.lib.syntax.ReturnStatement;
 import com.google.devtools.build.lib.syntax.StringLiteral;
 import com.google.devtools.build.lib.syntax.SyntaxTreeVisitor;
 import com.google.devtools.skylark.skylint.DocstringUtils.DocstringInfo;
@@ -33,6 +33,7 @@ import java.util.List;
 /** Checks the existence of docstrings. */
 public class DocstringChecker extends SyntaxTreeVisitor {
   private final List<Issue> issues = new ArrayList<>();
+  private boolean containsReturnWithValue = false;
 
   public static List<Issue> check(BuildFileAST ast) {
     DocstringChecker checker = new DocstringChecker();
@@ -47,19 +48,25 @@ public class DocstringChecker extends SyntaxTreeVisitor {
       issues.add(new Issue("file has no module docstring", node.getLocation()));
     } else {
       List<DocstringParseError> errors = new ArrayList<>();
-      parseDocString(moduleDocstring.getValue(), errors);
+      parseDocstring(moduleDocstring, errors);
       for (DocstringParseError error : errors) {
-        LinterLocation loc =
-            new LinterLocation(
-                moduleDocstring.getLocation().getStartLine() + error.lineNumber - 1, 1);
-        issues.add(new Issue("invalid docstring format: " + error.message, loc));
+        issues.add(docstringParseErrorToIssue(moduleDocstring, error));
       }
     }
     super.visit(node);
   }
 
   @Override
+  public void visit(ReturnStatement node) {
+    if (node.getReturnExpression() != null) {
+      containsReturnWithValue = true;
+    }
+  }
+
+  @Override
   public void visit(FunctionDefStatement node) {
+    containsReturnWithValue = false;
+    super.visit(node);
     StringLiteral functionDocstring = extractDocstring(node.getStatements());
     if (functionDocstring == null && !node.getIdentifier().getName().startsWith("_")) {
       issues.add(
@@ -71,19 +78,40 @@ public class DocstringChecker extends SyntaxTreeVisitor {
       return;
     }
     List<DocstringParseError> errors = new ArrayList<>();
-    DocstringInfo info = parseDocString(functionDocstring.getValue(), errors);
+    DocstringInfo info = parseDocstring(functionDocstring, errors);
     for (DocstringParseError error : errors) {
-      LinterLocation loc =
-          new LinterLocation(
-              functionDocstring.getLocation().getStartLine() + error.lineNumber - 1, 1);
-      issues.add(new Issue("invalid docstring format: " + error.message, loc));
+      issues.add(docstringParseErrorToIssue(functionDocstring, error));
+    }
+    if (!info.isSingleLineDocstring()) {
+      checkMultilineFunctionDocstring(
+          node, functionDocstring, info, containsReturnWithValue, issues);
+    }
+  }
+
+  private static DocstringInfo parseDocstring(
+      StringLiteral functionDocstring, List<DocstringParseError> errors) {
+    int indentation = functionDocstring.getLocation().getStartLineAndColumn().getColumn() - 1;
+    return DocstringUtils.parseDocstring(functionDocstring.getValue(), indentation, errors);
+  }
+
+  private static void checkMultilineFunctionDocstring(
+      FunctionDefStatement functionDef,
+      StringLiteral docstringLiteral,
+      DocstringInfo docstring,
+      boolean functionReturnsWithValue,
+      List<Issue> issues) {
+    if (functionReturnsWithValue && docstring.returns.isEmpty()) {
+      issues.add(
+          new Issue(
+              "incomplete docstring: the return value is not documented",
+              docstringLiteral.getLocation()));
     }
     List<String> documentedParams = new ArrayList<>();
-    for (ParameterDoc param : info.parameters) {
+    for (ParameterDoc param : docstring.parameters) {
       documentedParams.add(param.parameterName);
     }
     List<String> declaredParams = new ArrayList<>();
-    for (Parameter<Expression, Expression> param : node.getParameters()) {
+    for (Parameter<Expression, Expression> param : functionDef.getParameters()) {
       if (param.getName() != null) {
         String name = param.getName();
         if (param.isStar()) {
@@ -95,14 +123,19 @@ public class DocstringChecker extends SyntaxTreeVisitor {
         declaredParams.add(name);
       }
     }
+    checkParamListsMatch(docstringLiteral, documentedParams, declaredParams, issues);
+  }
+
+  private static void checkParamListsMatch(
+      StringLiteral docstringLiteral,
+      List<String> documentedParams,
+      List<String> declaredParams,
+      List<Issue> issues) {
     if (documentedParams.isEmpty() && !declaredParams.isEmpty()) {
-      // One-line docstrings don't have to specify parameters:
-      if (!info.longDescription.isEmpty()) {
-        issues.add(
-            new Issue(
-                "incomplete docstring: the function parameters are not documented",
-                functionDocstring.getLocation()));
-      }
+      issues.add(
+          new Issue(
+              "incomplete docstring: the function parameters are not documented",
+              docstringLiteral.getLocation()));
       return;
     }
     for (String param : declaredParams) {
@@ -110,7 +143,7 @@ public class DocstringChecker extends SyntaxTreeVisitor {
         issues.add(
             new Issue(
                 "incomplete docstring: parameter '" + param + "' not documented",
-                functionDocstring.getLocation()));
+                docstringLiteral.getLocation()));
       }
     }
     for (String param : documentedParams) {
@@ -120,7 +153,7 @@ public class DocstringChecker extends SyntaxTreeVisitor {
                 "inconsistent docstring: parameter '"
                     + param
                     + "' appears in docstring but not in function signature",
-                functionDocstring.getLocation()));
+                docstringLiteral.getLocation()));
       }
     }
     if (new LinkedHashSet<>(declaredParams).equals(new LinkedHashSet<>(documentedParams))
@@ -133,8 +166,13 @@ public class DocstringChecker extends SyntaxTreeVisitor {
               + "Documentation order: "
               + String.join(", ", documentedParams)
               + "\n";
-      issues.add(new Issue(message, functionDocstring.getLocation()));
+      issues.add(new Issue(message, docstringLiteral.getLocation()));
     }
   }
 
+  private Issue docstringParseErrorToIssue(StringLiteral docstring, DocstringParseError error) {
+    LinterLocation loc =
+        new LinterLocation(docstring.getLocation().getStartLine() + error.lineNumber - 1, 1);
+    return new Issue("invalid docstring format: " + error.message, loc);
+  }
 }
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringUtils.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringUtils.java
index 747aec2ca8..073bf44568 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringUtils.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringUtils.java
@@ -67,15 +67,21 @@ public final class DocstringUtils {
    *     must be indented by (at least) two spaces
    *   another_parameter (unused, mutable): a parameter may be followed
    *     by additional attributes in parentheses
+   *
+   * Returns:
+   *   Description of the return value.
+   *   Can span multiple lines.
    * """
    * }</pre>
    *
    * @param docstring a docstring of the format described above
+   * @param indentation the indentation level (number of spaces) of the docstring
    * @param parseErrors a list to which parsing error messages are written
    * @return the parsed docstring information
    */
-  static DocstringInfo parseDocString(String docstring, List<DocstringParseError> parseErrors) {
-    DocstringParser parser = new DocstringParser(docstring);
+  static DocstringInfo parseDocstring(
+      String docstring, int indentation, List<DocstringParseError> parseErrors) {
+    DocstringParser parser = new DocstringParser(docstring, indentation);
     DocstringInfo result = parser.parse();
     parseErrors.addAll(parser.errors);
     return result;
@@ -84,13 +90,20 @@ public final class DocstringUtils {
   static class DocstringInfo {
     final String summary;
     final List<ParameterDoc> parameters;
+    final String returns;
     final String longDescription;
 
-    public DocstringInfo(String summary, List<ParameterDoc> parameters, String longDescription) {
+    public DocstringInfo(
+        String summary, List<ParameterDoc> parameters, String returns, String longDescription) {
       this.summary = summary;
       this.parameters = ImmutableList.copyOf(parameters);
+      this.returns = returns;
       this.longDescription = longDescription;
     }
+
+    public boolean isSingleLineDocstring() {
+      return longDescription.isEmpty() && parameters.isEmpty() && returns.isEmpty();
+    }
   }
 
   static class ParameterDoc {
@@ -110,19 +123,23 @@ public final class DocstringUtils {
     private int startOfLineOffset = 0;
     private int endOfLineOffset = -1;
     private int lineNumber = 0;
-    private int expectedIndentation = 0;
+    private int baselineIndentation = 0;
+    private boolean blankLineBefore = false;
     private String line = "";
     private final List<DocstringParseError> errors = new ArrayList<>();
 
-    DocstringParser(String docstring) {
+    DocstringParser(String docstring, int indentation) {
       this.docstring = docstring;
       nextLine();
+      // the indentation is only relevant for the following lines, not the first one:
+      this.baselineIndentation = indentation;
     }
 
     boolean nextLine() {
       if (startOfLineOffset >= docstring.length()) {
         return false;
       }
+      blankLineBefore = line.isEmpty();
       lineNumber++;
       startOfLineOffset = endOfLineOffset + 1;
       if (startOfLineOffset >= docstring.length()) {
@@ -136,21 +153,26 @@ public final class DocstringUtils {
       line = docstring.substring(startOfLineOffset, endOfLineOffset);
       int indentation = getIndentation(line);
       if (!line.isEmpty()) {
-        if (indentation < expectedIndentation) {
+        if (indentation < baselineIndentation) {
           error(
               "line indented too little (here: "
                   + indentation
-                  + " spaces; before: "
-                  + expectedIndentation
+                  + " spaces; expected: "
+                  + baselineIndentation
                   + " spaces)");
-          expectedIndentation = indentation;
+          startOfLineOffset += indentation;
+        } else {
+          startOfLineOffset += baselineIndentation;
         }
-        startOfLineOffset += expectedIndentation;
       }
       line = docstring.substring(startOfLineOffset, endOfLineOffset);
       return true;
     }
 
+    private boolean eof() {
+      return startOfLineOffset >= docstring.length();
+    }
+
     private static int getIndentation(String line) {
       int index = 0;
       while (index < line.length() && line.charAt(index) == ' ') {
@@ -166,41 +188,45 @@ public final class DocstringUtils {
     DocstringInfo parse() {
       String summary = line;
       if (!nextLine()) {
-        return new DocstringInfo(summary, Collections.emptyList(), "");
+        return new DocstringInfo(summary, Collections.emptyList(), "", "");
       }
       if (!line.isEmpty()) {
         error("the one-line summary should be followed by a blank line");
       } else {
         nextLine();
       }
-      expectedIndentation = getIndentation(line);
-      line = line.substring(expectedIndentation);
       List<String> longDescriptionLines = new ArrayList<>();
       List<ParameterDoc> params = new ArrayList<>();
-      boolean sectionStart = true;
-      do {
-        if (line.startsWith(" ")) {
-          error(
-              "line indented too much (here: "
-                  + (expectedIndentation + getIndentation(line))
-                  + " spaces; expected: "
-                  + expectedIndentation
-                  + " spaces)");
+      String returns = "";
+      while (!eof()) {
+        switch (line) {
+          case "Args:":
+            if (!blankLineBefore) {
+              error("section should be preceded by a blank line");
+            }
+            if (!params.isEmpty()) {
+              error("parameters were already documented before");
+            }
+            if (!returns.isEmpty()) {
+              error("parameters should be documented before the return value");
+            }
+            params.addAll(parseParameters());
+            break;
+          case "Returns:":
+            if (!blankLineBefore) {
+              error("section should be preceded by a blank line");
+            }
+            if (!returns.isEmpty()) {
+              error("return value was already documented before");
+            }
+            returns = parseSectionAfterHeading();
+            break;
+          default:
+            longDescriptionLines.add(line);
+            nextLine();
         }
-        if (line.equals("Args:")) {
-          if (!sectionStart) {
-            error("section should be preceded by a blank line");
-          }
-          if (!params.isEmpty()) {
-            error("parameters were already documented before");
-          }
-          params.addAll(parseParameters());
-        } else {
-          longDescriptionLines.add(line);
-        }
-        sectionStart = line.isEmpty();
-      } while (nextLine());
-      return new DocstringInfo(summary, params, String.join("\n", longDescriptionLines));
+      }
+      return new DocstringInfo(summary, params, returns, String.join("\n", longDescriptionLines));
     }
 
     private static final Pattern paramLineMatcher =
@@ -212,13 +238,27 @@ public final class DocstringUtils {
     private List<ParameterDoc> parseParameters() {
       nextLine();
       List<ParameterDoc> params = new ArrayList<>();
-      while (!line.isEmpty()) {
-        if (getIndentation(line) != 2) {
-          error("parameter lines have to be indented by two spaces");
+      while (!eof()) {
+        if (line.isEmpty()) {
+          nextLine();
+          continue;
+        }
+        if (getIndentation(line) == 0) {
+          if (!blankLineBefore) {
+            error("end of 'Args' section without blank line");
+          }
+          break;
+        }
+        String trimmedLine;
+        if (getIndentation(line) < 2) {
+          error(
+              "parameter lines have to be indented by two spaces"
+                  + " (relative to the left margin of the docstring)");
+          trimmedLine = line.substring(getIndentation(line));
         } else {
-          line = line.substring(2);
+          trimmedLine = line.substring(2);
         }
-        Matcher matcher = paramLineMatcher.matcher(line);
+        Matcher matcher = paramLineMatcher.matcher(trimmedLine);
         if (!matcher.matches()) {
           error("invalid parameter documentation");
           nextLine();
@@ -231,13 +271,67 @@ public final class DocstringUtils {
             attributesString == null
                 ? Collections.emptyList()
                 : Arrays.asList(attributesSeparator.split(attributesString));
-        while (nextLine() && getIndentation(line) > 2) {
+        parseContinuedParamDescription(description);
+        params.add(new ParameterDoc(parameterName, attributes, description.toString().trim()));
+      }
+      return params;
+    }
+
+    /** Parses additional lines that can come after "param: foo" in an 'Args' section. */
+    private void parseContinuedParamDescription(StringBuilder description) {
+      while (nextLine()) {
+        if (line.isEmpty()) {
           description.append('\n');
-          description.append(line, getIndentation(line), line.length());
+          continue;
         }
-        params.add(new ParameterDoc(parameterName, attributes, description.toString()));
+        if (getIndentation(line) <= 2) {
+          break;
+        }
+        String trimmedLine;
+        if (getIndentation(line) < 4) {
+          error(
+              "continued parameter lines have to be indented by four spaces"
+                  + " (relative to the left margin of the docstring)");
+          trimmedLine = line.substring(getIndentation(line));
+        } else {
+          trimmedLine = line.substring(4);
+        }
+        description.append('\n');
+        description.append(trimmedLine);
       }
-      return params;
+    }
+
+    private String parseSectionAfterHeading() {
+      nextLine();
+      StringBuilder returns = new StringBuilder();
+      boolean firstLine = true;
+      while (!eof()) {
+        String trimmedLine;
+        if (line.isEmpty()) {
+          trimmedLine = line;
+        } else if (getIndentation(line) == 0) {
+          if (!blankLineBefore) {
+            error("end of section without blank line");
+          }
+          break;
+        } else {
+          if (getIndentation(line) < 2) {
+            error(
+                "text in a section has to be indented by two spaces"
+                    + " (relative to the left margin of the docstring)");
+            trimmedLine = line.substring(getIndentation(line));
+          } else {
+            trimmedLine = line.substring(2);
+          }
+        }
+        if (!firstLine) {
+          returns.append('\n');
+        }
+        returns.append(trimmedLine);
+        nextLine();
+        firstLine = false;
+      }
+      return returns.toString().trim();
     }
   }