Skylint: change the end location of missing function docstring warnings.

The end location now ends before the first statement of the function.

RELNOTES: none
PiperOrigin-RevId: 172578911

2 files changed, 19 insertions, 4 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 bc9edc37de..66b36a245f 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
@@ -16,6 +16,7 @@ package com.google.devtools.skylark.skylint;
 
 import static com.google.devtools.skylark.skylint.DocstringUtils.extractDocstring;
 
+import com.google.devtools.build.lib.events.Location.LineAndColumn;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.Expression;
 import com.google.devtools.build.lib.syntax.FunctionDefStatement;
@@ -79,11 +80,20 @@ public class DocstringChecker extends SyntaxTreeVisitor {
     super.visit(node);
     StringLiteral functionDocstring = extractDocstring(node.getStatements());
     if (functionDocstring == null && !node.getIdentifier().getName().startsWith("_")) {
+      Location start = Location.from(node.getLocation().getStartLineAndColumn());
+      Location end;
+      if (node.getStatements().isEmpty()) {
+        // The function body can be empty because the parser discards `pass` statements:
+        end = Location.from(node.getLocation().getEndLineAndColumn());
+      } else {
+        LineAndColumn lac = node.getStatements().get(0).getLocation().getStartLineAndColumn();
+        end = new Location(lac.getLine(), lac.getColumn() - 1); // right before the first statement
+      }
       issues.add(
-          Issue.create(
+          new Issue(
               MISSING_DOCSTRING_CATEGORY,
               "function '" + node.getIdentifier().getName() + "' has no docstring",
-              node.getLocation()));
+              new LocationRange(start, end)));
     }
     if (functionDocstring == null) {
       return;
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 377f0eed7c..876ce24688 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
@@ -38,12 +38,17 @@ public class DocstringCheckerTests {
   @Test
   public void reportMissingDocString() throws Exception {
     String errorMessage =
-        findIssues("# no module docstring", "def function():", "  pass # no function docstring")
+        findIssues("# no module docstring", "def function():", "  return # no function docstring")
             .toString();
     Truth.assertThat(errorMessage)
         .contains("1:1-2:1: file has no module docstring [missing-docstring]");
     Truth.assertThat(errorMessage)
-        .contains("2:1-3:30: function 'function' has no docstring [missing-docstring]");
+        .contains("2:1-3:2: function 'function' has no docstring [missing-docstring]");
+    // The following function has zero statements since the parser throws `pass` statements away.
+    // Hence we have to check this case to make sure the end location is set correctly.
+    errorMessage = findIssues("def function():", "  pass # no function docstring").toString();
+    Truth.assertThat(errorMessage)
+        .contains("1:1-2:30: function 'function' has no docstring [missing-docstring]");
   }
 
   @Test