From 9eb12f76a68aa2e9e7833c959a6bce399c90e807 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Mon, 10 Jun 2024 19:48:48 -0700 Subject: [PATCH 01/11] refactor chplcheck to make integrating it into CLS easier Signed-off-by: Jade Abraham --- tools/chplcheck/src/chplcheck.py | 34 ++------ tools/chplcheck/src/config.py | 73 ++++++++++++++++ tools/chplcheck/src/driver.py | 12 +-- tools/chplcheck/src/lsp.py | 139 +++++++++++++++++-------------- 4 files changed, 164 insertions(+), 94 deletions(-) create mode 100644 tools/chplcheck/src/config.py diff --git a/tools/chplcheck/src/chplcheck.py b/tools/chplcheck/src/chplcheck.py index 0cf0bcdffb65..5aeafea7b03b 100755 --- a/tools/chplcheck/src/chplcheck.py +++ b/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,13 +236,12 @@ 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: @@ -267,8 +249,8 @@ def main(): 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:") diff --git a/tools/chplcheck/src/config.py b/tools/chplcheck/src/config.py new file mode 100644 index 000000000000..3292e99933a8 --- /dev/null +++ b/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"], + ) diff --git a/tools/chplcheck/src/driver.py b/tools/chplcheck/src/driver.py index 617e7f292e8f..1060724ec1a5 100644 --- a/tools/chplcheck/src/driver.py +++ b/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) diff --git a/tools/chplcheck/src/lsp.py b/tools/chplcheck/src/lsp.py index e7beff937455..1329fe296045 100644 --- a/tools/chplcheck/src/lsp.py +++ b/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() From 74ce5150a048aef8bb500cd013bcc6b1520da397 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Mon, 10 Jun 2024 19:49:03 -0700 Subject: [PATCH 02/11] add chplcheck inside CLS Signed-off-by: Jade Abraham --- .../src/chpl-language-server.py | 80 ++++++++++++++++++- 1 file changed, 78 insertions(+), 2 deletions(-) diff --git a/tools/chpl-language-server/src/chpl-language-server.py b/tools/chpl-language-server/src/chpl-language-server.py index 6cdea80286eb..eff33a430412 100755 --- a/tools/chpl-language-server/src/chpl-language-server.py +++ b/tools/chpl-language-server/src/chpl-language-server.py @@ -40,7 +40,8 @@ import os import json import re - +import sys +import importlib.util import chapel from chapel.lsp import location_to_range, error_to_diagnostic @@ -142,6 +143,41 @@ import argparse import configargparse + +# attempt to import chplcheck +try: + chpl_home = os.environ.get("CHPL_HOME") + if chpl_home is None: + raise ValueError("CHPL_HOME not set") + chplcheck_path = os.path.join(chpl_home, "tools", "chplcheck", "src") + # Add chplcheck to the path, but load via importlib + sys.path.append(chplcheck_path) + def load_module(module_name: str): + file_path = os.path.join(chplcheck_path, module_name + ".py") + spec = importlib.util.spec_from_file_location(module_name, file_path) + if spec is None: + raise ValueError(f"Could not load module from {file_path}") + module = importlib.util.module_from_spec(spec) + sys.modules[module_name] = module + if spec.loader is None: + raise ValueError(f"Could not load module from {file_path}") + spec.loader.exec_module(module) + return module + chplcheck = load_module("chplcheck") + chplcheck_lsp = load_module("lsp") + chplcheck_config = load_module("config") + chplcheck_driver = load_module("driver") + chplcheck_rules = load_module("rules") + +except ValueError as e: + if os.environ.get("CHPL_DEVELOPER", None): + print("Error loading chplcheck: ", str(e), file=sys.stderr) + chplcheck = None + chplcheck_lsp = None + chplcheck_config = None + chplcheck_driver = None + chplcheck_rules = None + REAL_NUMBERIC = (chapel.RealLiteral, chapel.IntLiteral, chapel.UintLiteral) NUMERIC = REAL_NUMBERIC + (chapel.ImagLiteral,) @@ -952,6 +988,11 @@ def add_bool_flag(name: str, dest: str, default: bool): self.parser.add_argument("--end-markers", default="none") self.parser.add_argument("--end-marker-threshold", type=int, default=10) + add_bool_flag("lint", "do_linting", False) + if chplcheck_config: + chplcheck_config.Config.add_arguments(self.parser, "chplcheck-") + + def _parse_end_markers(self): self.args["end_markers"] = [ a.strip() for a in self.args["end_markers"].split(",") @@ -1005,6 +1046,10 @@ def __init__(self, config: CLSConfig): self.end_markers: List[str] = config.get("end_markers") self.end_marker_threshold: int = config.get("end_marker_threshold") self.end_marker_patterns = self._get_end_marker_patterns() + self.do_linting: bool = config.get("do_linting") + + self.lint_driver = None + self._setup_linter(config) self._setup_regexes() @@ -1020,6 +1065,25 @@ def _setup_regexes(self): # use pat2 first since it is more specific self._find_rename_deprecation_regex = re.compile(f"({pat2})|({pat1})") + def _setup_linter(self, clsConfig: CLSConfig): + """ + Setup the linter, if it is enabled + """ + if not (self.do_linting and chplcheck and chplcheck_lsp and chplcheck_config and chplcheck_driver and chplcheck_rules): + return + + config = chplcheck_config.Config.from_args(clsConfig.args) + self.lint_driver = chplcheck_driver.LintDriver(config) + + chplcheck_rules.register_rules(self.lint_driver) + + for p in config.add_rules: + chplcheck.load_module(self.lint_driver, os.path.abspath(p)) + + self.lint_driver.disable_rules(*config.disabled_rules) + self.lint_driver.enable_rules(*config.enabled_rules) + + def get_deprecation_replacement( self, text: str ) -> Tuple[Optional[str], Optional[str]]: @@ -1117,7 +1181,7 @@ def build_diagnostics(self, uri: str) -> List[Diagnostic]: as a list of LSP Diagnostics. """ - _, errors = self.get_file_info(uri, do_update=True) + fi, errors = self.get_file_info(uri, do_update=True) diagnostics = [] for e in errors: @@ -1130,6 +1194,12 @@ def build_diagnostics(self, uri: str) -> List[Diagnostic]: ] diagnostics.append(diag) + # get lint diagnostics if applicable + if self.lint_driver: + assert chplcheck_lsp + lint_diagnostics = chplcheck_lsp.get_lint_diagnostics(fi.context.context, self.lint_driver, fi.get_asts()) + diagnostics.extend(lint_diagnostics) + return diagnostics def get_text(self, text_doc: TextDocument, rng: Range) -> str: @@ -1685,6 +1755,12 @@ async def code_action(ls: ChapelLanguageServer, params: CodeActionParams): ) ) + # get lint fixits if applicable + if ls.lint_driver: + assert chplcheck_lsp + lint_actions = chplcheck_lsp.get_lint_actions(params.context.diagnostics) + actions.extend(lint_actions) + return actions @server.feature(TEXT_DOCUMENT_RENAME) From 20b9b2228f3026b628f2235e56bbbe7d8de5d892 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Mon, 10 Jun 2024 19:52:26 -0700 Subject: [PATCH 03/11] rename flag Signed-off-by: Jade Abraham --- tools/chpl-language-server/src/chpl-language-server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/chpl-language-server/src/chpl-language-server.py b/tools/chpl-language-server/src/chpl-language-server.py index eff33a430412..ee3281e878fe 100755 --- a/tools/chpl-language-server/src/chpl-language-server.py +++ b/tools/chpl-language-server/src/chpl-language-server.py @@ -988,7 +988,7 @@ def add_bool_flag(name: str, dest: str, default: bool): self.parser.add_argument("--end-markers", default="none") self.parser.add_argument("--end-marker-threshold", type=int, default=10) - add_bool_flag("lint", "do_linting", False) + add_bool_flag("chplcheck", "do_linting", False) if chplcheck_config: chplcheck_config.Config.add_arguments(self.parser, "chplcheck-") From a7ae01a53f3bd1abe055448dab9e4764d82c4c58 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Mon, 10 Jun 2024 20:07:24 -0700 Subject: [PATCH 04/11] update docs Signed-off-by: Jade Abraham --- .../chpl-language-server.rst | 28 +++++++++++++++---- doc/rst/tools/chplcheck/chplcheck.rst | 7 ----- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/doc/rst/tools/chpl-language-server/chpl-language-server.rst b/doc/rst/tools/chpl-language-server/chpl-language-server.rst index 4594a6dc4068..e36f27d9a00a 100644 --- a/doc/rst/tools/chpl-language-server/chpl-language-server.rst +++ b/doc/rst/tools/chpl-language-server/chpl-language-server.rst @@ -77,7 +77,7 @@ syntax highlighting in Emacs): (with-eval-after-load 'eglot (add-to-list 'eglot-server-programs - '(chpl-mode . ("chpl-language-server")))) + '(chpl-mode . ("chpl-language-server", "--chplcheck")))) This will enable using the language server with a particular ``.chpl`` file by calling ``M-x eglot``. @@ -91,11 +91,8 @@ additionally add the following to your ``.emacs`` file: .. note:: - There is currently a limitation with Eglot that only one language server can - be registered per language. We are investigating merging the support for - :ref:`readme-chplcheck` such that both can be used in Emacs at the same time, - stay tuned! - + The above uses the ``--chplcheck`` flag to enable additional diagnostics from + ``chplcheck``. If you do not want to use ``chplcheck``, you can remove this. Supported Features ------------------ @@ -191,6 +188,25 @@ The following features are extra visual aids: | Instantiations | instantiations of a generic function. | | +----------------+--------------------------------------------+---------------------------------------+ +Using ``chplcheck`` from ``CLS`` +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Although ``chplcheck`` is a separate tool from ``CLS``, it can be used from +``CLS`` to provide additional diagnostics. This is done by enabling the +``--chplcheck`` flag. This will incorporate the diagnostics and fixits from +``chplcheck``. + +You can also still pass many of the same ``chplcheck`` flags to ``CLS``, just +prefixed with ``--chplcheck-``. For example, the following command runs the +language server with linting enabled various ``chplcheck`` flags: + +.. code-block:: bash + + chpl-language-server --chplcheck \ + --chplcheck-enable-rule UseExplicitModules \ + --chplcheck-disable-rule UnusedLoopIndex \ + --chplcheck-add-rules path/to/my/myrules.py + Configuring Chapel Projects --------------------------- diff --git a/doc/rst/tools/chplcheck/chplcheck.rst b/doc/rst/tools/chplcheck/chplcheck.rst index b59f17c7b1e7..4693262ccd66 100644 --- a/doc/rst/tools/chplcheck/chplcheck.rst +++ b/doc/rst/tools/chplcheck/chplcheck.rst @@ -168,13 +168,6 @@ additionally add the following to your ``.emacs`` file: (add-hook 'chpl-mode-hook 'eglot-ensure) -.. note:: - - There is currently a limitation with Eglot that only one language server can - be registered per language. We are investigating merging the support for - :ref:`readme-chpl-language-server` such that both can be used in Emacs at the - same time, stay tuned! - Writing New Rules From 494ec0c097bb5f45cb9b4ca9881ce30400a58d8d Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Mon, 10 Jun 2024 20:11:33 -0700 Subject: [PATCH 05/11] add license Signed-off-by: Jade Abraham --- tools/chplcheck/src/config.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/tools/chplcheck/src/config.py b/tools/chplcheck/src/config.py index 3292e99933a8..d2a5ba6cf571 100644 --- a/tools/chplcheck/src/config.py +++ b/tools/chplcheck/src/config.py @@ -1,3 +1,22 @@ +# +# Copyright 2023-2024 Hewlett Packard Enterprise Development LP +# Other additional copyright holders may be indicated within. +# +# The entirety of this work is licensed under the Apache License, +# Version 2.0 (the "License"); you may not use this file except +# in compliance with the License. +# +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + from dataclasses import dataclass from typing import List, Union, Dict, Any import argparse @@ -22,14 +41,14 @@ def add_arguments(cls, parser: argparse.ArgumentParser, prefix: str = ""): action="append", dest="chplcheck_disabled_rules", default=[], - help="Disable a lint rule by name" + 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" + help="Enable a lint rule by name", ) parser.add_argument( f"--{prefix}skip-unstable", From a9b926b5efce7aabe3340bb4e5b098f24fb15ae7 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Mon, 10 Jun 2024 20:11:52 -0700 Subject: [PATCH 06/11] format Signed-off-by: Jade Abraham --- .../src/chpl-language-server.py | 21 ++++++++++++++----- tools/chplcheck/src/driver.py | 8 +++++-- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/tools/chpl-language-server/src/chpl-language-server.py b/tools/chpl-language-server/src/chpl-language-server.py index ee3281e878fe..0e3902746128 100755 --- a/tools/chpl-language-server/src/chpl-language-server.py +++ b/tools/chpl-language-server/src/chpl-language-server.py @@ -152,6 +152,7 @@ chplcheck_path = os.path.join(chpl_home, "tools", "chplcheck", "src") # Add chplcheck to the path, but load via importlib sys.path.append(chplcheck_path) + def load_module(module_name: str): file_path = os.path.join(chplcheck_path, module_name + ".py") spec = importlib.util.spec_from_file_location(module_name, file_path) @@ -163,6 +164,7 @@ def load_module(module_name: str): raise ValueError(f"Could not load module from {file_path}") spec.loader.exec_module(module) return module + chplcheck = load_module("chplcheck") chplcheck_lsp = load_module("lsp") chplcheck_config = load_module("config") @@ -992,7 +994,6 @@ def add_bool_flag(name: str, dest: str, default: bool): if chplcheck_config: chplcheck_config.Config.add_arguments(self.parser, "chplcheck-") - def _parse_end_markers(self): self.args["end_markers"] = [ a.strip() for a in self.args["end_markers"].split(",") @@ -1069,7 +1070,14 @@ def _setup_linter(self, clsConfig: CLSConfig): """ Setup the linter, if it is enabled """ - if not (self.do_linting and chplcheck and chplcheck_lsp and chplcheck_config and chplcheck_driver and chplcheck_rules): + if not ( + self.do_linting + and chplcheck + and chplcheck_lsp + and chplcheck_config + and chplcheck_driver + and chplcheck_rules + ): return config = chplcheck_config.Config.from_args(clsConfig.args) @@ -1083,7 +1091,6 @@ def _setup_linter(self, clsConfig: CLSConfig): self.lint_driver.disable_rules(*config.disabled_rules) self.lint_driver.enable_rules(*config.enabled_rules) - def get_deprecation_replacement( self, text: str ) -> Tuple[Optional[str], Optional[str]]: @@ -1197,7 +1204,9 @@ def build_diagnostics(self, uri: str) -> List[Diagnostic]: # get lint diagnostics if applicable if self.lint_driver: assert chplcheck_lsp - lint_diagnostics = chplcheck_lsp.get_lint_diagnostics(fi.context.context, self.lint_driver, fi.get_asts()) + lint_diagnostics = chplcheck_lsp.get_lint_diagnostics( + fi.context.context, self.lint_driver, fi.get_asts() + ) diagnostics.extend(lint_diagnostics) return diagnostics @@ -1758,7 +1767,9 @@ async def code_action(ls: ChapelLanguageServer, params: CodeActionParams): # get lint fixits if applicable if ls.lint_driver: assert chplcheck_lsp - lint_actions = chplcheck_lsp.get_lint_actions(params.context.diagnostics) + lint_actions = chplcheck_lsp.get_lint_actions( + params.context.diagnostics + ) actions.extend(lint_actions) return actions diff --git a/tools/chplcheck/src/driver.py b/tools/chplcheck/src/driver.py index 1060724ec1a5..f456930ca762 100644 --- a/tools/chplcheck/src/driver.py +++ b/tools/chplcheck/src/driver.py @@ -109,7 +109,9 @@ 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.config.internal_prefixes) + return any( + node.name().startswith(p) for p in self.config.internal_prefixes + ) @staticmethod def _is_unstable_module(node: chapel.AstNode): @@ -202,7 +204,9 @@ 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.config.skip_unstable and LintDriver._in_unstable_module(node): + if self.config.skip_unstable and LintDriver._in_unstable_module( + node + ): continue yield (node, name, fixits) From 0408cbaec28c85353f46bcbe2a9d1ea6cf8e653b Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Mon, 10 Jun 2024 20:32:15 -0700 Subject: [PATCH 07/11] add basic chplcheck integration test Signed-off-by: Jade Abraham --- tools/chpl-language-server/test/linting.py | 70 ++++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 tools/chpl-language-server/test/linting.py diff --git a/tools/chpl-language-server/test/linting.py b/tools/chpl-language-server/test/linting.py new file mode 100644 index 000000000000..65f808ed7f83 --- /dev/null +++ b/tools/chpl-language-server/test/linting.py @@ -0,0 +1,70 @@ +""" +Test that the linter is working properly from CLS. These tests are not meant to be exhaustive of the linter's capabilities, rather they are meant to ensure that the integration between CLS and chplcheck is working properly. +""" + +import sys + +from lsprotocol.types import ClientCapabilities +from lsprotocol.types import InitializeParams +from lsprotocol.types import CodeActionParams, CodeActionContext +import pytest +import pytest_lsp +from pytest_lsp import ClientServerConfig, LanguageClient + +from util.utils import * +from util.config import CLS_PATH + + +@pytest_lsp.fixture( + config=ClientServerConfig( + server_command=[sys.executable, CLS_PATH(), "--chplcheck"], + client_factory=get_base_client, + ) +) +async def client(lsp_client: LanguageClient): + # Setup + params = InitializeParams(capabilities=ClientCapabilities()) + await lsp_client.initialize_session(params) + + yield + + # Teardown + await lsp_client.shutdown_session() + + +@pytest.mark.asyncio +async def test_lint_warnings(client: LanguageClient): + """ + Test that lint warnings are reported properly + """ + file = """ + for i in {1..10} do { } + """ + async with source_file(client, file, 3) as doc: + pass + + +@pytest.mark.asyncio +async def test_lint_fixits(client: LanguageClient): + """ + Test that fixits work properly + """ + file = """ + for i in {1..10} { i; } + """ + async with source_file(client, file, None) as doc: + await save_file(client, doc) + assert len(client.diagnostics[doc.uri]) == 1 + + diagnostics = client.diagnostics[doc.uri] + actions = await client.text_document_code_action_async(CodeActionParams(doc, rng((0, 0), endpos(file)), CodeActionContext(diagnostics))) + assert actions is not None + # there should 2 actions, one to fixit and one to ignore. make sure that both exists + # TODO: check that the fixits are valid and what we expect for a given input string + # TODO: this should also be made into a helper function + assert len(actions) == 2 + assert "Apply Fix for" in actions[0].title and "Ignore" not in actions[0].title + assert "Ignore" in actions[1].title + # TODO: check that applying the fixits actually resolves the warning + + From 3d0fb258aec9ba7aa9726e55f00b356b0f646577 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Mon, 10 Jun 2024 20:33:24 -0700 Subject: [PATCH 08/11] format Signed-off-by: Jade Abraham --- tools/chpl-language-server/test/linting.py | 13 +++++++++---- tools/chplcheck/src/lsp.py | 11 +++++++---- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/tools/chpl-language-server/test/linting.py b/tools/chpl-language-server/test/linting.py index 65f808ed7f83..cbf0d0dafe5b 100644 --- a/tools/chpl-language-server/test/linting.py +++ b/tools/chpl-language-server/test/linting.py @@ -57,14 +57,19 @@ async def test_lint_fixits(client: LanguageClient): assert len(client.diagnostics[doc.uri]) == 1 diagnostics = client.diagnostics[doc.uri] - actions = await client.text_document_code_action_async(CodeActionParams(doc, rng((0, 0), endpos(file)), CodeActionContext(diagnostics))) + actions = await client.text_document_code_action_async( + CodeActionParams( + doc, rng((0, 0), endpos(file)), CodeActionContext(diagnostics) + ) + ) assert actions is not None # there should 2 actions, one to fixit and one to ignore. make sure that both exists # TODO: check that the fixits are valid and what we expect for a given input string # TODO: this should also be made into a helper function assert len(actions) == 2 - assert "Apply Fix for" in actions[0].title and "Ignore" not in actions[0].title + assert ( + "Apply Fix for" in actions[0].title + and "Ignore" not in actions[0].title + ) assert "Ignore" in actions[1].title # TODO: check that applying the fixits actually resolves the warning - - diff --git a/tools/chplcheck/src/lsp.py b/tools/chplcheck/src/lsp.py index 1329fe296045..ec93c43fa4de 100644 --- a/tools/chplcheck/src/lsp.py +++ b/tools/chplcheck/src/lsp.py @@ -43,7 +43,10 @@ def _get_location(node: chapel.AstNode): else: return chapel.lsp.location_to_range(node.location()) -def get_lint_diagnostics(context: chapel.Context, driver: LintDriver, asts: List[chapel.AstNode]) -> List[Diagnostic]: + +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. """ @@ -63,6 +66,7 @@ def get_lint_diagnostics(context: chapel.Context, driver: LintDriver, asts: List diagnostics.append(diagnostic) return diagnostics + def get_lint_actions(diagnostics: List[Diagnostic]) -> List[CodeAction]: """ Return LSP code actions for the given diagnostics. @@ -89,9 +93,7 @@ def get_lint_actions(diagnostics: List[Diagnostic]) -> List[CodeAction]: start = e.start end = e.end rng = Range( - start=Position( - max(start[0] - 1, 0), max(start[1] - 1, 0) - ), + 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) @@ -110,6 +112,7 @@ def get_lint_actions(diagnostics: List[Diagnostic]) -> List[CodeAction]: actions.append(action) return actions + def run_lsp(driver: LintDriver): """ Start a language server on the standard input/output, and use it to From d3afcf251f194654c0ad7cbec374709a7b2902d4 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 11 Jun 2024 08:10:57 -0700 Subject: [PATCH 09/11] apply reviewer suggestions to docs Signed-off-by: Jade Abraham --- .../chpl-language-server/chpl-language-server.rst | 10 ++++++---- doc/rst/tools/chplcheck/chplcheck.rst | 7 ++++++- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/doc/rst/tools/chpl-language-server/chpl-language-server.rst b/doc/rst/tools/chpl-language-server/chpl-language-server.rst index e36f27d9a00a..3917e84549e3 100644 --- a/doc/rst/tools/chpl-language-server/chpl-language-server.rst +++ b/doc/rst/tools/chpl-language-server/chpl-language-server.rst @@ -62,6 +62,8 @@ VSCode Install the ``chapel`` extension from the `Visual Studio Code marketplace `_. +.. _chpl-language-server-emacs: + Emacs ^^^^^ @@ -191,10 +193,10 @@ The following features are extra visual aids: Using ``chplcheck`` from ``CLS`` ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -Although ``chplcheck`` is a separate tool from ``CLS``, it can be used from -``CLS`` to provide additional diagnostics. This is done by enabling the -``--chplcheck`` flag. This will incorporate the diagnostics and fixits from -``chplcheck``. +Although :ref:`chplcheck ` is a separate tool from ``CLS``, +it can be used from ``CLS`` to provide additional diagnostics. This is done by +enabling the ``--chplcheck`` flag. This will incorporate the diagnostics and +fixits from ``chplcheck``. You can also still pass many of the same ``chplcheck`` flags to ``CLS``, just prefixed with ``--chplcheck-``. For example, the following command runs the diff --git a/doc/rst/tools/chplcheck/chplcheck.rst b/doc/rst/tools/chplcheck/chplcheck.rst index 4693262ccd66..43346de5f5ed 100644 --- a/doc/rst/tools/chplcheck/chplcheck.rst +++ b/doc/rst/tools/chplcheck/chplcheck.rst @@ -168,12 +168,17 @@ additionally add the following to your ``.emacs`` file: (add-hook 'chpl-mode-hook 'eglot-ensure) +.. note:: + There is currently a limitation with Eglot that only one language server can + be registered per language. To use ``chplcheck`` and ``chapel-language-server`` + at the same time, see the + :ref:`Emacs documentation for chapel-language-server `. Writing New Rules ----------------- -Rules are written using the :ref:`Python bindings for Chapel's compiler frontend`. In +Rules are written using the :ref:`Python bindings for Chapel's compiler frontend `. In essence, a rule is a Python function that is used to detect issues with the AST. When registered with ``chplcheck``, the name of the function becomes the name of the rule (which can be used to enable and disable the rule, as per the From e784e8c6dd6138864fc7c6b5a4c8beb553f95b24 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 11 Jun 2024 09:01:25 -0700 Subject: [PATCH 10/11] use proxy object for chplcheck Signed-off-by: Jade Abraham --- .../src/chpl-language-server.py | 114 +++++++++--------- tools/chplcheck/src/config.py | 24 ++-- 2 files changed, 67 insertions(+), 71 deletions(-) diff --git a/tools/chpl-language-server/src/chpl-language-server.py b/tools/chpl-language-server/src/chpl-language-server.py index 0e3902746128..c7abba06451c 100755 --- a/tools/chpl-language-server/src/chpl-language-server.py +++ b/tools/chpl-language-server/src/chpl-language-server.py @@ -32,6 +32,7 @@ TypeVar, Union, ) +from types import ModuleType from collections import defaultdict from dataclasses import dataclass, field from bisect_compat import bisect_left, bisect_right @@ -144,41 +145,53 @@ import configargparse -# attempt to import chplcheck -try: - chpl_home = os.environ.get("CHPL_HOME") - if chpl_home is None: - raise ValueError("CHPL_HOME not set") - chplcheck_path = os.path.join(chpl_home, "tools", "chplcheck", "src") - # Add chplcheck to the path, but load via importlib - sys.path.append(chplcheck_path) - - def load_module(module_name: str): - file_path = os.path.join(chplcheck_path, module_name + ".py") - spec = importlib.util.spec_from_file_location(module_name, file_path) - if spec is None: - raise ValueError(f"Could not load module from {file_path}") - module = importlib.util.module_from_spec(spec) - sys.modules[module_name] = module - if spec.loader is None: - raise ValueError(f"Could not load module from {file_path}") - spec.loader.exec_module(module) - return module - - chplcheck = load_module("chplcheck") - chplcheck_lsp = load_module("lsp") - chplcheck_config = load_module("config") - chplcheck_driver = load_module("driver") - chplcheck_rules = load_module("rules") - -except ValueError as e: - if os.environ.get("CHPL_DEVELOPER", None): - print("Error loading chplcheck: ", str(e), file=sys.stderr) - chplcheck = None - chplcheck_lsp = None - chplcheck_config = None - chplcheck_driver = None - chplcheck_rules = None +class ChplcheckProxy: + + def __init__(self, main: ModuleType, config: ModuleType, lsp: ModuleType, driver: ModuleType, rules: ModuleType): + self.main = main + self.config = config + self.lsp = lsp + self.driver = driver + self.rules = rules + + + @classmethod + def get(cls) -> Optional["ChplcheckProxy"]: + + def error(msg: str): + if os.environ.get("CHPL_DEVELOPER", None): + print("Error loading chplcheck: ", str(e), file=sys.stderr) + + chpl_home = os.environ.get("CHPL_HOME") + if chpl_home is None: + error("CHPL_HOME not set") + return None + + def load_module(module_name: str) -> Optional[ModuleType]: + file_path = os.path.join(chplcheck_path, module_name + ".py") + spec = importlib.util.spec_from_file_location(module_name, file_path) + if spec is None: + error(f"Could not load module from {file_path}") + return None + module = importlib.util.module_from_spec(spec) + sys.modules[module_name] = module + if spec.loader is None: + error(f"Could not load module from {file_path}") + return None + spec.loader.exec_module(module) + return module + + mods = [] + for mod in ["main", "config", "lsp", "driver", "rules"]: + m = load_module(mod) + if m is None: + return None + mods.append(m) + proxy = ChplcheckProxy(*mods) + + return proxy + +chplcheck = ChplcheckProxy.get() REAL_NUMBERIC = (chapel.RealLiteral, chapel.IntLiteral, chapel.UintLiteral) NUMERIC = REAL_NUMBERIC + (chapel.ImagLiteral,) @@ -991,8 +1004,8 @@ def add_bool_flag(name: str, dest: str, default: bool): self.parser.add_argument("--end-marker-threshold", type=int, default=10) add_bool_flag("chplcheck", "do_linting", False) - if chplcheck_config: - chplcheck_config.Config.add_arguments(self.parser, "chplcheck-") + if chplcheck: + chplcheck.config.Config.add_arguments(self.parser, "chplcheck-") def _parse_end_markers(self): self.args["end_markers"] = [ @@ -1070,23 +1083,16 @@ def _setup_linter(self, clsConfig: CLSConfig): """ Setup the linter, if it is enabled """ - if not ( - self.do_linting - and chplcheck - and chplcheck_lsp - and chplcheck_config - and chplcheck_driver - and chplcheck_rules - ): + if not (self.do_linting and chplcheck): return - config = chplcheck_config.Config.from_args(clsConfig.args) - self.lint_driver = chplcheck_driver.LintDriver(config) + config = chplcheck.config.Config.from_args(clsConfig.args) + self.lint_driver = chplcheck.driver.LintDriver(config) - chplcheck_rules.register_rules(self.lint_driver) + chplcheck.rules.register_rules(self.lint_driver) for p in config.add_rules: - chplcheck.load_module(self.lint_driver, os.path.abspath(p)) + chplcheck.main.load_module(self.lint_driver, os.path.abspath(p)) self.lint_driver.disable_rules(*config.disabled_rules) self.lint_driver.enable_rules(*config.enabled_rules) @@ -1202,9 +1208,8 @@ def build_diagnostics(self, uri: str) -> List[Diagnostic]: diagnostics.append(diag) # get lint diagnostics if applicable - if self.lint_driver: - assert chplcheck_lsp - lint_diagnostics = chplcheck_lsp.get_lint_diagnostics( + if self.lint_driver and chplcheck: + lint_diagnostics = chplcheck.lsp.get_lint_diagnostics( fi.context.context, self.lint_driver, fi.get_asts() ) diagnostics.extend(lint_diagnostics) @@ -1765,9 +1770,8 @@ async def code_action(ls: ChapelLanguageServer, params: CodeActionParams): ) # get lint fixits if applicable - if ls.lint_driver: - assert chplcheck_lsp - lint_actions = chplcheck_lsp.get_lint_actions( + if ls.lint_driver and chplcheck: + lint_actions = chplcheck.lsp.get_lint_actions( params.context.diagnostics ) actions.extend(lint_actions) diff --git a/tools/chplcheck/src/config.py b/tools/chplcheck/src/config.py index d2a5ba6cf571..2c1b6ce2b3c3 100644 --- a/tools/chplcheck/src/config.py +++ b/tools/chplcheck/src/config.py @@ -74,19 +74,11 @@ def add_arguments(cls, parser: argparse.ArgumentParser, prefix: str = ""): @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"], - ) + args = vars(args) if isinstance(args, argparse.Namespace) else args + 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"], + ) From 29a473b6c5202a214442b85a6304bb755a7a37d0 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 11 Jun 2024 09:08:41 -0700 Subject: [PATCH 11/11] formatting Signed-off-by: Jade Abraham --- .../src/chpl-language-server.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tools/chpl-language-server/src/chpl-language-server.py b/tools/chpl-language-server/src/chpl-language-server.py index c7abba06451c..494548623ea9 100755 --- a/tools/chpl-language-server/src/chpl-language-server.py +++ b/tools/chpl-language-server/src/chpl-language-server.py @@ -147,14 +147,20 @@ class ChplcheckProxy: - def __init__(self, main: ModuleType, config: ModuleType, lsp: ModuleType, driver: ModuleType, rules: ModuleType): + def __init__( + self, + main: ModuleType, + config: ModuleType, + lsp: ModuleType, + driver: ModuleType, + rules: ModuleType, + ): self.main = main self.config = config self.lsp = lsp self.driver = driver self.rules = rules - @classmethod def get(cls) -> Optional["ChplcheckProxy"]: @@ -169,7 +175,9 @@ def error(msg: str): def load_module(module_name: str) -> Optional[ModuleType]: file_path = os.path.join(chplcheck_path, module_name + ".py") - spec = importlib.util.spec_from_file_location(module_name, file_path) + spec = importlib.util.spec_from_file_location( + module_name, file_path + ) if spec is None: error(f"Could not load module from {file_path}") return None @@ -191,6 +199,7 @@ def load_module(module_name: str) -> Optional[ModuleType]: return proxy + chplcheck = ChplcheckProxy.get() REAL_NUMBERIC = (chapel.RealLiteral, chapel.IntLiteral, chapel.UintLiteral)