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

refactor(lsp): use LPeg for watchfiles matching #23788

Merged
merged 4 commits into from
Jun 5, 2023

Conversation

nojnhuh
Copy link
Contributor

@nojnhuh nojnhuh commented May 27, 2023

This PR updates the LSP glob matching implementation from a hand-rolled parser based on VSCode to be LPeg-based. The grammar is defined using LPeg and outputs an LPeg pattern used to match against incoming events. The motivation for this change is to simplify and improve performance for some of the necessary changes in #23500.

This also fixes a couple of outstanding edge cases. There is one new class of edge case with these changes regarding ** that's described in the TODO in the tests. Overall I think this is a net-positive change in terms of correctness though.

There should be no other user-facing changes unless users are calling the private _match function, which now accepts an LPeg pattern instead of an array of Lua patterns.

For reference, the LSP glob syntax is described here: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#pattern

@nojnhuh
Copy link
Contributor Author

nojnhuh commented May 27, 2023

Interesting that the test is passing on FreeBSD but nowhere else. I'm having trouble repro-ing but I'll keep digging.

@nojnhuh
Copy link
Contributor Author

nojnhuh commented Jun 4, 2023

I feel like I've made a reasonable effort to repro the CI failures locally by following along with the CI config but haven't had any luck. I can try hitting CI hard in a separate branch on my fork, but had a question to maybe help me not hit any usage limits:
When I've debugged tests locally, I've generally written to the LSP log. Is that available anywhere from CI? Or what else would be the simplest way to generate debug output that would be visible in CI?

@nojnhuh
Copy link
Contributor Author

nojnhuh commented Jun 4, 2023

I have a local repro now. It's a simple (but maybe not easy) bug in the new matching that was only surfacing based on the path used in the test.

@justinmk
Copy link
Member

justinmk commented Jun 4, 2023

When I've debugged tests locally, I've generally written to the LSP log. Is that available anywhere from CI? Or what else would be the simplest way to generate debug output that would be visible in CI?

$NVIM_LOG_FILE is printed on test failure, but currently only C code can log to that.
LSP log isn't captured as a CI artifact, but we could consider doing that until we have more holistic logging.

Comment on lines +61 to +62
-- TODO: '*' inside a {} condition is interpreted literally but should probably have the same
-- wildcard semantics it usually has.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW after a cursory look through a few server implementations, I didn't find any that would be affected by this.

Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

Awesome use of lpeg, nice code reduction.

@nojnhuh
Copy link
Contributor Author

nojnhuh commented Jun 5, 2023

This change is also compatible with 0.9 AFAICT, so backporting this to better enable #23500 to be backported would be great if the not-really-public interface changes and minor behavioral differences here aren't a big issue.

@mfussenegger mfussenegger merged commit 416fe8d into neovim:master Jun 5, 2023
11 of 12 checks passed
@nojnhuh nojnhuh deleted the watchfiles-lpeg branch June 5, 2023 05:20
@mfussenegger
Copy link
Member

This change is also compatible with 0.9

I think lpeg isn't available in 0.9, it was bundled later in #23216

@nojnhuh
Copy link
Contributor Author

nojnhuh commented Jun 6, 2023

My understanding of that PR is that it doesn't introduce lpeg but changes how lpeg is included in the build process and exposes it in the runtime as vim.lpeg, so I don't think that would affect changes like this. I tried make distclean install with these changes on top of release-0.9 and tests pass and a quick smoke test shows the file watching working.

@bfredl Does that sound right? Can modules in runtime/lua require('lpeg') on the release-0.9 branch which doesn't have #23216?

@clason
Copy link
Member

clason commented Jun 6, 2023

That's because you still have the build dependencies lying around. If you install Neovim via, e.g., homebrew, require'lpeg' definitely leads to an error.

@bfredl
Copy link
Member

bfredl commented Jun 6, 2023

It will work in a local build as it is a build time dependency, but it won't work with distributed binaries. Lpeg as a documented runtime dependency is new for 0.10 and thus this shouldn't be backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lsp refactor changes that are not features or bugfixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants