Make chplcheck available from chpl-language-server
#25214
Conversation
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
|
|
||
| # 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 | ||
|
|
There was a problem hiding this comment.
I suggest you wrap this in a class that contain the various modules. Something like this:
class ChplcheckProxy:
def __init__(self):
chpl_home = ...
...
self.main = self.load_module("chplcheck")
self.lsp = self.load_module("lsp")
chplcheck = None
try:
chplcheck = ChplcheckProxy()A few of things:
- Note that I've renamed the variables to avoid having to prefix them with the name chplcheck internally. Externally, you'd access them like
chplcheck.mainorchplcheck.lsp. - if the only errors that can occur are the
ValueErrors that you raise, consider just making this a static method and returningNoneto avoid thenone-try-somein the main body. - Ideally, you'd only execute this if
--chplcheckwas thrown, but then you'd need to have some kind of two-phase parsing.
There was a problem hiding this comment.
I've added a proxy object
Ideally, you'd only execute this if --chplcheck was thrown, but then you'd need to have some kind of two-phase parsing.
I think this is the ideal from a purity standpoint, but maybe not the most practical. Its possible to do parse_known_args, and then do some extra parsing of the command line arguments. But this adds extra complexity to our parsing that I don't want to have to maintain, and I am not sure there is much benefit
| 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) |
There was a problem hiding this comment.
If you have a ChplcheckProxy object, you can make self.lint_driver be self.chplcheck.lint_driver, and move this code into a method on the proxy. Seems cleaner that way.
There was a problem hiding this comment.
I would rather keep this code out of a global object
| self.skip_unstable: bool = skip_unstable | ||
| self.internal_prefixes: List[str] = internal_prefixes |
There was a problem hiding this comment.
I'd suggest keeping these aliases for fields in config for brevity.
There was a problem hiding this comment.
I would rather have less duplication, these aliases are only used once or twice
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
|
Cool! @lydia-duncan , have you been able to put this to work? |
|
I've been catching up on triage 😅 so not yet, but hoping soon |
|
Alright, just tried it and I'm getting a failure that appears to be coming from the language server: (note that I anonymized my path) |
Do'oh! That should be a simple fix |
Followup to #25214, fixes a missing variable forgotten when applying reviewer feedback Tested with `make test-cls` [Reviewed by @DanilaFe and @lydia-duncan ]
This makes it possible for
chpl-language-serverto supply linting diagnostics, without users needing to usechplcheckdirectly.Although using
chplcheckstandalone is the preferred config, some editors only allow one language server per language (like emacs). So this provides a workaround for those editors.This PR also adds some limited
chplcheckintegration testing withchpl-language-server[Reviewed by @DanilaFe and @lydia-duncan]