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

Disable reloading when changing language servers #18884

Merged
merged 32 commits into from
Apr 18, 2022

Conversation

kimadeline
Copy link

@kimadeline kimadeline commented Apr 8, 2022

For #18509

Behaviour before this PR:

  • Language server-specific code registered on extension activation
  • Language server check (if default value, if Python 2.7) done on language server start during activation
  • Changing the value of the python.languageServer setting would trigger a prompt asking to reload

Behaviour in this PR:

  • The only LS-related class registered on activation is LanguageServerWatcher
  • The LS watcher replaces the LanguageServerExtensionActivationService
  • The watcher listens to settings changes and interpreter changes, will start the relevant language server and dispose of the existing language server (if any)
  • In order to spin up the language server, the watcher will instantiate an object implementing the ILanguageServerExtensionManager interface for that specific language server, and then call ILanguageServerExtensionManager.startLanguageServer
  • The ILanguageServerExtensionManager classes replace the Jedi and Pylance activator classes, with some added functionality to start up and dispose of the language server classes
  • LS-specific classes are not injectable anymore, and all their dependencies are explicitly listed in their constructor, which means that they have gotten pretty big 😔
  • This PR also removes the NodeLanguageServerFolderService class, which was a remnant from the old MPLS days. It had a few "convenience" functions that were doing way too much work to infer the LS version, when we can pick it up using the VS Code API.

General architecture

classDiagram
      class ILanguageServerCapabilities {
          ILanguageServerProxy serverProxy
          Promise<ILanguageServer> get()
      }
      class ILanguageServerExtensionManager {
          Promise<void> startLanguageServer()
          void stopLanguageServer()
          boolean canStartLanguageServer()
          void languageServerNotAvailable()
          void dispose()
      }         
      ILanguageServerCapabilities <|-- LanguageServerCapabilities
      ILanguageServerCapabilities <|-- ILanguageServerExtensionManager
      LanguageServerCapabilities <|-- JediLSExtensionManager
      LanguageServerCapabilities <|-- PylanceLSExtensionManager
      ILanguageServerExtensionManager <|-- NoneLSExtensionManager
      ILanguageServerExtensionManager <|-- JediLSExtensionManager
      ILanguageServerExtensionManager <|-- PylanceLSExtensionManager
      class JediLSExtensionManager{
      }
      class PylanceLSExtensionManager {
      }
      class NoneLSExtensionManager {
      }
Loading

Activation flow

flowchart TD
    id1["Extension load"]--->|Register watcher singleton|id2["Activate services"]--->|"watcher.startLanguageServer()"|id3["watcher.createLanguageServer(lsType)"]--->|"LS extension manager created"|id4["lsExtensionManager.startLanguageServer()"]

Loading

@kimadeline kimadeline self-assigned this Apr 8, 2022
@kimadeline kimadeline marked this pull request as ready for review April 8, 2022 19:56
Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

Code appears way simpler now 🥳 great work. I have some concerns around the support for multiroot scenario, especially for Jedi.

src/client/languageServer/watcher.ts Outdated Show resolved Hide resolved
src/client/languageServer/watcher.ts Outdated Show resolved Hide resolved
src/client/languageServer/watcher.ts Outdated Show resolved Hide resolved
src/client/languageServer/watcher.ts Outdated Show resolved Hide resolved
src/client/languageServer/watcher.ts Outdated Show resolved Hide resolved
src/client/languageServer/watcher.ts Outdated Show resolved Hide resolved
@kimadeline kimadeline requested a review from karrtikr April 15, 2022 00:32
src/client/languageServer/watcher.ts Outdated Show resolved Hide resolved
src/client/languageServer/watcher.ts Show resolved Hide resolved
src/client/languageServer/watcher.ts Outdated Show resolved Hide resolved
src/client/languageServer/watcher.ts Outdated Show resolved Hide resolved
@kimadeline kimadeline requested a review from karrtikr April 18, 2022 18:48
Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

LGTM

startLanguageServer(
languageServerType: LanguageServerType,
resource?: Resource,
): Promise<ILanguageServerExtensionManager>;

Choose a reason for hiding this comment

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

Suggested change
): Promise<ILanguageServerExtensionManager>;
): Promise<void>;

Given it's not used anywhere outside the class, do we want to expose methods of ILanguageServerExtensionManager outside? Something to think about.

Copy link
Author

Choose a reason for hiding this comment

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

A subset of ILanguageServerExtensionManager methods are exposed on purpose for the classes implementing this interface.

I'll add to the comment above the definition of ILanguageServerExtensionManager that these methods shouldn't be used.

Copy link

@karrtikr karrtikr Apr 18, 2022

Choose a reason for hiding this comment

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

classes implementing this interface.

I understand they're exposed for the LanguageServerWatcher which is the class implementing ILanguageServerWatcher, it's fine to expose it for internal use.

But I don't think we need to expose ILanguageServerExtensionManager for external use (code outside language servers), so returning void here should be sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

Will not address this for now, captured in #18946.

@kimadeline kimadeline merged commit c9a70d7 into microsoft:main Apr 18, 2022
@kimadeline kimadeline deleted the 18509-disable-jedi-pvsc branch April 18, 2022 21:22
wesm pushed a commit to posit-dev/positron that referenced this pull request Mar 28, 2024
…thon#18884)

* Remove LSFolderService dependency

* No DI proof of concept

* Add safeguard when connecting/disconnecting

* Proper Pylance LS disposal

* Fix Jedi LS startup/disposal

* Add cache support

* Remove DI decorators + registry activation

* Do not reload window when Pylance not installed

* jedi/pylance/none extension managers

* languageServer/watcher.unit.test.ts

* News entry

* Add 2.7 behaviour + fix linting

* Remove deprecated LS settings

* Add support for 1 LS per workspace folder

* Add tests

* Update src/client/languageServer/watcher.ts

Co-authored-by: Kartik Raj <[email protected]>

* Add resource path to "starting ls" message

* Fix issue with get()

* Amend ILanguageServerExtensionManager comment

Co-authored-by: Kartik Raj <[email protected]>
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.

3 participants