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

Ensure expected document state in LSP responses #1334

Merged
merged 4 commits into from
Feb 14, 2024

Conversation

msujew
Copy link
Member

@msujew msujew commented Jan 8, 2024

Related to #1290

In the discussion in particular, the console warning appears due to the GLSP handler not awaiting the linking phase. However, all of our own LSP handlers haven't been awaiting that phase, instead opting to respond as quickly as possible. This sometimes leads to unexpected behavior in the response or tries to resolve a cross reference before it is ready.

This change attempts to mitigate the risk of this issue appearing by adding a waitUntil method to the DocumentBuilder. We use that method in the language server to wait until the required data is available for the respective service to do their work. This is usually either IndexedReferences (for any service that requires to resolve cross references) and Parsed (for services that operate on the AST/CST directly).

The new waitUntil method can also be interrupted in case it is no longer required. It will then reject (see tests).

@msujew msujew added the LSP Language Server Protocol integration label Jan 8, 2024
@msujew msujew added this to the v3.0.0 milestone Jan 8, 2024
Copy link
Contributor

@sailingKieler sailingKieler left a comment

Choose a reason for hiding this comment

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

I like the contribution of this PR, as it provides an explicit way of awaiting the completion of the builder. On the other hand, it's kind of a sugar API based on the build listeners, right, with the additional support for cancelation checking?

Actually, I'm undecided whether it's worth exposing it on the document builder. It could also be added to the LanguageServer as it's (mainly) used there, couldn't it?

packages/langium/test/workspace/document-builder.test.ts Outdated Show resolved Hide resolved
packages/langium/test/workspace/document-builder.test.ts Outdated Show resolved Hide resolved
packages/langium/test/workspace/document-builder.test.ts Outdated Show resolved Hide resolved
@@ -18,7 +18,7 @@ export default defineConfig({
interopDefault: true
},
include: ['**/test/**/*.test.ts'],
exclude: ['**/node_modules/**', '**/dist/**', '**/generated/**', '**/templates/**'],
exclude: ['**/node_modules/**', '**/dist/**', '**/generated/**', '**/templates/**', '**/examples/hello*/**'],
Copy link
Contributor

Choose a reason for hiding this comment

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

That's most likely not related to this PR, right?
Is the code examples/hello*/ a left-over of a previous run of generator-langium tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessarily, but I've found that the npm run test command restarts once the yeoman generator test has passed, since it updates the files. Adding this exclude rule seems to fix it.

@msujew
Copy link
Member Author

msujew commented Jan 22, 2024

@sailingKieler I've also thought about the waitUntil API placement. However, I don't think it makes a lot of sense in the LanguageServer service interface. While the API isn't used outside of the LSP context (yet), it's own functionality isn't really related to language servers at all. Assuming we merge the LSP split, that would make it hard for users to use the method without importing langium/lsp. I don't really see a reason to move it outside of the DocumentBuilder.

On the other hand, it's kind of a sugar API based on the build listeners, right, with the additional support for cancelation checking?

Also, I wouldn't say it's API sugar. It has two separate use cases:

  1. onBuildPhase triggers on every call of the build/update methods. The idea is to be able to intercept the builder process and run your own code in addition to that.
  2. waitUntil only observes the document builder once. It is supposed to be used to ensure that outside processes deal with a valid state of a document/workspace.

With that in mind, I believe it's worth to have it's own API.

Copy link
Contributor

@sailingKieler sailingKieler left a comment

Choose a reason for hiding this comment

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

Great!
Apart from my additional comment, I'm fine with the contribution.

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

Good improvement!

@msujew msujew merged commit 87715d1 into main Feb 14, 2024
5 checks passed
@msujew msujew deleted the msujew/lsp-workspace-state branch February 14, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LSP Language Server Protocol integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants