Allow users to select between multiple fixits (#24744)

Follow on to #24663

This PR allows rules to define multiple fixits for a single rule,
allowing rule writers to give multiple ways to resolve an issue

For example:
![Screenshot 2024-04-02 at 9 07 53
AM](https://github.com/chapel-lang/chapel/assets/15747900/3661b0a4-8689-40a3-86a7-c8fb34c2e381)

This PR changes how Fixit works. A Fixit is made of multiple Edits (to
support Fixits editing multiple places at once) and rules can define
multiple Fixits. This PR uses that functionally to add a Fixits for
`chplcheck.ignore`. For example, on `UnusedLoopIndex` warnings, users
can either remove the index or add `chplcheck.ignore`, this PR provides
both options.

This PR also adds a `--interactive` command line flag to let non-lsp
users select between multiple fixits.

[Reviewed by @DanilaFe]

test/chplcheck/MethodsAfterFields.chpl

@@ -51,4 +51,4 @@ module MethodsAfterFields {
         return field1;
     }
   }
-}
+}

test/chplcheck/MethodsAfterFields.good

@@ -2,3 +2,4 @@ MethodsAfterFields.chpl:4: node violates rule MethodsAfterFields
 MethodsAfterFields.chpl:11: node violates rule MethodsAfterFields
 MethodsAfterFields.chpl:31: node violates rule MethodsAfterFields
 MethodsAfterFields.chpl:38: node violates rule MethodsAfterFields
+[Success matching fixit for MethodsAfterFields]

test/chplcheck/MethodsAfterFields.good-fixit

@@ -0,0 +1,58 @@
+module MethodsAfterFields {
+  record emptyRecord {   }
+
+  @chplcheck.ignore("MethodsAfterFields")
+  record methodField {
+    proc method1() {
+      return 12;
+    }
+    var field1: int;
+  }
+
+  @chplcheck.ignore("MethodsAfterFields")
+  record fieldMethodField {
+    var field1: int;
+    proc method1() {
+      return field1;
+    }
+    var field2: int;
+  }
+
+  record fieldFieldMethod {
+    var field1: int;
+    const field2: real;
+
+    proc method1() {
+      return field1;
+    }
+  }
+
+  class EmptyClass {   
+  }
+
+  @chplcheck.ignore("MethodsAfterFields")
+  class MethodField {
+    proc method1() {
+      return 12;
+    }
+    var field1: int;
+  }
+
+  @chplcheck.ignore("MethodsAfterFields")
+  class FieldMethodField {
+    var field1: int;
+    proc method1() {
+      return field1;
+    }
+    var field2: int;
+  }
+
+  class FieldFieldMethod {
+    var field1: int;
+    const field2: real;
+
+    proc method1() {
+        return field1;
+    }
+  }
+}

test/chplcheck/MisleadingIndentation.good

@@ -1 +1,2 @@
 MisleadingIndentation.chpl:4: node violates rule MisleadingIndentation
+[Success matching fixit for MisleadingIndentation]

test/chplcheck/MisleadingIndentation.good-fixit

@@ -0,0 +1,11 @@
+module Indentation {
+  @chplcheck.ignore("MisleadingIndentation")
+  for i in 1..10 do
+    writeln(i);
+    writeln("second thing");
+
+  @chplcheck.ignore("MisleadingIndentation")
+  for i in 1..10 do
+    writeln(i);
+    writeln("second thing");
+}

test/chplcheck/fixit-interactive/COMPOPTS

@@ -0,0 +1 @@
+ --using-attribute-toolname chplcheck --stop-after-pass=parseAndConvertUast

test/chplcheck/fixit-interactive/PREDIFF

@@ -0,0 +1,16 @@
+#!/bin/bash
+
+$CHPL_HOME/tools/chplcheck/chplcheck --enable-rule ConsecutiveDecls \
+  --enable-rule BoolLitInCondStatement \
+  --enable-rule UseExplicitModules \
+  --enable-rule UnusedFormal \
+  --enable-rule CamelOrPascalCaseVariables \
+  --enable-rule NestedCoforalls \
+  --internal-prefix "myprefix_" --internal-prefix "_" \
+  --skip-unstable \
+  --fixit --fixit-suffix .fixed --interactive \
+  $1.chpl <$1.input >/dev/null
+
+# move the fixed file to the output
+mv $1.chpl.fixed $2
+

test/chplcheck/fixit-interactive/SKIPIF

@@ -0,0 +1,10 @@
+#!/bin/bash
+
+source $CHPL_HOME/util/config/run-in-venv-common.bash
+
+if python3 -c "import chapel" 2> /dev/null; then
+    echo "False"
+else
+    echo "True"
+fi
+

