Skylint: add categories for issues.

Having a short canonical name for each kind of finding (the category) makes it easier to document all the lints and for users to find the documentation for each lint.

RELNOTES: none
PiperOrigin-RevId: 171972645

10 files changed, 91 insertions, 34 deletions

diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java
index 8b08bdff65..7be8e8199d 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java
@@ -26,6 +26,8 @@ import java.util.List;
 
 /** Checks for operations that are deprecated */
 public class BadOperationChecker extends SyntaxTreeVisitor {
+  private static final String DEPRECATED_PLUS_DICT_CATEGORY = "deprecated-plus-dict";
+
   private final List<Issue> issues = new ArrayList<>();
 
   private BadOperationChecker() {}
@@ -45,7 +47,8 @@ public class BadOperationChecker extends SyntaxTreeVisitor {
           || node.getRhs() instanceof DictionaryLiteral
           || node.getRhs() instanceof DictComprehension) {
         issues.add(
-            new Issue(
+            Issue.create(
+                DEPRECATED_PLUS_DICT_CATEGORY,
                 "'+' operator is deprecated and should not be used on dictionaries",
                 node.getLocation()));
       }
@@ -58,7 +61,8 @@ public class BadOperationChecker extends SyntaxTreeVisitor {
     if (node.getExpression() instanceof DictionaryLiteral
         || node.getExpression() instanceof DictComprehension) {
       issues.add(
-          new Issue(
+          Issue.create(
+              DEPRECATED_PLUS_DICT_CATEGORY,
               "'+' operator is deprecated and should not be used on dictionaries",
               node.getLocation()));
     }
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java
index c32e0ac9c6..63af0dff75 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java
@@ -45,6 +45,9 @@ import javax.annotation.Nullable;
  */
 // TODO(skylark-team): Check for unreachable statements
 public class ControlFlowChecker extends SyntaxTreeVisitor {
+  private static final String MISSING_RETURN_VALUE_CATEGORY = "missing-return-value";
+  public static final String UNREACHABLE_STATEMENT_CATEGORY = "unreachable-statement";
+
   private final List<Issue> issues = new ArrayList<>();
 
   /**
@@ -75,7 +78,9 @@ public class ControlFlowChecker extends SyntaxTreeVisitor {
     boolean alreadyReported = false;
     for (Statement stmt : statements) {
       if (!cfi.reachable && !alreadyReported) {
-        issues.add(new Issue("unreachable statement", stmt.getLocation()));
+        issues.add(
+            Issue.create(
+                UNREACHABLE_STATEMENT_CATEGORY, "unreachable statement", stmt.getLocation()));
         alreadyReported = true;
       }
       visit(stmt);
@@ -156,12 +161,14 @@ public class ControlFlowChecker extends SyntaxTreeVisitor {
     super.visit(node);
     if (cfi.hasReturnWithValue && (!cfi.returnsAlwaysExplicitly || cfi.hasReturnWithoutValue)) {
       issues.add(
-          new Issue(
+          Issue.create(
+              MISSING_RETURN_VALUE_CATEGORY,
               "some but not all execution paths of '" + node.getIdentifier() + "' return a value",
               node.getLocation()));
       for (Wrapper<ReturnStatement> returnWrapper : cfi.returnStatementsWithoutValue) {
         issues.add(
-            new Issue(
+            Issue.create(
+                MISSING_RETURN_VALUE_CATEGORY,
                 "return value missing (you can `return None` if this is desired)",
                 unwrapReturn(returnWrapper).getLocation()));
       }
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java
index 923f6360d6..23ce556efa 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java
@@ -30,6 +30,8 @@ import java.util.Map;
 
 /** Checks for usage of deprecated functions. */
 public class DeprecationChecker extends AstVisitorWithNameResolution {
+  private static final String DEPRECATED_SYMBOL_CATEGORY = "deprecated-symbol";
+
   private final List<Issue> issues = new ArrayList<>();
   /** Maps a global function name to its deprecation warning, if any. */
   private final Map<String, String> functionToDeprecationWarning = new HashMap<>();
@@ -83,7 +85,8 @@ public class DeprecationChecker extends AstVisitorWithNameResolution {
         && info.kind != Kind.LOCAL) {
       String deprecationMessage = functionToDeprecationWarning.get(info.name);
       issues.add(
-          new Issue(
+          Issue.create(
+              DEPRECATED_SYMBOL_CATEGORY,
               "usage of '" + info.name + "' is deprecated: " + deprecationMessage,
               ident.getLocation()));
     }
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 c19a68b58f..bc9edc37de 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
@@ -33,6 +33,10 @@ import java.util.List;
 
 /** Checks the existence of docstrings. */
 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";
+
   private final List<Issue> issues = new ArrayList<>();
   private boolean containsReturnWithValue = false;
 
@@ -51,7 +55,7 @@ public class DocstringChecker extends SyntaxTreeVisitor {
       // This location is invalid if the file is empty but this edge case is not worth the trouble.
       Location end = new Location(2, 1);
       LocationRange range = new LocationRange(start, end);
-      issues.add(new Issue("file has no module docstring", range));
+      issues.add(new Issue(MISSING_DOCSTRING_CATEGORY, "file has no module docstring", range));
     } else {
       List<DocstringParseError> errors = new ArrayList<>();
       DocstringUtils.parseDocstring(moduleDocstring, errors);
@@ -76,7 +80,8 @@ public class DocstringChecker extends SyntaxTreeVisitor {
     StringLiteral functionDocstring = extractDocstring(node.getStatements());
     if (functionDocstring == null && !node.getIdentifier().getName().startsWith("_")) {
       issues.add(
-          new Issue(
+          Issue.create(
+              MISSING_DOCSTRING_CATEGORY,
               "function '" + node.getIdentifier().getName() + "' has no docstring",
               node.getLocation()));
     }
@@ -102,7 +107,8 @@ public class DocstringChecker extends SyntaxTreeVisitor {
       List<Issue> issues) {
     if (functionReturnsWithValue && docstring.returns.isEmpty()) {
       issues.add(
-          new Issue(
+          Issue.create(
+              INCONSISTENT_DOCSTRING_CATEGORY,
               "incomplete docstring: the return value is not documented",
               docstringLiteral.getLocation()));
     }
@@ -133,7 +139,8 @@ public class DocstringChecker extends SyntaxTreeVisitor {
       List<Issue> issues) {
     if (documentedParams.isEmpty() && !declaredParams.isEmpty()) {
       issues.add(
-          new Issue(
+          Issue.create(
+              INCONSISTENT_DOCSTRING_CATEGORY,
               "incomplete docstring: the function parameters are not documented",
               docstringLiteral.getLocation()));
       return;
@@ -141,7 +148,8 @@ public class DocstringChecker extends SyntaxTreeVisitor {
     for (String param : declaredParams) {
       if (!documentedParams.contains(param)) {
         issues.add(
-            new Issue(
+            Issue.create(
+                INCONSISTENT_DOCSTRING_CATEGORY,
                 "incomplete docstring: parameter '" + param + "' not documented",
                 docstringLiteral.getLocation()));
       }
@@ -149,7 +157,8 @@ public class DocstringChecker extends SyntaxTreeVisitor {
     for (String param : documentedParams) {
       if (!declaredParams.contains(param)) {
         issues.add(
-            new Issue(
+            Issue.create(
+                INCONSISTENT_DOCSTRING_CATEGORY,
                 "inconsistent docstring: parameter '"
                     + param
                     + "' appears in docstring but not in function signature",
@@ -166,7 +175,8 @@ public class DocstringChecker extends SyntaxTreeVisitor {
               + "Documentation order: "
               + String.join(", ", documentedParams)
               + "\n";
-      issues.add(new Issue(message, docstringLiteral.getLocation()));
+      issues.add(
+          Issue.create(INCONSISTENT_DOCSTRING_CATEGORY, message, docstringLiteral.getLocation()));
     }
   }
 
@@ -185,6 +195,9 @@ public class DocstringChecker extends SyntaxTreeVisitor {
     }
     Location start = new Location(startLine, startColumn);
     Location end = new Location(startLine, startColumn + error.line.length() - 1);
-    return new Issue("invalid docstring format: " + error.message, new LocationRange(start, end));
+    return new Issue(
+        BAD_DOCSTRING_FORMAT_CATEGORY,
+        "bad docstring format: " + error.message,
+        new LocationRange(start, end));
   }
 }
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Issue.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Issue.java
index 435c47c312..0891857424 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Issue.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Issue.java
@@ -18,26 +18,27 @@ import com.google.devtools.build.lib.events.Location;
 
 /** An issue found by the linter. */
 public class Issue {
-  // TODO(skylark-team): Represent issues more efficiently than just by a string
+  public final String category;
   public final String message;
   public final LocationRange location;
 
-  public Issue(String message, LocationRange location) {
+  public Issue(String category, String message, LocationRange location) {
+    this.category = category;
     this.message = message;
     this.location = location;
   }
 
-  public Issue(String message, Location location) {
-    this(message, LocationRange.from(location));
+  public static Issue create(String category, String message, Location location) {
+    return new Issue(category, message, LocationRange.from(location));
   }
 
   @Override
   public String toString() {
-    return location + ": " + message;
+    return location + ": " + message + " [" + category + "]";
   }
 
   public String prettyPrint(String path) {
-    return path + ":" + location + ": " + message;
+    return path + ":" + toString();
   }
 
   public static int compareLocation(Issue i1, Issue i2) {
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java
index 0296b3d293..d403ecf771 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java
@@ -33,6 +33,7 @@ import java.util.Set;
  * <p>Most users of the linter library should only need to use this class.
  */
 public class Linter {
+  private static final String PARSE_ERROR_CATEGORY = "parse-error";
   /** Map of all checks and their names. */
   private static final ImmutableMap<String, Check> nameToCheck =
       ImmutableMap.<String, Check>builder()
@@ -77,7 +78,8 @@ public class Linter {
         BuildFileAST.parseString(
             event -> {
               if (event.getKind() == EventKind.ERROR || event.getKind() == EventKind.WARNING) {
-                issues.add(new Issue(event.getMessage(), event.getLocation()));
+                issues.add(
+                    Issue.create(PARSE_ERROR_CATEGORY, event.getMessage(), event.getLocation()));
               }
             },
             content);
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/LoadStatementChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/LoadStatementChecker.java
index 5c33d9caee..ce8fa3c673 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/LoadStatementChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/LoadStatementChecker.java
@@ -22,6 +22,8 @@ import java.util.List;
 
 /** Checks that load statements are at the top of a file (after the docstring). */
 public class LoadStatementChecker {
+  private static final String LOAD_AT_TOP_CATEGORY = "load-at-top";
+
   private LoadStatementChecker() {}
 
   public static List<Issue> check(BuildFileAST ast) {
@@ -34,7 +36,8 @@ public class LoadStatementChecker {
       if (statement instanceof LoadStatement) {
         if (!loadStatementsExpected) {
           issues.add(
-              new Issue(
+              Issue.create(
+                  LOAD_AT_TOP_CATEGORY,
                   "load statement should be at the top of the file (after the docstring)",
                   statement.getLocation()));
         }
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/NamingConventionsChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/NamingConventionsChecker.java
index e796a1d9a1..92bda52ec6 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/NamingConventionsChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/NamingConventionsChecker.java
@@ -46,8 +46,11 @@ import java.util.List;
  */
 // TODO(skylark-team): Check that UPPERCASE_VARIABLES are never mutated
 public class NamingConventionsChecker extends AstVisitorWithNameResolution {
+  private static final String NAME_WITH_WRONG_CASE_CATEGORY = "name-with-wrong-case";
+  private static final String CONFUSING_NAME_CATEGORY = "confusing-name";
   private static final ImmutableList<String> CONFUSING_NAMES = ImmutableList.of("O", "I", "l");
   private static final ImmutableSet<String> BUILTIN_NAMES;
+
   private final List<Issue> issues = new ArrayList<>();
 
   static {
@@ -84,7 +87,8 @@ public class NamingConventionsChecker extends AstVisitorWithNameResolution {
   private void checkSnakeCase(String name, Location location) {
     if (!isSnakeCase(name)) {
       issues.add(
-          new Issue(
+          Issue.create(
+              NAME_WITH_WRONG_CASE_CATEGORY,
               "identifier '"
                   + name
                   + "' should be lower_snake_case (for variables)"
@@ -95,33 +99,44 @@ public class NamingConventionsChecker extends AstVisitorWithNameResolution {
 
   private void checkLowerSnakeCase(String name, Location location) {
     if (!isLowerSnakeCase(name)) {
-      issues.add(new Issue("identifier '" + name + "' should be lower_snake_case", location));
+      issues.add(
+          Issue.create(
+              NAME_WITH_WRONG_CASE_CATEGORY,
+              "identifier '" + name + "' should be lower_snake_case",
+              location));
     }
   }
 
   private void checkProviderName(String name, Location location) {
     if (!isUpperCamelCase(name)) {
-      issues.add(new Issue("provider name '" + name + "' should be UpperCamelCase", location));
+      issues.add(
+          Issue.create(
+              NAME_WITH_WRONG_CASE_CATEGORY,
+              "provider name '" + name + "' should be UpperCamelCase",
+              location));
     }
   }
 
   private void checkNameNotConfusing(String name, Location location) {
     if (CONFUSING_NAMES.contains(name)) {
       issues.add(
-          new Issue(
+          Issue.create(
+              CONFUSING_NAME_CATEGORY,
               "never use 'l', 'I', or 'O' as names "
                   + "(they're too easily confused with 'I', 'l', or '0')",
               location));
     }
     if (BUILTIN_NAMES.contains(name)) {
       issues.add(
-          new Issue(
+          Issue.create(
+              CONFUSING_NAME_CATEGORY,
               "identifier '" + name + "' shadows a builtin; please pick a different name",
               location));
     }
     if (name.chars().allMatch(c -> c == '_') && name.length() >= 2) {
       issues.add(
-          new Issue(
+          Issue.create(
+              CONFUSING_NAME_CATEGORY,
               "identifier '"
                   + name
                   + "' consists only of underscores; please pick a different name",
@@ -133,7 +148,8 @@ public class NamingConventionsChecker extends AstVisitorWithNameResolution {
   void use(Identifier identifier) {
     if (identifier.getName().equals("_")) {
       issues.add(
-          new Issue(
+          Issue.create(
+              CONFUSING_NAME_CATEGORY,
               "don't use '_' as an identifier, only to ignore the result in an assignment",
               identifier.getLocation()));
     }
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/StatementWithoutEffectChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/StatementWithoutEffectChecker.java
index 842cc6cf24..ba97ab2719 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/StatementWithoutEffectChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/StatementWithoutEffectChecker.java
@@ -28,6 +28,8 @@ import java.util.List;
 
 /** Checks for statements that have no effect. */
 public class StatementWithoutEffectChecker extends SyntaxTreeVisitor {
+  private static final String NO_EFFECT_CATEGORY = "no-effect";
+
   private final List<Issue> issues = new ArrayList<>();
   private boolean hasEffect = false;
   private boolean topLevel = true;
@@ -77,7 +79,7 @@ public class StatementWithoutEffectChecker extends SyntaxTreeVisitor {
       // list]
       return;
     }
-    issues.add(new Issue("expression result not used", node.getLocation()));
+    issues.add(Issue.create(NO_EFFECT_CATEGORY, "expression result not used", node.getLocation()));
   }
 
   @Override
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java
index db185ee40b..36614e75ba 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java
@@ -39,6 +39,9 @@ import java.util.Set;
 
 /** Checks that every import, private function or variable definition is used somewhere. */
 public class UsageChecker extends AstVisitorWithNameResolution {
+  private static final String UNUSED_BINDING_CATEGORY = "unused-binding";
+  private static final String UNINITIALIZED_VARIABLE_CATEGORY = "uninitialized-variable";
+
   private final List<Issue> issues = new ArrayList<>();
   private UsageInfo ui = UsageInfo.empty();
   private final SetMultimap<Integer, Wrapper<ASTNode>> idToAllDefinitions =
@@ -159,7 +162,7 @@ public class UsageChecker extends AstVisitorWithNameResolution {
       // symbol might be loaded in another file
       return;
     }
-    String message = "unused definition of '" + name + "'";
+    String message = "unused binding of '" + name + "'";
     if (nameInfo.kind == Kind.PARAMETER) {
       message +=
           ". If this is intentional, "
@@ -168,15 +171,18 @@ public class UsageChecker extends AstVisitorWithNameResolution {
       message += ". If this is intentional, you can use '_' or rename it to '_" + name + "'.";
     }
     for (Wrapper<ASTNode> definition : unusedDefinitions) {
-      issues.add(new Issue(message, unwrapNode(definition).getLocation()));
+      issues.add(
+          Issue.create(UNUSED_BINDING_CATEGORY, message, unwrapNode(definition).getLocation()));
     }
   }
 
   private void checkInitialized(NameInfo info, Identifier node) {
     if (ui.reachable && !ui.initializedIdentifiers.contains(info.id) && info.kind != Kind.BUILTIN) {
       issues.add(
-          new Issue(
-              "identifier '" + info.name + "' may not have been initialized", node.getLocation()));
+          Issue.create(
+              UNINITIALIZED_VARIABLE_CATEGORY,
+              "variable '" + info.name + "' may not have been initialized",
+              node.getLocation()));
     }
   }