Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jun 24, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0f7a39e
Ensure we register for interpreter change when moving from untrusted …
karthiknadig Jun 23, 2022
5b2aed6
Tweak LanguageServerChangeHandler dispose
karthiknadig Jun 23, 2022
8512a7f
Use disposables instead on context.subscriptions for consistency.
karthiknadig Jun 23, 2022
9eeb3f7
Address comments.
karthiknadig Jun 23, 2022
1144f58
Fix tests.
karthiknadig Jun 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/client/activation/node/lspNotebooksExperiment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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();
}

Expand Down
20 changes: 8 additions & 12 deletions src/client/extensionActivation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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],
}),
Expand All @@ -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();
Expand All @@ -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();

Expand Down
6 changes: 3 additions & 3 deletions src/client/extensionInit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
71 changes: 40 additions & 31 deletions src/client/languageServer/watcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);
}

Expand All @@ -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,
Expand Down
21 changes: 17 additions & 4 deletions src/test/languageServer/watcher.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ suite('Language server watcher', () => {
{} as LspNotebooksExperiment,
disposables,
);

watcher.register();
});

teardown(() => {
Expand Down Expand Up @@ -131,7 +133,7 @@ suite('Language server watcher', () => {
{} as LspNotebooksExperiment,
disposables,
);

watcher.register();
assert.strictEqual(disposables.length, 11);
});

Expand Down Expand Up @@ -177,7 +179,7 @@ suite('Language server watcher', () => {
{} as LspNotebooksExperiment,
disposables,
);

watcher.register();
assert.strictEqual(disposables.length, 10);
});

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -330,6 +333,7 @@ suite('Language server watcher', () => {
{} as LspNotebooksExperiment,
disposables,
);
watcher.register();

await watcher.startLanguageServer(LanguageServerType.None);

Expand Down Expand Up @@ -409,7 +413,7 @@ suite('Language server watcher', () => {
{} as LspNotebooksExperiment,
disposables,
);

watcher.register();
const startLanguageServerSpy = sandbox.spy(watcher, 'startLanguageServer');

await watcher.startLanguageServer(LanguageServerType.None);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -542,7 +547,7 @@ suite('Language server watcher', () => {
{} as LspNotebooksExperiment,
disposables,
);

watcher.register();
await watcher.startLanguageServer(LanguageServerType.Jedi);

assert.ok(startLanguageServerStub.calledOnce);
Expand Down Expand Up @@ -606,6 +611,7 @@ suite('Language server watcher', () => {
{} as LspNotebooksExperiment,
disposables,
);
watcher.register();

await watcher.startLanguageServer(LanguageServerType.Node);

Expand Down Expand Up @@ -664,6 +670,7 @@ suite('Language server watcher', () => {
{} as LspNotebooksExperiment,
disposables,
);
watcher.register();

await watcher.startLanguageServer(LanguageServerType.Jedi);

Expand Down Expand Up @@ -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'));
Expand Down Expand Up @@ -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'));
Expand Down Expand Up @@ -920,6 +929,7 @@ suite('Language server watcher', () => {
{} as LspNotebooksExperiment,
disposables,
);
watcher.register();

const startLanguageServerSpy = sandbox.spy(watcher, 'startLanguageServer');

Expand Down Expand Up @@ -1000,6 +1010,7 @@ suite('Language server watcher', () => {
{} as LspNotebooksExperiment,
disposables,
);
watcher.register();

const startLanguageServerSpy = sandbox.spy(watcher, 'startLanguageServer');

Expand Down Expand Up @@ -1083,6 +1094,7 @@ suite('Language server watcher', () => {
{} as LspNotebooksExperiment,
disposables,
);
watcher.register();

const startLanguageServerSpy = sandbox.spy(watcher, 'startLanguageServer');

Expand Down Expand Up @@ -1166,6 +1178,7 @@ suite('Language server watcher', () => {
{} as LspNotebooksExperiment,
disposables,
);
watcher.register();

const startLanguageServerSpy = sandbox.spy(watcher, 'startLanguageServer');

Expand Down