Linter: don't require docstrings for short functions

RELNOTES: none
PiperOrigin-RevId: 174330237

2 files changed, 38 insertions, 12 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 5ecadded21..47c02e1b73 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
@@ -17,11 +17,13 @@ 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.ASTNode;
 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.Statement;
 import com.google.devtools.build.lib.syntax.StringLiteral;
 import com.google.devtools.build.lib.syntax.SyntaxTreeVisitor;
 import com.google.devtools.skylark.skylint.DocstringUtils.DocstringInfo;
@@ -37,6 +39,8 @@ public class DocstringChecker extends SyntaxTreeVisitor {
   private static final String MISSING_DOCSTRING_CATEGORY = "missing-docstring";
   private static final String INCONSISTENT_DOCSTRING_CATEGORY = "inconsistent-docstring";
   private static final String BAD_DOCSTRING_FORMAT_CATEGORY = "bad-docstring-format";
+  /** If a function is at least this many statements long, a docstring is required. */
+  private static final int FUNCTION_LENGTH_DOCSTRING_THRESHOLD = 5;
 
   private final List<Issue> issues = new ArrayList<>();
   private boolean containsReturnWithValue = false;
@@ -79,7 +83,9 @@ public class DocstringChecker extends SyntaxTreeVisitor {
     containsReturnWithValue = false;
     super.visit(node);
     StringLiteral functionDocstring = extractDocstring(node.getStatements());
-    if (functionDocstring == null && !node.getIdentifier().getName().startsWith("_")) {
+    if (functionDocstring == null
+        && !node.getIdentifier().getName().startsWith("_")
+        && countNestedStatements(node) >= FUNCTION_LENGTH_DOCSTRING_THRESHOLD) {
       Location start = Location.from(node.getLocation().getStartLineAndColumn());
       Location end;
       if (node.getStatements().isEmpty()) {
@@ -116,6 +122,21 @@ public class DocstringChecker extends SyntaxTreeVisitor {
     }
   }
 
+  private static class StatementCounter extends SyntaxTreeVisitor {
+    public int count = 0;
+
+    @Override
+    public void visitBlock(List<Statement> statements) {
+      count += statements.size();
+    }
+  }
+
+  private static int countNestedStatements(ASTNode node) {
+    StatementCounter counter = new StatementCounter();
+    counter.visit(node);
+    return counter.count;
+  }
+
   private static void checkMultilineFunctionDocstring(
       FunctionDefStatement functionDef,
       StringLiteral docstringLiteral,
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 ddfc910b61..ebaabfdaf4 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
@@ -38,22 +38,21 @@ public class DocstringCheckerTest {
   @Test
   public void reportMissingDocString() throws Exception {
     String errorMessage =
-        findIssues("# no module docstring", "def function():", "  return # no function docstring")
+        findIssues(
+                "# no module docstring",
+                "def function():",
+                "  # no function docstring",
+                "  print(1) # make sure the function is long enough",
+                "  print(2)",
+                "  print(3)",
+                "  print(4)",
+                "  print(5)")
             .toString();
     Truth.assertThat(errorMessage)
         .contains("1:1-2:1: file has no module docstring [missing-docstring]");
     Truth.assertThat(errorMessage)
         .contains(
-            "2:1-3:2: function 'function' has no docstring"
-                + " (if this function is intended to be private,"
-                + " the name should start with an underscore: '_function')"
-                + " [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:2: function 'function' has no docstring"
+            "2:1-4:2: function 'function' has no docstring"
                 + " (if this function is intended to be private,"
                 + " the name should start with an underscore: '_function')"
                 + " [missing-docstring]");
@@ -244,6 +243,12 @@ public class DocstringCheckerTest {
   }
 
   @Test
+  public void dontReportShortFunctionWithoutDocstring() throws Exception {
+    Truth.assertThat(findIssues("\"\"\"Module docstring\"\"\"", "def function():", "  pass"))
+        .isEmpty();
+  }
+
+  @Test
   public void dontReportCorrectFunctionDocstring() throws Exception {
     Truth.assertThat(
             findIssues(