From 542ff3894b231e9b76a9b2dbb3681489e8655c08 Mon Sep 17 00:00:00 2001 From: Anthony Kim <62267334+anthonykim1@users.noreply.github.com> Date: Tue, 12 Nov 2024 09:24:29 -0800 Subject: [PATCH] do not use shell integration for Python subshell if Python setting is disabled (#24418) Resolves: https://github.com/microsoft/vscode-python/issues/24422 If the user has explicitly set their Python shell integration setting to false, then do not use shell integration inside the sub-shell (Python Terminal REPL in this case), regardless of upper shell integration status (Terminal shell integration setting in vscode). We should still be safe from manual installation of shell integration with the setting disabled(https://code.visualstudio.com/docs/terminal/shell-integration#_manual-installation) since we are still reading the value of terminal.shellIntegration, and looking at Python's shell integration setting first to begin with. --- src/client/common/terminal/service.ts | 16 +++- .../common/terminal/syncTerminalService.ts | 4 +- src/client/common/terminal/types.ts | 2 +- .../codeExecution/terminalCodeExecution.ts | 2 +- .../common/terminals/service.unit.test.ts | 81 +++++++++++++++++++ .../terminalCodeExec.unit.test.ts | 8 +- 6 files changed, 102 insertions(+), 11 deletions(-) diff --git a/src/client/common/terminal/service.ts b/src/client/common/terminal/service.ts index 45ce9afac47e..d3a9652acb1f 100644 --- a/src/client/common/terminal/service.ts +++ b/src/client/common/terminal/service.ts @@ -20,6 +20,7 @@ import { TerminalShellType, } from './types'; import { traceVerbose } from '../../logging'; +import { getConfiguration } from '../vscodeApis/workspaceApis'; @injectable() export class TerminalService implements ITerminalService, Disposable { @@ -64,7 +65,7 @@ export class TerminalService implements ITerminalService, Disposable { this.terminal!.show(true); } - await this.executeCommand(text); + await this.executeCommand(text, false); } /** @deprecated */ public async sendText(text: string): Promise { @@ -74,7 +75,10 @@ export class TerminalService implements ITerminalService, Disposable { } this.terminal!.sendText(text); } - public async executeCommand(commandLine: string): Promise { + public async executeCommand( + commandLine: string, + isPythonShell: boolean, + ): Promise { const terminal = this.terminal!; if (!this.options?.hideFromUser) { terminal.show(true); @@ -98,7 +102,13 @@ export class TerminalService implements ITerminalService, Disposable { await promise; } - if (terminal.shellIntegration) { + const config = getConfiguration('python'); + const pythonrcSetting = config.get('terminal.shellIntegration.enabled'); + if (isPythonShell && !pythonrcSetting) { + // If user has explicitly disabled SI for Python, use sendText for inside Terminal REPL. + terminal.sendText(commandLine); + return undefined; + } else if (terminal.shellIntegration) { const execution = terminal.shellIntegration.executeCommand(commandLine); traceVerbose(`Shell Integration is enabled, executeCommand: ${commandLine}`); return execution; diff --git a/src/client/common/terminal/syncTerminalService.ts b/src/client/common/terminal/syncTerminalService.ts index 60f8ed7a6847..0b46a86ee51e 100644 --- a/src/client/common/terminal/syncTerminalService.ts +++ b/src/client/common/terminal/syncTerminalService.ts @@ -145,8 +145,8 @@ export class SynchronousTerminalService implements ITerminalService, Disposable public sendText(text: string): Promise { return this.terminalService.sendText(text); } - public executeCommand(commandLine: string): Promise { - return this.terminalService.executeCommand(commandLine); + public executeCommand(commandLine: string, isPythonShell: boolean): Promise { + return this.terminalService.executeCommand(commandLine, isPythonShell); } public show(preserveFocus?: boolean | undefined): Promise { return this.terminalService.show(preserveFocus); diff --git a/src/client/common/terminal/types.ts b/src/client/common/terminal/types.ts index db2b7f80e4b1..3e54458a57fd 100644 --- a/src/client/common/terminal/types.ts +++ b/src/client/common/terminal/types.ts @@ -54,7 +54,7 @@ export interface ITerminalService extends IDisposable { ): Promise; /** @deprecated */ sendText(text: string): Promise; - executeCommand(commandLine: string): Promise; + executeCommand(commandLine: string, isPythonShell: boolean): Promise; show(preserveFocus?: boolean): Promise; } diff --git a/src/client/terminals/codeExecution/terminalCodeExecution.ts b/src/client/terminals/codeExecution/terminalCodeExecution.ts index 3cba6141763b..ea444af4d89e 100644 --- a/src/client/terminals/codeExecution/terminalCodeExecution.ts +++ b/src/client/terminals/codeExecution/terminalCodeExecution.ts @@ -59,7 +59,7 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService { this.configurationService.updateSetting('REPL.enableREPLSmartSend', false, resource); } } else { - await this.getTerminalService(resource).executeCommand(code); + await this.getTerminalService(resource).executeCommand(code, true); } } diff --git a/src/test/common/terminals/service.unit.test.ts b/src/test/common/terminals/service.unit.test.ts index f0754948a233..7859b6d29e49 100644 --- a/src/test/common/terminals/service.unit.test.ts +++ b/src/test/common/terminals/service.unit.test.ts @@ -3,6 +3,7 @@ import { expect } from 'chai'; import * as path from 'path'; +import * as sinon from 'sinon'; import * as TypeMoq from 'typemoq'; import { Disposable, @@ -22,6 +23,7 @@ import { IDisposableRegistry } from '../../../client/common/types'; import { IServiceContainer } from '../../../client/ioc/types'; import { ITerminalAutoActivation } from '../../../client/terminals/types'; import { createPythonInterpreter } from '../../utils/interpreters'; +import * as workspaceApis from '../../../client/common/vscodeApis/workspaceApis'; suite('Terminal Service', () => { let service: TerminalService; @@ -37,6 +39,9 @@ suite('Terminal Service', () => { let terminalShellIntegration: TypeMoq.IMock; let onDidEndTerminalShellExecutionEmitter: EventEmitter; let event: TerminalShellExecutionEndEvent; + let getConfigurationStub: sinon.SinonStub; + let pythonConfig: TypeMoq.IMock; + let editorConfig: TypeMoq.IMock; setup(() => { terminal = TypeMoq.Mock.ofType(); @@ -88,12 +93,22 @@ suite('Terminal Service', () => { mockServiceContainer.setup((c) => c.get(IWorkspaceService)).returns(() => workspaceService.object); mockServiceContainer.setup((c) => c.get(ITerminalActivator)).returns(() => terminalActivator.object); mockServiceContainer.setup((c) => c.get(ITerminalAutoActivation)).returns(() => terminalAutoActivator.object); + getConfigurationStub = sinon.stub(workspaceApis, 'getConfiguration'); + pythonConfig = TypeMoq.Mock.ofType(); + editorConfig = TypeMoq.Mock.ofType(); + getConfigurationStub.callsFake((section: string) => { + if (section === 'python') { + return pythonConfig.object; + } + return editorConfig.object; + }); }); teardown(() => { if (service) { service.dispose(); } disposables.filter((item) => !!item).forEach((item) => item.dispose()); + sinon.restore(); }); test('Ensure terminal is disposed', async () => { @@ -103,6 +118,7 @@ suite('Terminal Service', () => { const os: string = 'windows'; service = new TerminalService(mockServiceContainer.object); const shellPath = 'powershell.exe'; + // TODO: switch over legacy Terminal code to use workspace getConfiguration from workspaceApis instead of directly from vscode.workspace workspaceService .setup((w) => w.getConfiguration(TypeMoq.It.isValue('terminal.integrated.shell'))) .returns(() => { @@ -110,6 +126,7 @@ suite('Terminal Service', () => { workspaceConfig.setup((c) => c.get(os)).returns(() => shellPath); return workspaceConfig.object; }); + pythonConfig.setup((p) => p.get('terminal.shellIntegration.enabled')).returns(() => false); platformService.setup((p) => p.isWindows).returns(() => os === 'windows'); platformService.setup((p) => p.isLinux).returns(() => os === 'linux'); @@ -134,6 +151,7 @@ suite('Terminal Service', () => { }); test('Ensure command is sent to terminal and it is shown', async () => { + pythonConfig.setup((p) => p.get('terminal.shellIntegration.enabled')).returns(() => false); terminalHelper .setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns(() => Promise.resolve(undefined)); @@ -171,6 +189,69 @@ suite('Terminal Service', () => { terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1)); }); + test('Ensure sendText is used when Python shell integration is disabled', async () => { + pythonConfig + .setup((p) => p.get('terminal.shellIntegration.enabled')) + .returns(() => false) + .verifiable(TypeMoq.Times.once()); + + terminalHelper + .setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns(() => Promise.resolve(undefined)); + service = new TerminalService(mockServiceContainer.object); + const textToSend = 'Some Text'; + terminalHelper.setup((h) => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); + terminalManager.setup((t) => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); + + service.ensureTerminal(); + service.executeCommand(textToSend, true); + + terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(1)); + terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1)); + }); + + test('Ensure sendText is called when terminal.shellIntegration enabled but Python shell integration disabled', async () => { + pythonConfig + .setup((p) => p.get('terminal.shellIntegration.enabled')) + .returns(() => false) + .verifiable(TypeMoq.Times.once()); + + terminalHelper + .setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns(() => Promise.resolve(undefined)); + service = new TerminalService(mockServiceContainer.object); + const textToSend = 'Some Text'; + terminalHelper.setup((h) => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); + terminalManager.setup((t) => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); + + service.ensureTerminal(); + service.executeCommand(textToSend, true); + + terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(1)); + terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1)); + }); + + test('Ensure sendText is NOT called when Python shell integration and terminal shell integration are both enabled', async () => { + pythonConfig + .setup((p) => p.get('terminal.shellIntegration.enabled')) + .returns(() => true) + .verifiable(TypeMoq.Times.once()); + + terminalHelper + .setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns(() => Promise.resolve(undefined)); + service = new TerminalService(mockServiceContainer.object); + const textToSend = 'Some Text'; + terminalHelper.setup((h) => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash); + terminalManager.setup((t) => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object); + + service.ensureTerminal(); + service.executeCommand(textToSend, true); + + terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(1)); + terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.never()); + }); + test('Ensure terminal is not shown if `hideFromUser` option is set to `true`', async () => { terminalHelper .setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())) diff --git a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts index 4b5537f515d2..b5bcecd971ea 100644 --- a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts +++ b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts @@ -643,10 +643,10 @@ suite('Terminal - Code Execution', () => { terminalSettings.setup((t) => t.launchArgs).returns(() => terminalArgs); await executor.execute('cmd1'); - terminalService.verify(async (t) => t.executeCommand('cmd1'), TypeMoq.Times.once()); + terminalService.verify(async (t) => t.executeCommand('cmd1', true), TypeMoq.Times.once()); await executor.execute('cmd2'); - terminalService.verify(async (t) => t.executeCommand('cmd2'), TypeMoq.Times.once()); + terminalService.verify(async (t) => t.executeCommand('cmd2', true), TypeMoq.Times.once()); }); test('Ensure code is sent to the same terminal for a particular resource', async () => { @@ -668,10 +668,10 @@ suite('Terminal - Code Execution', () => { terminalSettings.setup((t) => t.launchArgs).returns(() => terminalArgs); await executor.execute('cmd1', resource); - terminalService.verify(async (t) => t.executeCommand('cmd1'), TypeMoq.Times.once()); + terminalService.verify(async (t) => t.executeCommand('cmd1', true), TypeMoq.Times.once()); await executor.execute('cmd2', resource); - terminalService.verify(async (t) => t.executeCommand('cmd2'), TypeMoq.Times.once()); + terminalService.verify(async (t) => t.executeCommand('cmd2', true), TypeMoq.Times.once()); }); }); });