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, 192 insertions, 22 deletions

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