Linter: Tolerate whitespace-only lines and complain about misaligned closing quotes

RELNOTES: None
PiperOrigin-RevId: 174319420

3 files changed, 134 insertions, 22 deletions

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 1d37bc6389..1863bccb1c 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
@@ -219,7 +219,7 @@ public final class DocstringUtils {
     /** Start offset of the current line. */
     private int startOfLineOffset = 0;
     /** End offset of the current line. */
-    private int endOfLineOffset = -1;
+    private int endOfLineOffset = 0;
     /** Current line number within the docstring. */
     private int lineNumber = 0;
     /**
@@ -261,12 +261,19 @@ public final class DocstringUtils {
       if (startOfLineOffset >= docstring.length()) {
         return false;
       }
-      blankLineBefore = line.isEmpty();
-      startOfLineOffset = endOfLineOffset + 1;
+      blankLineBefore = line.trim().isEmpty();
+      startOfLineOffset = endOfLineOffset;
       if (startOfLineOffset >= docstring.length()) {
+        // Previous line was the last; previous line had no trailing newline character.
         line = "";
         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) {
@@ -274,9 +281,18 @@ public final class DocstringUtils {
       }
       String originalLine = docstring.substring(startOfLineOffset, endOfLineOffset);
       originalLines.add(originalLine);
-      line = originalLine;
-      int indentation = getIndentation(line);
-      if (!line.isEmpty()) {
+      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: "
@@ -288,11 +304,21 @@ public final class DocstringUtils {
         } else {
           startOfLineOffset += baselineIndentation;
         }
+        line = docstring.substring(startOfLineOffset, endOfLineOffset);
       }
-      line = docstring.substring(startOfLineOffset, endOfLineOffset);
       return true;
     }
 
+    /**
+     * Returns whether the current line is the last one in the docstring.
+     *
+     * <p>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();
     }
@@ -360,7 +386,9 @@ public final class DocstringUtils {
             if (deprecated.isEmpty() && nonStandardDeprecation.isEmpty()) {
               nonStandardDeprecation = checkForNonStandardDeprecation(line);
             }
-            longDescriptionLines.add(line);
+            if (!(onLastLine() && line.trim().isEmpty())) {
+              longDescriptionLines.add(line);
+            }
             nextLine();
         }
       }
diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTest.java
index 8c2b85ddd5..ddfc910b61 100644
--- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTest.java
+++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTest.java
@@ -68,12 +68,12 @@ public class DocstringCheckerTest {
             "  \"\"\"summary",
             "",
             "  more description",
-            "\"\"\"",
+            "  \"\"\"",
             "  pass");
     Truth.assertThat(errors).hasSize(1);
     Truth.assertThat(errors.toString())
         .contains(
-            "3:3-6:3: incomplete docstring: the function parameters are not documented"
+            "3:3-6:5: incomplete docstring: the function parameters are not documented"
                 + " (no 'Args:' section found)\n"
                 + "The parameter documentation should look like this:\n"
                 + "\n"
@@ -216,7 +216,9 @@ public class DocstringCheckerTest {
                 "\"\"\"This is a module docstring",
                 "\n\"\"\"",
                 "def function():",
-                "  \"\"\" This is a function docstring\n\"\"\""))
+                "  \"\"\" This is a function docstring",
+                "",
+                "  \"\"\""))
         .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 888407c63e..ee07fd488f 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
@@ -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", 0, errors);
+    DocstringInfo info = DocstringUtils.parseDocstring("summary\n\nfoo\n\nbar\n", 0, errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
     Truth.assertThat(info.parameters).isEmpty();
     Truth.assertThat(info.longDescription).isEqualTo("foo\n\nbar");
@@ -63,7 +63,7 @@ public class DocstringUtilsTest {
   public void inconsistentIndentation() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
     DocstringInfo info =
-        DocstringUtils.parseDocstring("summary\n" + "\n" + "  foo\n" + "bar", 2, errors);
+        DocstringUtils.parseDocstring("summary\n" + "\n" + "  foo\n" + "bar\n  ", 2, errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
     Truth.assertThat(info.parameters).isEmpty();
     Truth.assertThat(info.longDescription).isEqualTo("foo\nbar");
@@ -76,7 +76,8 @@ public class DocstringUtilsTest {
             "summary\n"
                 + "\n"
                 + "  baseline indentation\n"
-                + "    more indentation can be useful (e.g. for example code)\n",
+                + "    more indentation can be useful (e.g. for example code)\n"
+                + "  ",
             2,
             errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
@@ -135,6 +136,78 @@ public class DocstringUtilsTest {
   }
 
   @Test
+  public void whitespaceOnlyLinesCountAsBlank() throws Exception {
+    List<DocstringParseError> errors = new ArrayList<>();
+    DocstringInfo info =
+        DocstringUtils.parseDocstring(
+            "summary\n"
+                + "        \n" // if not blank, would have too much indent
+                + "    description\n"
+                + "  \n"       // if not blank, would have too little indent
+                + "    Args:\n"
+                + "      foo: foo description\n"
+                + "      \n"   // if not blank, would be indented just the right amount to end param
+                               //   description but not Args section
+                + "    Returns:\n"
+                + "      returns description\n"
+                + "     \n"    // if not blank, would be section content that's indented too little
+                + "    ",
+            4,
+            errors);
+    Truth.assertThat(info.parameters).hasSize(1);
+    Truth.assertThat(info.parameters.get(0).description).isEqualTo("foo description");
+    Truth.assertThat(info.returns).isEqualTo("returns description");
+    Truth.assertThat(errors).isEmpty();
+  }
+
+  @Test
+  public void closingQuoteMustBeProperlyIndented() throws Exception {
+    List<DocstringParseError> errors = new ArrayList<>();
+    DocstringUtils.parseDocstring("summary", 4, errors);
+    Truth.assertThat(errors).isEmpty();
+
+    errors = new ArrayList<>();
+    DocstringUtils.parseDocstring(
+        "summary\n"
+            + "\n"
+            + "    more description",
+        4, errors);
+    Truth.assertThat(errors.toString())
+        .contains("3: closing docstring quote should be on its own line, indented the same as the "
+            + "opening quote");
+
+    errors = new ArrayList<>();
+    DocstringUtils.parseDocstring(
+        "summary\n"
+            + "\n"
+            + "    more description\n"
+            + "  ",      // too little
+        4, errors);
+    Truth.assertThat(errors.toString())
+        .contains("4: closing docstring quote should be indented the same as the opening quote");
+
+    errors = new ArrayList<>();
+    DocstringUtils.parseDocstring(
+        "summary\n"
+            + "\n"
+            + "    more description\n"
+            + "      ",  // too much
+        4, errors);
+    Truth.assertThat(errors.toString())
+        .contains("4: closing docstring quote should be indented the same as the opening quote");
+
+    errors = new ArrayList<>();
+    DocstringUtils.parseDocstring(
+        "summary\n"
+            + "\n"
+            + "    more description\n"
+            + "",        // too little (empty line special case)
+        4, errors);
+    Truth.assertThat(errors.toString())
+        .contains("4: closing docstring quote should be indented the same as the opening quote");
+  }
+
+  @Test
   public void emptySection() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
     DocstringUtils.parseDocstring(
@@ -169,7 +242,8 @@ public class DocstringUtilsTest {
                 + "  Returns:\n"
                 + "    line 1\n"
                 + "\n"
-                + "    line 2\n",
+                + "    line 2\n"
+                + "  ",
             2,
             errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
@@ -188,7 +262,8 @@ public class DocstringUtilsTest {
                 + "  Deprecated:\n"
                 + "    line 1\n"
                 + "\n"
-                + "    line 2\n",
+                + "    line 2\n"
+                + "  ",
             2,
             errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
@@ -201,7 +276,7 @@ public class DocstringUtilsTest {
   public void docstringDeprecatedTheWrongWay() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
     DocstringInfo info =
-        DocstringUtils.parseDocstring("summary\n" + "\n" + "  Deprecated: foo\n", 2, errors);
+        DocstringUtils.parseDocstring("summary\n" + "\n" + "  Deprecated: foo\n  ", 2, errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
     Truth.assertThat(info.deprecated).isEqualTo("Deprecated: foo");
     Truth.assertThat(info.longDescription).isEqualTo("Deprecated: foo");
@@ -211,7 +286,10 @@ public class DocstringUtilsTest {
             "3: use a 'Deprecated:' section for deprecations, similar to a 'Returns:' section");
 
     errors = new ArrayList<>();
-    info = DocstringUtils.parseDocstring("summary\n" + "\n" + "  This is DEPRECATED.\n", 2, errors);
+    info = DocstringUtils.parseDocstring(
+        "summary\n" + "\n" + "  This is DEPRECATED.\n  ",
+        2,
+        errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
     Truth.assertThat(info.deprecated).isEqualTo("This is DEPRECATED.");
     Truth.assertThat(info.longDescription).isEqualTo("This is DEPRECATED.");
@@ -233,7 +311,8 @@ public class DocstringUtilsTest {
                 + "      line\n"
                 + "    param2 (mutable, unused): bar\n"
                 + "    *args: args\n"
-                + "    **kwargs: kwargs\n",
+                + "    **kwargs: kwargs\n"
+                + "  ",
             2,
             errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
@@ -275,7 +354,8 @@ public class DocstringUtilsTest {
                 + "\n"
                 + "      line\n"
                 + "\n"
-                + "    param2: foo\n",
+                + "    param2: foo\n"
+                + "  ",
             2,
             errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
@@ -358,7 +438,8 @@ public class DocstringUtilsTest {
                 + "  Args:\n"
                 + "    param1: foo\n"
                 + "\n"
-                + "  description\n",
+                + "  description\n"
+                + "  ",
             2,
             errors);
     Truth.assertThat(info.summary).isEqualTo("summary");
@@ -387,7 +468,8 @@ public class DocstringUtilsTest {
                 + "    param1: foo\n"
                 + "\n"
                 + "  line 1\n"
-                + "  line 2\n",
+                + "  line 2\n"
+                + "  ",
             2,
             errors);
     Truth.assertThat(info.summary).isEqualTo("summary");