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

feat: definitions and references lsp hint #1016

Merged
merged 20 commits into from
Jun 7, 2024
Merged

Conversation

rszyma
Copy link
Contributor

@rszyma rszyma commented May 6, 2024

Describe your changes. Use imperative present tense.

Add goto definition support for LSP consumers. Adding support for it directly in LSP server could maybe be doable, but unreliable (e.g. templates are complicating things) and so a lot of parser logic would need to be reimplemented for all edge cases, so I think this is better added directly to kanata-parser.

For now I've added definition and references for aliases. I'd like to wait for recieve some feeback (i.e. if this is a direction we want to take) before adding the rest (variables, layers, etc.).

Currently the code is working for aliases when paired with rszyma/vscode-kanata#32

Implementation notes:

I couldn't add lsp_hints to ParserState, because it would require changing & to &mut on s param in many places which would be really ugly, and even then, when trying to switch to &mut at some point I got an error regarding multiple borrows of &mut s at the same time. So instead I've added additional lsp_hints param to many functions.

Other done things:

  • Move lsp_hint_inactive_code from ParserState to IntermediateConfig

Other thoughts:

Perhaps for performance purposes LSP features could be behind a feature flag, but it doesn't seem like it causes any major performance degradation + parser code runs only once, so for simplicity I didn't introduce the flag.

Checklist 1

Go-to definition and references implemented for:

  • aliases
  • variables
  • virtual_keys (+sequences)
  • chord_groups
  • layers
  • includes
  • templates

Checklist 2

  • Add documentation to docs/config.adoc
    • Yes or N/A
  • Add example and basic docs to cfg_samples/kanata.kbd
    • Yes or N/A
  • Update error messages
    • Yes or N/A
  • Added tests, or did manual testing
    • Yes

@rszyma
Copy link
Contributor Author

rszyma commented May 6, 2024

Also this would enable semantic highlighting I think?

@jtroo
Copy link
Owner

jtroo commented May 6, 2024

I couldn't add lsp_hints to ParserState, because it would require changing & to &mut on s param in many places which would be really ugly, and even then, when trying to switch to &mut at some point I got an error regarding multiple borrows of &mut s at the same time. So instead I've added additional lsp_hints param to many functions.

Consider using RefCell for this; I think it will suit the purpose well enough.


Perhaps for performance purposes LSP features could be behind a feature flag, but it doesn't seem like it causes any major performance degradation + parser code runs only once, so for simplicity I didn't introduce the flag.

I think we should do it for this new code and retroactively do it for the inactive code hints too. Probably good to compartmentalize the bulk of the new stuff into a new module.

We could make the functions always be called, and then the function body can be conditionally compiled (with an empty body by default); and rely on dead code elimination to do the right thing. Or add conditional compilation at the call-site to guarantee the optimization.

.push_from_atom(atom);
vkey_expr
.atom(s.vars())
.expect("must be atom since we're matching atom")
Copy link
Owner

Choose a reason for hiding this comment

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

Fly-by comment

The way this new code works isn't right because matching an expr and finding an Atom might find you a $var that is actually a list.

You might want to add span_atom equivalent of span_list, which I haven't yet added because I didn't have the need for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I've refactored in 99be6a2. No need for span_atom

This also fixes wasm build.

Also simplify main.rs.
Also:
- fix misleading error message when list after deflayermap token is
empty.
- capture only layer name in deflayer/deflayermap. Previously it
captured span of a list where layer name is defined
@rszyma
Copy link
Contributor Author

rszyma commented Jun 2, 2024

one problematic aspect of implementing defs/refs for variables is that there are 2 types of variables: normal (globally scoped) variables, and variables in deftemplate which have limited scope. deftemplate scoped variables shadow global ones?

Edit: I'd say let's just ignore existence of template variables from perspective of refs/defs.

@rszyma
Copy link
Contributor Author

rszyma commented Jun 2, 2024

implementing for chord groups doesn't seem that important, but another cool thing about defs/refs hints is that it also will allow implementing semantic highlighting

Also adds "lsp" cargo feature.
TODO: conditionally compile all lsp-related features behind this flag
@rszyma
Copy link
Contributor Author

rszyma commented Jun 2, 2024

I've implemented defs/refs for variables.

SExpr.list and SExpr.atom don't have access to ParserState right now, so we can't access ParserState.lsp_hints though it too. We could add a lsp_hints parameter to it, but that requires 100+ changes and passing it in every list or atom call.

To minimize amount of changes and taking into consideration that it will be used only with "lsp" cargo flag, I think it's pretty reasonable to go for something like a global static mut variable.
One thing though, as I've mentioned in code comment, it makes parse_cfg_raw_string (and all majority of functions that are called in parse_cfg_raw_string) unsafe for concurrent calls. And so it's not a good idea to run tests with "lsp" flag on (we can't use "all" features flag in CI. Edit: I think we already don't?).

What do you think @jtroo?

@rszyma rszyma marked this pull request as ready for review June 2, 2024 21:01
Copy link
Owner

@jtroo jtroo left a comment

Choose a reason for hiding this comment

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

I've implemented defs/refs for variables.
...
To minimize amount of changes and taking into consideration that it will be used only with "lsp" cargo flag, I think it's pretty reasonable to go for something like a global static mut variable.

The higher-level idea is sensible. The current implementation needs to be changed though; static mut is rarely if ever correct to do. The idea can be implemented without unsafe, and so should be. The parser is not multi-threaded, so one option is to use thread_local. Using thread_local also means parser+lsp tests should be able to be run in parallel just.

Although due to how str_to_oscode works, parser tests already shouldn't run in parallel anyway, so another option is to use Lazy<Mutex<...>> like how CUSTOM_STRS_TO_OSCODES does it.

@rszyma
Copy link
Contributor Author

rszyma commented Jun 3, 2024

thread_local looks interesting, but I'm not sure if I fully understand it yet. Couldn't we also use thread_local in CUSTOM_STRS_TO_OSCODES to enable parrarel tests?

@rszyma
Copy link
Contributor Author

rszyma commented Jun 3, 2024

The parser is not multi-threaded, so one option is to use thread_local

Done. Not sure if I'm using it correctly, but seems to work.

@jtroo
Copy link
Owner

jtroo commented Jun 3, 2024

thread_local looks interesting, but I'm not sure if I fully understand it yet. Couldn't we also use thread_local in CUSTOM_STRS_TO_OSCODES to enable parrarel tests?

I recall (perhaps incorrectly) that there are other threads other than the one the parser runs on for the real Kanata program. If I'm correct, using thread_local would mean the other threads do not see results from deflocalkeys, which would be incorrect behaviour.

Copy link
Owner

@jtroo jtroo left a comment

Choose a reason for hiding this comment

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

Needs a bit more cleanup and refining around conditional compilation; once that's done will be good to merge.

@@ -138,7 +138,7 @@ pub(crate) fn filter_env_specific_cfg(
env_var_val.is_empty(),
) {
(None, false) => {
lsp_hint_inactive_code.push(LspHintInactiveCode {
lsp_hint_inactive_code.push(lsp_hints::InactiveCode {
Copy link
Owner

Choose a reason for hiding this comment

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

Can we add the conditional compilation with LSP here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parser/src/lsp_hints.rs Show resolved Hide resolved
@jtroo jtroo merged commit 306e172 into jtroo:main Jun 7, 2024
4 checks passed
@rszyma rszyma deleted the definition-location branch June 7, 2024 18:58
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.

2 participants