diff options
6 files changed, 835 insertions, 2 deletions
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/AstVisitorWithNameResolution.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/AstVisitorWithNameResolution.java new file mode 100644 index 0000000000..c25f30337e --- /dev/null +++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/AstVisitorWithNameResolution.java @@ -0,0 +1,205 @@ +// Copyright 2017 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.skylark.skylint; + +import com.google.devtools.build.lib.syntax.ASTNode; +import com.google.devtools.build.lib.syntax.AbstractComprehension; +import com.google.devtools.build.lib.syntax.BuildFileAST; +import com.google.devtools.build.lib.syntax.DotExpression; +import com.google.devtools.build.lib.syntax.Expression; +import com.google.devtools.build.lib.syntax.FunctionDefStatement; +import com.google.devtools.build.lib.syntax.Identifier; +import com.google.devtools.build.lib.syntax.LValue; +import com.google.devtools.build.lib.syntax.ListComprehension; +import com.google.devtools.build.lib.syntax.LoadStatement; +import com.google.devtools.build.lib.syntax.Parameter; +import com.google.devtools.build.lib.syntax.Statement; +import com.google.devtools.build.lib.syntax.SyntaxTreeVisitor; +import com.google.devtools.build.lib.util.Preconditions; +import java.util.Collection; +import java.util.Map; +import java.util.Map.Entry; + +/** + * AST visitor that keeps track of which symbols are in scope. + * + * <p>The methods {@code enterBlock}, {@code exitBlock}, {@code declare} and {@code reassign} can be + * overridden by a subclass to handle these "events" during AST traversal. + */ +public class AstVisitorWithNameResolution extends SyntaxTreeVisitor { + protected Environment env; + + public AstVisitorWithNameResolution(Environment env) { + this.env = env; + } + + @Override + public void visit(BuildFileAST node) { + enterBlock(); + // First process all global symbols ... + for (Statement stmt : node.getStatements()) { + if (stmt instanceof FunctionDefStatement) { + Identifier fun = ((FunctionDefStatement) stmt).getIdentifier(); + env.addIdentifier(fun.getName(), fun); + declare(fun.getName(), fun); + } else { + visit(stmt); + } + } + // ... then check the functions + for (Statement stmt : node.getStatements()) { + if (stmt instanceof FunctionDefStatement) { + visit(stmt); + } + } + visitAll(node.getComments()); + Preconditions.checkState(env.inGlobalBlock(), "didn't exit some blocks"); + exitBlock(); + } + + @Override + public void visit(Identifier node) { + // TODO(skylark-team): Check for unresolved identifiers once the complete list of builtins can + // be obtained more easily + } + + @Override + public void visit(LoadStatement node) { + Map<Identifier, String> symbolMap = node.getSymbolMap(); + for (Entry<Identifier, String> entry : symbolMap.entrySet()) { + String name = entry.getKey().getName(); + env.addImported(name, entry.getKey()); + declare(name, entry.getKey()); + } + } + + @Override + public void visit(LValue node) { + initializeOrReassignLValue(node); + visitLvalue(node.getExpression()); + } + + /** + * Visits an lvalue. + * + * <p>This method is meant to be overridden. For example, identifiers in lvalues are usually + * supposed to be handled differently. + * + * @param expr the lvalue expression + */ + protected void visitLvalue(Expression expr) { + visit(expr); + } + + @Override + public void visit(FunctionDefStatement node) { + // First visit the default values for parameters in the global environment ... + for (Parameter<Expression, Expression> param : node.getParameters()) { + Expression expr = param.getDefaultValue(); + if (expr != null) { + visit(expr); + } + } + // ... then visit everything else in the local environment + enterBlock(); + for (Parameter<Expression, Expression> param : node.getParameters()) { + String name = param.getName(); + if (name != null) { + env.addParameter(name, param); + declare(name, param); + } + } + // The function identifier was already added to the globals before, so we skip it + visitAll(node.getParameters()); + visitBlock(node.getStatements()); + exitBlock(); + } + + @Override + public void visit(DotExpression node) { + // Don't visit the identifier field because it's not a variable and would confuse the identifier + // visitor + visit(node.getObject()); + } + + @Override + public void visit(AbstractComprehension node) { + enterBlock(); + for (ListComprehension.Clause clause : node.getClauses()) { + visit(clause.getExpression()); + LValue lvalue = clause.getLValue(); + if (lvalue != null) { + Collection<Identifier> boundIdents = lvalue.boundIdentifiers(); + for (Identifier ident : boundIdents) { + env.addIdentifier(ident.getName(), ident); + declare(ident.getName(), ident); + } + visit(lvalue); + } + } + visitAll(node.getOutputExpressions()); + exitBlock(); + } + + private void initializeOrReassignLValue(LValue lvalue) { + Iterable<Identifier> identifiers = lvalue.boundIdentifiers(); + for (Identifier identifier : identifiers) { + if (env.isDefined(identifier.getName())) { + reassign(identifier.getName(), identifier); + } else { + env.addIdentifier(identifier.getName(), identifier); + declare(identifier.getName(), identifier); + } + } + } + + /** + * Invoked when a symbol is defined during AST traversal. + * + * <p>This method is there to be overridden in subclasses, it doesn't do anything by itself. + * + * @param name name of the variable declared + * @param node {@code ASTNode} where it was declared + */ + void declare(String name, ASTNode node) {} + + /** + * Invoked when a variable is reassigned during AST traversal. + * + * <p>This method is there to be overridden in subclasses, it doesn't do anything by itself. + * + * @param name name of the variable declared + * @param ident {@code Identifier} that was reassigned + */ + void reassign(String name, Identifier ident) {} + + /** + * Invoked when a lexical block is entered during AST traversal. + * + * <p>This method is there to be overridden in subclasses. + */ + void enterBlock() { + env.enterBlock(); + } + + /** + * Invoked when a lexical block is entered during AST traversal. + * + * <p>This method is there to be overridden in subclasses. + */ + void exitBlock() { + env.exitBlock(); + } +} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Environment.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Environment.java new file mode 100644 index 0000000000..0616fff9b2 --- /dev/null +++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Environment.java @@ -0,0 +1,205 @@ +// Copyright 2017 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.skylark.skylint; + +import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature; +import com.google.devtools.build.lib.syntax.ASTNode; +import com.google.devtools.build.lib.syntax.BazelLibrary; +import com.google.devtools.build.lib.syntax.Comment; +import com.google.devtools.build.lib.syntax.Identifier; +import com.google.devtools.build.lib.syntax.MethodLibrary; +import com.google.devtools.skylark.skylint.Environment.NameInfo.Kind; +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import javax.annotation.Nullable; + +/** Holds the information about which symbols are in scope during AST traversal. */ +public class Environment { + private final List<LexicalBlock> blocks = new ArrayList<>(); + private final Map<Integer, NameInfo> idToNameInfo = new HashMap<>(); + private int nextId = 0; + private static final int BUILTINS_INDEX = 0; // index of the block containing builtins + private static final int GLOBALS_INDEX = 1; // index of the block containing globals + + public static Environment empty() { + Environment env = new Environment(); + env.blocks.add(new LexicalBlock()); // for builtins + return env; + } + + public static Environment defaultBazel() { + Environment env = empty(); + env.addBuiltin("None"); + env.addBuiltin("True"); + env.addBuiltin("False"); + env.setupFunctions(MethodLibrary.class, BazelLibrary.class); + return env; + } + + private void setupFunctions(Class<?>... classes) { + // Iterate through the skylark functions declared inline within the classes + // retrieving and storing their names. + for (Class<?> c : classes) { + for (Field field : c.getDeclaredFields()) { + // Skylark functions are defined inline as fields within the class, annotated + // by @SkylarkSignature. + SkylarkSignature builtinFuncSignature = field.getAnnotation(SkylarkSignature.class); + if (builtinFuncSignature != null && builtinFuncSignature.objectType() == Object.class) { + addBuiltin(builtinFuncSignature.name()); + } + } + } + } + + public void enterBlock() { + blocks.add(new LexicalBlock()); + } + + public Collection<Integer> exitBlock() { + if (blocks.size() <= GLOBALS_INDEX) { + throw new IllegalStateException("no blocks to exit from"); + } + return blocks.remove(blocks.size() - 1).getAllSymbols(); + } + + public boolean inGlobalBlock() { + return blocks.size() - 1 == GLOBALS_INDEX; + } + + public boolean isDefined(String name) { + return resolveName(name) != null; + } + + @Nullable + public NameInfo resolveName(String name) { + for (int i = blocks.size() - 1; i >= 0; i--) { + Integer id = blocks.get(i).resolve(name); + if (id != null) { + return idToNameInfo.get(id); + } + } + return null; + } + + public NameInfo resolveExistingName(String name) { + NameInfo info = resolveName(name); + if (info == null) { + throw new IllegalArgumentException("name '" + name + "' doesn't exist"); + } + return info; + } + + private void addName(int block, NameInfo nameInfo) { + NameInfo prev = idToNameInfo.putIfAbsent(nameInfo.id, nameInfo); + if (prev != null) { + throw new IllegalStateException("id " + nameInfo.id + " is already used!"); + } + blocks.get(block).add(nameInfo.name, nameInfo.id); + } + + private void addBuiltin(String name) { + addName(BUILTINS_INDEX, createNameInfo(name, new Comment("builtin"), Kind.BUILTIN)); + } + + public void addImported(String name, Identifier node) { + addName(GLOBALS_INDEX, createNameInfo(name, node, Kind.IMPORTED)); + } + + public void addIdentifier(String name, ASTNode node) { + Kind kind = blocks.size() - 1 == GLOBALS_INDEX ? Kind.GLOBAL : Kind.LOCAL; + addName(blocks.size() - 1, createNameInfo(name, node, kind)); + } + + public void addParameter(String name, ASTNode param) { + addName(blocks.size() - 1, createNameInfo(name, param, Kind.PARAMETER)); + } + + private NameInfo createNameInfo(String name, ASTNode definition, Kind kind) { + return new NameInfo(name, definition, newId(), kind); + } + + private int newId() { + int ret = nextId; + nextId++; + return ret; + } + + public Collection<Integer> getNameIdsInCurrentBlock() { + return getNameIdsInBlock(blocks.size() - 1); + } + + private Collection<Integer> getNameIdsInBlock(int block) { + return blocks.get(block).nameToId.values(); + } + + public NameInfo getNameInfo(int id) { + NameInfo info = idToNameInfo.get(id); + if (info == null) { + throw new IllegalArgumentException("id " + id + " doesn't exist"); + } + return info; + } + + /** + * Represents a lexical block, e.g. global, function-local or further nested (in a comprehension). + */ + private static class LexicalBlock { + private final Map<String, Integer> nameToId = new HashMap<>(); + + @Nullable + private Integer resolve(String name) { + return nameToId.get(name); + } + + private void add(String name, int id) { + Integer entry = nameToId.putIfAbsent(name, id); + if (entry != null) { + throw new IllegalArgumentException("name '" + name + "' is already defined"); + } + } + + public Collection<Integer> getAllSymbols() { + return nameToId.values(); + } + } + + /** Holds information about a name/symbol. */ + public static class NameInfo { + final int id; + final String name; + final ASTNode definition; + final Kind kind; + + /** Kind of a symbol (builtin, imported, global, parameter, or local). */ + public enum Kind { + BUILTIN, + IMPORTED, + GLOBAL, + PARAMETER, + LOCAL, + } + + private NameInfo(String name, ASTNode definition, int id, Kind kind) { + this.id = id; + this.name = name; + this.definition = definition; + this.kind = kind; + } + } +} 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 f4fdd063b1..b64d51dcf9 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 @@ -47,7 +47,7 @@ public class NamingConventionsChecker extends SyntaxTreeVisitor { public static List<Issue> check(BuildFileAST ast) { NamingConventionsChecker checker = new NamingConventionsChecker(); - ast.accept(checker); + checker.visit(ast); return checker.issues; } @@ -84,7 +84,10 @@ public class NamingConventionsChecker extends SyntaxTreeVisitor { @Override public void visit(Parameter<Expression, Expression> node) { - checkLowerSnakeCase(node.getName(), node.getLocation()); + String name = node.getName(); + if (name != null) { + checkLowerSnakeCase(name, node.getLocation()); + } } @Override diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Skylint.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Skylint.java index 92e2f5ae1d..d95a0778a4 100644 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Skylint.java +++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Skylint.java @@ -38,6 +38,7 @@ public class Skylint { issues.addAll(NamingConventionsChecker.check(ast)); issues.addAll(ControlFlowChecker.check(ast)); issues.addAll(StatementWithoutEffectChecker.check(ast)); + issues.addAll(UsageChecker.check(ast)); issues.sort(Issue::compare); if (!issues.isEmpty()) { System.out.println(path); 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 new file mode 100644 index 0000000000..a464e2645d --- /dev/null +++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java @@ -0,0 +1,211 @@ +// Copyright 2017 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.skylark.skylint; + +import com.google.common.collect.LinkedHashMultimap; +import com.google.common.collect.SetMultimap; +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.ForStatement; +import com.google.devtools.build.lib.syntax.Identifier; +import com.google.devtools.build.lib.syntax.IfStatement; +import com.google.devtools.build.lib.syntax.IfStatement.ConditionalStatements; +import com.google.devtools.build.lib.syntax.ListLiteral; +import com.google.devtools.skylark.skylint.Environment.NameInfo; +import com.google.devtools.skylark.skylint.Environment.NameInfo.Kind; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import javax.annotation.Nullable; + +/** Checks that every import, private function or variable definition is used somewhere. */ +public class UsageChecker extends AstVisitorWithNameResolution { + private final List<Issue> issues = new ArrayList<>(); + private UsageInfo ui = UsageInfo.empty(); + private SetMultimap<Integer, Node> idToAllDefinitions = LinkedHashMultimap.create(); + + private UsageChecker(Environment env) { + super(env); + } + + public static List<Issue> check(BuildFileAST ast) { + UsageChecker checker = new UsageChecker(Environment.defaultBazel()); + checker.visit(ast); + return checker.issues; + } + + @Override + public void visit(Identifier node) { + super.visit(node); + NameInfo info = env.resolveName(node.getName()); + // TODO(skylark-team): Don't ignore unresolved symbols in the future but report an error + if (info != null) { + ui.usedDefinitions.addAll(ui.idToLastDefinitions.get(info.id)); + } + } + + @Override + protected void visitLvalue(Expression expr) { + if (expr instanceof Identifier) { + super.visit((Identifier) expr); // don't call this.visit because it doesn't count as usage + } else if (expr instanceof ListLiteral) { + for (Expression e : ((ListLiteral) expr).getElements()) { + visitLvalue(e); + } + } else { + visit(expr); + } + } + + @Override + public void visit(IfStatement node) { + UsageInfo input = ui; + List<UsageInfo> outputs = new ArrayList<>(); + for (ConditionalStatements clause : node.getThenBlocks()) { + ui = input.copy(); + visit(clause); + outputs.add(ui); + } + ui = input.copy(); + visitBlock(node.getElseBlock()); + outputs.add(ui); + ui = UsageInfo.join(outputs); + } + + @Override + public void visit(ForStatement node) { + visit(node.getCollection()); + visit(node.getVariable()); + UsageInfo noIteration = ui.copy(); + visitBlock(node.getBlock()); + UsageInfo oneIteration = ui.copy(); + // We need to visit the block again in case a variable was reassigned in the last iteration and + // the new value isn't used until the next iteration + visit(node.getVariable()); + visitBlock(node.getBlock()); + UsageInfo manyIterations = ui.copy(); + ui = UsageInfo.join(Arrays.asList(noIteration, oneIteration, manyIterations)); + } + + @Override + protected void declare(String name, ASTNode node) { + int id = env.resolveExistingName(name).id; + ui.idToLastDefinitions.removeAll(id); + ui.idToLastDefinitions.put(id, new Node(node)); + idToAllDefinitions.put(id, new Node(node)); + } + + @Override + protected void reassign(String name, Identifier ident) { + declare(name, ident); + } + + @Override + public void exitBlock() { + Collection<Integer> ids = env.getNameIdsInCurrentBlock(); + for (Integer id : ids) { + checkUsed(id); + } + super.exitBlock(); + } + + private void checkUsed(Integer id) { + Set<Node> unusedDefinitions = idToAllDefinitions.get(id); + unusedDefinitions.removeAll(ui.usedDefinitions); + NameInfo nameInfo = env.getNameInfo(id); + String name = nameInfo.name; + if ("_".equals(name) || nameInfo.kind == Kind.BUILTIN) { + return; + } + if ((nameInfo.kind == Kind.LOCAL || nameInfo.kind == Kind.PARAMETER) && name.startsWith("_")) { + // local variables starting with an underscore need not be used + return; + } + if (nameInfo.kind == Kind.GLOBAL && !name.startsWith("_")) { + // symbol might be loaded in another file + return; + } + String message = "unused definition of '" + name + "'"; + if (nameInfo.kind == Kind.PARAMETER) { + message += + ". If this is intentional, " + + "you can add `_ignore = [<param1>, <param2>, ...]` to the function body."; + } else if (nameInfo.kind == Kind.LOCAL) { + message += ". If this is intentional, you can use '_' or rename it to '_" + name + "'."; + } + for (Node definition : unusedDefinitions) { + issues.add(new Issue(message, definition.node.getLocation())); + } + } + + private static class UsageInfo { + /** + * Stores for each variable ID the definitions that are "live", i.e. are the most recent ones on + * some execution path. + * + * <p>There can be more than one last definition if branches are involved, e.g. if foo: x = 1; + * else x = 2; + */ + private final SetMultimap<Integer, Node> idToLastDefinitions; + /** Set of definitions that have already been used at some point. */ + private final Set<Node> usedDefinitions; + + private UsageInfo(SetMultimap<Integer, Node> idToLastDefinitions, Set<Node> usedDefinitions) { + this.idToLastDefinitions = idToLastDefinitions; + this.usedDefinitions = usedDefinitions; + } + + static UsageInfo empty() { + return new UsageInfo(LinkedHashMultimap.create(), new HashSet<>()); + } + + UsageInfo copy() { + return new UsageInfo( + LinkedHashMultimap.create(idToLastDefinitions), new HashSet<>(usedDefinitions)); + } + + static UsageInfo join(Collection<UsageInfo> uis) { + UsageInfo result = UsageInfo.empty(); + for (UsageInfo ui : uis) { + result.idToLastDefinitions.putAll(ui.idToLastDefinitions); + result.usedDefinitions.addAll(ui.usedDefinitions); + } + return result; + } + } + + /** Wrapper for ASTNode that can be put in a HashSet. */ + private static class Node { + ASTNode node; + + public Node(ASTNode node) { + this.node = node; + } + + @Override + public boolean equals(@Nullable Object other) { + return other instanceof Node && ((Node) other).node == node; + } + + @Override + public int hashCode() { + return System.identityHashCode(node); + } + } +} 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 new file mode 100644 index 0000000000..86163fab47 --- /dev/null +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java @@ -0,0 +1,208 @@ +// Copyright 2017 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.skylark.skylint; + +import com.google.common.truth.Truth; +import com.google.devtools.build.lib.syntax.BuildFileAST; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests the lint done by {@link NamingConventionsChecker}. */ +@RunWith(JUnit4.class) +public class UsageCheckerTest { + private static List<Issue> findIssues(String... lines) { + String content = String.join("\n", lines); + BuildFileAST ast = + BuildFileAST.parseSkylarkString( + event -> { + throw new IllegalArgumentException(event.getMessage()); + }, + content); + return UsageChecker.check(ast); + } + + @Test + public void reportUnusedImports() throws Exception { + String message = findIssues("load('foo', 'x', 'y', _z = 'Z')").toString(); + Truth.assertThat(message).contains(":1:13: unused definition of 'x'"); + Truth.assertThat(message).contains(":1:18: unused definition of 'y'"); + Truth.assertThat(message).contains(":1:23: unused definition of '_z'"); + } + + @Test + public void reportUnusedLocals() throws Exception { + String message = findIssues("def some_function(param):", " local, local2 = 1, 3").toString(); + Truth.assertThat(message).contains(":1:19: unused definition of 'param'"); + Truth.assertThat(message) + .contains("you can add `_ignore = [<param1>, <param2>, ...]` to the function body."); + Truth.assertThat(message).contains(":2:3: unused definition of 'local'"); + Truth.assertThat(message).contains("you can use '_' or rename it to '_local'"); + Truth.assertThat(message).contains(":2:10: unused definition of 'local2'"); + Truth.assertThat(message).contains("you can use '_' or rename it to '_local2'"); + } + + @Test + public void reportUnusedComprehensionVariable() throws Exception { + String message = findIssues("[[2 for y in []] for x in []]").toString(); + Truth.assertThat(message).contains(":1:9: unused definition of 'y'"); + Truth.assertThat(message).contains("you can use '_' or rename it to '_y'"); + Truth.assertThat(message).contains(":1:22: unused definition of 'x'"); + Truth.assertThat(message).contains("you can use '_' or rename it to '_x'"); + } + + @Test + public void reportShadowingVariable() throws Exception { + Truth.assertThat( + findIssues( + "def some_function_name_foo_bar_baz_qux():", + " x = [[] for x in []]", + " print(x)") + .toString()) + .contains(":2:15: unused definition of 'x'"); + } + + @Test + public void reportShadowedVariable() throws Exception { + Truth.assertThat(findIssues("def some_function():", " x = [x for x in []]").toString()) + .contains(":2:3: unused definition of 'x'"); + } + + @Test + public void reportUnusedGlobals() throws Exception { + String message = findIssues("_UNUSED = len([])", "def _unused(): pass").toString(); + Truth.assertThat(message).contains(":1:1: unused definition of '_UNUSED'"); + Truth.assertThat(message).contains(":2:5: unused definition of '_unused'"); + } + + @Test + public void reportReassignedUnusedVariable() throws Exception { + Truth.assertThat( + findIssues("def some_function():", " x = 1", " print(x)", " x += 2").toString()) + .contains(":4:3: unused definition of 'x'"); + } + + @Test + public void reportUnusedBeforeIfElse() throws Exception { + Truth.assertThat( + findIssues( + "def f(y):", + " x = 2", + " if y:", + " x = 3", + " else:", + " x = 4", + " print(x)") + .toString()) + .contains(":2:3: unused definition of 'x'"); + } + + @Test + public void reportUnusedInForLoop() throws Exception { + String messages = + findIssues( + "def some_function_foo_bar_baz_qux():", + " for x in []:", + " print(x)", + " x = 2") + .toString(); + Truth.assertThat(messages).contains(":4:5: unused definition of 'x'"); + } + + @Test + public void dontReportUsedAfterIf() throws Exception { + Truth.assertThat( + findIssues( + "def some_function(parameter):", + " x = 2", + " if parameter:", + " x = 3", + " print(x)")) + .isEmpty(); + } + + @Test + public void dontReportUsedInNextIteration() throws Exception { + Truth.assertThat( + findIssues( + "def some_function_foo_bar_baz_qux():", + " x = 0", + " for _ in []:", + " print(x)", + " x += 1")) + .isEmpty(); + Truth.assertThat( + findIssues( + "def foo():", + " for i in range(5):", + " if i % 2 == 1:", + " print(x)", + " else:", + " x = \"abc\"")) + .isEmpty(); + } + + @Test + public void dontReportUnusedBuiltins() throws Exception { + Truth.assertThat(findIssues()).isEmpty(); + } + + @Test + public void dontReportPublicGlobals() throws Exception { + Truth.assertThat( + findIssues( + "GLOBAL = 0", + "def global_function_name_foo_bar_baz_qux(parameter):", + " print(parameter)")) + .isEmpty(); + } + + @Test + public void dontReportUsedGlobals() throws Exception { + Truth.assertThat( + findIssues( + "_GLOBAL = 0", + "def _global_function(param):", + " print(param)", + "_global_function(_GLOBAL)")) + .isEmpty(); + } + + @Test + public void dontReportUsedLocals() throws Exception { + Truth.assertThat( + findIssues( + "def f(x,y):", + " a = x", + " b = x", + " if x:", + " a = y", + " if y:", + " b = a", + " print(b)")) + .isEmpty(); + } + + @Test + public void dontReportUnderscore() throws Exception { + Truth.assertThat(findIssues("GLOBAL = [1 for _ in []]")).isEmpty(); + } + + @Test + public void dontReportLocalsStartingWithUnderscore() throws Exception { + Truth.assertThat(findIssues("def f(_param):", " _local = [[] for _x in []]")).isEmpty(); + } +} |