test/chplcheck/fixit-interactive/errorHandling.chpl

@@ -0,0 +1,3 @@
+module errorHandling {
+  for i in 1..10 {}
+}

test/chplcheck/fixit-interactive/errorHandling.good

@@ -0,0 +1,3 @@
+module errorHandling {
+  for 1..10 {}
+}

test/chplcheck/fixit-interactive/errorHandling.input

@@ -0,0 +1,5 @@
+am
+
+4
+-1
+1

test/chplcheck/fixit-interactive/simple.chpl

@@ -0,0 +1,14 @@
+module M {
+
+  for {1..10} {}
+
+  for i in 1..10 {}
+
+  record R {
+    proc foo() {}
+    var x: int;
+  }
+
+  proc foo(a: int) {}
+
+}

test/chplcheck/fixit-interactive/simple.good

@@ -0,0 +1,16 @@
+module M {
+
+  for 1..10 {}
+
+  @chplcheck.ignore("UnusedLoopIndex")
+  for i in 1..10 {}
+
+  @chplcheck.ignore("MethodsAfterFields")
+  record R {
+    proc foo() {}
+    var x: int;
+  }
+
+  proc foo(a: int) {}
+
+}

test/chplcheck/fixit-interactive/simple.input

@@ -0,0 +1,4 @@
+1
+2
+1
+0

tools/chapel-py/src/chapel/__init__.py

