do not use shell integration for Python subshell if Python setting is disabled (#24418)

anthonykim1 · web-flow · commit 542ff3894b23 · 2024-11-12T09:24:29.000-08:00
Resolves: #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.
diff --git 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<void> {
@@ -74,7 +75,10 @@ export class TerminalService implements ITerminalService, Disposable {
         }
         this.terminal!.sendText(text);
     }
-    public async executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined> {
+    public async executeCommand(
+        commandLine: string,
+        isPythonShell: boolean,
+    ): Promise<TerminalShellExecution | undefined> {
         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<boolean>('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
@@ -145,8 +145,8 @@ export class SynchronousTerminalService implements ITerminalService, Disposable
     public sendText(text: string): Promise<void> {
         return this.terminalService.sendText(text);
     }
-    public executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined> {
-        return this.terminalService.executeCommand(commandLine);
+    public executeCommand(commandLine: string, isPythonShell: boolean): Promise<TerminalShellExecution | undefined> {
+        return this.terminalService.executeCommand(commandLine, isPythonShell);
     }
     public show(preserveFocus?: boolean | undefined): Promise<void> {
         return this.terminalService.show(preserveFocus);
diff --git a/src/client/common/terminal/types.ts b/src/client/common/terminal/types.ts
@@ -54,7 +54,7 @@ export interface ITerminalService extends IDisposable {
     ): Promise<void>;
     /** @deprecated */
     sendText(text: string): Promise<void>;
-    executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined>;
+    executeCommand(commandLine: string, isPythonShell: boolean): Promise<TerminalShellExecution | undefined>;
     show(preserveFocus?: boolean): Promise<void>;
 }
 
diff --git 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
@@ -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<TerminalShellIntegration>;
     let onDidEndTerminalShellExecutionEmitter: EventEmitter<TerminalShellExecutionEndEvent>;
     let event: TerminalShellExecutionEndEvent;
+    let getConfigurationStub: sinon.SinonStub;
+    let pythonConfig: TypeMoq.IMock<WorkspaceConfiguration>;
+    let editorConfig: TypeMoq.IMock<WorkspaceConfiguration>;
 
     setup(() => {
         terminal = TypeMoq.Mock.ofType<VSCodeTerminal>();
@@ -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<WorkspaceConfiguration>();
+        editorConfig = TypeMoq.Mock.ofType<WorkspaceConfiguration>();
+        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,13 +118,15 @@ 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(() => {
                 const workspaceConfig = TypeMoq.Mock.ofType<WorkspaceConfiguration>();
                 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
@@ -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());
             });
         });
     });

Original file line number	Diff line number	Diff line change
`@@ -145,8 +145,8 @@ export class SynchronousTerminalService implements ITerminalService, Disposable`
`145`	`145`	`public sendText(text: string): Promise<void> {`
`146`	`146`	`return this.terminalService.sendText(text);`
`147`	`147`	`}`
`148`		`- public executeCommand(commandLine: string): Promise<TerminalShellExecution \| undefined> {`
`149`		`- return this.terminalService.executeCommand(commandLine);`
	`148`	`+ public executeCommand(commandLine: string, isPythonShell: boolean): Promise<TerminalShellExecution \| undefined> {`
	`149`	`+ return this.terminalService.executeCommand(commandLine, isPythonShell);`
`150`	`150`	`}`
`151`	`151`	`public show(preserveFocus?: boolean \| undefined): Promise<void> {`
`152`	`152`	`return this.terminalService.show(preserveFocus);`
Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ export interface ITerminalService extends IDisposable {`
`54`	`54`	`): Promise<void>;`
`55`	`55`	`/** @deprecated */`
`56`	`56`	`sendText(text: string): Promise<void>;`
`57`		`- executeCommand(commandLine: string): Promise<TerminalShellExecution \| undefined>;`
	`57`	`+ executeCommand(commandLine: string, isPythonShell: boolean): Promise<TerminalShellExecution \| undefined>;`
`58`	`58`	`show(preserveFocus?: boolean): Promise<void>;`
`59`	`59`	`}`
`60`	`60`
Original file line number	Diff line number	Diff line change
`@@ -59,7 +59,7 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {`
`59`	`59`	`this.configurationService.updateSetting('REPL.enableREPLSmartSend', false, resource);`
`60`	`60`	`}`
`61`	`61`	`} else {`
`62`		`- await this.getTerminalService(resource).executeCommand(code);`
	`62`	`+ await this.getTerminalService(resource).executeCommand(code, true);`
`63`	`63`	`}`
`64`	`64`	`}`
`65`	`65`