refactor chplcheck to make integrating it into CLS easier

Signed-off-by: Jade Abraham <jade.abraham@hpe.com>

tools/chplcheck/src/chplcheck.py

@@ -32,6 +32,7 @@
 from lsp import run_lsp
 from rules import register_rules
 from fixits import Fixit, Edit
+from config import Config
 
 
 def print_violation(node: chapel.AstNode, name: str):
@@ -201,26 +202,8 @@ def main():
         prog="chplcheck", description="A linter for the Chapel language"
     )
     parser.add_argument("filenames", nargs="*")
-    parser.add_argument(
-        "--disable-rule", action="append", dest="disabled_rules", default=[]
-    )
-    parser.add_argument(
-        "--enable-rule", action="append", dest="enabled_rules", default=[]
-    )
+    Config.add_arguments(parser)
     parser.add_argument("--lsp", action="store_true", default=False)
-    parser.add_argument("--skip-unstable", action="store_true", default=False)
-    parser.add_argument(
-        "--internal-prefix",
-        action="append",
-        dest="internal_prefixes",
-        default=[],
-    )
-    parser.add_argument(
-        "--add-rules",
-        action="append",
-        default=[],
-        help="Add a custom rule file",
-    )
     parser.add_argument(
         "--list-rules",
         action="store_true",
@@ -253,22 +236,21 @@ def main():
     )
     args = parser.parse_args()
 
-    driver = LintDriver(
-        skip_unstable=args.skip_unstable,
-        internal_prefixes=args.internal_prefixes,
-    )
+    config = Config.from_args(args)
+    driver = LintDriver(config)
+
     # register rules before enabling/disabling
     register_rules(driver)
-    for p in args.add_rules:
+    for p in config.add_rules:
         load_module(driver, os.path.abspath(p))
 
     if args.list_rules:
         print("Available rules (default rules marked with *):")
         print_rules(driver)
         return
 
-    driver.disable_rules(*args.disabled_rules)
-    driver.enable_rules(*args.enabled_rules)
+    driver.disable_rules(*config.disabled_rules)
+    driver.enable_rules(*config.enabled_rules)
 
     if args.list_active_rules:
         print("Active rules:")

tools/chplcheck/src/config.py

@@ -0,0 +1,73 @@
+from dataclasses import dataclass
+from typing import List, Union, Dict, Any
+import argparse
+
+
+@dataclass
+class Config:
+    """
+    Configuration options that are specific to linting, not the CLI
+    """
+
+    disabled_rules: List[str]
+    enabled_rules: List[str]
+    skip_unstable: bool
+    internal_prefixes: List[str]
+    add_rules: List[str]
+
+    @classmethod
+    def add_arguments(cls, parser: argparse.ArgumentParser, prefix: str = ""):
+        parser.add_argument(
+            f"--{prefix}disable-rule",
+            action="append",
+            dest="chplcheck_disabled_rules",
+            default=[],
+            help="Disable a lint rule by name"
+        )
+        parser.add_argument(
+            f"--{prefix}enable-rule",
+            action="append",
+            dest="chplcheck_enabled_rules",
+            default=[],
+            help="Enable a lint rule by name"
+        )
+        parser.add_argument(
+            f"--{prefix}skip-unstable",
+            action="store_true",
+            dest="chplcheck_skip_unstable",
+            default=False,
+            help="Skip unstable rules when linting",
+        )
+        parser.add_argument(
+            f"--{prefix}internal-prefix",
+            action="append",
+            dest="chplcheck_internal_prefixes",
+            default=[],
+            help="Add a prefix to the list of internal prefixes used when linting",
+        )
+        parser.add_argument(
+            f"--{prefix}add-rules",
+            action="append",
+            dest="chplcheck_add_rules",
+            default=[],
+            help="Add a custom rule file for chplcheck",
+        )
+
+    @classmethod
+    def from_args(cls, args: Union[argparse.Namespace, Dict[str, Any]]):
+        if isinstance(args, argparse.Namespace):
+            return cls(
+                disabled_rules=args.chplcheck_disabled_rules,
+                enabled_rules=args.chplcheck_enabled_rules,
+                skip_unstable=args.chplcheck_skip_unstable,
+                internal_prefixes=args.chplcheck_internal_prefixes,
+                add_rules=args.chplcheck_add_rules,
+            )
+        else:
+            return cls(
+                disabled_rules=args["chplcheck_disabled_rules"],
+                enabled_rules=args["chplcheck_enabled_rules"],
+                skip_unstable=args["chplcheck_skip_unstable"],
+                internal_prefixes=args["chplcheck_internal_prefixes"],
+                add_rules=args["chplcheck_add_rules"],
+            )

