diff options
Diffstat (limited to 'src/tools')
-rw-r--r-- | src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java | 41 | ||||
-rw-r--r-- | src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java | 48 |
2 files changed, 88 insertions, 1 deletions
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 36614e75ba..a1e00adc27 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 @@ -19,7 +19,9 @@ import com.google.common.base.Equivalence.Wrapper; 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.AssignmentStatement; import com.google.devtools.build.lib.syntax.BuildFileAST; +import com.google.devtools.build.lib.syntax.Expression; import com.google.devtools.build.lib.syntax.ExpressionStatement; import com.google.devtools.build.lib.syntax.FlowStatement; import com.google.devtools.build.lib.syntax.ForStatement; @@ -46,6 +48,7 @@ public class UsageChecker extends AstVisitorWithNameResolution { private UsageInfo ui = UsageInfo.empty(); private final SetMultimap<Integer, Wrapper<ASTNode>> idToAllDefinitions = LinkedHashMultimap.create(); + private final Set<Wrapper<ASTNode>> initializationsWithNone = new LinkedHashSet<>(); public static List<Issue> check(BuildFileAST ast) { UsageChecker checker = new UsageChecker(); @@ -109,6 +112,38 @@ public class UsageChecker extends AstVisitorWithNameResolution { } } + @Override + public void visit(AssignmentStatement node) { + super.visit(node); + /* If a variable is initialized with None, and there exist other assignments to the variable, + * then this initialization is itself considered as a usage. This is because it's good practice + * to place a "declaration" of a variable in a location that dominates all its uses, especially + * so if you want to document the variable. Example: + * + * var = None # don't warn about the unused binding + * if condition: + * var = 0 + * else: + * var = 1 + * + * Unfortunately, as a side-effect, the following won't trigger a warning either: + * + * var = None # doesn't warn either but ideally should + * var = 0 + */ + Expression lhs = node.getLValue().getExpression(); + Expression rhs = node.getExpression(); + if (lhs instanceof Identifier + && rhs instanceof Identifier + && ((Identifier) rhs).getName().equals("None")) { + NameInfo info = env.resolveName(((Identifier) lhs).getName()); + // if it's an initialization: + if (info != null && idToAllDefinitions.get(info.id).size() == 1) { + initializationsWithNone.add(wrapNode(lhs)); + } + } + } + private void defineIdentifier(NameInfo name, ASTNode node) { ui.idToLastDefinitions.removeAll(name.id); ui.idToLastDefinitions.put(name.id, wrapNode(node)); @@ -147,7 +182,7 @@ public class UsageChecker extends AstVisitorWithNameResolution { } private void checkUsed(Integer id) { - Set<Wrapper<ASTNode>> unusedDefinitions = idToAllDefinitions.get(id); + Set<Wrapper<ASTNode>> unusedDefinitions = new LinkedHashSet<>(idToAllDefinitions.get(id)); unusedDefinitions.removeAll(ui.usedDefinitions); NameInfo nameInfo = env.getNameInfo(id); String name = nameInfo.name; @@ -171,6 +206,10 @@ public class UsageChecker extends AstVisitorWithNameResolution { message += ". If this is intentional, you can use '_' or rename it to '_" + name + "'."; } for (Wrapper<ASTNode> definition : unusedDefinitions) { + if (initializationsWithNone.contains(definition) && idToAllDefinitions.get(id).size() > 1) { + // initializations with None are OK, cf. visit(AssignmentStatement) above + continue; + } issues.add( Issue.create(UNUSED_BINDING_CATEGORY, message, unwrapNode(definition).getLocation())); } 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 b477cce1a8..3fd9760d2c 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 @@ -287,4 +287,52 @@ public class UsageCheckerTest { public void dontReportLocalsStartingWithUnderscore() throws Exception { Truth.assertThat(findIssues("def f(_param):", " _local = [[] for _x in []]")).isEmpty(); } + + @Test + public void dontReportInitializationWithNoneAsDeclaration() throws Exception { + Truth.assertThat( + findIssues( + "def foo(bar):", + " baz = None # here should be no unused warning", + " # because we want to allow people to 'declare' a variable in one location", + " if bar:", + " baz = 0", + " else:", + " baz = 1", + " print(baz)")) + .isEmpty(); + } + + @Test + public void reportUnusedInitializationWithNone() throws Exception { + Truth.assertThat( + findIssues("def foo():", " baz = None # warn here because 'baz' is never used") + .toString()) + .contains("2:3-2:5: unused binding of 'baz'"); + } + + @Test + public void reportSubsequentInitializations() throws Exception { + Truth.assertThat( + findIssues( + "def foo():", + " baz = None", + " baz = None # do warn here (not an initialization)") + .toString()) + .contains("3:3-3:5: unused binding of 'baz'"); + Truth.assertThat( + findIssues( + "def foo():", + " baz = None", + " baz = 0 # do warn here (it's a regular assignment)") + .toString()) + .contains("3:3-3:5: unused binding of 'baz'"); + Truth.assertThat( + findIssues( + "def foo():", + " baz = 0", + " baz = None # do warn here (not an initialization)") + .toString()) + .contains("3:3-3:5: unused binding of 'baz'"); + } } |