Skylint: don't warn for unused initialization with 'None'.

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. Hence we want no warning saying this declaration is unused.

Example:
var = None # don't warn about the unused declaration here
if condition:
  var = 0
else:
  var = 1

As noted in the code, this also has the undesired side-effect of suppressing the warning here:
var = None # doesn't warn either but ideally should
var = 0

Fixing the latter problem is not worth the increased code complexity, in my opinion.

RELNOTES: none
PiperOrigin-RevId: 172442402

1 files changed, 40 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()));
     }