diff options
Diffstat (limited to 'src/tools/skylark')
19 files changed, 239 insertions, 113 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())); } } diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java index 00138b6b6c..ce121ffb59 100644 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java @@ -37,29 +37,45 @@ public class BadOperationCheckerTest { @Test public void dictionaryLiteralPlusOperator() { Truth.assertThat(findIssues("{} + foo").toString()) - .contains("1:1-1:8: '+' operator is deprecated and should not be used on dictionaries"); + .contains( + "1:1-1:8: '+' operator is deprecated and should not be used on dictionaries" + + " [deprecated-plus-dict]"); Truth.assertThat(findIssues("foo + {}").toString()) - .contains("1:1-1:8: '+' operator is deprecated and should not be used on dictionaries"); + .contains( + "1:1-1:8: '+' operator is deprecated and should not be used on dictionaries" + + " [deprecated-plus-dict]"); Truth.assertThat(findIssues("foo += {}").toString()) - .contains("1:1-1:9: '+' operator is deprecated and should not be used on dictionaries"); + .contains( + "1:1-1:9: '+' operator is deprecated and should not be used on dictionaries" + + " [deprecated-plus-dict]"); } @Test public void dictionaryComprehensionPlusOperator() { Truth.assertThat(findIssues("{k:v for k,v in []} + foo").toString()) - .contains("1:1-1:25: '+' operator is deprecated and should not be used on dictionaries"); + .contains( + "1:1-1:25: '+' operator is deprecated and should not be used on dictionaries" + + " [deprecated-plus-dict]"); Truth.assertThat(findIssues("foo + {k:v for k,v in []}").toString()) - .contains("1:1-1:25: '+' operator is deprecated and should not be used on dictionaries"); + .contains( + "1:1-1:25: '+' operator is deprecated and should not be used on dictionaries" + + " [deprecated-plus-dict]"); Truth.assertThat(findIssues("foo += {k:v for k,v in []}").toString()) - .contains("1:1-1:26: '+' operator is deprecated and should not be used on dictionaries"); + .contains( + "1:1-1:26: '+' operator is deprecated and should not be used on dictionaries" + + " [deprecated-plus-dict]"); } @Test public void dictionaryPlusOperatorNested() { Truth.assertThat(findIssues("foo + ({} + bar)").toString()) - .contains("1:7-1:16: '+' operator is deprecated and should not be used on dictionaries"); + .contains( + "1:7-1:16: '+' operator is deprecated and should not be used on dictionaries" + + " [deprecated-plus-dict]"); Truth.assertThat(findIssues("foo + (bar + {})").toString()) - .contains("1:7-1:16: '+' operator is deprecated and should not be used on dictionaries"); + .contains( + "1:7-1:16: '+' operator is deprecated and should not be used on dictionaries" + + " [deprecated-plus-dict]"); } @Test diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java index f10aba0391..c522dcbee4 100644 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java @@ -51,7 +51,9 @@ public class ControlFlowCheckerTest { " else:", " return x") .toString()) - .contains("1:1-5:12: some but not all execution paths of 'some_function' return a value"); + .contains( + "1:1-5:12: some but not all execution paths of 'some_function' return a value" + + " [missing-return-value]"); } @Test @@ -65,8 +67,13 @@ public class ControlFlowCheckerTest { " return # missing value") .toString(); Truth.assertThat(messages) - .contains("1:1-5:26: some but not all execution paths of 'some_function' return a value"); - Truth.assertThat(messages).contains("5:5-5:10: return value missing"); + .contains( + "1:1-5:26: some but not all execution paths of 'some_function' return a value" + + " [missing-return-value]"); + Truth.assertThat(messages) + .contains( + "5:5-5:10: return value missing (you can `return None` if this is desired)" + + " [missing-return-value]"); } @Test @@ -81,7 +88,9 @@ public class ControlFlowCheckerTest { " else:", " return not x") .toString()) - .contains("1:1-7:16: some but not all execution paths of 'f' return a value"); + .contains( + "1:1-7:16: some but not all execution paths of 'f' return a value" + + " [missing-return-value]"); } @Test @@ -97,7 +106,9 @@ public class ControlFlowCheckerTest { " else:", " return x") .toString()) - .contains("1:1-8:12: some but not all execution paths of 'f' return a value"); + .contains( + "1:1-8:12: some but not all execution paths of 'f' return a value" + + " [missing-return-value]"); } @Test @@ -110,7 +121,9 @@ public class ControlFlowCheckerTest { " elif not x:", " return not x") .toString()) - .contains("1:1-5:16: some but not all execution paths of 'some_function' return a value"); + .contains( + "1:1-5:16: some but not all execution paths of 'some_function' return a value" + + " [missing-return-value]"); } @Test @@ -123,7 +136,9 @@ public class ControlFlowCheckerTest { " print('foo')", " # return missing here") .toString()) - .contains("1:1-5:23: some but not all execution paths of 'some_function' return a value"); + .contains( + "1:1-5:23: some but not all execution paths of 'some_function' return a value" + + " [missing-return-value]"); } @Test @@ -136,7 +151,9 @@ public class ControlFlowCheckerTest { " print('foo')", " # return missing here") .toString()) - .contains("1:1-5:23: some but not all execution paths of 'some_function' return a value"); + .contains( + "1:1-5:23: some but not all execution paths of 'some_function' return a value" + + " [missing-return-value]"); } @Test @@ -149,8 +166,13 @@ public class ControlFlowCheckerTest { " return x") .toString(); Truth.assertThat(messages) - .contains("1:1-4:10: some but not all execution paths of 'some_function' return a value"); - Truth.assertThat(messages).contains("3:5-3:10: return value missing"); + .contains( + "1:1-4:10: some but not all execution paths of 'some_function' return a value" + + " [missing-return-value]"); + Truth.assertThat(messages) + .contains( + "3:5-3:10: return value missing (you can `return None` if this is desired)" + + " [missing-return-value]"); } @Test @@ -164,7 +186,7 @@ public class ControlFlowCheckerTest { " fail('fail')", " print('This line is unreachable')") .toString(); - Truth.assertThat(messages).contains("6:3-6:35: unreachable statement"); + Truth.assertThat(messages).contains("6:3-6:35: unreachable statement [unreachable-statement]"); } @Test @@ -190,7 +212,7 @@ public class ControlFlowCheckerTest { " continue", " print('unreachable')") .toString(); - Truth.assertThat(messages).contains("7:5-7:24: unreachable statement"); + Truth.assertThat(messages).contains("7:5-7:24: unreachable statement [unreachable-statement]"); } @Test @@ -280,7 +302,8 @@ public class ControlFlowCheckerTest { " return x", " # no else branch but doesn't matter since it's unreachable"); Truth.assertThat(issues).hasSize(1); - Truth.assertThat(issues.toString()).contains("7:3-7:14: unreachable statement"); + Truth.assertThat(issues.toString()) + .contains("7:3-7:14: unreachable statement [unreachable-statement]"); } @Test diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java index 5d55eb7a57..cd12f4a7c8 100644 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java @@ -54,9 +54,12 @@ public class DeprecationCheckerTest { " The deprecation should really be documented in a 'Deprecated:' section", " but the linter should recognize this kind of deprecation as well'''") .toString(); - Truth.assertThat(errorMessages).contains("2:3: usage of 'g' is deprecated: reason"); Truth.assertThat(errorMessages) - .contains("3:3: usage of 'h' is deprecated: This function is DEPRECATED for some reason."); + .contains("2:3: usage of 'g' is deprecated: reason [deprecated-symbol]"); + Truth.assertThat(errorMessages) + .contains( + "3:3: usage of 'h' is deprecated: This function is DEPRECATED for some reason." + + " [deprecated-symbol]"); } @Test @@ -72,7 +75,8 @@ public class DeprecationCheckerTest { " Deprecated:", " reason'''") .toString(); - Truth.assertThat(errorMessages).contains("2:7: usage of 'g' is deprecated: reason"); + Truth.assertThat(errorMessages) + .contains("2:7: usage of 'g' is deprecated: reason [deprecated-symbol]"); } @Test 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 8b399ccd34..377f0eed7c 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 @@ -40,8 +40,10 @@ public class DocstringCheckerTests { String errorMessage = findIssues("# no module docstring", "def function():", " pass # no function docstring") .toString(); - Truth.assertThat(errorMessage).contains("1:1-2:1: file has no module docstring"); - Truth.assertThat(errorMessage).contains("2:1-3:30: function 'function' has no docstring"); + 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]"); } @Test @@ -57,7 +59,9 @@ public class DocstringCheckerTests { " pass"); Truth.assertThat(errors).hasSize(1); Truth.assertThat(errors.toString()) - .contains("3:3-6:3: incomplete docstring: the function parameters are not documented"); + .contains( + "3:3-6:3: incomplete docstring: the function parameters are not documented" + + " [inconsistent-docstring]"); } @Test @@ -73,9 +77,13 @@ public class DocstringCheckerTests { " pass") .toString(); Truth.assertThat(errorMessage) - .contains("2:3-6:5: incomplete docstring: parameter 'foo' not documented"); + .contains( + "2:3-6:5: incomplete docstring: parameter 'foo' not documented" + + " [inconsistent-docstring]"); Truth.assertThat(errorMessage) - .contains("2:3-6:5: incomplete docstring: parameter 'baz' not documented"); + .contains( + "2:3-6:5: incomplete docstring: parameter 'baz' not documented" + + " [inconsistent-docstring]"); } @Test @@ -95,11 +103,11 @@ public class DocstringCheckerTests { Truth.assertThat(errorMessage) .contains( "2:3-8:5: inconsistent docstring: parameter 'foo' appears in docstring" - + " but not in function signature"); + + " but not in function signature [inconsistent-docstring]"); Truth.assertThat(errorMessage) .contains( "2:3-8:5: inconsistent docstring: parameter 'baz' appears in docstring" - + " but not in function signature"); + + " but not in function signature [inconsistent-docstring]"); } @Test @@ -120,7 +128,8 @@ public class DocstringCheckerTests { "2:3-7:5:" + " inconsistent docstring: order of parameters differs from function signature\n" + "Declaration order: p1, p2\n" - + "Documentation order: p2, p1"); + + "Documentation order: p2, p1\n" + + " [inconsistent-docstring]"); } @Test @@ -128,8 +137,8 @@ public class DocstringCheckerTests { String errorMessage = findIssues("\"\"\"summary", "missing blank line\"\"\"").toString(); Truth.assertThat(errorMessage) .contains( - "2:1-2:18: invalid docstring format: " - + "the one-line summary should be followed by a blank line"); + "2:1-2:18: bad docstring format: " + + "the one-line summary should be followed by a blank line [bad-docstring-format]"); errorMessage = findIssues( @@ -142,8 +151,9 @@ public class DocstringCheckerTests { .toString(); Truth.assertThat(errorMessage) .contains( - "5:1-5:29: invalid docstring format: " - + "line indented too little (here: 1 spaces; expected: 2 spaces)"); + "5:1-5:29: bad docstring format: " + + "line indented too little (here: 1 spaces; expected: 2 spaces)" + + " [bad-docstring-format]"); } @Test @@ -159,7 +169,9 @@ public class DocstringCheckerTests { " return True"); Truth.assertThat(errors).hasSize(1); Truth.assertThat(errors.toString()) - .contains("3:3-6:5: incomplete docstring: the return value is not documented"); + .contains( + "3:3-6:5: incomplete docstring: the return value is not documented" + + " [inconsistent-docstring]"); } @Test diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java index 148aa4845e..96ff5d47f5 100644 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java @@ -53,6 +53,6 @@ public class LinterTest { .contains("should be lower_snake_case"); // naming conventions checker Truth.assertThat(errorMessages) .contains("expression result not used"); // checker statements without effect - Truth.assertThat(errorMessages).contains("unused definition of"); // usage checker + Truth.assertThat(errorMessages).contains("unused binding of"); // usage checker } } diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LoadStatementCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LoadStatementCheckerTest.java index c6cb41b543..35857e3722 100644 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LoadStatementCheckerTest.java +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LoadStatementCheckerTest.java @@ -59,7 +59,8 @@ public class LoadStatementCheckerTest { .toString(); Truth.assertThat(errorMessage) .contains( - "3:1-3:23: load statement should be at the top of the file (after the docstring)"); + "3:1-3:23: load statement should be at the top of the file (after the docstring)" + + " [load-at-top]"); errorMessage = findIssues( "'''Docstring'''", @@ -69,6 +70,7 @@ public class LoadStatementCheckerTest { .toString(); Truth.assertThat(errorMessage) .contains( - "4:1-4:23: load statement should be at the top of the file (after the docstring)"); + "4:1-4:23: load statement should be at the top of the file (after the docstring)" + + " [load-at-top]"); } } diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NamingConventionsCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NamingConventionsCheckerTest.java index 0dc837c432..6361705c29 100644 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NamingConventionsCheckerTest.java +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NamingConventionsCheckerTest.java @@ -46,13 +46,15 @@ public class NamingConventionsCheckerTest { Truth.assertThat(errorMessage) .contains( "'badGlobalVariableName' should be lower_snake_case (for variables)" - + " or UPPER_SNAKE_CASE (for constants)"); - Truth.assertThat(errorMessage).contains("'BAD_FUNCTION_NAME' should be lower_snake_case"); - Truth.assertThat(errorMessage).contains("'BadParameterName' should be lower_snake_case"); + + " or UPPER_SNAKE_CASE (for constants) [name-with-wrong-case]"); + Truth.assertThat(errorMessage) + .contains("'BAD_FUNCTION_NAME' should be lower_snake_case [name-with-wrong-case]"); + Truth.assertThat(errorMessage) + .contains("'BadParameterName' should be lower_snake_case [name-with-wrong-case]"); Truth.assertThat(errorMessage) .contains( "'badLocalVariableName' should be lower_snake_case (for variables)" - + " or UPPER_SNAKE_CASE (for constants)"); + + " or UPPER_SNAKE_CASE (for constants) [name-with-wrong-case]"); } @Test @@ -61,11 +63,13 @@ public class NamingConventionsCheckerTest { Truth.assertThat(errorMessage) .contains( "never use 'l', 'I', or 'O' as names" - + " (they're too easily confused with 'I', 'l', or '0')"); + + " (they're too easily confused with 'I', 'l', or '0') [confusing-name]"); Truth.assertThat(errorMessage) - .contains("identifier 'fail' shadows a builtin; please pick a different name"); + .contains( + "identifier 'fail' shadows a builtin; please pick a different name [confusing-name]"); Truth.assertThat(errorMessage) - .contains("identifier 'True' shadows a builtin; please pick a different name"); + .contains( + "identifier 'True' shadows a builtin; please pick a different name [confusing-name]"); } @Test @@ -74,11 +78,13 @@ public class NamingConventionsCheckerTest { Truth.assertThat(findIssues("_ = 1", "print(_)").toString()) .contains( "2:7-2:7:" - + " don't use '_' as an identifier, only to ignore the result in an assignment"); + + " don't use '_' as an identifier, only to ignore the result in an assignment" + + " [confusing-name]"); Truth.assertThat(findIssues("__ = 1").toString()) .contains( "1:1-1:2:" - + " identifier '__' consists only of underscores; please pick a different name"); + + " identifier '__' consists only of underscores; please pick a different name" + + " [confusing-name]"); } @Test @@ -102,7 +108,7 @@ public class NamingConventionsCheckerTest { public void testProviderNameMustBeCamelCase() throws Exception { Truth.assertThat(findIssues("FooBar = provider()")).isEmpty(); Truth.assertThat(findIssues("foo_bar = provider()").toString()) - .contains("provider name 'foo_bar' should be UpperCamelCase"); + .contains("provider name 'foo_bar' should be UpperCamelCase [name-with-wrong-case]"); } @Test diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/StatementWithoutEffectCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/StatementWithoutEffectCheckerTest.java index d160d2357e..beac94c3c7 100644 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/StatementWithoutEffectCheckerTest.java +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/StatementWithoutEffectCheckerTest.java @@ -38,12 +38,12 @@ public class StatementWithoutEffectCheckerTest { public void reportUselessExpressionStatements() throws Exception { String messages = findIssues("1", "len", "'string'", "'a'.len", "1 + 1", "[1, 2, 3]").toString(); - Truth.assertThat(messages).contains("1:1-1:1: expression result not used"); - Truth.assertThat(messages).contains("2:1-2:3: expression result not used"); - Truth.assertThat(messages).contains("3:1-3:8: expression result not used"); - Truth.assertThat(messages).contains("4:1-4:7: expression result not used"); - Truth.assertThat(messages).contains("5:1-5:5: expression result not used"); - Truth.assertThat(messages).contains("6:1-6:9: expression result not used"); + Truth.assertThat(messages).contains("1:1-1:1: expression result not used [no-effect]"); + Truth.assertThat(messages).contains("2:1-2:3: expression result not used [no-effect]"); + Truth.assertThat(messages).contains("3:1-3:8: expression result not used [no-effect]"); + Truth.assertThat(messages).contains("4:1-4:7: expression result not used [no-effect]"); + Truth.assertThat(messages).contains("5:1-5:5: expression result not used [no-effect]"); + Truth.assertThat(messages).contains("6:1-6:9: expression result not used [no-effect]"); } @Test @@ -58,7 +58,7 @@ public class StatementWithoutEffectCheckerTest { findIssues( "def f():", " [print(x) for x in range(5)] # should be replaced by for-loop") .toString()) - .contains("2:3-2:30: expression result not used"); + .contains("2:3-2:30: expression result not used [no-effect]"); } @Test diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java index de61011a0e..b477cce1a8 100644 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java @@ -38,37 +38,41 @@ public class UsageCheckerTest { @Test public void reportUnusedImports() throws Exception { String message = findIssues("load('foo', 'x', 'y', _z = 'Z')").toString(); - Truth.assertThat(message).contains("1:13-1:15: unused definition of 'x'"); - Truth.assertThat(message).contains("1:18-1:20: unused definition of 'y'"); - Truth.assertThat(message).contains("1:23-1:24: unused definition of '_z'"); + Truth.assertThat(message).contains("1:13-1:15: unused binding of 'x' [unused-binding]"); + Truth.assertThat(message).contains("1:18-1:20: unused binding of 'y' [unused-binding]"); + Truth.assertThat(message).contains("1:23-1:24: unused binding of '_z' [unused-binding]"); } @Test public void reportUnusedGlobals() throws Exception { String message = findIssues("_UNUSED = len([])", "def _unused(): pass").toString(); - Truth.assertThat(message).contains("1:1-1:7: unused definition of '_UNUSED'"); - Truth.assertThat(message).contains("2:5-2:11: unused definition of '_unused'"); + Truth.assertThat(message).contains("1:1-1:7: unused binding of '_UNUSED' [unused-binding]"); + Truth.assertThat(message).contains("2:5-2:11: unused binding of '_unused' [unused-binding]"); } @Test public void reportUnusedLocals() throws Exception { String message = findIssues("def some_function(param):", " local, local2 = 1, 3").toString(); - Truth.assertThat(message).contains("1:19-1:23: unused definition of 'param'"); + Truth.assertThat(message).contains("1:19-1:23: unused binding of 'param'"); Truth.assertThat(message) - .contains("you can add `_ignore = [<param1>, <param2>, ...]` to the function body."); - Truth.assertThat(message).contains("2:3-2:7: unused definition of 'local'"); - Truth.assertThat(message).contains("you can use '_' or rename it to '_local'"); - Truth.assertThat(message).contains("2:10-2:15: unused definition of 'local2'"); - Truth.assertThat(message).contains("you can use '_' or rename it to '_local2'"); + .contains( + "you can add `_ignore = [<param1>, <param2>, ...]` to the function body." + + " [unused-binding]"); + Truth.assertThat(message).contains("2:3-2:7: unused binding of 'local'"); + Truth.assertThat(message) + .contains("you can use '_' or rename it to '_local'. [unused-binding]"); + Truth.assertThat(message).contains("2:10-2:15: unused binding of 'local2'"); + Truth.assertThat(message) + .contains("you can use '_' or rename it to '_local2'. [unused-binding]"); } @Test public void reportUnusedComprehensionVariable() throws Exception { String message = findIssues("[[2 for y in []] for x in []]").toString(); - Truth.assertThat(message).contains("1:9-1:9: unused definition of 'y'"); - Truth.assertThat(message).contains("you can use '_' or rename it to '_y'"); - Truth.assertThat(message).contains("1:22-1:22: unused definition of 'x'"); - Truth.assertThat(message).contains("you can use '_' or rename it to '_x'"); + Truth.assertThat(message).contains("1:9-1:9: unused binding of 'y'"); + Truth.assertThat(message).contains("you can use '_' or rename it to '_y'. [unused-binding]"); + Truth.assertThat(message).contains("1:22-1:22: unused binding of 'x'"); + Truth.assertThat(message).contains("you can use '_' or rename it to '_x'. [unused-binding]"); } @Test @@ -79,19 +83,19 @@ public class UsageCheckerTest { " x = [[] for x in []]", " print(x)") .toString()) - .contains("2:15-2:15: unused definition of 'x'"); + .contains("2:15-2:15: unused binding of 'x'"); } @Test public void reportShadowedVariable() throws Exception { Truth.assertThat(findIssues("def some_function():", " x = [x for x in []]").toString()) - .contains("2:3-2:3: unused definition of 'x'"); + .contains("2:3-2:3: unused binding of 'x'"); } @Test public void reportReassignedUnusedVariable() throws Exception { Truth.assertThat(findIssues("def some_function():", " x = 1", " x += 2").toString()) - .contains("3:3-3:3: unused definition of 'x'"); + .contains("3:3-3:3: unused binding of 'x'"); } @Test @@ -106,7 +110,7 @@ public class UsageCheckerTest { " x = 4", " print(x)") .toString()) - .contains("2:3-2:3: unused definition of 'x'"); + .contains("2:3-2:3: unused binding of 'x'"); } @Test @@ -118,7 +122,7 @@ public class UsageCheckerTest { " print(x)", " x = 2") .toString(); - Truth.assertThat(messages).contains("4:5-4:5: unused definition of 'x'"); + Truth.assertThat(messages).contains("4:5-4:5: unused binding of 'x'"); } @Test @@ -135,14 +139,16 @@ public class UsageCheckerTest { " y += 2", " print(y)") .toString(); - Truth.assertThat(message).contains("8:3-8:3: identifier 'y' may not have been initialized"); + Truth.assertThat(message) + .contains("8:3-8:3: variable 'y' may not have been initialized [uninitialized-variable]"); } @Test public void reportUninitializedAfterForLoop() throws Exception { String message = findIssues("def some_function():", " for _ in []:", " y = 1", " print(y)").toString(); - Truth.assertThat(message).contains("4:9-4:9: identifier 'y' may not have been initialized"); + Truth.assertThat(message) + .contains("4:9-4:9: variable 'y' may not have been initialized [uninitialized-variable]"); } @Test |