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

Allow language servers to store Client as struct field #199

Merged
merged 1 commit into from
Aug 4, 2020

Conversation

ebkalderon
Copy link
Owner

Changed

  • Change LspService::new() to accept an FnOnce(Client) -> T closure instead of an already initialized T value.
  • Remove all &Client parameters from all LanguageServer trait methods.

These changes should enable users to initialize LanguageServer implementers with tower_lsp::Client conveniently stored as a struct field instead of having it passed into each callback method as an argument. Initialization would theoretically look like this:

#[tokio::main]
async fn main() {
    let stdin = tokio::io::stdin();
    let stdout = tokio::io::stdout();

    // let (service, messages) = LspService::new(Backend { ??? }); // Current
    let (service, messages) = LspService::new(|client| Backend { client }); // New
    Server::new(stdin, stdout)
        .interleave(messages)
        .serve(service)
        .await;
}

Additional resources required by the language server struct can be initialized ahead of time in main() and passed into the closure, if needed.

CC @Timmmm @icsaszar

Closes #189.

@ebkalderon ebkalderon self-assigned this Aug 3, 2020
Currently, `Client` is only ever exposed to the user as an argument
explicitly passed by reference into some (but not all) `LanguageServer`
trait methods. This was originally done for the sake of implementation
simplicity and getting a quick MVP out the door, but it is rapidly
becoming clear that this design does not scale.

User input has shown that non-trivial language server designs would
greatly benefit from being passed an owned `Client` at initialization
time instead of being passed in at runtime through a callback.
Additionally, routing the `Client` through each individual callback is
expensive to maintain in the long run. The current design, however,
makes migrating away from the status quo impossible.

To achieve this new goal, we modify the `LspService::new()` constructor
to accept a closure of type `FnOnce(Client) -> T` instead of an
initialized `T` value directly. This allows the construction of the
server struct to be deferred until the `Client` has been initialized
internally. With this approach, `LanguageServer` implementers can be
initialized with a `Client` field in a type-safe way without requiring
the user to manually track the initialization state of `Client` in their
code. Additional resources required by the language server struct can be
initialized ahead of time in `main()` and passed into the closure, if
needed.
@Timmmm
Copy link

Timmmm commented Aug 3, 2020

This seems much better, thanks!

@ebkalderon
Copy link
Owner Author

Awesome! In that case, I think I'll merge this pull request and consider #189 to be resolved. Feel free to open a new issue or submit a follow-up PR in case of further concerns or pain points you'd like to raise. Thanks again to all for the valuable input in previous discussions!

@ebkalderon ebkalderon merged commit c351713 into master Aug 4, 2020
@ebkalderon ebkalderon deleted the store-client-as-struct-field branch August 4, 2020 06:44
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.

Improve Client ergonomics
2 participants