langium/lsp: made LSP service minimum document state settings configurable in startLanguageServer(...)

[sailingKieler]

This is a proposal of making the LSP services' requirements on the affected documents' states configurable for optimizing purposes. @msujew what do you think?

Besides, I

• added opportunity to distinguish required documents states and workspace states;
• relaxed some doc state requirements to increase the LS' responsiveness from 'IndexedReferences' to 'Linked', as it is sufficient that the current document is (ideally) linked and all required documents are in state 'ComputedScopes', and current document should (ideally) be linked:
  • completion,
  • declaration,
  • definition,
  • typeDefinition,
  • hover,
  • semanticToken
• switched requirements of some services to workspace state 'IndexedReferences':
  • findReferences,
  • getImplementation,
  • documentHighlight,
  • callHierarchy,
  • rename

[Copilot]

Pull Request Overview

Externalizes LSP service minimum document state configurations into the dependency injection (DI) modules, making the document state requirements configurable for performance optimization purposes. The PR also relaxes some document state requirements from 'IndexedReferences' to 'Linked' for improved responsiveness in specific services.

• Adds a new serviceRequirements configuration object to LangiumSharedLSPServices type definition
• Updates all LSP service handlers to use configurable document state requirements instead of hardcoded values
• Provides default document state configurations in the default LSP module with optimized requirements for completion, declaration, definition, implementation, typeDefinition, hover, and semantic token services

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
packages/langium/src/lsp/lsp-services.ts	Adds serviceRequirements configuration interface to LangiumSharedLSPServices type
packages/langium/src/lsp/language-server.ts	Replaces hardcoded DocumentState values with configurable service requirements across all LSP handlers
packages/langium/src/lsp/default-lsp-module.ts	Implements default serviceRequirements configuration with optimized document states

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[msujew]

I think the general approach to this issue is good - however, I feel like the serviceRequirements approach misuses the DI system. The DI container is mostly reserved for services, and while the change in the PR works from a technical perspective, it feels a bit hacked into the DI system. I'm open to merge your approach anyway.

I would like to discuss an alternative approach: Since calls like connection.onDefinition are simple callbacks (they're not events!), we can register them multiple times, with only the last one actually sticking. Maybe we can use this to our advantage?

Perhaps, we can rewrite the code like this:

- export function addGotoDefinitionHandler(connection: Connection, services: LangiumSharedServices): void {
+ export function addGotoDefinitionHandler(services: LangiumSharedServices, state = DocumentState.IndexedReferences): void {

Adopters can then simply call addGotoDefinitionHandler(services, DocumentState.Validated); to change the state. However, I can also see how this is less maintainable (for adopters) than the DI approach that you're proposing.

@sailingKieler What do you think? Would you rather stick your approach?

[sailingKieler]

I would like to discuss an alternative approach: Since calls like connection.onDefinition are simple callbacks (they're not events!), we can register them multiple times, with only the last one actually sticking. Maybe we can use this to our advantage?

Perhaps, we can rewrite the code like this:

- export function addGotoDefinitionHandler(connection: Connection, services: LangiumSharedServices): void {
+ export function addGotoDefinitionHandler(services: LangiumSharedServices, state = DocumentState.IndexedReferences): void {

I also thought into that direction at the beginning. However, I wasn't aware of the fact that the connection impl uses a simple Map for keeping the request handlers. Is the fact that a connection object will keep at most 1 handler per message type documented as API, i.e. is it reliable? This approach feels a bit hack-ish to me, to be honest.

A compromise solution could be to combine your additional state parameters with an additional optional parameter to startLanguageServer(services), like

startLanguageServer(services: LangiumSharedServices, requiredStates: Partial<ServiceRequirements> = {}) {
    ...
    addCallHierarchyHandler(connection, services, requiredStates.CallHierarchyProvider);
    ...
}

interface ServiceRequirements {
    readonly CallHierarchyProvider: DocumentState
    readonly CodeActionProvider: DocumentState
    readonly CodeLensProvider: DocumentState
    readonly CompletionProvider: DocumentState
    readonly DeclarationProvider: DocumentState
    readonly DefinitionProvider: DocumentState
    readonly DocumentHighlightProvider: DocumentState
    readonly DocumentLinkProvider: DocumentState
    readonly DocumentSymbolProvider: DocumentState
    readonly FoldingRangeProvider: DocumentState
    readonly Formatter: DocumentState
    readonly HoverProvider: DocumentState
    readonly ImplementationProvider: DocumentState
    readonly InlayHintProvider: DocumentState
    readonly ReferencesProvider: DocumentState
    readonly RenameProvider: DocumentState
    readonly SemanticTokenProvider: DocumentState
    readonly SignatureHelp: DocumentState
    readonly TypeHierarchyProvider: DocumentState
    readonly TypeProvider: DocumentState
    readonly WorkspaceSymbolProvider: DocumentState
}

Technically, the latter approach is almost identical to the container-based approach.

[sailingKieler]

An aspect that is not covered by the aforementioned ideas is the potential need for rename requests to be blocked until the entire workspace reaches state IndexedReferences instead of the just the current file.
The same might apply for find references and highlight requests.

Adding an additional indicator representing this degree of freedom for all services feels quite over-engineered to me,
the alternative of hard-coding that aspect and having the state aspect configurable feels half-baked to be honest.

[msujew]

A compromise solution could be to combine your additional state parameters with an additional optional parameter to startLanguageServer(services), like

I actually like that the most, since it's IMO the least invasive solution that still gives adopters the same amount of freedom as the service approach.

Is the fact that a connection object will keep at most 1 handler per message type documented as API, i.e. is it reliable? This approach feels a bit hack-ish to me, to be honest.

I don't think it is documented, but AFAIK it's entirely intended. But with the solution above, we don't need to rely on it.

Adding an additional indicator representing this degree of freedom for all services feels quite over-engineered to me,
the alternative of hard-coding that aspect and having the state aspect configurable feels half-baked to be honest.

IMO, I'd be fine with having something like this:

interface ServiceRequirements {
    ...
    readonly RenameProvider: DocumentState | LanguageServiceAwaitState;
    ....
}

// Feel free to rename, feels clunky to me
interface LanguageServiceAwaitState {
  // Either wait for the specific document, or for the whole workspace to arrive at the state
  type: 'document' | 'workspace';
  documentState: DocumentState;
}

What do you think?

[sailingKieler]

IMO, I'd be fine with having something like this:

interface ServiceRequirements {
    ...
    readonly RenameProvider: DocumentState | LanguageServiceAwaitState;
    ....
}

// Feel free to rename, feels clunky to me
interface LanguageServiceAwaitState {
  // Either wait for the specific document, or for the whole workspace to arrive at the state
  type: 'document' | 'workspace';
  documentState: DocumentState;
}

What do you think?

I already had something similar in mind -- nice that we match on that. 🙂
I'll prepare an implementation in the upcoming days.

[sailingKieler]

@msujew I finished my rework, you may have look again.
I tested this version in my commercial project and it looks/works pretty well!