chplcheck: warn for unused pattern matching like (_, _) (chapel-lan…

…g#24859)

Fixes chapel-lang#24838.

I believe it's better to warn about unused pattern matching separately
from unused loop indices (one might avoid one but not the other), so
this PR adds a new rule. This rule has an auto-fixit so you can turn
unused `(_, _)` into `_` automatically.

Reviewed by @jabraham17 -- thanks!

## Testing
- [x] dyno tests
- [x] `test/chplcheck`

test/chplcheck/UnusedTupleUnpack.chpl

@@ -0,0 +1,6 @@
+foreach (i, _) in zip(1..10, 1..10) { i; }
+foreach (_, j) in zip(1..10, 1..10) { j; }
+foreach (_, _) in zip(1..10, 1..10) {}
+foreach (i, (_, _)) in zip(1..10, foreach pair in zip(1..10, 1..10) do pair) { i; }
+foreach (_, (_, _)) in zip(1..10, foreach pair in zip(1..10, 1..10) do pair) { i; }
+foreach ((_, _), _) in zip(foreach pair in zip(1..10, 1..10) do pair, 1..10) { i; }

test/chplcheck/UnusedTupleUnpack.good

@@ -0,0 +1,8 @@
+UnusedTupleUnpack.chpl:1: node violates rule UseExplicitModules
+UnusedTupleUnpack.chpl:3: node violates rule UnusedTupleUnpack
+UnusedTupleUnpack.chpl:4: node violates rule UnusedTupleUnpack
+UnusedTupleUnpack.chpl:5: node violates rule UnusedTupleUnpack
+UnusedTupleUnpack.chpl:5: node violates rule UnusedTupleUnpack
+UnusedTupleUnpack.chpl:6: node violates rule UnusedTupleUnpack
+UnusedTupleUnpack.chpl:6: node violates rule UnusedTupleUnpack
+[Success matching fixit for UnusedTupleUnpack]

test/chplcheck/UnusedTupleUnpack.good-fixit

@@ -0,0 +1,6 @@
+foreach (i, _) in zip(1..10, 1..10) { i; }
+foreach (_, j) in zip(1..10, 1..10) { j; }
+foreach zip(1..10, 1..10) {}
+foreach (i, _) in zip(1..10, foreach pair in zip(1..10, 1..10) do pair) { i; }
+foreach (_, _) in zip(1..10, foreach pair in zip(1..10, 1..10) do pair) { i; }
+foreach (_, _) in zip(foreach pair in zip(1..10, 1..10) do pair, 1..10) { i; }

test/chplcheck/activerules.good

@@ -13,3 +13,4 @@ Active rules:
   PascalCaseModules            Warn for modules that are not 'PascalCase'.
   SimpleDomainAsRange          Warn for simple domains in loops that can be ranges.
   UnusedLoopIndex              Warn for unused index variables in loops.
+  UnusedTupleUnpack            Warn for unused tuple unpacking, such as '(_, _)'.

test/chplcheck/allrules.good

@@ -17,4 +17,5 @@ Available rules (default rules marked with *):
   * SimpleDomainAsRange          Warn for simple domains in loops that can be ranges.
     UnusedFormal                 Warn for unused formals in functions.
   * UnusedLoopIndex              Warn for unused index variables in loops.
+  * UnusedTupleUnpack            Warn for unused tuple unpacking, such as '(_, _)'.
     UseExplicitModules           Warn for code that relies on auto-inserted implicit modules.

tools/chplcheck/src/chplcheck.py

@@ -137,9 +137,16 @@ def apply_edits(edits: List[Edit], suffix: Optional[str]):
         edits.sort(key=lambda f: f.start, reverse=True)
         with open(file, "r") as f:
             lines = f.readlines()
+
+        prev_start = None
         for edit in edits:
-            (line_start, char_start) = edit.start
-            (line_end, char_end) = edit.end
+            line_start, char_start = edit.start
+            line_end, char_end = edit.end
+
+            # Skip overlapping fixits
+            if prev_start is not None and edit.end > prev_start:
+                continue
+
             lines[line_start - 1] = (
                 lines[line_start - 1][: char_start - 1]
                 + edit.text
@@ -148,6 +155,8 @@ def apply_edits(edits: List[Edit], suffix: Optional[str]):
             if line_start != line_end:
                 lines[line_start:line_end] = [""] * (line_end - line_start)
 
+            prev_start = edit.start
+
         outfile = file if suffix is None else file + suffix
         with open(outfile, "w") as f:
             f.writelines(lines)

tools/chplcheck/src/rules.py

@@ -26,6 +26,47 @@
 from fixits import Fixit, Edit
 from rule_types import BasicRuleResult, AdvancedRuleResult
 
+def variables(node: AstNode):
+    if isinstance(node, Variable):
+        if node.name() != "_":
+            yield node
+    elif isinstance(node, TupleDecl):
+        for child in node:
+            yield from variables(child)
+
+def fixit_remove_unused_node(node: AstNode, lines: Optional[List[str]] = None, context: Optional[Context] = None) -> Optional[Fixit]:
+    """
+    Given an unused variable that's either a child of a TupleDecl or an
+    iterand in a loop, construct a Fixit that removes it or replaces it
+    with '_' as appropriate.
+
+    Expects either a lines list (containing the lines in the file where
+    the node is being fixed up) or a context object that will be used to
+    determine these lines. Raises if neither is provided.
+    """
+
+    parent = node.parent()
+    if parent is None:
+        return None
+
+    if lines is None:
+        if context is None:
+            raise ValueError("Either 'lines' or 'context' must be provided")
+        else:
+            lines = chapel.get_file_lines(context, parent)
+
+    if parent and isinstance(parent, TupleDecl):
+        return Fixit.build(Edit.build(node.location(), "_"))
+    elif parent and isinstance(parent, IndexableLoop):
+        loc = parent.header_location() or parent.location()
+        before_loc = loc - node.location()
+        after_loc = loc.clamp_left(parent.iterand().location())
+        before_lines = range_to_text(before_loc, lines)
+        after_lines = range_to_text(after_loc, lines)
+
+        return Fixit.build(Edit.build(loc, before_lines + after_lines))
+    return None
+
 def name_for_linting(context: Context, node: NamedDecl) -> str:
     name = node.name()
 
@@ -229,7 +270,21 @@ def EmptyStmts(_, node: EmptyStmt):
             # dont warn if the EmptyStmt is the only statement in a block
             return True
 
-        return BasicRuleResult(Fixit.build(Edit.build(node.location(), "")))
+        return BasicRuleResult(node, fixits=Fixit.build(Edit.build(node.location(), "")))
+
+    @driver.basic_rule(TupleDecl)
+    def UnusedTupleUnpack(context: Context, node: TupleDecl):
+        """
+        Warn for unused tuple unpacking, such as '(_, _)'.
+        """
+
+        varset = set(variables(node))
+        if len(varset) == 0:
+            fixit = fixit_remove_unused_node(node, context = context)
+            if fixit is not None:
+                return BasicRuleResult(node, fixits=fixit)
+            return False
+        return True
 
     # Five things have to match between consecutive decls for this to warn:
     # 1. same type
@@ -414,14 +469,6 @@ def UnusedLoopIndex(context: Context, root: AstNode):
         indices = dict()
         uses = set()
 
-        def variables(node):
-            if isinstance(node, Variable):
-                if node.name() != "_":
-                    yield node
-            elif isinstance(node, TupleDecl):
-                for child in node:
-                    yield from variables(child)
-
         for loop, _ in chapel.each_matching(root, IndexableLoop):
             node = loop.index()
             if node is None:
@@ -439,18 +486,7 @@ def variables(node):
         for unused in indices.keys() - uses:
             node, loop = indices[unused]
             fixit = None
-            parent = node.parent()
-            if parent and isinstance(parent, TupleDecl):
-                fixit = Fixit.build(Edit.build(node.location(), "_"))
-            elif parent and isinstance(parent, IndexableLoop):
-                loc = parent.header_location() or parent.location()
-                before_loc = loc - node.location()
-                after_loc = loc.clamp_left(parent.iterand().location())
-                before_lines = range_to_text(before_loc, lines)
-                after_lines = range_to_text(after_loc, lines)
-
-                fixit = Fixit.build(Edit.build(loc, before_lines + after_lines))
-
+            fixit = fixit_remove_unused_node(node, lines)
             fixits = [fixit] if fixit else []
             yield AdvancedRuleResult(node, loop, fixits=fixits)