@@ -401,3 +401,10 @@ def range_to_text(rng: Location, lines: List[str]) -> str:
     Convert a Chapel location to a single string
     """
     return "\n".join(range_to_lines(rng, lines))
+
+def get_file_lines(context: Context, node: AstNode) -> typing.List[str]:
+    """
+    Get the lines of the file containing the given node
+    """
+    path = node.location().path()
+    return context.get_file_text(path).splitlines()

tools/chplcheck/src/chplcheck.py

@@ -24,14 +24,14 @@
 import importlib.util
 import os
 import sys
-from typing import List, Optional
+from typing import List, Tuple, Optional
 
 import chapel
 import chapel.replace
 from driver import LintDriver
 from lsp import run_lsp
 from rules import register_rules
-from fixits import Fixit
+from fixits import Fixit, Edit
 
 
 def print_violation(node: chapel.AstNode, name: str):
@@ -66,26 +66,83 @@ def load_module(driver: LintDriver, file_path: str):
     rule_func(driver)
 
 
-def apply_fixits(fixits: List[Fixit], suffix: Optional[str]=None):
+def int_to_nth(n: int) -> str:
+    d = {1: "first", 2: "second", 3: "third", 4: "fourth", 5: "fifth", 6: "sixth", 7: "seventh", 8: "eighth", 9: "ninth", 10: "tenth"}
+    return d.get(n, f"{n}th")
+
+def apply_fixits(
+    violations: List[Tuple[chapel.AstNode, str, Optional[List[Fixit]]]],
+    suffix: Optional[str],
+    interactive: bool,
+) -> List[Tuple[chapel.AstNode, str, Optional[List[Fixit]]]]:
+    """
+    Apply fixits to the Chapel source code based on user input
+    Any violations not applied are returned
+    """
+    not_applied = []
+    edits_to_apply = []
+    for node, rule, fixits in violations:
+        if fixits is None or len(fixits) == 0:
+            # no fixits to apply, skip
+            not_applied.append((node, rule, None))
+            continue
+        if not interactive:
+            # apply the first fixit
+            edits_to_apply.extend(fixits[0].edits)
+            continue
+
+        options = ["Skip"]
+        for idx, fixit in enumerate(fixits, start=1):
+            if fixit.description is not None:
+                options.append(f"{fixit.description}")
+            else:
+                s = int_to_nth(idx)
+                options.append(f"Apply {s} Fix")
+        print_violation(node, rule)
+        for i, opt in enumerate(options):
+            print(f"  {i}. {opt}")
+        done = False
+        while not done:
+            try:
+                choice = input("Choose an option: ")
+                if choice == "0":
+                    not_applied.append((node, rule, fixits))
+                    done = True
+                else:
+                    try:
+                        fixit = fixits[int(choice) - 1]
+                        edits_to_apply.extend(fixit.edits)
+                        done = True
+                    except (ValueError, IndexError):
+                        print("Please enter a number corresponding to an option")
+            except KeyboardInterrupt:
+                # apply no edits, return the original violations
+                return violations
+
+    apply_edits(edits_to_apply, suffix)
+    return not_applied
+
+
+def apply_edits(edits: List[Edit], suffix: Optional[str]):
     """
     Apply a list of fixits
     """
-    fixit_per_file = defaultdict(lambda: [])
-    for fixit in fixits:
-        fixit_per_file[fixit.path].append(fixit)
-
-    # Apply fixits in reverse order to avoid invalidating the locations of
-    # subsequent fixits
-    for file, fixits in fixit_per_file.items():
-        fixits.sort(key=lambda f: f.start, reverse=True)
+    edits_per_file = defaultdict(lambda: [])
+    for edit in edits:
+        edits_per_file[edit.path].append(edit)
+
+    # Apply edits in reverse order to avoid invalidating the locations of
+    # subsequent edits
+    for file, edits in edits_per_file.items():
+        edits.sort(key=lambda f: f.start, reverse=True)
         with open(file, "r") as f:
             lines = f.readlines()
-        for fixit in fixits:
-            line_start, char_start = fixit.start
-            line_end, char_end = fixit.end
+        for edit in edits:
+            (line_start, char_start) = edit.start
+            (line_end, char_end) = edit.end
             lines[line_start - 1] = (
                 lines[line_start - 1][: char_start - 1]
-                + fixit.text
+                + edit.text
                 + lines[line_end - 1][char_end - 1 :]
             )
             if line_start != line_end:
@@ -129,6 +186,7 @@ def main():
     parser.add_argument("--list-active-rules", action="store_true", default=False,  help="List all currently enabled rules")
     parser.add_argument("--fixit", action="store_true", default=False, help="Apply fixits for the relevant rules")
     parser.add_argument("--fixit-suffix", default=None, help="Suffix to append to the original file name when applying fixits. If not set (the default), the original file will be overwritten.")
+    parser.add_argument("--interactive", "-i", action="store_true", default=False, help="Apply fixits interactively, requires --fixit")
     args = parser.parse_args()
 
     driver = LintDriver(
@@ -163,24 +221,16 @@ def main():
         context.set_module_paths([], [])
 
         # Silence errors, warnings etc. -- we're just linting.
-        with context.track_errors() as errors:
+        with context.track_errors() as _:
             asts = context.parse(filename)
             violations = list(driver.run_checks(context, asts))
 
-            if args.fixit:
-                # apply fixits, if any, then print remaining violations
-                fixits = [fixits for (_, _, fixits) in violations if fixits]
-                # flatten the list of fixits
-                fixits = [f for sublist in fixits for f in sublist]
-                apply_fixits(fixits, suffix=args.fixit_suffix)
-                violations = [
-                    (node, rule, fixit)
-                    for (node, rule, fixit) in violations
-                    if not fixit
-                ]
-
             # sort the failures in order of appearance
             violations.sort(key=lambda f: f[0].location().start()[0])
+
+            if args.fixit:
+                violations = apply_fixits(violations, args.fixit_suffix, args.interactive)
+
             for node, rule, _ in violations:
                 print_violation(node, rule)
                 printed_warning = True

tools/chplcheck/src/driver.py

@@ -164,11 +164,10 @@ def _check_basic_rule(
                 continue
 
             val = func(context, node)
-            check, fixit = None, None
+            check, fixit = None, []
             if isinstance(val, rule_types.BasicRuleResult):
-                check, fixit = False, val.fixit
-                if fixit is not None and not isinstance(fixit, list):
-                    fixit = [fixit]
+                check = False
+                fixit = val.fixits(context, name)
             else:
                 check = val
             if not check:
@@ -189,16 +188,15 @@ def _check_advanced_rule(
 
         for result in func(context, root):
             if isinstance(result, rule_types.AdvancedRuleResult):
-                node, anchor, fixit = result.node, result.anchor, result.fixit
+                node, anchor = result.node, result.anchor
+                fixits = result.fixits(context, name)
                 if anchor is not None and not self._should_check_rule(
                     name, anchor
                 ):
                     continue
-                if fixit is not None and not isinstance(fixit, list):
-                    fixit = [fixit]
             else:
                 node = result
-                fixit = None
+                fixits = None
 
             # For advanced rules, the traversal of the AST is out of our hands,
             # so we can't stop it from going into unstable modules. Instead,
@@ -207,7 +205,7 @@ def _check_advanced_rule(
             if self.skip_unstable and LintDriver._in_unstable_module(node):
                 continue
 
-            yield (node, name, fixit)
+            yield (node, name, fixits)
 
     def basic_rule(self, pat, default=True):
         """