Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make chplcheck available from chpl-language-server #25214

Merged
merged 11 commits into from
Jun 11, 2024

Conversation

jabraham17
Copy link
Member

@jabraham17 jabraham17 commented Jun 11, 2024

This makes it possible for chpl-language-server to supply linting diagnostics, without users needing to use chplcheck directly.

Although using chplcheck standalone 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 chplcheck integration testing with chpl-language-server

[Reviewed by @DanilaFe and @lydia-duncan]

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>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
@jabraham17 jabraham17 marked this pull request as ready for review June 11, 2024 13:45
@jabraham17 jabraham17 requested a review from DanilaFe June 11, 2024 13:45
Copy link
Member

@lydia-duncan lydia-duncan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for hammering this out so quickly! Just a couple of documentation suggestions that'd be nice to have but aren't essential

doc/rst/tools/chplcheck/chplcheck.rst Show resolved Hide resolved
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Comment on lines 146 to 182

# 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.main or chplcheck.lsp.
  2. if the only errors that can occur are the ValueErrors that you raise, consider just making this a static method and returning None to avoid the none-try-some in the main body.
  3. Ideally, you'd only execute this if --chplcheck was thrown, but then you'd need to have some kind of two-phase parsing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tools/chpl-language-server/src/chpl-language-server.py Outdated Show resolved Hide resolved
Comment on lines 1084 to 1092
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather keep this code out of a global object

tools/chplcheck/src/config.py Outdated Show resolved Hide resolved
Comment on lines -66 to -67
self.skip_unstable: bool = skip_unstable
self.internal_prefixes: List[str] = internal_prefixes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest keeping these aliases for fields in config for brevity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@jabraham17 jabraham17 merged commit 11ff993 into chapel-lang:main Jun 11, 2024
8 checks passed
@jabraham17 jabraham17 deleted the chplcheck-as-sub-command branch June 11, 2024 16:38
@bradcray
Copy link
Member

Cool! @lydia-duncan , have you been able to put this to work?

@lydia-duncan
Copy link
Member

I've been catching up on triage 😅 so not yet, but hoping soon

@lydia-duncan
Copy link
Member

Alright, just tried it and I'm getting a failure that appears to be coming from the language server:

$ chpl-language-server --help
Traceback (most recent call last):
  File "$CHPL_HOME/tools/chpl-language-server/src/chpl-language-server.py", line 203, in <module>
    chplcheck = ChplcheckProxy.get()
                ^^^^^^^^^^^^^^^^^^^^
  File "$CHPL_HOME/tools/chpl-language-server/src/chpl-language-server.py", line 194, in get
    m = load_module(mod)
        ^^^^^^^^^^^^^^^^
  File "$CHPL_HOME/tools/chpl-language-server/src/chpl-language-server.py", line 177, in load_module
    file_path = os.path.join(chplcheck_path, module_name + ".py")
                             ^^^^^^^^^^^^^^
NameError: name 'chplcheck_path' is not defined

(note that I anonymized my path)
This is after remaking both the language server and chplcheck

@jabraham17
Copy link
Member Author

Alright, just tried it and I'm getting a failure that appears to be coming from the language server:

Do'oh! That should be a simple fix

jabraham17 added a commit that referenced this pull request Jun 11, 2024
Followup to #25214, fixes a
missing variable forgotten when applying reviewer feedback

Tested with `make test-cls`

[Reviewed by @DanilaFe and @lydia-duncan ]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants