Ensure we register for interpreter change when moving from untrusted to trusted. (#19351)

karthiknadig · web-flow · commit 1ffa23ad1b98 · 2022-06-24T11:04:49.000-07:00
* Ensure we register for interpreter change when moving from untrusted to trusted.

* Tweak LanguageServerChangeHandler dispose

* Use disposables instead on context.subscriptions for consistency.

* Address comments.

* Fix tests.
diff --git a/src/client/activation/node/lspNotebooksExperiment.ts b/src/client/activation/node/lspNotebooksExperiment.ts
@@ -3,7 +3,7 @@
 import { inject, injectable } from 'inversify';
 import * as semver from 'semver';
 import { Disposable, extensions } from 'vscode';
-import { IConfigurationService } from '../../common/types';
+import { IConfigurationService, IDisposableRegistry } from '../../common/types';
 import { sendTelemetryEvent } from '../../telemetry';
 import { EventName } from '../../telemetry/constants';
 import { JUPYTER_EXTENSION_ID, PYLANCE_EXTENSION_ID } from '../../common/constants';
@@ -28,16 +28,18 @@ export class LspNotebooksExperiment implements IExtensionSingleActivationService
     constructor(
         @inject(IServiceContainer) private readonly serviceContainer: IServiceContainer,
         @inject(IConfigurationService) private readonly configurationService: IConfigurationService,
+        @inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
         @inject(IJupyterExtensionDependencyManager) jupyterDependencyManager: IJupyterExtensionDependencyManager,
     ) {
-        if (!LspNotebooksExperiment.isPylanceInstalled()) {
-            this.pylanceExtensionChangeHandler = extensions.onDidChange(this.pylanceExtensionsChangeHandler.bind(this));
-        }
-
         this.isJupyterInstalled = jupyterDependencyManager.isJupyterExtensionInstalled;
     }
 
     public async activate(): Promise<void> {
+        if (!LspNotebooksExperiment.isPylanceInstalled()) {
+            this.pylanceExtensionChangeHandler = extensions.onDidChange(this.pylanceExtensionsChangeHandler.bind(this));
+            this.disposables.push(this.pylanceExtensionChangeHandler);
+        }
+
         this.updateExperimentSupport();
     }
 
diff --git a/src/client/extensionActivation.ts b/src/client/extensionActivation.ts
@@ -159,27 +159,25 @@ async function activateLegacy(ext: ExtensionState): Promise<ActivationResult> {
 
             serviceManager.get<ICodeExecutionManager>(ICodeExecutionManager).registerCommands();
 
-            context.subscriptions.push(new LinterCommands(serviceManager));
+            disposables.push(new LinterCommands(serviceManager));
 
             if (
                 pythonSettings &&
                 pythonSettings.formatting &&
                 pythonSettings.formatting.provider !== 'internalConsole'
             ) {
                 const formatProvider = new PythonFormattingEditProvider(context, serviceContainer);
-                context.subscriptions.push(languages.registerDocumentFormattingEditProvider(PYTHON, formatProvider));
-                context.subscriptions.push(
-                    languages.registerDocumentRangeFormattingEditProvider(PYTHON, formatProvider),
-                );
+                disposables.push(languages.registerDocumentFormattingEditProvider(PYTHON, formatProvider));
+                disposables.push(languages.registerDocumentRangeFormattingEditProvider(PYTHON, formatProvider));
             }
 
-            context.subscriptions.push(new ReplProvider(serviceContainer));
+            disposables.push(new ReplProvider(serviceContainer));
 
             const terminalProvider = new TerminalProvider(serviceContainer);
             terminalProvider.initialize(window.activeTerminal).ignoreErrors();
-            context.subscriptions.push(terminalProvider);
+            disposables.push(terminalProvider);
 
-            context.subscriptions.push(
+            disposables.push(
                 languages.registerCodeActionsProvider(PYTHON, new PythonCodeActionProvider(), {
                     providedCodeActionKinds: [CodeActionKind.SourceOrganizeImports],
                 }),
@@ -188,9 +186,7 @@ async function activateLegacy(ext: ExtensionState): Promise<ActivationResult> {
             serviceContainer
                 .getAll<DebugConfigurationProvider>(IDebugConfigurationService)
                 .forEach((debugConfigProvider) => {
-                    context.subscriptions.push(
-                        debug.registerDebugConfigurationProvider(DebuggerTypeName, debugConfigProvider),
-                    );
+                    disposables.push(debug.registerDebugConfigurationProvider(DebuggerTypeName, debugConfigProvider));
                 });
 
             serviceContainer.get<IDebuggerBanner>(IDebuggerBanner).initialize();
@@ -200,7 +196,7 @@ async function activateLegacy(ext: ExtensionState): Promise<ActivationResult> {
     // "activate" everything else
 
     const manager = serviceContainer.get<IExtensionActivationManager>(IExtensionActivationManager);
-    context.subscriptions.push(manager);
+    disposables.push(manager);
 
     const activationPromise = manager.activate();
 
diff --git a/src/client/extensionInit.ts b/src/client/extensionInit.ts
@@ -54,16 +54,16 @@ export function initializeGlobals(
     serviceManager.addSingletonInstance<IExtensionContext>(IExtensionContext, context);
 
     const standardOutputChannel = window.createOutputChannel(OutputChannelNames.python);
-    context.subscriptions.push(standardOutputChannel);
-    context.subscriptions.push(registerLogger(new OutputChannelLogger(standardOutputChannel)));
+    disposables.push(standardOutputChannel);
+    disposables.push(registerLogger(new OutputChannelLogger(standardOutputChannel)));
 
     const workspaceService = new WorkspaceService();
     const unitTestOutChannel =
         workspaceService.isVirtualWorkspace || !workspaceService.isTrusted
             ? // Do not create any test related output UI when using virtual workspaces.
               instance(mock<IOutputChannel>())
             : window.createOutputChannel(OutputChannelNames.pythonTest);
-    context.subscriptions.push(unitTestOutChannel);
+    disposables.push(unitTestOutChannel);
 
     serviceManager.addSingletonInstance<OutputChannel>(IOutputChannel, standardOutputChannel, STANDARD_OUTPUT_CHANNEL);
     serviceManager.addSingletonInstance<OutputChannel>(IOutputChannel, unitTestOutChannel, TEST_OUTPUT_CHANNEL);
diff --git a/src/client/languageServer/watcher.ts b/src/client/languageServer/watcher.ts
@@ -59,7 +59,7 @@ export class LanguageServerWatcher
     // When using Pylance, there will only be one language server for the project.
     private workspaceLanguageServers: Map<string, ILanguageServerExtensionManager | undefined>;
 
-    private languageServerChangeHandler: LanguageServerChangeHandler;
+    private registered = false;
 
     constructor(
         @inject(IServiceContainer) private readonly serviceContainer: IServiceContainer,
@@ -81,41 +81,12 @@ export class LanguageServerWatcher
         this.workspaceInterpreters = new Map();
         this.workspaceLanguageServers = new Map();
         this.languageServerType = this.configurationService.getSettings().languageServer;
-
-        disposables.push(this.workspaceService.onDidChangeConfiguration(this.onDidChangeConfiguration.bind(this)));
-
-        disposables.push(
-            this.workspaceService.onDidChangeWorkspaceFolders(this.onDidChangeWorkspaceFolders.bind(this)),
-        );
-
-        disposables.push(
-            this.interpreterService.onDidChangeInterpreterInformation(this.onDidChangeInterpreterInformation, this),
-        );
-
-        if (this.workspaceService.isTrusted) {
-            disposables.push(this.interpreterPathService.onDidChange(this.onDidChangeInterpreter.bind(this)));
-        }
-
-        this.languageServerChangeHandler = new LanguageServerChangeHandler(
-            this.languageServerType,
-            this.extensions,
-            this.applicationShell,
-            this.commandManager,
-            this.workspaceService,
-            this.configurationService,
-        );
-        disposables.push(this.languageServerChangeHandler);
-
-        disposables.push(
-            extensions.onDidChange(async () => {
-                await this.extensionsChangeHandler();
-            }),
-        );
     }
 
     // IExtensionActivationService
 
     public async activate(resource?: Resource): Promise<void> {
+        this.register();
         await this.startLanguageServer(this.languageServerType, resource);
     }
 
@@ -124,6 +95,44 @@ export class LanguageServerWatcher
         await this.startAndGetLanguageServer(languageServerType, resource);
     }
 
+    public register(): void {
+        if (!this.registered) {
+            this.registered = true;
+            this.disposables.push(
+                this.workspaceService.onDidChangeConfiguration(this.onDidChangeConfiguration.bind(this)),
+            );
+
+            this.disposables.push(
+                this.workspaceService.onDidChangeWorkspaceFolders(this.onDidChangeWorkspaceFolders.bind(this)),
+            );
+
+            this.disposables.push(
+                this.interpreterService.onDidChangeInterpreterInformation(this.onDidChangeInterpreterInformation, this),
+            );
+
+            if (this.workspaceService.isTrusted) {
+                this.disposables.push(this.interpreterPathService.onDidChange(this.onDidChangeInterpreter.bind(this)));
+            }
+
+            this.disposables.push(
+                this.extensions.onDidChange(async () => {
+                    await this.extensionsChangeHandler();
+                }),
+            );
+
+            this.disposables.push(
+                new LanguageServerChangeHandler(
+                    this.languageServerType,
+                    this.extensions,
+                    this.applicationShell,
+                    this.commandManager,
+                    this.workspaceService,
+                    this.configurationService,
+                ),
+            );
+        }
+    }
+
     private async startAndGetLanguageServer(
         languageServerType: LanguageServerType,
         resource?: Resource,
diff --git a/src/test/languageServer/watcher.unit.test.ts b/src/test/languageServer/watcher.unit.test.ts
@@ -83,6 +83,8 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
+
+        watcher.register();
     });
 
     teardown(() => {
@@ -131,7 +133,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
-
+        watcher.register();
         assert.strictEqual(disposables.length, 11);
     });
 
@@ -177,7 +179,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
-
+        watcher.register();
         assert.strictEqual(disposables.length, 10);
     });
 
@@ -254,6 +256,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
+        watcher.register();
 
         // First start, get the reference to the extension manager.
         await watcher.startLanguageServer(LanguageServerType.None);
@@ -330,6 +333,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
+        watcher.register();
 
         await watcher.startLanguageServer(LanguageServerType.None);
 
@@ -409,7 +413,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
-
+        watcher.register();
         const startLanguageServerSpy = sandbox.spy(watcher, 'startLanguageServer');
 
         await watcher.startLanguageServer(LanguageServerType.None);
@@ -479,6 +483,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
+        watcher.register();
 
         // Use a fake here so we don't actually start up language servers.
         const startLanguageServerFake = sandbox.fake.resolves(undefined);
@@ -542,7 +547,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
-
+        watcher.register();
         await watcher.startLanguageServer(LanguageServerType.Jedi);
 
         assert.ok(startLanguageServerStub.calledOnce);
@@ -606,6 +611,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
+        watcher.register();
 
         await watcher.startLanguageServer(LanguageServerType.Node);
 
@@ -664,6 +670,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
+        watcher.register();
 
         await watcher.startLanguageServer(LanguageServerType.Jedi);
 
@@ -753,6 +760,7 @@ suite('Language server watcher', () => {
                 {} as LspNotebooksExperiment,
                 disposables,
             );
+            watcher.register();
 
             await watcher.startLanguageServer(languageServer, Uri.parse('folder1'));
             await watcher.startLanguageServer(languageServer, Uri.parse('folder2'));
@@ -832,6 +840,7 @@ suite('Language server watcher', () => {
                 {} as LspNotebooksExperiment,
                 disposables,
             );
+            watcher.register();
 
             await watcher.startLanguageServer(languageServer, Uri.parse('workspace1'));
             await watcher.startLanguageServer(languageServer, Uri.parse('workspace2'));
@@ -920,6 +929,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
+        watcher.register();
 
         const startLanguageServerSpy = sandbox.spy(watcher, 'startLanguageServer');
 
@@ -1000,6 +1010,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
+        watcher.register();
 
         const startLanguageServerSpy = sandbox.spy(watcher, 'startLanguageServer');
 
@@ -1083,6 +1094,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
+        watcher.register();
 
         const startLanguageServerSpy = sandbox.spy(watcher, 'startLanguageServer');
 
@@ -1166,6 +1178,7 @@ suite('Language server watcher', () => {
             {} as LspNotebooksExperiment,
             disposables,
         );
+        watcher.register();
 
         const startLanguageServerSpy = sandbox.spy(watcher, 'startLanguageServer');
 
