Creates a new issue pointing out "Arguments:" as not preferred instead of reporting that there is a missing 'Args:' section.

RELNOTES: None.
PiperOrigin-RevId: 186717757

3 files changed, 72 insertions, 11 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 35f99e9f50..54f33f405c 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
@@ -40,6 +40,7 @@ public class DocstringChecker extends SyntaxTreeVisitor {
   private static final String MISSING_FUNCTION_DOCSTRING_CATEGORY = "missing-function-docstring";
   private static final String INCONSISTENT_DOCSTRING_CATEGORY = "inconsistent-docstring";
   private static final String BAD_DOCSTRING_FORMAT_CATEGORY = "bad-docstring-format";
+  private static final String ARGS_ARGUMENTS_DOCSTRING_CATEGORY = "args-arguments-docstring";
   /** If a function is at least this many statements long, a docstring is required. */
   private static final int FUNCTION_LENGTH_DOCSTRING_THRESHOLD = 5;
 
@@ -122,6 +123,20 @@ public class DocstringChecker extends SyntaxTreeVisitor {
       checkMultilineFunctionDocstring(
           node, functionDocstring, info, containsReturnWithValue, issues);
     }
+    if (info.argumentsLocation != null) {
+      int lineOffset = functionDocstring.getLocation().getStartLine() - 1;
+      issues.add(
+          new Issue(
+              ARGS_ARGUMENTS_DOCSTRING_CATEGORY,
+              "Prefer 'Args:' to 'Arguments:' when documenting function arguments.",
+              new LocationRange(
+                  new Location(
+                      info.argumentsLocation.start.line + lineOffset,
+                      info.argumentsLocation.start.column),
+                  new Location(
+                      info.argumentsLocation.end.line + lineOffset,
+                      info.argumentsLocation.end.column))));
+    }
   }
 
   private static class StatementCounter extends SyntaxTreeVisitor {
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 0c5486e602..96c86ec6a2 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
@@ -25,6 +25,7 @@ import com.google.devtools.build.lib.syntax.FunctionDefStatement;
 import com.google.devtools.build.lib.syntax.Identifier;
 import com.google.devtools.build.lib.syntax.Statement;
 import com.google.devtools.build.lib.syntax.StringLiteral;
+import com.google.devtools.skylark.skylint.LocationRange.Location;
 import java.util.AbstractMap;
 import java.util.AbstractMap.SimpleEntry;
 import java.util.ArrayList;
@@ -183,18 +184,22 @@ public final class DocstringUtils {
     final String deprecated;
     /** Rest of the docstring that is not part of any of the special sections above. */
     final String longDescription;
+    /** The texual location of the 'Arguments:' (not 'Args:') section in the function. */
+    final LocationRange argumentsLocation;
 
     public DocstringInfo(
         String summary,
         List<ParameterDoc> parameters,
         String returns,
         String deprecated,
-        String longDescription) {
+        String longDescription,
+        LocationRange argumentsLocation) {
       this.summary = summary;
       this.parameters = ImmutableList.copyOf(parameters);
       this.returns = returns;
       this.deprecated = deprecated;
       this.longDescription = longDescription;
+      this.argumentsLocation = argumentsLocation;
     }
 
     public boolean isSingleLineDocstring() {
@@ -339,11 +344,24 @@ public final class DocstringUtils {
       errors.add(new DocstringParseError(message, lineNumber, originalLines.get(lineNumber - 1)));
     }
 
+    private void parseArgumentSection(
+        List<ParameterDoc> params, String returns, String deprecated) {
+      checkSectionStart(!params.isEmpty());
+      if (!returns.isEmpty()) {
+        error("'Args:' section should go before the 'Returns:' section");
+      }
+      if (!deprecated.isEmpty()) {
+        error("'Args:' section should go before the 'Deprecated:' section");
+      }
+      params.addAll(parseParameters());
+    }
+
     DocstringInfo parse() {
       String summary = line;
       String nonStandardDeprecation = checkForNonStandardDeprecation(line);
       if (!nextLine()) {
-        return new DocstringInfo(summary, Collections.emptyList(), "", nonStandardDeprecation, "");
+        return new DocstringInfo(
+            summary, Collections.emptyList(), "", nonStandardDeprecation, "", null);
       }
       if (!line.isEmpty()) {
         error("the one-line summary should be followed by a blank line");
@@ -355,17 +373,21 @@ public final class DocstringUtils {
       String returns = "";
       String deprecated = "";
       boolean descriptionBodyAfterSpecialSectionsReported = false;
+      LocationRange argumentsLocation = null;
       while (!eof()) {
         switch (line) {
           case "Args:":
-            checkSectionStart(!params.isEmpty());
-            if (!returns.isEmpty()) {
-              error("'Args:' section should go before the 'Returns:' section");
-            }
-            if (!deprecated.isEmpty()) {
-              error("'Args:' section should go before the 'Deprecated:' section");
-            }
-            params.addAll(parseParameters());
+            parseArgumentSection(params, returns, deprecated);
+            break;
+          case "Arguments:":
+            // Setting the location indicates an issue will be reported.
+            argumentsLocation =
+                new LocationRange(
+                    new Location(lineNumber, baselineIndentation + 1),
+                    // 10 is the length of "Arguments:".
+                    // The 1 is for the character after the base indentation.
+                    new Location(lineNumber, baselineIndentation + 1 + 10));
+            parseArgumentSection(params, returns, deprecated);
             break;
           case "Returns:":
             checkSectionStart(!returns.isEmpty());
@@ -403,7 +425,12 @@ public final class DocstringUtils {
         deprecated = nonStandardDeprecation;
       }
       return new DocstringInfo(
-          summary, params, returns, deprecated, String.join("\n", longDescriptionLines));
+          summary,
+          params,
+          returns,
+          deprecated,
+          String.join("\n", longDescriptionLines),
+          argumentsLocation);
     }
 
     private void checkSectionStart(boolean duplicateSection) {
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 30d17fea24..31090f6527 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
@@ -107,6 +107,25 @@ public class DocstringCheckerTest {
   }
 
   @Test
+  public void reportArgumentsUse() throws Exception {
+    String errorMessage =
+        findIssues(
+                "def function(foo, bar):",
+                "  \"\"\"summary",
+                "",
+                "  Arguments:",
+                "    foo: bla",
+                "    bar: blabla",
+                "  \"\"\"",
+                "  pass")
+            .toString();
+    Truth.assertThat(errorMessage)
+        .contains(
+            "4:3-4:13: Prefer 'Args:' to 'Arguments:' when documenting function arguments."
+                + " [args-arguments-docstring]");
+  }
+
+  @Test
   public void reportObsoleteParameterDocumentation() throws Exception {
     String errorMessage =
         findIssues(