tools/chplcheck/src/driver.py

@@ -23,6 +23,7 @@
 import chapel
 from fixits import Fixit
 import rule_types
+from config import Config
 
 IgnoreAttr = ("chplcheck.ignore", ["rule", "comment"])
 
@@ -59,12 +60,11 @@ class LintDriver:
     for registering new rules.
     """
 
-    def __init__(self, skip_unstable: bool, internal_prefixes: List[str]):
+    def __init__(self, config: Config):
+        self.config: Config = config
         self.SilencedRules: List[str] = []
         self.BasicRules: List[Tuple[str, Any, rule_types.BasicRule]] = []
         self.AdvancedRules: List[Tuple[str, rule_types.AdvancedRule]] = []
-        self.skip_unstable: bool = skip_unstable
-        self.internal_prefixes: List[str] = internal_prefixes
 
     def rules_and_descriptions(self):
         # Use a dict in case a rule is registered multiple times.
@@ -109,7 +109,7 @@ def _should_check_rule(
     def _has_internal_name(self, node: chapel.AstNode):
         if not hasattr(node, "name"):
             return False
-        return any(node.name().startswith(p) for p in self.internal_prefixes)
+        return any(node.name().startswith(p) for p in self.config.internal_prefixes)
 
     @staticmethod
     def _is_unstable_module(node: chapel.AstNode):
@@ -130,7 +130,7 @@ def _in_unstable_module(node: chapel.AstNode):
         return False
 
     def _preorder_skip_unstable_modules(self, node):
-        if not self.skip_unstable:
+        if not self.config.skip_unstable:
             yield from chapel.preorder(node)
             return
 
@@ -202,7 +202,7 @@ def _check_advanced_rule(
             # so we can't stop it from going into unstable modules. Instead,
             # once the rule emits a warning, check by traversing the AST
             # if the warning target should be skipped.
-            if self.skip_unstable and LintDriver._in_unstable_module(node):
+            if self.config.skip_unstable and LintDriver._in_unstable_module(node):
                 continue
 
             yield (node, name, fixits)

tools/chplcheck/src/lsp.py

@@ -17,7 +17,7 @@
 # limitations under the License.
 #
 
-from typing import Union
+from typing import Union, List
 import chapel
 import chapel.lsp
 from pygls.server import LanguageServer
@@ -36,6 +36,80 @@
 from driver import LintDriver
 
 
+def _get_location(node: chapel.AstNode):
+    """Helper to get the location of a node"""
+    if isinstance(node, chapel.NamedDecl):
+        return chapel.lsp.location_to_range(node.name_location())
+    else:
+        return chapel.lsp.location_to_range(node.location())
+
+def get_lint_diagnostics(context: chapel.Context, driver: LintDriver, asts: List[chapel.AstNode]) -> List[Diagnostic]:
+    """
+    Run the linter rules on the Chapel ASTs and return them as LSP diagnostics.
+    """
+    diagnostics = []
+    # Silence errors from scope resolution etc., especially since they
+    # may be emitted from other files (dependencies).
+    with context.track_errors() as _:
+        for node, rule, fixits in driver.run_checks(context, asts):
+            diagnostic = Diagnostic(
+                range=_get_location(node),
+                message="Lint: rule [{}] violated".format(rule),
+                severity=DiagnosticSeverity.Warning,
+            )
+            if fixits:
+                fixits = [Fixit.to_dict(f) for f in fixits]
+                diagnostic.data = {"rule": rule, "fixits": fixits}
+            diagnostics.append(diagnostic)
+    return diagnostics
+
+def get_lint_actions(diagnostics: List[Diagnostic]) -> List[CodeAction]:
+    """
+    Return LSP code actions for the given diagnostics.
+    """
+    actions = []
+    for d in diagnostics:
+        if not d.data:
+            continue
+        name = d.data.get("rule", None)
+        if not name:
+            continue
+        if "fixits" not in d.data:
+            continue
+        fixits = [Fixit.from_dict(f) for f in d.data["fixits"]]
+        if not fixits:
+            continue
+
+        for f in fixits:
+            if not f:
+                continue
+            changes = dict()
+            for e in f.edits:
+                uri = "file://" + e.path
+                start = e.start
+                end = e.end
+                rng = Range(
+                    start=Position(
+                        max(start[0] - 1, 0), max(start[1] - 1, 0)
+                    ),
+                    end=Position(max(end[0] - 1, 0), max(end[1] - 1, 0)),
+                )
+                edit = TextEdit(range=rng, new_text=e.text)
+                if uri not in changes:
+                    changes[uri] = []
+                changes[uri].append(edit)
+            title = "Apply Fix for {}".format(name)
+            if f.description:
+                title += " ({})".format(f.description)
+            action = CodeAction(
+                title=title,
+                kind=CodeActionKind.QuickFix,
+                diagnostics=[d],
+                edit=WorkspaceEdit(changes=changes),
+            )
+            actions.append(action)
+    return actions
+
 def run_lsp(driver: LintDriver):
     """
     Start a language server on the standard input/output, and use it to
