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

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'");
+  }
 }