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

4 files changed, 392 insertions, 90 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();
     }
   }
 
diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTests.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTests.java
index 5642f1c76a..d5cee2e2cd 100644
--- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTests.java
+++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTests.java
@@ -52,7 +52,7 @@ public class DocstringCheckerTests {
             "def f(param1, param2):",
             "  \"\"\"summary",
             "",
-            "more description",
+            "  more description",
             "\"\"\"",
             "  pass");
     Truth.assertThat(errors).hasSize(1);
@@ -142,7 +142,37 @@ public class DocstringCheckerTests {
     Truth.assertThat(errorMessage)
         .contains(
             ":5:1: invalid docstring format: "
-                + "line indented too little (here: 1 spaces; before: 2 spaces)");
+                + "line indented too little (here: 1 spaces; expected: 2 spaces)");
+  }
+
+  @Test
+  public void reportMissingReturnDocumentation() throws Exception {
+    List<Issue> errors =
+        findIssues(
+            "\"\"\" module docstring \"\"\"",
+            "def f():",
+            "  \"\"\"summary",
+            "",
+            "  more description",
+            "  \"\"\"",
+            "  return True");
+    Truth.assertThat(errors).hasSize(1);
+    Truth.assertThat(errors.toString())
+        .contains(":3:3: incomplete docstring: the return value is not documented");
+  }
+
+  @Test
+  public void dontReportReturnDocumentationIfNoReturnValue() throws Exception {
+    Truth.assertThat(
+            findIssues(
+                "\"\"\" module docstring \"\"\"",
+                "def f():",
+                "  \"\"\"summary",
+                "",
+                "  more description",
+                "  \"\"\"",
+                "  return"))
+        .isEmpty();
   }
 
   @Test
@@ -157,12 +187,13 @@ public class DocstringCheckerTests {
   }
 
   @Test
-  public void dontReportSummaryDocstringWithoutParameters() throws Exception {
+  public void dontReportSingleLineDocstring() throws Exception {
     Truth.assertThat(
             findIssues(
                 "\"\"\"module docstring\"\"\"",
                 "def function(param1, param2):",
-                "  \"\"\"summary without parameter docs is fine\"\"\""))
+                "  \"\"\"single line docstring is fine\"\"\"",
+                "  return True"))
         .isEmpty();
   }
 
@@ -177,7 +208,7 @@ public class DocstringCheckerTests {
   }
 
   @Test
-  public void dontReportFunctionDocstringWithCorrectParameters() throws Exception {
+  public void dontReportCorrectFunctionDocstring() throws Exception {
     Truth.assertThat(
             findIssues(
                 "\"\"\" module docstring \"\"\"",
@@ -189,8 +220,11 @@ public class DocstringCheckerTests {
                 "    param2 (foo, bar): baz",
                 "    *args: foo",
                 "    **kwargs: bar",
+                "",
+                "  Returns:",
+                "    True",
                 "  \"\"\"",
-                "  pass"))
+                "  return True"))
         .isEmpty();
   }
 }
diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringUtilsTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringUtilsTest.java
index d8fdb86cff..eeddd0f491 100644
--- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringUtilsTest.java
+++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringUtilsTest.java
@@ -31,7 +31,7 @@ public class DocstringUtilsTest {
   @Test
   public void oneLineDocstring() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
-    DocstringInfo info = DocstringUtils.parseDocString("summary", errors);
+    DocstringInfo info = DocstringUtils.parseDocstring("summary", 0, errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
     Truth.assertThat(info.parameters).isEmpty();
     Truth.assertThat(info.longDescription).isEmpty();
@@ -41,7 +41,7 @@ public class DocstringUtilsTest {
   @Test
   public void missingBlankLineAfterSummary() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
-    DocstringInfo info = DocstringUtils.parseDocString("summary\nfoo", errors);
+    DocstringInfo info = DocstringUtils.parseDocstring("summary\nfoo", 0, errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
     Truth.assertThat(info.parameters).isEmpty();
     Truth.assertThat(info.longDescription).isEqualTo("foo");
@@ -52,7 +52,7 @@ public class DocstringUtilsTest {
   @Test
   public void multiParagraphDocstring() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
-    DocstringInfo info = DocstringUtils.parseDocString("summary\n\nfoo\n\nbar", errors);
+    DocstringInfo info = DocstringUtils.parseDocstring("summary\n\nfoo\n\nbar", 0, errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
     Truth.assertThat(info.parameters).isEmpty();
     Truth.assertThat(info.longDescription).isEqualTo("foo\n\nbar");
@@ -63,38 +63,106 @@ public class DocstringUtilsTest {
   public void inconsistentIndentation() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
     DocstringInfo info =
-        DocstringUtils.parseDocString("summary\n" + "\n" + "  foo\n" + "bar", errors);
+        DocstringUtils.parseDocstring("summary\n" + "\n" + "  foo\n" + "bar", 2, errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
     Truth.assertThat(info.parameters).isEmpty();
     Truth.assertThat(info.longDescription).isEqualTo("foo\nbar");
     Truth.assertThat(errors.toString())
-        .contains(":4: line indented too little (here: 0 spaces; before: 2 spaces)");
+        .contains(":4: line indented too little (here: 0 spaces; expected: 2 spaces)");
 
     errors = new ArrayList<>();
-    info = DocstringUtils.parseDocString("summary\n" + "\n" + "  foo\n" + "    bar\n", errors);
+    info =
+        DocstringUtils.parseDocstring(
+            "summary\n"
+                + "\n"
+                + "  baseline indentation\n"
+                + "    more indentation can be useful (e.g. for example code)\n",
+            2,
+            errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
     Truth.assertThat(info.parameters).isEmpty();
-    Truth.assertThat(info.longDescription).isEqualTo("foo\n  bar");
+    Truth.assertThat(info.longDescription)
+        .isEqualTo(
+            "baseline indentation\n  more indentation can be useful (e.g. for example code)");
+    Truth.assertThat(errors).isEmpty();
+  }
+
+  @Test
+  public void inconsistentIndentationInSection() throws Exception {
+    List<DocstringParseError> errors = new ArrayList<>();
+    DocstringInfo info =
+        DocstringUtils.parseDocstring(
+            "summary\n"
+                + "\n"
+                + "Returns:\n"
+                + " only one space indentation\n"
+                + "unindented line after",
+            0,
+            errors);
+    Truth.assertThat(info.returns).isEqualTo("only one space indentation");
+    Truth.assertThat(info.longDescription).isEqualTo("unindented line after");
     Truth.assertThat(errors.toString())
-        .contains(":4: line indented too much (here: 4 spaces; expected: 2 spaces)");
+        .contains(
+            ":4: text in a section has to be indented by two spaces"
+                + " (relative to the left margin of the docstring)");
+    Truth.assertThat(errors.toString()).contains(":5: end of section without blank line");
   }
 
   @Test
   public void inconsistentIndentationInParameters() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
     DocstringInfo info =
-        DocstringUtils.parseDocString("summary\n" + "\n" + "  Args:\n" + "  param1: foo\n", errors);
+        DocstringUtils.parseDocstring(
+            "summary\n"
+                + "\n"
+                + "Args:\n"
+                + " param1: only one space indentation\n"
+                + "   only three spaces indentation\n"
+                + "unindented line after",
+            0,
+            errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
     Truth.assertThat(info.parameters).hasSize(1);
+    ParameterDoc param = info.parameters.get(0);
+    Truth.assertThat(param.description)
+        .isEqualTo("only one space indentation\nonly three spaces indentation");
     Truth.assertThat(errors.toString())
-        .contains(":4: parameter lines have to be indented by two spaces");
+        .contains(
+            ":4: parameter lines have to be indented by two spaces"
+                + " (relative to the left margin of the docstring)");
+    Truth.assertThat(errors.toString())
+        .contains(
+            ":5: continued parameter lines have to be indented by four spaces"
+                + " (relative to the left margin of the docstring)");
+    Truth.assertThat(errors.toString()).contains(":6: end of 'Args' section without blank line");
+  }
+
+  @Test
+  public void docstringReturn() throws Exception {
+    List<DocstringParseError> errors = new ArrayList<>();
+    DocstringInfo info =
+        DocstringUtils.parseDocstring(
+            "summary\n"
+                + "\n"
+                + "  Returns:\n"
+                + "    line 1\n"
+                + "\n"
+                + "    line 2\n"
+                + "\n"
+                + "  remaining description",
+            2,
+            errors);
+    Truth.assertThat(info.summary).isEqualTo("summary");
+    Truth.assertThat(info.returns).isEqualTo("line 1\n\nline 2");
+    Truth.assertThat(info.longDescription).isEqualTo("remaining description");
+    Truth.assertThat(errors).isEmpty();
   }
 
   @Test
   public void docstringParameters() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
     DocstringInfo info =
-        DocstringUtils.parseDocString(
+        DocstringUtils.parseDocstring(
             "summary\n"
                 + "\n"
                 + "  Args:\n"
@@ -105,6 +173,7 @@ public class DocstringUtilsTest {
                 + "    **kwargs: kwargs\n"
                 + "\n"
                 + "  description",
+            2,
             errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
     Truth.assertThat(info.parameters).hasSize(4);
@@ -134,17 +203,54 @@ public class DocstringUtilsTest {
   }
 
   @Test
-  public void argsSectionNotPrecededByNewline() throws Exception {
+  public void docstringParametersWithBlankLines() throws Exception {
+    List<DocstringParseError> errors = new ArrayList<>();
+    DocstringInfo info =
+        DocstringUtils.parseDocstring(
+            "summary\n"
+                + "\n"
+                + "  Args:\n"
+                + "    param1: multi-\n"
+                + "\n"
+                + "      line\n"
+                + "\n"
+                + "    param2: foo\n"
+                + "\n"
+                + "  description",
+            2,
+            errors);
+    Truth.assertThat(info.summary).isEqualTo("summary");
+    Truth.assertThat(info.parameters).hasSize(2);
+    Truth.assertThat(info.longDescription).isEqualTo("description");
+    ParameterDoc firstParam = info.parameters.get(0);
+    ParameterDoc secondParam = info.parameters.get(1);
+
+    Truth.assertThat(firstParam.parameterName).isEqualTo("param1");
+    Truth.assertThat(firstParam.attributes).isEmpty();
+    Truth.assertThat(firstParam.description).isEqualTo("multi-\n\nline");
+
+    Truth.assertThat(secondParam.parameterName).isEqualTo("param2");
+    Truth.assertThat(secondParam.attributes).isEmpty();
+    Truth.assertThat(secondParam.description).isEqualTo("foo");
+
+    Truth.assertThat(errors).isEmpty();
+  }
+
+  @Test
+  public void sectionNotPrecededByNewline() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
-    DocstringUtils.parseDocString("summary\n" + "\n" + "  foo\n" + "  Args:", errors);
+    DocstringUtils.parseDocstring("summary\n" + "\n" + "  foo\n" + "  Args:", 2, errors);
+    Truth.assertThat(errors.toString()).contains(":4: section should be preceded by a blank line");
+    errors = new ArrayList<>();
+    DocstringUtils.parseDocstring("summary\n" + "\n" + "  foo\n" + "  Returns:", 2, errors);
     Truth.assertThat(errors.toString()).contains(":4: section should be preceded by a blank line");
   }
 
   @Test
-  public void twoArgsSectionsError() throws Exception {
+  public void duplicatedSectionsError() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
     DocstringInfo info =
-        DocstringUtils.parseDocString(
+        DocstringUtils.parseDocstring(
             "summary\n"
                 + "\n"
                 + "  Args:\n"
@@ -153,23 +259,53 @@ public class DocstringUtilsTest {
                 + "  Args:\n"
                 + "    param2: bar\n"
                 + "\n"
+                + "  Returns:\n"
+                + "    foo\n"
+                + "\n"
+                + "  Returns:\n"
+                + "    bar\n"
+                + "\n"
                 + "  description",
+            2,
             errors);
     Truth.assertThat(info.parameters).hasSize(2);
     Truth.assertThat(errors.toString()).contains(":6: parameters were already documented before");
+    Truth.assertThat(errors.toString()).contains(":12: return value was already documented before");
+  }
+
+  @Test
+  public void sectionsInWrongOrderError() throws Exception {
+    List<DocstringParseError> errors = new ArrayList<>();
+    DocstringInfo info =
+        DocstringUtils.parseDocstring(
+            "summary\n"
+                + "\n"
+                + "  Returns:\n"
+                + "    foo\n"
+                + "\n"
+                + "  Args:\n"
+                + "    param1: foo\n"
+                + "\n"
+                + "  description",
+            2,
+            errors);
+    Truth.assertThat(info.parameters).hasSize(1);
+    Truth.assertThat(errors.toString())
+        .contains(":6: parameters should be documented before the return value");
   }
 
   @Test
   public void invalidParameterDoc() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
     DocstringInfo info =
-        DocstringUtils.parseDocString(
+        DocstringUtils.parseDocstring(
             "summary\n"
                 + "\n"
                 + "  Args:\n"
                 + "    invalid parameter doc\n"
                 + "\n"
                 + "  description",
+            2,
             errors);
     Truth.assertThat(info.parameters).isEmpty();
     Truth.assertThat(errors.toString()).contains(":4: invalid parameter documentation");