@@ -80,12 +154,6 @@ def parse_file(context: chapel.Context, uri: str):
 
         return context.parse(uri[len("file://") :])
 
-    def get_location(node: chapel.AstNode):
-        if isinstance(node, chapel.NamedDecl):
-            return chapel.lsp.location_to_range(node.name_location())
-        else:
-            return chapel.lsp.location_to_range(node.location())
-
     def build_diagnostics(uri: str):
         """
         Parse a file at a particular URI, run the linter rules on the resulting
@@ -96,20 +164,7 @@ def build_diagnostics(uri: str):
         with context.track_errors() as errors:
             asts = parse_file(context, uri)
 
-        # Silence errors from scope resolution etc., especially since they
-        # may be emitted from other files (dependencies).
-        with context.track_errors() as _:
-            diagnostics = []
-            for node, rule, fixits in driver.run_checks(context, asts):
-                diagnostic = Diagnostic(
-                    range=get_location(node),
-                    message="Lint: rule [{}] violated".format(rule),
-                    severity=DiagnosticSeverity.Warning,
-                )
-                if fixits:
-                    fixits = [Fixit.to_dict(f) for f in fixits]
-                    diagnostic.data = {"rule": rule, "fixits": fixits}
-                diagnostics.append(diagnostic)
+        diagnostics = get_lint_diagnostics(context, driver, asts)
 
         # process the errors from syntax/scope resolution
         # TODO: should chplcheck still do this?
@@ -130,47 +185,7 @@ async def did_open(
     @server.feature(TEXT_DOCUMENT_CODE_ACTION)
     async def code_action(ls: LanguageServer, params: CodeActionParams):
         diagnostics = params.context.diagnostics
-        actions = []
-        for d in diagnostics:
-            if not d.data:
-                continue
-            name = d.data.get("rule", None)
-            if not name:
-                continue
-            if "fixits" not in d.data:
-                continue
-            fixits = [Fixit.from_dict(f) for f in d.data["fixits"]]
-            if not fixits:
-                continue
-
-            for f in fixits:
-                if not f:
-                    continue
-                changes = dict()
-                for e in f.edits:
-                    uri = "file://" + e.path
-                    start = e.start
-                    end = e.end
-                    rng = Range(
-                        start=Position(
-                            max(start[0] - 1, 0), max(start[1] - 1, 0)
-                        ),
-                        end=Position(max(end[0] - 1, 0), max(end[1] - 1, 0)),
-                    )
-                    edit = TextEdit(range=rng, new_text=e.text)
-                    if uri not in changes:
-                        changes[uri] = []
-                    changes[uri].append(edit)
-                title = "Apply Fix for {}".format(name)
-                if f.description:
-                    title += " ({})".format(f.description)
-                action = CodeAction(
-                    title=title,
-                    kind=CodeActionKind.QuickFix,
-                    diagnostics=[d],
-                    edit=WorkspaceEdit(changes=changes),
-                )
-                actions.append(action)
+        actions = get_lint_actions(diagnostics)
         return actions
 
     server.start_io()