From c919359153eafdc7a1573c10e37cf1e84edf664e Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Mon, 23 Oct 2023 16:59:54 -0700 Subject: [PATCH 01/26] Extract the 'files into buckets' functionality into 'chapel' Signed-off-by: Danila Fedorin --- tools/chapel-py/chapel/__init__.py | 29 +++++++++++++++++++++++++++++ tools/chapel-py/chapel/replace.py | 24 ++---------------------- tools/chapel-py/chplcheck.py | 26 +++++++++++++++----------- 3 files changed, 46 insertions(+), 33 deletions(-) diff --git a/tools/chapel-py/chapel/__init__.py b/tools/chapel-py/chapel/__init__.py index 942f4458d965..11f02cf0c586 100644 --- a/tools/chapel-py/chapel/__init__.py +++ b/tools/chapel-py/chapel/__init__.py @@ -19,6 +19,8 @@ # from . import core +from collections import defaultdict +import os def preorder(node): """ @@ -219,3 +221,30 @@ def each_matching(node, pattern): variables = match_pattern(child, pattern) if variables is not None: yield (child, variables) + +def files_with_contexts(files): + """ + Some files might have the same name, which Dyno really doesn't like. + Stratify files into "buckets"; within each bucket, all filenames are + unique. Between each bucket, re-create the Dyno context to avoid giving + it complicting files. + + Yields files from the argument, as well as the context created for them. + """ + + basenames = defaultdict(lambda: 0) + buckets = defaultdict(lambda: []) + for filename in files: + filename = os.path.realpath(os.path.expandvars(filename)) + + basename = os.path.basename(filename) + bucket = basenames[basename] + basenames[basename] += 1 + buckets[bucket].append(filename) + + for bucket in buckets: + ctx = core.Context() + to_yield = buckets[bucket] + + for filename in to_yield: + yield (filename, ctx) diff --git a/tools/chapel-py/chapel/replace.py b/tools/chapel-py/chapel/replace.py index f4a90929a7aa..141caf148723 100644 --- a/tools/chapel-py/chapel/replace.py +++ b/tools/chapel-py/chapel/replace.py @@ -21,7 +21,6 @@ import argparse import chapel import chapel.core -from collections import defaultdict import os import sys @@ -213,27 +212,8 @@ def run(finder, name='replace', description='A tool to search-and-replace Chapel parser.add_argument('--in-place', dest='inplace', action='store_true', default=False) args = parser.parse_args() - # Some files might have the same name, which Dyno really doesn't like. - # Strateify files into "buckets"; within each bucket, all filenames are - # unique. Between each bucket, re-create the Dyno context to avoid giving - # it complicting files. - - basenames = defaultdict(lambda: 0) - buckets = defaultdict(lambda: []) - for filename in args.filenames: - filename = os.path.realpath(os.path.expandvars(filename)) - - basename = os.path.basename(filename) - bucket = basenames[basename] - basenames[basename] += 1 - buckets[bucket].append(filename) - - for bucket in buckets: - ctx = chapel.core.Context() - to_replace = buckets[bucket] - - for filename in to_replace: - _do_replace(finder, ctx, filename, args.suffix, args.inplace) + for (filename, ctx) in chapel.files_with_contexts(args.filenames): + _do_replace(finder, ctx, filename, args.suffix, args.inplace) def fuse(*args): """ diff --git a/tools/chapel-py/chplcheck.py b/tools/chapel-py/chplcheck.py index 5133e45c01bb..83584eda69c1 100755 --- a/tools/chapel-py/chplcheck.py +++ b/tools/chapel-py/chplcheck.py @@ -45,7 +45,7 @@ def ignores_rule(node, rulename): return False def report_violation(node, name): - if name in args.ignored_rules: + if name in SilencedRules: return location = node.location() @@ -162,25 +162,29 @@ def check_misleading_indentation(node): ("ChplPrefixReserved", chapel.core.NamedDecl, check_reserved_prefix), ] +SilencedRules = [ "CamelCaseVariables", "ConsecutiveDecls" ] + def main(): - global args + global ctx parser = argparse.ArgumentParser( prog='chplcheck', description='A linter for the Chapel language') - parser.add_argument('filename') + parser.add_argument('filenames', nargs='*') parser.add_argument('--ignore-rule', action='append', dest='ignored_rules', default=[]) args = parser.parse_args() - ctx = chapel.core.Context() - ast = ctx.parse(args.filename) + SilencedRules.extend(args.ignored_rules) + + for (filename, ctx) in chapel.files_with_contexts(args.filenames): + ast = ctx.parse(filename) - for rule in Rules: - check_basic_rule(ast, rule) + for rule in Rules: + check_basic_rule(ast, rule) - for group in consecutive_decls(ast): - report_violation(group[1], "ConsecutiveDecls") + for group in consecutive_decls(ast): + report_violation(group[1], "ConsecutiveDecls") - for node in check_misleading_indentation(ast): - report_violation(node, "MisleadingIndentation") + for node in check_misleading_indentation(ast): + report_violation(node, "MisleadingIndentation") if __name__ == "__main__": main() From aa28eb03d37e77159f80b3cc7abbf5cc8e9ef0d7 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Mon, 23 Oct 2023 17:00:14 -0700 Subject: [PATCH 02/26] Add an 'advance to next revision' method to the Context Signed-off-by: Danila Fedorin --- tools/chapel-py/chapel.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tools/chapel-py/chapel.cpp b/tools/chapel-py/chapel.cpp index 3ab934d9f679..4ab66d5699c6 100644 --- a/tools/chapel-py/chapel.cpp +++ b/tools/chapel-py/chapel.cpp @@ -369,9 +369,22 @@ static PyObject* ContextObject_is_bundled_path(ContextObject *self, PyObject* ar return PyBool_FromLong(isInternalPath); } +static PyObject* ContextObject_advance_to_next_revision(ContextObject *self, PyObject* args) { + auto context = &self->context; + bool prepareToGc; + if (!PyArg_ParseTuple(args, "b", &prepareToGc)) { + PyErr_BadArgument(); + return nullptr; + } + + context->advanceToNextRevision(prepareToGc); + Py_RETURN_NONE; +} + static PyMethodDef ContextObject_methods[] = { { "parse", (PyCFunction) ContextObject_parse, METH_VARARGS, "Parse a top-level AST node from the given file" }, { "is_bundled_path", (PyCFunction) ContextObject_is_bundled_path, METH_VARARGS, "Check if the given file path is within the bundled (built-in) Chapel files" }, + { "advance_to_next_revision", (PyCFunction) ContextObject_advance_to_next_revision, METH_VARARGS, "Advance the context to the next revision" }, {NULL, NULL, 0, NULL} /* Sentinel */ }; From 28689803da1cebb6cb8c07f096bc718c9d884704 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Mon, 23 Oct 2023 17:00:53 -0700 Subject: [PATCH 03/26] Add integration with the language server protocol Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck.py | 84 +++++++++++++++++++++++++++++++----- 1 file changed, 73 insertions(+), 11 deletions(-) diff --git a/tools/chapel-py/chplcheck.py b/tools/chapel-py/chplcheck.py index 83584eda69c1..3a6b039ba44a 100755 --- a/tools/chapel-py/chplcheck.py +++ b/tools/chapel-py/chplcheck.py @@ -57,7 +57,7 @@ def check_basic_rule(root, rule): for (node, _) in chapel.each_matching(root, nodetype): if ignores_rule(node, name): continue - if not func(node): report_violation(node, name) + if not func(node): yield (node, name) # === "user-defined" linter rule functions, used to implement warnings. === @@ -164,27 +164,89 @@ def check_misleading_indentation(node): SilencedRules = [ "CamelCaseVariables", "ConsecutiveDecls" ] +def run_checks(asts): + for ast in asts: + for rule in Rules: + yield from check_basic_rule(ast, rule) + + for group in consecutive_decls(ast): + yield (group[1], "ConsecutiveDecls") + + for node in check_misleading_indentation(ast): + yield (node, "MisleadingIndentation") + +def run_lsp(): + from pygls.server import LanguageServer + from lsprotocol.types import TEXT_DOCUMENT_DID_OPEN, DidOpenTextDocumentParams + from lsprotocol.types import TEXT_DOCUMENT_DID_SAVE, DidSaveTextDocumentParams + from lsprotocol.types import Diagnostic, Range, Position, DiagnosticSeverity + + server = LanguageServer('chplcheck', 'v0.1') + contexts = {} + + def get_updated_context(uri): + if uri in contexts: + context = contexts[uri] + context.advance_to_next_revision(False) + else: + context = chapel.core.Context() + contexts[uri] = context + return context + + def parse_file(uri): + context = get_updated_context(uri) + return context.parse(uri[len("file://"):]) + + def build_diagnostics(uri): + asts = parse_file(uri) + diagnostics = [] + for (node, rule) in run_checks(asts): + location = node.location() + start = location.start() + end = location.end() + + diagnostic = Diagnostic( + range=Range( + start=Position(start[0]-1, start[1]-1), + end=Position(end[0]-1, end[1]-1) + ), + message="Lint: rule [{}] violated".format(rule), + severity=DiagnosticSeverity.Warning + ) + diagnostics.append(diagnostic) + return diagnostics + + @server.feature(TEXT_DOCUMENT_DID_OPEN) + async def did_open(ls, params: DidOpenTextDocumentParams): + text_doc = ls.workspace.get_text_document(params.text_document.uri) + ls.publish_diagnostics(text_doc.uri, build_diagnostics(text_doc.uri)) + + @server.feature(TEXT_DOCUMENT_DID_SAVE) + async def did_save(ls, params: DidSaveTextDocumentParams): + text_doc = ls.workspace.get_text_document(params.text_document.uri) + ls.publish_diagnostics(text_doc.uri, build_diagnostics(text_doc.uri)) + + server.start_io() + def main(): global ctx parser = argparse.ArgumentParser( prog='chplcheck', description='A linter for the Chapel language') parser.add_argument('filenames', nargs='*') parser.add_argument('--ignore-rule', action='append', dest='ignored_rules', default=[]) + parser.add_argument('--lsp', action='store_true', default=False) args = parser.parse_args() SilencedRules.extend(args.ignored_rules) - for (filename, ctx) in chapel.files_with_contexts(args.filenames): - ast = ctx.parse(filename) - - for rule in Rules: - check_basic_rule(ast, rule) - - for group in consecutive_decls(ast): - report_violation(group[1], "ConsecutiveDecls") + if args.lsp: + run_lsp() + return - for node in check_misleading_indentation(ast): - report_violation(node, "MisleadingIndentation") + for (filename, ctx) in chapel.files_with_contexts(args.filenames): + asts = ctx.parse(filename) + for (node, rule) in run_checks(asts): + report_violation(node, rule) if __name__ == "__main__": main() From de13d6cea7d765c352c52e3df63262b863a17af5 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 12:38:04 -0700 Subject: [PATCH 04/26] Use decorators to register linting rules Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck.py | 168 ++++++++++++++++++++++------------- 1 file changed, 105 insertions(+), 63 deletions(-) diff --git a/tools/chapel-py/chplcheck.py b/tools/chapel-py/chplcheck.py index 3a6b039ba44a..9c4b717e5911 100755 --- a/tools/chapel-py/chplcheck.py +++ b/tools/chapel-py/chplcheck.py @@ -44,25 +44,113 @@ def ignores_rule(node, rulename): return False -def report_violation(node, name): - if name in SilencedRules: - return +def should_check_rule(node, rulename): + if rulename in SilencedRules: + return False + + if node is not None and ignores_rule(node, rulename): + return False + return True + +def print_violation(node, name): location = node.location() first_line, _ = location.start() print("{}:{}: node violates rule {}".format(location.path(), first_line, name)) def check_basic_rule(root, rule): + # If we should ignore the rule no matter the node, no reason to run + # a traversal and match the pattern. + if not should_check_rule(None, rule): + return + (name, nodetype, func) = rule for (node, _) in chapel.each_matching(root, nodetype): - if ignores_rule(node, name): continue + if not should_check_rule(node, name): + continue + + if not func(node): + yield (node, name) + +def check_advanced_rule(root, rule): + # If we should ignore the rule no matter the node, no reason to run + # a traversal and match the pattern. + if not should_check_rule(None, rule): + return + + (name, func) = rule + for node in func(root): + yield (node, name) - if not func(node): yield (node, name) +BasicRules = [] +def basic_rule(nodetype): + def wrapper(func): + BasicRules.append((func.__name__, nodetype, func)) + return func + return wrapper +AdvancedRules = [] +def advanced_rule(func): + AdvancedRules.append((func.__name__, func)) + return func # === "user-defined" linter rule functions, used to implement warnings. === -def consecutive_decls(node): +def name_for_linting(node): + name = node.name() + if name.startswith("chpl_"): + name = name.removeprefix("chpl_") + return name + +def check_camel_case(node): + return re.fullmatch(r'_?([a-z]+([A-Z][a-z]+|\d+)*|[A-Z]+)\$?', name_for_linting(node)) + +def check_pascal_case(node): + return re.fullmatch(r'_?(([A-Z][a-z]+|\d+)+|[A-Z]+)\$?', name_for_linting(node)) + +@basic_rule(chapel.core.VarLikeDecl) +def CamelCaseVariables(node): + if node.name() == "_": return True + return check_camel_case(node) + +@basic_rule(chapel.core.Record) +def CamelCaseRecords(node): + return check_camel_case(node) + +@basic_rule(chapel.core.Class) +def PascalCaseClasses(node): + return check_pascal_case(node) + +@basic_rule(chapel.core.Module) +def PascalCaseModules(node): + return check_pascal_case(node) + +@basic_rule(chapel.core.Loop) +def DoKeywordAndBlock(node): + return node.block_style() != "unnecessary" + +@basic_rule(chapel.core.Coforall) +def NestedCoforalls(node): + parent = node.parent() + while parent is not None: + if isinstance(parent, chapel.core.Coforall): + return False + parent = parent.parent() + return True + +@basic_rule([chapel.core.Conditional, chapel.core.BoolLiteral, chapel.rest]) +def BoolLitInCondStmt(node): + return False + +@basic_rule(chapel.core.NamedDecl) +def ChplPrefixReserved(node): + if node.name().startswith("chpl_"): + path = node.location().path() + return ctx.is_bundled_path(path) + return True + +@advanced_rule +def ConsecutiveDecls(root): def is_relevant_decl(node): if isinstance(node, chapel.core.MultiDecl): for child in node: @@ -92,7 +180,7 @@ def recurse(node, skip_direct = False): # If we ran out of compatible decls, see if we can return them. if not compatible_kinds: if len(consecutive) > 1: - yield consecutive + yield consecutive[1] consecutive = [] # If this could be a compatible decl, start a new list. @@ -100,47 +188,15 @@ def recurse(node, skip_direct = False): consecutive.append(child) if len(consecutive) > 1: - yield consecutive - - yield from recurse(node) - -def check_nested_coforall(node): - parent = node.parent() - while parent is not None: - if isinstance(parent, chapel.core.Coforall): - return False - parent = parent.parent() - return True - -def name_for_linting(node): - name = node.name() - if name.startswith("chpl_"): - name = name.removeprefix("chpl_") - return name - -def check_camel_case(node): - return re.fullmatch(r'_?([a-z]+([A-Z][a-z]+|\d+)*|[A-Z]+)\$?', name_for_linting(node)) + yield consecutive[1] -def check_camel_case_var(node): - if node.name() == "_": return True - return check_camel_case(node) - -def check_pascal_case(node): - return re.fullmatch(r'_?(([A-Z][a-z]+|\d+)+|[A-Z]+)\$?', name_for_linting(node)) + yield from recurse(root) -def check_reserved_prefix(node): - if node.name().startswith("chpl_"): - path = node.location().path() - return ctx.is_bundled_path(path) - return True - -def check_redundant_block(node): - return node.block_style() != "unnecessary" - -def check_misleading_indentation(node): +@advanced_rule +def MisleadingIndentation(root): prev = None - for child in node: - yield from check_misleading_indentation(child) + for child in root: + yield from MisleadingIndentation(child) if prev is not None: if child.location().start()[1] == prev.location().start()[1]: @@ -151,29 +207,15 @@ def check_misleading_indentation(node): if len(grandchildren) > 0: prev = list(grandchildren[-1])[0] -Rules = [ - ("CamelCaseVariables", chapel.core.VarLikeDecl, check_camel_case_var), - ("CamelCaseRecords", chapel.core.Record, check_camel_case), - ("PascalCaseClasses", chapel.core.Class, check_pascal_case), - ("PascalCaseModules", chapel.core.Module, check_pascal_case), - ("DoKeywordAndBlock", chapel.core.Loop, check_redundant_block), - ("NestedCoforalls", chapel.core.Coforall, check_nested_coforall), - ("BoolLitInCondStmt", [chapel.core.Conditional, chapel.core.BoolLiteral, chapel.rest], lambda node: False), - ("ChplPrefixReserved", chapel.core.NamedDecl, check_reserved_prefix), -] - SilencedRules = [ "CamelCaseVariables", "ConsecutiveDecls" ] def run_checks(asts): for ast in asts: - for rule in Rules: + for rule in BasicRules: yield from check_basic_rule(ast, rule) - for group in consecutive_decls(ast): - yield (group[1], "ConsecutiveDecls") - - for node in check_misleading_indentation(ast): - yield (node, "MisleadingIndentation") + for rule in AdvancedRules: + yield from check_advanced_rule(ast, rule) def run_lsp(): from pygls.server import LanguageServer @@ -246,7 +288,7 @@ def main(): for (filename, ctx) in chapel.files_with_contexts(args.filenames): asts = ctx.parse(filename) for (node, rule) in run_checks(asts): - report_violation(node, rule) + print_violation(node, rule) if __name__ == "__main__": main() From 6938c2b3e8f4a1dbab0588c06ed3cfa9ce1c320e Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 12:41:20 -0700 Subject: [PATCH 05/26] Rename the chplcheck bash script to runChplcheck Signed-off-by: Danila Fedorin --- tools/chapel-py/{chplcheck => runChplcheck} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tools/chapel-py/{chplcheck => runChplcheck} (100%) diff --git a/tools/chapel-py/chplcheck b/tools/chapel-py/runChplcheck similarity index 100% rename from tools/chapel-py/chplcheck rename to tools/chapel-py/runChplcheck From a0e2b3b44d3a25762b0dda680a14aa2ea52e5644 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 12:45:49 -0700 Subject: [PATCH 06/26] Start splitting the linter into smaller files Signed-off-by: Danila Fedorin --- .../{chplcheck.py => chplcheck/__init__.py} | 70 +------------------ tools/chapel-py/chplcheck/driver.py | 70 +++++++++++++++++++ 2 files changed, 72 insertions(+), 68 deletions(-) rename tools/chapel-py/{chplcheck.py => chplcheck/__init__.py} (79%) create mode 100644 tools/chapel-py/chplcheck/driver.py diff --git a/tools/chapel-py/chplcheck.py b/tools/chapel-py/chplcheck/__init__.py similarity index 79% rename from tools/chapel-py/chplcheck.py rename to tools/chapel-py/chplcheck/__init__.py index 9c4b717e5911..1ac5c3219b5b 100755 --- a/tools/chapel-py/chplcheck.py +++ b/tools/chapel-py/chplcheck/__init__.py @@ -26,73 +26,7 @@ import re import sys import argparse - -# === Driver Functions: the code of the linter === - -IgnoreAttr = ("chplcheck.ignore", ["rule", "comment"]) -def ignores_rule(node, rulename): - ag = node.attribute_group() - - if ag is None: return False - for attr in ag: - attr_call = chapel.parse_attribute(attr, IgnoreAttr) - if attr_call is None: continue - - ignored_rule = attr_call["rule"] - if ignored_rule is not None and ignored_rule.value() == rulename: - return True - - return False - -def should_check_rule(node, rulename): - if rulename in SilencedRules: - return False - - if node is not None and ignores_rule(node, rulename): - return False - - return True - -def print_violation(node, name): - location = node.location() - first_line, _ = location.start() - print("{}:{}: node violates rule {}".format(location.path(), first_line, name)) - -def check_basic_rule(root, rule): - # If we should ignore the rule no matter the node, no reason to run - # a traversal and match the pattern. - if not should_check_rule(None, rule): - return - - (name, nodetype, func) = rule - for (node, _) in chapel.each_matching(root, nodetype): - if not should_check_rule(node, name): - continue - - if not func(node): - yield (node, name) - -def check_advanced_rule(root, rule): - # If we should ignore the rule no matter the node, no reason to run - # a traversal and match the pattern. - if not should_check_rule(None, rule): - return - - (name, func) = rule - for node in func(root): - yield (node, name) - -BasicRules = [] -def basic_rule(nodetype): - def wrapper(func): - BasicRules.append((func.__name__, nodetype, func)) - return func - return wrapper - -AdvancedRules = [] -def advanced_rule(func): - AdvancedRules.append((func.__name__, func)) - return func +from driver import * # === "user-defined" linter rule functions, used to implement warnings. === @@ -207,7 +141,7 @@ def MisleadingIndentation(root): if len(grandchildren) > 0: prev = list(grandchildren[-1])[0] -SilencedRules = [ "CamelCaseVariables", "ConsecutiveDecls" ] +SilencedRules.extend([ "CamelCaseVariables", "ConsecutiveDecls" ]) def run_checks(asts): for ast in asts: diff --git a/tools/chapel-py/chplcheck/driver.py b/tools/chapel-py/chplcheck/driver.py new file mode 100644 index 000000000000..432356e7f36c --- /dev/null +++ b/tools/chapel-py/chplcheck/driver.py @@ -0,0 +1,70 @@ +import chapel +import chapel.core + +SilencedRules = [] +BasicRules = [] +AdvancedRules = [] + +IgnoreAttr = ("chplcheck.ignore", ["rule", "comment"]) +def ignores_rule(node, rulename): + ag = node.attribute_group() + + if ag is None: return False + for attr in ag: + attr_call = chapel.parse_attribute(attr, IgnoreAttr) + if attr_call is None: continue + + ignored_rule = attr_call["rule"] + if ignored_rule is not None and ignored_rule.value() == rulename: + return True + + return False + +def should_check_rule(node, rulename): + if rulename in SilencedRules: + return False + + if node is not None and ignores_rule(node, rulename): + return False + + return True + +def print_violation(node, name): + location = node.location() + first_line, _ = location.start() + print("{}:{}: node violates rule {}".format(location.path(), first_line, name)) + +def check_basic_rule(root, rule): + # If we should ignore the rule no matter the node, no reason to run + # a traversal and match the pattern. + if not should_check_rule(None, rule): + return + + (name, nodetype, func) = rule + for (node, _) in chapel.each_matching(root, nodetype): + if not should_check_rule(node, name): + continue + + if not func(node): + yield (node, name) + +def check_advanced_rule(root, rule): + # If we should ignore the rule no matter the node, no reason to run + # a traversal and match the pattern. + if not should_check_rule(None, rule): + return + + (name, func) = rule + for node in func(root): + yield (node, name) + +def basic_rule(nodetype): + def wrapper(func): + BasicRules.append((func.__name__, nodetype, func)) + return func + return wrapper + +def advanced_rule(func): + AdvancedRules.append((func.__name__, func)) + return func + From 368bce7239f384685ba4b029c236418e60f1274d Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 12:51:05 -0700 Subject: [PATCH 07/26] Extract the rules into a separate file. Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/__init__.py | 115 +------------------------- tools/chapel-py/chplcheck/rules.py | 115 ++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 114 deletions(-) create mode 100644 tools/chapel-py/chplcheck/rules.py diff --git a/tools/chapel-py/chplcheck/__init__.py b/tools/chapel-py/chplcheck/__init__.py index 1ac5c3219b5b..1945805205c9 100755 --- a/tools/chapel-py/chplcheck/__init__.py +++ b/tools/chapel-py/chplcheck/__init__.py @@ -23,123 +23,10 @@ import chapel import chapel.core import chapel.replace -import re import sys import argparse from driver import * - -# === "user-defined" linter rule functions, used to implement warnings. === - -def name_for_linting(node): - name = node.name() - if name.startswith("chpl_"): - name = name.removeprefix("chpl_") - return name - -def check_camel_case(node): - return re.fullmatch(r'_?([a-z]+([A-Z][a-z]+|\d+)*|[A-Z]+)\$?', name_for_linting(node)) - -def check_pascal_case(node): - return re.fullmatch(r'_?(([A-Z][a-z]+|\d+)+|[A-Z]+)\$?', name_for_linting(node)) - -@basic_rule(chapel.core.VarLikeDecl) -def CamelCaseVariables(node): - if node.name() == "_": return True - return check_camel_case(node) - -@basic_rule(chapel.core.Record) -def CamelCaseRecords(node): - return check_camel_case(node) - -@basic_rule(chapel.core.Class) -def PascalCaseClasses(node): - return check_pascal_case(node) - -@basic_rule(chapel.core.Module) -def PascalCaseModules(node): - return check_pascal_case(node) - -@basic_rule(chapel.core.Loop) -def DoKeywordAndBlock(node): - return node.block_style() != "unnecessary" - -@basic_rule(chapel.core.Coforall) -def NestedCoforalls(node): - parent = node.parent() - while parent is not None: - if isinstance(parent, chapel.core.Coforall): - return False - parent = parent.parent() - return True - -@basic_rule([chapel.core.Conditional, chapel.core.BoolLiteral, chapel.rest]) -def BoolLitInCondStmt(node): - return False - -@basic_rule(chapel.core.NamedDecl) -def ChplPrefixReserved(node): - if node.name().startswith("chpl_"): - path = node.location().path() - return ctx.is_bundled_path(path) - return True - -@advanced_rule -def ConsecutiveDecls(root): - def is_relevant_decl(node): - if isinstance(node, chapel.core.MultiDecl): - for child in node: - if isinstance(child, chapel.core.Variable): return child.kind() - elif isinstance(node, chapel.core.Variable): - return node.kind() - - return None - - def recurse(node, skip_direct = False): - consecutive = [] - last_kind = None - last_has_attribute = False - - for child in node: - yield from recurse(child, skip_direct = isinstance(child, chapel.core.MultiDecl)) - - if skip_direct: continue - - new_kind = is_relevant_decl(child) - has_attribute = child.attribute_group() is not None - any_has_attribute = last_has_attribute or has_attribute - compatible_kinds = not any_has_attribute and (last_kind is None or last_kind == new_kind) - last_kind = new_kind - last_has_attribute = has_attribute - - # If we ran out of compatible decls, see if we can return them. - if not compatible_kinds: - if len(consecutive) > 1: - yield consecutive[1] - consecutive = [] - - # If this could be a compatible decl, start a new list. - if new_kind is not None: - consecutive.append(child) - - if len(consecutive) > 1: - yield consecutive[1] - - yield from recurse(root) - -@advanced_rule -def MisleadingIndentation(root): - prev = None - for child in root: - yield from MisleadingIndentation(child) - - if prev is not None: - if child.location().start()[1] == prev.location().start()[1]: - yield child - - if isinstance(child, chapel.core.Loop) and child.block_style() == "implicit": - grandchildren = list(child) - if len(grandchildren) > 0: - prev = list(grandchildren[-1])[0] +from rules import * SilencedRules.extend([ "CamelCaseVariables", "ConsecutiveDecls" ]) diff --git a/tools/chapel-py/chplcheck/rules.py b/tools/chapel-py/chplcheck/rules.py new file mode 100644 index 000000000000..fc7bc70943c4 --- /dev/null +++ b/tools/chapel-py/chplcheck/rules.py @@ -0,0 +1,115 @@ +import chapel +import chapel.core +import re +from driver import basic_rule, advanced_rule + +def name_for_linting(node): + name = node.name() + if name.startswith("chpl_"): + name = name.removeprefix("chpl_") + return name + +def check_camel_case(node): + return re.fullmatch(r'_?([a-z]+([A-Z][a-z]+|\d+)*|[A-Z]+)\$?', name_for_linting(node)) + +def check_pascal_case(node): + return re.fullmatch(r'_?(([A-Z][a-z]+|\d+)+|[A-Z]+)\$?', name_for_linting(node)) + +@basic_rule(chapel.core.VarLikeDecl) +def CamelCaseVariables(node): + if node.name() == "_": return True + return check_camel_case(node) + +@basic_rule(chapel.core.Record) +def CamelCaseRecords(node): + return check_camel_case(node) + +@basic_rule(chapel.core.Class) +def PascalCaseClasses(node): + return check_pascal_case(node) + +@basic_rule(chapel.core.Module) +def PascalCaseModules(node): + return check_pascal_case(node) + +@basic_rule(chapel.core.Loop) +def DoKeywordAndBlock(node): + return node.block_style() != "unnecessary" + +@basic_rule(chapel.core.Coforall) +def NestedCoforalls(node): + parent = node.parent() + while parent is not None: + if isinstance(parent, chapel.core.Coforall): + return False + parent = parent.parent() + return True + +@basic_rule([chapel.core.Conditional, chapel.core.BoolLiteral, chapel.rest]) +def BoolLitInCondStmt(node): + return False + +@basic_rule(chapel.core.NamedDecl) +def ChplPrefixReserved(node): + if node.name().startswith("chpl_"): + path = node.location().path() + return ctx.is_bundled_path(path) + return True + +@advanced_rule +def ConsecutiveDecls(root): + def is_relevant_decl(node): + if isinstance(node, chapel.core.MultiDecl): + for child in node: + if isinstance(child, chapel.core.Variable): return child.kind() + elif isinstance(node, chapel.core.Variable): + return node.kind() + + return None + + def recurse(node, skip_direct = False): + consecutive = [] + last_kind = None + last_has_attribute = False + + for child in node: + yield from recurse(child, skip_direct = isinstance(child, chapel.core.MultiDecl)) + + if skip_direct: continue + + new_kind = is_relevant_decl(child) + has_attribute = child.attribute_group() is not None + any_has_attribute = last_has_attribute or has_attribute + compatible_kinds = not any_has_attribute and (last_kind is None or last_kind == new_kind) + last_kind = new_kind + last_has_attribute = has_attribute + + # If we ran out of compatible decls, see if we can return them. + if not compatible_kinds: + if len(consecutive) > 1: + yield consecutive[1] + consecutive = [] + + # If this could be a compatible decl, start a new list. + if new_kind is not None: + consecutive.append(child) + + if len(consecutive) > 1: + yield consecutive[1] + + yield from recurse(root) + +@advanced_rule +def MisleadingIndentation(root): + prev = None + for child in root: + yield from MisleadingIndentation(child) + + if prev is not None: + if child.location().start()[1] == prev.location().start()[1]: + yield child + + if isinstance(child, chapel.core.Loop) and child.block_style() == "implicit": + grandchildren = list(child) + if len(grandchildren) > 0: + prev = list(grandchildren[-1])[0] From d43cbfbfee76bec20f1e0c1ccbb54038af94398e Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 12:58:14 -0700 Subject: [PATCH 08/26] Update the runner script to the right path Signed-off-by: Danila Fedorin --- tools/chapel-py/runChplcheck | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/chapel-py/runChplcheck b/tools/chapel-py/runChplcheck index 582eaebba07f..9ae590e7ed8c 100755 --- a/tools/chapel-py/runChplcheck +++ b/tools/chapel-py/runChplcheck @@ -26,5 +26,5 @@ if [ -z "$CHPL_HOME" ]; then fi exec $CHPL_HOME/util/config/run-in-venv-with-python-bindings.bash \ - $CHPL_HOME/tools/chapel-py/chplcheck.py "$@" + $CHPL_HOME/tools/chapel-py/chplcheck/__init__.py "$@" From 22b7b7b040424bd0bb9fc2c4aaea6d4264ceea48 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 12:58:27 -0700 Subject: [PATCH 09/26] Fix bug causing rules to be run despite being silenced Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/driver.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tools/chapel-py/chplcheck/driver.py b/tools/chapel-py/chplcheck/driver.py index 432356e7f36c..659cd1820204 100644 --- a/tools/chapel-py/chplcheck/driver.py +++ b/tools/chapel-py/chplcheck/driver.py @@ -35,12 +35,13 @@ def print_violation(node, name): print("{}:{}: node violates rule {}".format(location.path(), first_line, name)) def check_basic_rule(root, rule): - # If we should ignore the rule no matter the node, no reason to run - # a traversal and match the pattern. - if not should_check_rule(None, rule): + (name, nodetype, func) = rule + + if not should_check_rule(None, name): return - (name, nodetype, func) = rule + # If we should ignore the rule no matter the node, no reason to run + # a traversal and match the pattern. for (node, _) in chapel.each_matching(root, nodetype): if not should_check_rule(node, name): continue @@ -49,12 +50,13 @@ def check_basic_rule(root, rule): yield (node, name) def check_advanced_rule(root, rule): + (name, func) = rule + # If we should ignore the rule no matter the node, no reason to run # a traversal and match the pattern. - if not should_check_rule(None, rule): + if not should_check_rule(None, name): return - (name, func) = rule for node in func(root): yield (node, name) From 1148ede6a86cb3e321c7497832befad4e153588d Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 13:30:23 -0700 Subject: [PATCH 10/26] Avoid using global variables using a Driver class Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/__init__.py | 32 ++--- tools/chapel-py/chplcheck/driver.py | 84 +++++------ tools/chapel-py/chplcheck/rules.py | 195 +++++++++++++------------- 3 files changed, 156 insertions(+), 155 deletions(-) diff --git a/tools/chapel-py/chplcheck/__init__.py b/tools/chapel-py/chplcheck/__init__.py index 1945805205c9..e6f96733277f 100755 --- a/tools/chapel-py/chplcheck/__init__.py +++ b/tools/chapel-py/chplcheck/__init__.py @@ -25,20 +25,10 @@ import chapel.replace import sys import argparse -from driver import * -from rules import * +from driver import LintDriver +from rules import register_rules -SilencedRules.extend([ "CamelCaseVariables", "ConsecutiveDecls" ]) - -def run_checks(asts): - for ast in asts: - for rule in BasicRules: - yield from check_basic_rule(ast, rule) - - for rule in AdvancedRules: - yield from check_advanced_rule(ast, rule) - -def run_lsp(): +def run_lsp(driver): from pygls.server import LanguageServer from lsprotocol.types import TEXT_DOCUMENT_DID_OPEN, DidOpenTextDocumentParams from lsprotocol.types import TEXT_DOCUMENT_DID_SAVE, DidSaveTextDocumentParams @@ -63,7 +53,7 @@ def parse_file(uri): def build_diagnostics(uri): asts = parse_file(uri) diagnostics = [] - for (node, rule) in run_checks(asts): + for (node, rule) in driver.run_checks(asts): location = node.location() start = location.start() end = location.end() @@ -91,6 +81,11 @@ async def did_save(ls, params: DidSaveTextDocumentParams): server.start_io() +def print_violation(node, name): + location = node.location() + first_line, _ = location.start() + print("{}:{}: node violates rule {}".format(location.path(), first_line, name)) + def main(): global ctx @@ -100,15 +95,18 @@ def main(): parser.add_argument('--lsp', action='store_true', default=False) args = parser.parse_args() - SilencedRules.extend(args.ignored_rules) + driver = LintDriver() + driver.SilencedRules.extend([ "CamelCaseVariables", "ConsecutiveDecls" ]) + driver.SilencedRules.extend(args.ignored_rules) + register_rules(driver) if args.lsp: - run_lsp() + run_lsp(driver) return for (filename, ctx) in chapel.files_with_contexts(args.filenames): asts = ctx.parse(filename) - for (node, rule) in run_checks(asts): + for (node, rule) in driver.run_checks(asts): print_violation(node, rule) if __name__ == "__main__": diff --git a/tools/chapel-py/chplcheck/driver.py b/tools/chapel-py/chplcheck/driver.py index 659cd1820204..111e1499a425 100644 --- a/tools/chapel-py/chplcheck/driver.py +++ b/tools/chapel-py/chplcheck/driver.py @@ -1,10 +1,6 @@ import chapel import chapel.core -SilencedRules = [] -BasicRules = [] -AdvancedRules = [] - IgnoreAttr = ("chplcheck.ignore", ["rule", "comment"]) def ignores_rule(node, rulename): ag = node.attribute_group() @@ -20,53 +16,61 @@ def ignores_rule(node, rulename): return False -def should_check_rule(node, rulename): - if rulename in SilencedRules: - return False +class LintDriver: + def __init__(self): + self.SilencedRules = [] + self.BasicRules = [] + self.AdvancedRules = [] - if node is not None and ignores_rule(node, rulename): - return False + def should_check_rule(self, node, rulename): + if rulename in self.SilencedRules: + return False - return True + if node is not None and ignores_rule(node, rulename): + return False -def print_violation(node, name): - location = node.location() - first_line, _ = location.start() - print("{}:{}: node violates rule {}".format(location.path(), first_line, name)) + return True -def check_basic_rule(root, rule): - (name, nodetype, func) = rule + def check_basic_rule(self, root, rule): + (name, nodetype, func) = rule - if not should_check_rule(None, name): - return + if not self.should_check_rule(None, name): + return - # If we should ignore the rule no matter the node, no reason to run - # a traversal and match the pattern. - for (node, _) in chapel.each_matching(root, nodetype): - if not should_check_rule(node, name): - continue + # If we should ignore the rule no matter the node, no reason to run + # a traversal and match the pattern. + for (node, _) in chapel.each_matching(root, nodetype): + if not self.should_check_rule(node, name): + continue - if not func(node): - yield (node, name) + if not func(node): + yield (node, name) -def check_advanced_rule(root, rule): - (name, func) = rule + def check_advanced_rule(self, root, rule): + (name, func) = rule - # If we should ignore the rule no matter the node, no reason to run - # a traversal and match the pattern. - if not should_check_rule(None, name): - return + # If we should ignore the rule no matter the node, no reason to run + # a traversal and match the pattern. + if not self.should_check_rule(None, name): + return + + for node in func(root): + yield (node, name) - for node in func(root): - yield (node, name) + def basic_rule(self, nodetype): + def wrapper(func): + self.BasicRules.append((func.__name__, nodetype, func)) + return func + return wrapper -def basic_rule(nodetype): - def wrapper(func): - BasicRules.append((func.__name__, nodetype, func)) + def advanced_rule(self, func): + self.AdvancedRules.append((func.__name__, func)) return func - return wrapper -def advanced_rule(func): - AdvancedRules.append((func.__name__, func)) - return func + def run_checks(self, asts): + for ast in asts: + for rule in self.BasicRules: + yield from self.check_basic_rule(ast, rule) + for rule in self.AdvancedRules: + yield from self.check_advanced_rule(ast, rule) diff --git a/tools/chapel-py/chplcheck/rules.py b/tools/chapel-py/chplcheck/rules.py index fc7bc70943c4..cc06d5ed4be2 100644 --- a/tools/chapel-py/chplcheck/rules.py +++ b/tools/chapel-py/chplcheck/rules.py @@ -1,7 +1,6 @@ import chapel import chapel.core import re -from driver import basic_rule, advanced_rule def name_for_linting(node): name = node.name() @@ -15,101 +14,101 @@ def check_camel_case(node): def check_pascal_case(node): return re.fullmatch(r'_?(([A-Z][a-z]+|\d+)+|[A-Z]+)\$?', name_for_linting(node)) -@basic_rule(chapel.core.VarLikeDecl) -def CamelCaseVariables(node): - if node.name() == "_": return True - return check_camel_case(node) - -@basic_rule(chapel.core.Record) -def CamelCaseRecords(node): - return check_camel_case(node) - -@basic_rule(chapel.core.Class) -def PascalCaseClasses(node): - return check_pascal_case(node) - -@basic_rule(chapel.core.Module) -def PascalCaseModules(node): - return check_pascal_case(node) - -@basic_rule(chapel.core.Loop) -def DoKeywordAndBlock(node): - return node.block_style() != "unnecessary" - -@basic_rule(chapel.core.Coforall) -def NestedCoforalls(node): - parent = node.parent() - while parent is not None: - if isinstance(parent, chapel.core.Coforall): - return False - parent = parent.parent() - return True - -@basic_rule([chapel.core.Conditional, chapel.core.BoolLiteral, chapel.rest]) -def BoolLitInCondStmt(node): - return False - -@basic_rule(chapel.core.NamedDecl) -def ChplPrefixReserved(node): - if node.name().startswith("chpl_"): - path = node.location().path() - return ctx.is_bundled_path(path) - return True - -@advanced_rule -def ConsecutiveDecls(root): - def is_relevant_decl(node): - if isinstance(node, chapel.core.MultiDecl): +def register_rules(driver): + @driver.basic_rule(chapel.core.VarLikeDecl) + def CamelCaseVariables(node): + if node.name() == "_": return True + return check_camel_case(node) + + @driver.basic_rule(chapel.core.Record) + def CamelCaseRecords(node): + return check_camel_case(node) + + @driver.basic_rule(chapel.core.Class) + def PascalCaseClasses(node): + return check_pascal_case(node) + + @driver.basic_rule(chapel.core.Module) + def PascalCaseModules(node): + return check_pascal_case(node) + + @driver.basic_rule(chapel.core.Loop) + def DoKeywordAndBlock(node): + return node.block_style() != "unnecessary" + + @driver.basic_rule(chapel.core.Coforall) + def NestedCoforalls(node): + parent = node.parent() + while parent is not None: + if isinstance(parent, chapel.core.Coforall): + return False + parent = parent.parent() + return True + + @driver.basic_rule([chapel.core.Conditional, chapel.core.BoolLiteral, chapel.rest]) + def BoolLitInCondStmt(node): + return False + + @driver.basic_rule(chapel.core.NamedDecl) + def ChplPrefixReserved(node): + if node.name().startswith("chpl_"): + path = node.location().path() + return ctx.is_bundled_path(path) + return True + + @driver.advanced_rule + def ConsecutiveDecls(root): + def is_relevant_decl(node): + if isinstance(node, chapel.core.MultiDecl): + for child in node: + if isinstance(child, chapel.core.Variable): return child.kind() + elif isinstance(node, chapel.core.Variable): + return node.kind() + return None + + def recurse(node, skip_direct = False): + consecutive = [] + last_kind = None + last_has_attribute = False + for child in node: - if isinstance(child, chapel.core.Variable): return child.kind() - elif isinstance(node, chapel.core.Variable): - return node.kind() - - return None - - def recurse(node, skip_direct = False): - consecutive = [] - last_kind = None - last_has_attribute = False - - for child in node: - yield from recurse(child, skip_direct = isinstance(child, chapel.core.MultiDecl)) - - if skip_direct: continue - - new_kind = is_relevant_decl(child) - has_attribute = child.attribute_group() is not None - any_has_attribute = last_has_attribute or has_attribute - compatible_kinds = not any_has_attribute and (last_kind is None or last_kind == new_kind) - last_kind = new_kind - last_has_attribute = has_attribute - - # If we ran out of compatible decls, see if we can return them. - if not compatible_kinds: - if len(consecutive) > 1: - yield consecutive[1] - consecutive = [] - - # If this could be a compatible decl, start a new list. - if new_kind is not None: - consecutive.append(child) - - if len(consecutive) > 1: - yield consecutive[1] - - yield from recurse(root) - -@advanced_rule -def MisleadingIndentation(root): - prev = None - for child in root: - yield from MisleadingIndentation(child) - - if prev is not None: - if child.location().start()[1] == prev.location().start()[1]: - yield child - - if isinstance(child, chapel.core.Loop) and child.block_style() == "implicit": - grandchildren = list(child) - if len(grandchildren) > 0: - prev = list(grandchildren[-1])[0] + yield from recurse(child, skip_direct = isinstance(child, chapel.core.MultiDecl)) + + if skip_direct: continue + + new_kind = is_relevant_decl(child) + has_attribute = child.attribute_group() is not None + any_has_attribute = last_has_attribute or has_attribute + compatible_kinds = not any_has_attribute and (last_kind is None or last_kind == new_kind) + last_kind = new_kind + last_has_attribute = has_attribute + + # If we ran out of compatible decls, see if we can return them. + if not compatible_kinds: + if len(consecutive) > 1: + yield consecutive[1] + consecutive = [] + + # If this could be a compatible decl, start a new list. + if new_kind is not None: + consecutive.append(child) + + if len(consecutive) > 1: + yield consecutive[1] + + yield from recurse(root) + + @driver.advanced_rule + def MisleadingIndentation(root): + prev = None + for child in root: + yield from MisleadingIndentation(child) + + if prev is not None: + if child.location().start()[1] == prev.location().start()[1]: + yield child + + if isinstance(child, chapel.core.Loop) and child.block_style() == "implicit": + grandchildren = list(child) + if len(grandchildren) > 0: + prev = list(grandchildren[-1])[0] From d78e6644f984e7a56312856878aafeb06aa563a1 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 13:35:30 -0700 Subject: [PATCH 11/26] Remove the global contex variable, threading context through Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/__init__.py | 16 +++++++--------- tools/chapel-py/chplcheck/driver.py | 14 +++++++------- tools/chapel-py/chplcheck/rules.py | 22 +++++++++++----------- 3 files changed, 25 insertions(+), 27 deletions(-) diff --git a/tools/chapel-py/chplcheck/__init__.py b/tools/chapel-py/chplcheck/__init__.py index e6f96733277f..d0f4572dc85a 100755 --- a/tools/chapel-py/chplcheck/__init__.py +++ b/tools/chapel-py/chplcheck/__init__.py @@ -46,14 +46,14 @@ def get_updated_context(uri): contexts[uri] = context return context - def parse_file(uri): - context = get_updated_context(uri) + def parse_file(context, uri): return context.parse(uri[len("file://"):]) def build_diagnostics(uri): - asts = parse_file(uri) + context = get_updated_context(uri) + asts = parse_file(context, uri) diagnostics = [] - for (node, rule) in driver.run_checks(asts): + for (node, rule) in driver.run_checks(context, asts): location = node.location() start = location.start() end = location.end() @@ -87,8 +87,6 @@ def print_violation(node, name): print("{}:{}: node violates rule {}".format(location.path(), first_line, name)) def main(): - global ctx - parser = argparse.ArgumentParser( prog='chplcheck', description='A linter for the Chapel language') parser.add_argument('filenames', nargs='*') parser.add_argument('--ignore-rule', action='append', dest='ignored_rules', default=[]) @@ -104,9 +102,9 @@ def main(): run_lsp(driver) return - for (filename, ctx) in chapel.files_with_contexts(args.filenames): - asts = ctx.parse(filename) - for (node, rule) in driver.run_checks(asts): + for (filename, context) in chapel.files_with_contexts(args.filenames): + asts = context.parse(filename) + for (node, rule) in driver.run_checks(context, asts): print_violation(node, rule) if __name__ == "__main__": diff --git a/tools/chapel-py/chplcheck/driver.py b/tools/chapel-py/chplcheck/driver.py index 111e1499a425..7b76a3643060 100644 --- a/tools/chapel-py/chplcheck/driver.py +++ b/tools/chapel-py/chplcheck/driver.py @@ -31,7 +31,7 @@ def should_check_rule(self, node, rulename): return True - def check_basic_rule(self, root, rule): + def check_basic_rule(self, context, root, rule): (name, nodetype, func) = rule if not self.should_check_rule(None, name): @@ -43,10 +43,10 @@ def check_basic_rule(self, root, rule): if not self.should_check_rule(node, name): continue - if not func(node): + if not func(context, node): yield (node, name) - def check_advanced_rule(self, root, rule): + def check_advanced_rule(self, context, root, rule): (name, func) = rule # If we should ignore the rule no matter the node, no reason to run @@ -54,7 +54,7 @@ def check_advanced_rule(self, root, rule): if not self.should_check_rule(None, name): return - for node in func(root): + for node in func(context, root): yield (node, name) def basic_rule(self, nodetype): @@ -67,10 +67,10 @@ def advanced_rule(self, func): self.AdvancedRules.append((func.__name__, func)) return func - def run_checks(self, asts): + def run_checks(self, context, asts): for ast in asts: for rule in self.BasicRules: - yield from self.check_basic_rule(ast, rule) + yield from self.check_basic_rule(context, ast, rule) for rule in self.AdvancedRules: - yield from self.check_advanced_rule(ast, rule) + yield from self.check_advanced_rule(context, ast, rule) diff --git a/tools/chapel-py/chplcheck/rules.py b/tools/chapel-py/chplcheck/rules.py index cc06d5ed4be2..1bc979bc5d3a 100644 --- a/tools/chapel-py/chplcheck/rules.py +++ b/tools/chapel-py/chplcheck/rules.py @@ -16,28 +16,28 @@ def check_pascal_case(node): def register_rules(driver): @driver.basic_rule(chapel.core.VarLikeDecl) - def CamelCaseVariables(node): + def CamelCaseVariables(context, node): if node.name() == "_": return True return check_camel_case(node) @driver.basic_rule(chapel.core.Record) - def CamelCaseRecords(node): + def CamelCaseRecords(context, node): return check_camel_case(node) @driver.basic_rule(chapel.core.Class) - def PascalCaseClasses(node): + def PascalCaseClasses(context, node): return check_pascal_case(node) @driver.basic_rule(chapel.core.Module) - def PascalCaseModules(node): + def PascalCaseModules(context, node): return check_pascal_case(node) @driver.basic_rule(chapel.core.Loop) - def DoKeywordAndBlock(node): + def DoKeywordAndBlock(context, node): return node.block_style() != "unnecessary" @driver.basic_rule(chapel.core.Coforall) - def NestedCoforalls(node): + def NestedCoforalls(context, node): parent = node.parent() while parent is not None: if isinstance(parent, chapel.core.Coforall): @@ -46,18 +46,18 @@ def NestedCoforalls(node): return True @driver.basic_rule([chapel.core.Conditional, chapel.core.BoolLiteral, chapel.rest]) - def BoolLitInCondStmt(node): + def BoolLitInCondStmt(context, node): return False @driver.basic_rule(chapel.core.NamedDecl) - def ChplPrefixReserved(node): + def ChplPrefixReserved(context, node): if node.name().startswith("chpl_"): path = node.location().path() - return ctx.is_bundled_path(path) + return context.is_bundled_path(path) return True @driver.advanced_rule - def ConsecutiveDecls(root): + def ConsecutiveDecls(context, root): def is_relevant_decl(node): if isinstance(node, chapel.core.MultiDecl): for child in node: @@ -99,7 +99,7 @@ def recurse(node, skip_direct = False): yield from recurse(root) @driver.advanced_rule - def MisleadingIndentation(root): + def MisleadingIndentation(context, root): prev = None for child in root: yield from MisleadingIndentation(child) From 5d7af8b49f7c9d07631774a03d14661596b165e0 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 13:36:19 -0700 Subject: [PATCH 12/26] Use global import when implementing rules Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/rules.py | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/tools/chapel-py/chplcheck/rules.py b/tools/chapel-py/chplcheck/rules.py index 1bc979bc5d3a..7e37899b4c06 100644 --- a/tools/chapel-py/chplcheck/rules.py +++ b/tools/chapel-py/chplcheck/rules.py @@ -1,5 +1,5 @@ import chapel -import chapel.core +from chapel.core import * import re def name_for_linting(node): @@ -15,41 +15,41 @@ def check_pascal_case(node): return re.fullmatch(r'_?(([A-Z][a-z]+|\d+)+|[A-Z]+)\$?', name_for_linting(node)) def register_rules(driver): - @driver.basic_rule(chapel.core.VarLikeDecl) + @driver.basic_rule(VarLikeDecl) def CamelCaseVariables(context, node): if node.name() == "_": return True return check_camel_case(node) - @driver.basic_rule(chapel.core.Record) + @driver.basic_rule(Record) def CamelCaseRecords(context, node): return check_camel_case(node) - @driver.basic_rule(chapel.core.Class) + @driver.basic_rule(Class) def PascalCaseClasses(context, node): return check_pascal_case(node) - @driver.basic_rule(chapel.core.Module) + @driver.basic_rule(Module) def PascalCaseModules(context, node): return check_pascal_case(node) - @driver.basic_rule(chapel.core.Loop) + @driver.basic_rule(Loop) def DoKeywordAndBlock(context, node): return node.block_style() != "unnecessary" - @driver.basic_rule(chapel.core.Coforall) + @driver.basic_rule(Coforall) def NestedCoforalls(context, node): parent = node.parent() while parent is not None: - if isinstance(parent, chapel.core.Coforall): + if isinstance(parent, Coforall): return False parent = parent.parent() return True - @driver.basic_rule([chapel.core.Conditional, chapel.core.BoolLiteral, chapel.rest]) + @driver.basic_rule([Conditional, BoolLiteral, chapel.rest]) def BoolLitInCondStmt(context, node): return False - @driver.basic_rule(chapel.core.NamedDecl) + @driver.basic_rule(NamedDecl) def ChplPrefixReserved(context, node): if node.name().startswith("chpl_"): path = node.location().path() @@ -59,10 +59,10 @@ def ChplPrefixReserved(context, node): @driver.advanced_rule def ConsecutiveDecls(context, root): def is_relevant_decl(node): - if isinstance(node, chapel.core.MultiDecl): + if isinstance(node, MultiDecl): for child in node: - if isinstance(child, chapel.core.Variable): return child.kind() - elif isinstance(node, chapel.core.Variable): + if isinstance(child, Variable): return child.kind() + elif isinstance(node, Variable): return node.kind() return None @@ -72,7 +72,7 @@ def recurse(node, skip_direct = False): last_has_attribute = False for child in node: - yield from recurse(child, skip_direct = isinstance(child, chapel.core.MultiDecl)) + yield from recurse(child, skip_direct = isinstance(child, MultiDecl)) if skip_direct: continue @@ -108,7 +108,7 @@ def MisleadingIndentation(context, root): if child.location().start()[1] == prev.location().start()[1]: yield child - if isinstance(child, chapel.core.Loop) and child.block_style() == "implicit": + if isinstance(child, Loop) and child.block_style() == "implicit": grandchildren = list(child) if len(grandchildren) > 0: prev = list(grandchildren[-1])[0] From e4773aab68ce1277c03e995853e697a232a278a5 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 13:40:29 -0700 Subject: [PATCH 13/26] Add access to silenced rules via a method Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/__init__.py | 4 ++-- tools/chapel-py/chplcheck/driver.py | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/tools/chapel-py/chplcheck/__init__.py b/tools/chapel-py/chplcheck/__init__.py index d0f4572dc85a..9363b05a6bdc 100755 --- a/tools/chapel-py/chplcheck/__init__.py +++ b/tools/chapel-py/chplcheck/__init__.py @@ -94,8 +94,8 @@ def main(): args = parser.parse_args() driver = LintDriver() - driver.SilencedRules.extend([ "CamelCaseVariables", "ConsecutiveDecls" ]) - driver.SilencedRules.extend(args.ignored_rules) + driver.silence_rules("CamelCaseVariables", "ConsecutiveDecls") + driver.silence_rules(*args.ignored_rules) register_rules(driver) if args.lsp: diff --git a/tools/chapel-py/chplcheck/driver.py b/tools/chapel-py/chplcheck/driver.py index 7b76a3643060..fff6e15df1e7 100644 --- a/tools/chapel-py/chplcheck/driver.py +++ b/tools/chapel-py/chplcheck/driver.py @@ -22,6 +22,9 @@ def __init__(self): self.BasicRules = [] self.AdvancedRules = [] + def silence_rules(self, *rules): + self.SilencedRules.extend(rules) + def should_check_rule(self, node, rulename): if rulename in self.SilencedRules: return False From 098af657d23cb21c5591214a7f9e356297e28e5d Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 13:44:53 -0700 Subject: [PATCH 14/26] Extract the LSP functionality into its own module Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/__init__.py | 54 +-------------------------- tools/chapel-py/chplcheck/lsp.py | 53 ++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 53 deletions(-) create mode 100644 tools/chapel-py/chplcheck/lsp.py diff --git a/tools/chapel-py/chplcheck/__init__.py b/tools/chapel-py/chplcheck/__init__.py index 9363b05a6bdc..b014be04b238 100755 --- a/tools/chapel-py/chplcheck/__init__.py +++ b/tools/chapel-py/chplcheck/__init__.py @@ -27,59 +27,7 @@ import argparse from driver import LintDriver from rules import register_rules - -def run_lsp(driver): - from pygls.server import LanguageServer - from lsprotocol.types import TEXT_DOCUMENT_DID_OPEN, DidOpenTextDocumentParams - from lsprotocol.types import TEXT_DOCUMENT_DID_SAVE, DidSaveTextDocumentParams - from lsprotocol.types import Diagnostic, Range, Position, DiagnosticSeverity - - server = LanguageServer('chplcheck', 'v0.1') - contexts = {} - - def get_updated_context(uri): - if uri in contexts: - context = contexts[uri] - context.advance_to_next_revision(False) - else: - context = chapel.core.Context() - contexts[uri] = context - return context - - def parse_file(context, uri): - return context.parse(uri[len("file://"):]) - - def build_diagnostics(uri): - context = get_updated_context(uri) - asts = parse_file(context, uri) - diagnostics = [] - for (node, rule) in driver.run_checks(context, asts): - location = node.location() - start = location.start() - end = location.end() - - diagnostic = Diagnostic( - range=Range( - start=Position(start[0]-1, start[1]-1), - end=Position(end[0]-1, end[1]-1) - ), - message="Lint: rule [{}] violated".format(rule), - severity=DiagnosticSeverity.Warning - ) - diagnostics.append(diagnostic) - return diagnostics - - @server.feature(TEXT_DOCUMENT_DID_OPEN) - async def did_open(ls, params: DidOpenTextDocumentParams): - text_doc = ls.workspace.get_text_document(params.text_document.uri) - ls.publish_diagnostics(text_doc.uri, build_diagnostics(text_doc.uri)) - - @server.feature(TEXT_DOCUMENT_DID_SAVE) - async def did_save(ls, params: DidSaveTextDocumentParams): - text_doc = ls.workspace.get_text_document(params.text_document.uri) - ls.publish_diagnostics(text_doc.uri, build_diagnostics(text_doc.uri)) - - server.start_io() +from lsp import run_lsp def print_violation(node, name): location = node.location() diff --git a/tools/chapel-py/chplcheck/lsp.py b/tools/chapel-py/chplcheck/lsp.py new file mode 100644 index 000000000000..e27b485f898f --- /dev/null +++ b/tools/chapel-py/chplcheck/lsp.py @@ -0,0 +1,53 @@ +from pygls.server import LanguageServer +from lsprotocol.types import TEXT_DOCUMENT_DID_OPEN, DidOpenTextDocumentParams +from lsprotocol.types import TEXT_DOCUMENT_DID_SAVE, DidSaveTextDocumentParams +from lsprotocol.types import Diagnostic, Range, Position, DiagnosticSeverity + +def location_to_range(location): + start = location.start() + end = location.end() + return Range( + start=Position(start[0]-1, start[1]-1), + end=Position(end[0]-1, end[1]-1) + ) + +def run_lsp(driver): + server = LanguageServer('chplcheck', 'v0.1') + contexts = {} + + def get_updated_context(uri): + if uri in contexts: + context = contexts[uri] + context.advance_to_next_revision(False) + else: + context = chapel.core.Context() + contexts[uri] = context + return context + + def parse_file(context, uri): + return context.parse(uri[len("file://"):]) + + def build_diagnostics(uri): + context = get_updated_context(uri) + asts = parse_file(context, uri) + diagnostics = [] + for (node, rule) in driver.run_checks(context, asts): + diagnostic = Diagnostic( + range= location_to_range(node.location()), + message="Lint: rule [{}] violated".format(rule), + severity=DiagnosticSeverity.Warning + ) + diagnostics.append(diagnostic) + return diagnostics + + @server.feature(TEXT_DOCUMENT_DID_OPEN) + async def did_open(ls, params: DidOpenTextDocumentParams): + text_doc = ls.workspace.get_text_document(params.text_document.uri) + ls.publish_diagnostics(text_doc.uri, build_diagnostics(text_doc.uri)) + + @server.feature(TEXT_DOCUMENT_DID_SAVE) + async def did_save(ls, params: DidSaveTextDocumentParams): + text_doc = ls.workspace.get_text_document(params.text_document.uri) + ls.publish_diagnostics(text_doc.uri, build_diagnostics(text_doc.uri)) + + server.start_io() From a93f9be63550f94bda7fd008a27b0b3fa21dd487 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 13:54:30 -0700 Subject: [PATCH 15/26] Fix a couple of bugs introduced in earlier refactoring Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/lsp.py | 1 + tools/chapel-py/chplcheck/rules.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/chapel-py/chplcheck/lsp.py b/tools/chapel-py/chplcheck/lsp.py index e27b485f898f..4094bccf72b7 100644 --- a/tools/chapel-py/chplcheck/lsp.py +++ b/tools/chapel-py/chplcheck/lsp.py @@ -1,3 +1,4 @@ +import chapel.core from pygls.server import LanguageServer from lsprotocol.types import TEXT_DOCUMENT_DID_OPEN, DidOpenTextDocumentParams from lsprotocol.types import TEXT_DOCUMENT_DID_SAVE, DidSaveTextDocumentParams diff --git a/tools/chapel-py/chplcheck/rules.py b/tools/chapel-py/chplcheck/rules.py index 7e37899b4c06..5efc32bd2cf4 100644 --- a/tools/chapel-py/chplcheck/rules.py +++ b/tools/chapel-py/chplcheck/rules.py @@ -102,7 +102,7 @@ def recurse(node, skip_direct = False): def MisleadingIndentation(context, root): prev = None for child in root: - yield from MisleadingIndentation(child) + yield from MisleadingIndentation(context, child) if prev is not None: if child.location().start()[1] == prev.location().start()[1]: From 6931a7a561f73f12d76edb63ef707b2efb1c33c8 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 14:40:17 -0700 Subject: [PATCH 16/26] Fix lambda capture bug in replace.py Signed-off-by: Danila Fedorin --- tools/chapel-py/chapel/replace.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tools/chapel-py/chapel/replace.py b/tools/chapel-py/chapel/replace.py index 141caf148723..2777f968a265 100644 --- a/tools/chapel-py/chapel/replace.py +++ b/tools/chapel-py/chapel/replace.py @@ -95,11 +95,14 @@ def rename_formals(rc, fn, renames): updates that perform the formal renaming. """ + def name_replacer(name): + return lambda child_text: child_text.replace(name, renames[name]) + for child in fn.formals(): name = child.name() if name not in renames: continue - yield (child, lambda child_text: child_text.replace(name, renames[name])) + yield (child, name_replacer(name)) def rename_named_actuals(rc, call, renames): """ @@ -128,6 +131,10 @@ def _do_replace(finder, ctx, filename, suffix, inplace): # and apply the transformations. nodes_to_replace = {} + + def compose(outer, inner): + return lambda text: outer(inner(text)) + for ast in asts: for (node, replace_with) in finder(rc, ast): uid = node.unique_id() @@ -140,7 +147,7 @@ def _do_replace(finder, ctx, filename, suffix, inplace): elif uid in nodes_to_replace: # Old substitution is also a callable; need to create composition. if callable(nodes_to_replace[uid]): - nodes_to_replace[uid] = lambda text: replace_with(nodes_to_replace[uid](text)) + nodes_to_replace[uid] = compose(replace_with, nodes_to_replace[uid]) # Old substitution is a string; we can apply the callable to get # another string. else: From 249b0e64fe1f624bdc3d2a1d255c10c7e7d210c7 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Wed, 25 Oct 2023 14:43:07 -0700 Subject: [PATCH 17/26] Add license comments Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/driver.py | 20 ++++++++++++++++++++ tools/chapel-py/chplcheck/lsp.py | 20 ++++++++++++++++++++ tools/chapel-py/chplcheck/rules.py | 20 ++++++++++++++++++++ 3 files changed, 60 insertions(+) diff --git a/tools/chapel-py/chplcheck/driver.py b/tools/chapel-py/chplcheck/driver.py index fff6e15df1e7..8f0e697158af 100644 --- a/tools/chapel-py/chplcheck/driver.py +++ b/tools/chapel-py/chplcheck/driver.py @@ -1,3 +1,23 @@ +# +# Copyright 2020-2023 Hewlett Packard Enterprise Development LP +# Copyright 2004-2019 Cray Inc. +# 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. +# + import chapel import chapel.core diff --git a/tools/chapel-py/chplcheck/lsp.py b/tools/chapel-py/chplcheck/lsp.py index 4094bccf72b7..87d0336ee5b0 100644 --- a/tools/chapel-py/chplcheck/lsp.py +++ b/tools/chapel-py/chplcheck/lsp.py @@ -1,3 +1,23 @@ +# +# Copyright 2020-2023 Hewlett Packard Enterprise Development LP +# Copyright 2004-2019 Cray Inc. +# 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. +# + import chapel.core from pygls.server import LanguageServer from lsprotocol.types import TEXT_DOCUMENT_DID_OPEN, DidOpenTextDocumentParams diff --git a/tools/chapel-py/chplcheck/rules.py b/tools/chapel-py/chplcheck/rules.py index 5efc32bd2cf4..b9e7da4df070 100644 --- a/tools/chapel-py/chplcheck/rules.py +++ b/tools/chapel-py/chplcheck/rules.py @@ -1,3 +1,23 @@ +# +# Copyright 2020-2023 Hewlett Packard Enterprise Development LP +# Copyright 2004-2019 Cray Inc. +# 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. +# + import chapel from chapel.core import * import re From 73523d78269092cc8b43afdac2022a4ad08dc800 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 26 Oct 2023 10:45:05 -0700 Subject: [PATCH 18/26] Add comments to driver.py Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/driver.py | 66 +++++++++++++++++++++++------ 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/tools/chapel-py/chplcheck/driver.py b/tools/chapel-py/chplcheck/driver.py index 8f0e697158af..b4bc771e0ed1 100644 --- a/tools/chapel-py/chplcheck/driver.py +++ b/tools/chapel-py/chplcheck/driver.py @@ -23,6 +23,11 @@ IgnoreAttr = ("chplcheck.ignore", ["rule", "comment"]) def ignores_rule(node, rulename): + """ + Given an AST node, check if it has an attribute telling it to silence + warnings for a given rule. + """ + ag = node.attribute_group() if ag is None: return False @@ -37,15 +42,28 @@ def ignores_rule(node, rulename): return False class LintDriver: + """ + Driver class containing the state and methods for linting. Among other + things, contains the rules for emitting warnings, as well as the + list of rules that should be silenced. + + Provides the @driver.basic_rule and @driver.advanced_rule decorators + for registering new rules. + """ + def __init__(self): self.SilencedRules = [] self.BasicRules = [] self.AdvancedRules = [] def silence_rules(self, *rules): + """ + Tell the driver to silence / skip warning for the given rules. + """ + self.SilencedRules.extend(rules) - def should_check_rule(self, node, rulename): + def _should_check_rule(self, node, rulename): if rulename in self.SilencedRules: return False @@ -54,46 +72,70 @@ def should_check_rule(self, node, rulename): return True - def check_basic_rule(self, context, root, rule): + def _check_basic_rule(self, context, root, rule): (name, nodetype, func) = rule - if not self.should_check_rule(None, name): - return - # If we should ignore the rule no matter the node, no reason to run # a traversal and match the pattern. + if not self._should_check_rule(None, name): + return + for (node, _) in chapel.each_matching(root, nodetype): - if not self.should_check_rule(node, name): + if not self._should_check_rule(node, name): continue if not func(context, node): yield (node, name) - def check_advanced_rule(self, context, root, rule): + def _check_advanced_rule(self, context, root, rule): (name, func) = rule # If we should ignore the rule no matter the node, no reason to run # a traversal and match the pattern. - if not self.should_check_rule(None, name): + if not self._should_check_rule(None, name): return for node in func(context, root): yield (node, name) - def basic_rule(self, nodetype): + def basic_rule(self, pat): + """ + This method is a decorator factory for adding 'basic' rules to the + driver. A basic rule is a function returning a boolean that gets called + on any node that matches a pattern. If the function returns 'True', the + node is good, and no warning is emitted. However, if the function returns + 'False', the node violates the rule. + + The name of the decorated function is used as the name of the rule. + """ + def wrapper(func): - self.BasicRules.append((func.__name__, nodetype, func)) + self.BasicRules.append((func.__name__, pat, func)) return func return wrapper def advanced_rule(self, func): + """ + This method is a decorator for adding 'advanced' rules to the driver. + An advanced rule is a function that gets called on a root AST node, + and is expected to traverse that AST to find places where warnings + need to be emitted. + + The name of the decorated function is used as the name of the rule. + """ + self.AdvancedRules.append((func.__name__, func)) return func def run_checks(self, context, asts): + """ + Runs all the rules registered with this node, yielding warnings for + all non-silenced rules that are violated in the given ASTs. + """ + for ast in asts: for rule in self.BasicRules: - yield from self.check_basic_rule(context, ast, rule) + yield from self._check_basic_rule(context, ast, rule) for rule in self.AdvancedRules: - yield from self.check_advanced_rule(context, ast, rule) + yield from self._check_advanced_rule(context, ast, rule) From 940506d7ec5c1204d6964fc79fd736e920853c85 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 26 Oct 2023 10:45:17 -0700 Subject: [PATCH 19/26] Be more principled about '$' in identifiers Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/rules.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tools/chapel-py/chplcheck/rules.py b/tools/chapel-py/chplcheck/rules.py index b9e7da4df070..74e22e842f14 100644 --- a/tools/chapel-py/chplcheck/rules.py +++ b/tools/chapel-py/chplcheck/rules.py @@ -26,13 +26,17 @@ def name_for_linting(node): name = node.name() if name.startswith("chpl_"): name = name.removeprefix("chpl_") + + # Strip dollar signs. + name = name.replace("$", "") + return name def check_camel_case(node): - return re.fullmatch(r'_?([a-z]+([A-Z][a-z]+|\d+)*|[A-Z]+)\$?', name_for_linting(node)) + return re.fullmatch(r'_?([a-z]+([A-Z][a-z]+|\d+)*|[A-Z]+)?', name_for_linting(node)) def check_pascal_case(node): - return re.fullmatch(r'_?(([A-Z][a-z]+|\d+)+|[A-Z]+)\$?', name_for_linting(node)) + return re.fullmatch(r'_?(([A-Z][a-z]+|\d+)+|[A-Z]+)?', name_for_linting(node)) def register_rules(driver): @driver.basic_rule(VarLikeDecl) From 505488e07a8e0cfa1b6c336c51fc18e15991aeab Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 26 Oct 2023 11:14:55 -0700 Subject: [PATCH 20/26] Add comments to LSP module Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/lsp.py | 39 +++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/tools/chapel-py/chplcheck/lsp.py b/tools/chapel-py/chplcheck/lsp.py index 87d0336ee5b0..c20f9cafa11f 100644 --- a/tools/chapel-py/chplcheck/lsp.py +++ b/tools/chapel-py/chplcheck/lsp.py @@ -25,6 +25,11 @@ from lsprotocol.types import Diagnostic, Range, Position, DiagnosticSeverity def location_to_range(location): + """ + Convert a Chapel location into a lsprotocol.types Range, which is + used for e.g. reporting diagnostics. + """ + start = location.start() end = location.end() return Range( @@ -33,10 +38,29 @@ def location_to_range(location): ) def run_lsp(driver): + """ + Start a language server on the standard input/output, and use it to + report linter warnings as LSP diagnostics. + """ + server = LanguageServer('chplcheck', 'v0.1') - contexts = {} + contexts = {} def get_updated_context(uri): + """ + The LSP driver maintains one Chapel context per-file. This is to avoid + having to reset all files' text etc. when a single file is updated. + There may be a more principled approach we can take in the future. + + This function returns an _update_ context, which is effectively a context + in which we can save / make use of updated file text. If there wasn't + a context for a URI, a brand new context will do. For existing contexts, + an older version of the file's text is probably stored, so advance + the context to next revision, invalidating that cache. + + Thus, this method is effectively allocate-or-advance-context. + """ + if uri in contexts: context = contexts[uri] context.advance_to_next_revision(False) @@ -46,9 +70,20 @@ def get_updated_context(uri): return context def parse_file(context, uri): + """ + Given a file URI, return the ASTs making up that file. Advances + the context if one already exists to make sure an updated result + is returned. + """ + return context.parse(uri[len("file://"):]) def build_diagnostics(uri): + """ + Parse a file at a particular URI, run the linter rules on the resulting + ASTs, and return them as LSP diagnostics. + """ + context = get_updated_context(uri) asts = parse_file(context, uri) diagnostics = [] @@ -61,6 +96,8 @@ def build_diagnostics(uri): diagnostics.append(diagnostic) return diagnostics + # The following functions are handlers for LSP events received by the server. + @server.feature(TEXT_DOCUMENT_DID_OPEN) async def did_open(ls, params: DidOpenTextDocumentParams): text_doc = ls.workspace.get_text_document(params.text_document.uri) From 9c8a278a0374e8c42c86e6e83c29361e8ee46c10 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 26 Oct 2023 11:17:16 -0700 Subject: [PATCH 21/26] Put pygls import behind a try/catch. Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/__init__.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tools/chapel-py/chplcheck/__init__.py b/tools/chapel-py/chplcheck/__init__.py index b014be04b238..be7b85ca7242 100755 --- a/tools/chapel-py/chplcheck/__init__.py +++ b/tools/chapel-py/chplcheck/__init__.py @@ -27,7 +27,6 @@ import argparse from driver import LintDriver from rules import register_rules -from lsp import run_lsp def print_violation(node, name): location = node.location() @@ -47,7 +46,12 @@ def main(): register_rules(driver) if args.lsp: - run_lsp(driver) + try: + from lsp import run_lsp + run_lsp(driver) + except ModuleNotFoundError: + print("language server support relies on pygls. please install it using pip.") + exit(1) return for (filename, context) in chapel.files_with_contexts(args.filenames): From 18089fd05108e80fdb6f177fe754197ca8bb9cd7 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 26 Oct 2023 11:31:56 -0700 Subject: [PATCH 22/26] List pygls as a dependency for the Chapel Python bindings. It's kind of a big hammer to require chplcheck's deps for Python bindings as a whole, but maybe it's fine. Signed-off-by: Danila Fedorin --- third-party/chpl-venv/Makefile | 3 ++- third-party/chpl-venv/Makefile.include | 1 + third-party/chpl-venv/chplcheck-requirements.txt | 5 +++++ tools/chapel-py/chplcheck/__init__.py | 8 ++------ 4 files changed, 10 insertions(+), 7 deletions(-) create mode 100644 third-party/chpl-venv/chplcheck-requirements.txt diff --git a/third-party/chpl-venv/Makefile b/third-party/chpl-venv/Makefile index 955139b68b2d..1f6bc78b8aea 100644 --- a/third-party/chpl-venv/Makefile +++ b/third-party/chpl-venv/Makefile @@ -99,7 +99,8 @@ chapel-py-venv: install-chpldeps export VIRTUAL_ENV=$(CHPL_VENV_VIRTUALENV_DIR) && \ $(PIP) install --upgrade $(CHPL_PIP_INSTALL_PARAMS) $(LOCAL_PIP_FLAGS) \ --target $(CHPL_VENV_CHPLDEPS) \ - -e $(CHPL_MAKE_HOME)/tools/chapel-py + -e $(CHPL_MAKE_HOME)/tools/chapel-py \ + -r $(CHPL_VENV_CHPLCHECK_REQUIREMENTS_FILE) install-requirements: install-chpldeps diff --git a/third-party/chpl-venv/Makefile.include b/third-party/chpl-venv/Makefile.include index aa1121631b60..0a45307a71b8 100644 --- a/third-party/chpl-venv/Makefile.include +++ b/third-party/chpl-venv/Makefile.include @@ -10,6 +10,7 @@ CHPL_VENV_CHPLDOC_REQUIREMENTS_FILE1=$(CHPL_VENV_DIR)/chpldoc-requirements1.txt CHPL_VENV_CHPLDOC_REQUIREMENTS_FILE2=$(CHPL_VENV_DIR)/chpldoc-requirements2.txt CHPL_VENV_CHPLDOC_REQUIREMENTS_FILE3=$(CHPL_VENV_DIR)/chpldoc-requirements3.txt CHPL_VENV_C2CHAPEL_REQUIREMENTS_FILE=$(CHPL_VENV_DIR)/c2chapel-requirements.txt +CHPL_VENV_CHPLCHECK_REQUIREMENTS_FILE=$(CHPL_VENV_DIR)/chplcheck-requirements.txt CHPL_VENV_CHPLSPELL_REQUIREMENTS_FILE=$(CHPL_VENV_DIR)/chplspell-requirements.txt CHPL_VENV_CHPLSPELL_REQS=$(CHPL_VENV_VIRTUALENV_DIR)/chpl-chplspell-reqs diff --git a/third-party/chpl-venv/chplcheck-requirements.txt b/third-party/chpl-venv/chplcheck-requirements.txt new file mode 100644 index 000000000000..f13f3acf1e01 --- /dev/null +++ b/third-party/chpl-venv/chplcheck-requirements.txt @@ -0,0 +1,5 @@ +attrs==23.1.0 +cattrs==23.1.2 +lsprotocol==2023.0.0b1 +pygls==1.1.1 +typeguard==3.0.2 diff --git a/tools/chapel-py/chplcheck/__init__.py b/tools/chapel-py/chplcheck/__init__.py index be7b85ca7242..b014be04b238 100755 --- a/tools/chapel-py/chplcheck/__init__.py +++ b/tools/chapel-py/chplcheck/__init__.py @@ -27,6 +27,7 @@ import argparse from driver import LintDriver from rules import register_rules +from lsp import run_lsp def print_violation(node, name): location = node.location() @@ -46,12 +47,7 @@ def main(): register_rules(driver) if args.lsp: - try: - from lsp import run_lsp - run_lsp(driver) - except ModuleNotFoundError: - print("language server support relies on pygls. please install it using pip.") - exit(1) + run_lsp(driver) return for (filename, context) in chapel.files_with_contexts(args.filenames): From 5ac308c7bfbb8c9a5801e9f340a3adef495479ec Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 26 Oct 2023 11:34:05 -0700 Subject: [PATCH 23/26] Add a comment about silencing advanced rules. Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/driver.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/chapel-py/chplcheck/driver.py b/tools/chapel-py/chplcheck/driver.py index b4bc771e0ed1..686bb3b74223 100644 --- a/tools/chapel-py/chplcheck/driver.py +++ b/tools/chapel-py/chplcheck/driver.py @@ -96,6 +96,10 @@ def _check_advanced_rule(self, context, root, rule): return for node in func(context, root): + # It's not clear how, if it all, advanced rules should be silenced + # by attributes (i.e., where do you put the @chplcheck.ignore + # attribute?). For now, do not silence them on a per-node basis. + yield (node, name) def basic_rule(self, pat): From 86cf27a7ed10f6855ac4c884c7cbb27a78aa7a4b Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 26 Oct 2023 11:39:55 -0700 Subject: [PATCH 24/26] Add a command-line flag to disable rules. Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/__init__.py | 8 +++++--- tools/chapel-py/chplcheck/driver.py | 10 +++++++++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/tools/chapel-py/chplcheck/__init__.py b/tools/chapel-py/chplcheck/__init__.py index b014be04b238..aaa4f7d15fd1 100755 --- a/tools/chapel-py/chplcheck/__init__.py +++ b/tools/chapel-py/chplcheck/__init__.py @@ -37,13 +37,15 @@ def print_violation(node, name): def main(): parser = argparse.ArgumentParser( prog='chplcheck', description='A linter for the Chapel language') parser.add_argument('filenames', nargs='*') - parser.add_argument('--ignore-rule', action='append', dest='ignored_rules', default=[]) + parser.add_argument('--disable-rule', action='append', dest='disabled_rules', default=[]) + parser.add_argument('--enable-rule', action='append', dest='enabled_rules', default=[]) parser.add_argument('--lsp', action='store_true', default=False) args = parser.parse_args() driver = LintDriver() - driver.silence_rules("CamelCaseVariables", "ConsecutiveDecls") - driver.silence_rules(*args.ignored_rules) + driver.disable_rules("CamelCaseVariables", "ConsecutiveDecls") + driver.disable_rules(*args.disabled_rules) + driver.enable_rules(*args.enabled_rules) register_rules(driver) if args.lsp: diff --git a/tools/chapel-py/chplcheck/driver.py b/tools/chapel-py/chplcheck/driver.py index 686bb3b74223..c9fe2907a94b 100644 --- a/tools/chapel-py/chplcheck/driver.py +++ b/tools/chapel-py/chplcheck/driver.py @@ -56,13 +56,21 @@ def __init__(self): self.BasicRules = [] self.AdvancedRules = [] - def silence_rules(self, *rules): + def disable_rules(self, *rules): """ Tell the driver to silence / skip warning for the given rules. """ self.SilencedRules.extend(rules) + def enable_rules(self, *rules): + """ + Tell the driver to warning for the given rules even if they were + previously disabled. + """ + + self.SilencedRules = list(set(self.SilencedRules) - set(rules)) + def _should_check_rule(self, node, rulename): if rulename in self.SilencedRules: return False From 959569a288b0ff1957ef172b25cc0df39787c7d3 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 26 Oct 2023 11:43:09 -0700 Subject: [PATCH 25/26] Make node an optional argument to should_check_rule. Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/driver.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/chapel-py/chplcheck/driver.py b/tools/chapel-py/chplcheck/driver.py index c9fe2907a94b..a1e470f55213 100644 --- a/tools/chapel-py/chplcheck/driver.py +++ b/tools/chapel-py/chplcheck/driver.py @@ -71,7 +71,7 @@ def enable_rules(self, *rules): self.SilencedRules = list(set(self.SilencedRules) - set(rules)) - def _should_check_rule(self, node, rulename): + def _should_check_rule(self,rulename, node = None): if rulename in self.SilencedRules: return False @@ -85,11 +85,11 @@ def _check_basic_rule(self, context, root, rule): # If we should ignore the rule no matter the node, no reason to run # a traversal and match the pattern. - if not self._should_check_rule(None, name): + if not self._should_check_rule(name): return for (node, _) in chapel.each_matching(root, nodetype): - if not self._should_check_rule(node, name): + if not self._should_check_rule(name, node): continue if not func(context, node): @@ -100,7 +100,7 @@ def _check_advanced_rule(self, context, root, rule): # If we should ignore the rule no matter the node, no reason to run # a traversal and match the pattern. - if not self._should_check_rule(None, name): + if not self._should_check_rule(name): return for node in func(context, root): From e38632619c6919daf26d4a23118f8a7cacc6d7d6 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 26 Oct 2023 14:57:43 -0700 Subject: [PATCH 26/26] Fix typo Signed-off-by: Danila Fedorin --- tools/chapel-py/chplcheck/driver.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/chapel-py/chplcheck/driver.py b/tools/chapel-py/chplcheck/driver.py index a1e470f55213..4d114d78625a 100644 --- a/tools/chapel-py/chplcheck/driver.py +++ b/tools/chapel-py/chplcheck/driver.py @@ -65,7 +65,7 @@ def disable_rules(self, *rules): def enable_rules(self, *rules): """ - Tell the driver to warning for the given rules even if they were + Tell the driver to warn for the given rules even if they were previously disabled. """