From 3c552f9d636b9fdd04de3e34e1b435ceb2b951b3 Mon Sep 17 00:00:00 2001 From: Anthony Kim <62267334+anthonykim1@users.noreply.github.com> Date: Fri, 17 Nov 2023 20:32:54 -0800 Subject: [PATCH] Show warning and allow user to turn off smart send for deprecated Python code (#22353) Resolves: #22341 #22340 Showing warning message after detecting user is on Python file with deprecated Python code, and are attempting to run smart send via shift+enter action. Allow user to turn off this via workspace setting. --------- Co-authored-by: Courtney Webster <60238438+cwebster-99@users.noreply.github.com> Co-authored-by: Kartik Raj --- package.json | 6 ++ package.nls.json | 1 + pythonFiles/normalizeSelection.py | 27 +++++- src/client/common/application/commands.ts | 1 + src/client/common/configSettings.ts | 6 +- src/client/common/types.ts | 5 + src/client/common/utils/localize.ts | 7 ++ .../codeExecution/djangoShellCodeExecution.ts | 1 + src/client/terminals/codeExecution/helper.ts | 19 +++- src/client/terminals/codeExecution/repl.ts | 4 +- .../codeExecution/terminalCodeExecution.ts | 19 +++- .../sample_invalid_smart_selection.py | 10 ++ .../terminals/codeExecution/smartSend.test.ts | 93 ++++++++++++++++++- .../terminalCodeExec.unit.test.ts | 2 + 14 files changed, 184 insertions(+), 17 deletions(-) create mode 100644 src/test/pythonFiles/terminalExec/sample_invalid_smart_selection.py diff --git a/package.json b/package.json index 2f655522f5f3..6bb410fa3fba 100644 --- a/package.json +++ b/package.json @@ -704,6 +704,12 @@ "scope": "resource", "type": "array" }, + "python.REPL.enableREPLSmartSend": { + "default": true, + "description": "%python.EnableREPLSmartSend.description%", + "scope": "resource", + "type": "boolean" + }, "python.testing.autoTestDiscoverOnSaveEnabled": { "default": true, "description": "%python.testing.autoTestDiscoverOnSaveEnabled.description%", diff --git a/package.nls.json b/package.nls.json index ab1eec6d3065..7bec9275ea61 100644 --- a/package.nls.json +++ b/package.nls.json @@ -59,6 +59,7 @@ "python.missingPackage.severity.description": "Set severity of missing packages in requirements.txt or pyproject.toml", "python.pipenvPath.description": "Path to the pipenv executable to use for activation.", "python.poetryPath.description": "Path to the poetry executable.", + "python.EnableREPLSmartSend.description": "Toggle Smart Send for the Python REPL. Smart Send enables sending the smallest runnable block of code to the REPL on Shift+Enter and moves the cursor accordingly.", "python.tensorBoard.logDirectory.description": "Set this setting to your preferred TensorBoard log directory to skip log directory prompt when starting TensorBoard.", "python.tensorBoard.logDirectory.markdownDeprecationMessage": "Tensorboard support has been moved to the extension [Tensorboard extension](https://marketplace.visualstudio.com/items?itemName=ms-toolsai.tensorboard). Instead use the setting `tensorBoard.logDirectory`.", "python.tensorBoard.logDirectory.deprecationMessage": "Tensorboard support has been moved to the extension Tensorboard extension. Instead use the setting `tensorBoard.logDirectory`.", diff --git a/pythonFiles/normalizeSelection.py b/pythonFiles/normalizeSelection.py index 7ace42daa901..7608ce8860f6 100644 --- a/pythonFiles/normalizeSelection.py +++ b/pythonFiles/normalizeSelection.py @@ -150,8 +150,18 @@ def traverse_file(wholeFileContent, start_line, end_line, was_highlighted): or a multiline dictionary, or differently styled multi-line list comprehension, etc. Then call the normalize_lines function to normalize our smartly selected code block. """ + parsed_file_content = None + + try: + parsed_file_content = ast.parse(wholeFileContent) + except Exception: + # Handle case where user is attempting to run code where file contains deprecated Python code. + # Let typescript side know and show warning message. + return { + "normalized_smart_result": "deprecated", + "which_line_next": 0, + } - parsed_file_content = ast.parse(wholeFileContent) smart_code = "" should_run_top_blocks = [] @@ -267,7 +277,11 @@ def get_next_block_lineno(which_line_next): data = None which_line_next = 0 - if empty_Highlight and contents.get("smartSendExperimentEnabled"): + if ( + empty_Highlight + and contents.get("smartSendExperimentEnabled") + and contents.get("smartSendSettingsEnabled") + ): result = traverse_file( contents["wholeFileContent"], vscode_start_line, @@ -276,9 +290,12 @@ def get_next_block_lineno(which_line_next): ) normalized = result["normalized_smart_result"] which_line_next = result["which_line_next"] - data = json.dumps( - {"normalized": normalized, "nextBlockLineno": result["which_line_next"]} - ) + if normalized == "deprecated": + data = json.dumps({"normalized": normalized}) + else: + data = json.dumps( + {"normalized": normalized, "nextBlockLineno": result["which_line_next"]} + ) else: normalized = normalize_lines(contents["code"]) data = json.dumps({"normalized": normalized}) diff --git a/src/client/common/application/commands.ts b/src/client/common/application/commands.ts index ba29f0dcd956..51afbdc951b8 100644 --- a/src/client/common/application/commands.ts +++ b/src/client/common/application/commands.ts @@ -72,6 +72,7 @@ export interface ICommandNameArgumentTypeMapping extends ICommandNameWithoutArgu ]; ['workbench.action.files.openFolder']: []; ['workbench.action.openWorkspace']: []; + ['workbench.action.openSettings']: [string]; ['setContext']: [string, boolean] | ['python.vscode.channel', Channel]; ['python.reloadVSCode']: [string]; ['revealLine']: [{ lineNumber: number; at: 'top' | 'center' | 'bottom' }]; diff --git a/src/client/common/configSettings.ts b/src/client/common/configSettings.ts index 3fc98702f659..8bcc3f418cd6 100644 --- a/src/client/common/configSettings.ts +++ b/src/client/common/configSettings.ts @@ -28,6 +28,7 @@ import { IInterpreterPathService, IInterpreterSettings, IPythonSettings, + IREPLSettings, ITensorBoardSettings, ITerminalSettings, Resource, @@ -111,6 +112,8 @@ export class PythonSettings implements IPythonSettings { public globalModuleInstallation = false; + public REPL!: IREPLSettings; + public experiments!: IExperiments; public languageServer: LanguageServerType = LanguageServerType.Node; @@ -360,7 +363,8 @@ export class PythonSettings implements IPythonSettings { activateEnvInCurrentTerminal: false, }; - const experiments = systemVariables.resolveAny(pythonSettings.get('experiments'))!; + this.REPL = pythonSettings.get('REPL')!; + const experiments = pythonSettings.get('experiments')!; if (this.experiments) { Object.assign(this.experiments, experiments); } else { diff --git a/src/client/common/types.ts b/src/client/common/types.ts index 742948a49652..67fcf5c7b700 100644 --- a/src/client/common/types.ts +++ b/src/client/common/types.ts @@ -178,6 +178,7 @@ export interface IPythonSettings { readonly languageServerIsDefault: boolean; readonly defaultInterpreterPath: string; readonly tensorBoard: ITensorBoardSettings | undefined; + readonly REPL: IREPLSettings; register(): void; } @@ -197,6 +198,10 @@ export interface ITerminalSettings { readonly activateEnvInCurrentTerminal: boolean; } +export interface IREPLSettings { + readonly enableREPLSmartSend: boolean; +} + export interface IExperiments { /** * Return `true` if experiments are enabled, else `false`. diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index fc9e7fff6b2f..ae4d7efa38a2 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -4,6 +4,7 @@ 'use strict'; import { l10n } from 'vscode'; +import { Commands } from '../constants'; /* eslint-disable @typescript-eslint/no-namespace, no-shadow */ @@ -42,6 +43,9 @@ export namespace Diagnostics { export const pylanceDefaultMessage = l10n.t( "The Python extension now includes Pylance to improve completions, code navigation, overall performance and much more! You can learn more about the update and learn how to change your language server [here](https://aka.ms/new-python-bundle).\n\nRead Pylance's license [here](https://marketplace.visualstudio.com/items/ms-python.vscode-pylance/license).", ); + export const invalidSmartSendMessage = l10n.t(`Python is unable to parse the code provided. Please + turn off Smart Send if you wish to always run line by line or explicitly select code + to force run. See [logs](command:${Commands.ViewOutput}) for more details`); } export namespace Common { @@ -92,6 +96,9 @@ export namespace AttachProcess { export const refreshList = l10n.t('Refresh process list'); } +export namespace Repl { + export const disableSmartSend = l10n.t('Disable Smart Send'); +} export namespace Pylance { export const remindMeLater = l10n.t('Remind me later'); diff --git a/src/client/terminals/codeExecution/djangoShellCodeExecution.ts b/src/client/terminals/codeExecution/djangoShellCodeExecution.ts index c70cd896b225..67b877429a2e 100644 --- a/src/client/terminals/codeExecution/djangoShellCodeExecution.ts +++ b/src/client/terminals/codeExecution/djangoShellCodeExecution.ts @@ -36,6 +36,7 @@ export class DjangoShellCodeExecutionProvider extends TerminalCodeExecutionProvi disposableRegistry, platformService, interpreterService, + commandManager, ); this.terminalTitle = 'Django Shell'; disposableRegistry.push(new DjangoContextInitializer(documentManager, workspace, fileSystem, commandManager)); diff --git a/src/client/terminals/codeExecution/helper.ts b/src/client/terminals/codeExecution/helper.ts index 058c78e332a3..48a435c8710a 100644 --- a/src/client/terminals/codeExecution/helper.ts +++ b/src/client/terminals/codeExecution/helper.ts @@ -6,6 +6,7 @@ import { inject, injectable } from 'inversify'; import { l10n, Position, Range, TextEditor, Uri } from 'vscode'; import { + IActiveResourceService, IApplicationShell, ICommandManager, IDocumentManager, @@ -36,6 +37,8 @@ export class CodeExecutionHelper implements ICodeExecutionHelper { private readonly commandManager: ICommandManager; + private activeResourceService: IActiveResourceService; + // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-expect-error TS6133: 'configSettings' is declared but its value is never read. private readonly configSettings: IConfigurationService; @@ -47,6 +50,7 @@ export class CodeExecutionHelper implements ICodeExecutionHelper { this.interpreterService = serviceContainer.get(IInterpreterService); this.configSettings = serviceContainer.get(IConfigurationService); this.commandManager = serviceContainer.get(ICommandManager); + this.activeResourceService = this.serviceContainer.get(IActiveResourceService); } public async normalizeLines(code: string, wholeFileContent?: string, resource?: Uri): Promise { @@ -90,6 +94,13 @@ export class CodeExecutionHelper implements ICodeExecutionHelper { const endLineVal = activeEditor?.selection?.end.line ?? 0; const emptyHighlightVal = activeEditor?.selection?.isEmpty ?? true; const smartSendExperimentEnabledVal = pythonSmartSendEnabled(this.serviceContainer); + let smartSendSettingsEnabledVal = false; + const configuration = this.serviceContainer.get(IConfigurationService); + if (configuration) { + const pythonSettings = configuration.getSettings(this.activeResourceService.getActiveResource()); + smartSendSettingsEnabledVal = pythonSettings.REPL.enableREPLSmartSend; + } + const input = JSON.stringify({ code, wholeFileContent, @@ -97,6 +108,7 @@ export class CodeExecutionHelper implements ICodeExecutionHelper { endLine: endLineVal, emptyHighlight: emptyHighlightVal, smartSendExperimentEnabled: smartSendExperimentEnabledVal, + smartSendSettingsEnabled: smartSendSettingsEnabledVal, }); observable.proc?.stdin?.write(input); observable.proc?.stdin?.end(); @@ -105,7 +117,12 @@ export class CodeExecutionHelper implements ICodeExecutionHelper { const result = await normalizeOutput.promise; const object = JSON.parse(result); - if (activeEditor?.selection) { + if ( + activeEditor?.selection && + smartSendExperimentEnabledVal && + smartSendSettingsEnabledVal && + object.normalized !== 'deprecated' + ) { const lineOffset = object.nextBlockLineno - activeEditor!.selection.start.line - 1; await this.moveToNextBlock(lineOffset, activeEditor); } diff --git a/src/client/terminals/codeExecution/repl.ts b/src/client/terminals/codeExecution/repl.ts index e3a4bc7582c2..45f19798c3d8 100644 --- a/src/client/terminals/codeExecution/repl.ts +++ b/src/client/terminals/codeExecution/repl.ts @@ -5,7 +5,7 @@ import { inject, injectable } from 'inversify'; import { Disposable } from 'vscode'; -import { IWorkspaceService } from '../../common/application/types'; +import { ICommandManager, IWorkspaceService } from '../../common/application/types'; import { IPlatformService } from '../../common/platform/types'; import { ITerminalServiceFactory } from '../../common/terminal/types'; import { IConfigurationService, IDisposableRegistry } from '../../common/types'; @@ -21,6 +21,7 @@ export class ReplProvider extends TerminalCodeExecutionProvider { @inject(IDisposableRegistry) disposableRegistry: Disposable[], @inject(IPlatformService) platformService: IPlatformService, @inject(IInterpreterService) interpreterService: IInterpreterService, + @inject(ICommandManager) commandManager: ICommandManager, ) { super( terminalServiceFactory, @@ -29,6 +30,7 @@ export class ReplProvider extends TerminalCodeExecutionProvider { disposableRegistry, platformService, interpreterService, + commandManager, ); this.terminalTitle = 'REPL'; } diff --git a/src/client/terminals/codeExecution/terminalCodeExecution.ts b/src/client/terminals/codeExecution/terminalCodeExecution.ts index 50270c3586c4..a257fff20dbf 100644 --- a/src/client/terminals/codeExecution/terminalCodeExecution.ts +++ b/src/client/terminals/codeExecution/terminalCodeExecution.ts @@ -6,15 +6,17 @@ import { inject, injectable } from 'inversify'; import * as path from 'path'; import { Disposable, Uri } from 'vscode'; -import { IWorkspaceService } from '../../common/application/types'; +import { ICommandManager, IWorkspaceService } from '../../common/application/types'; import '../../common/extensions'; import { IPlatformService } from '../../common/platform/types'; import { ITerminalService, ITerminalServiceFactory } from '../../common/terminal/types'; import { IConfigurationService, IDisposableRegistry, Resource } from '../../common/types'; +import { Diagnostics, Repl } from '../../common/utils/localize'; +import { showWarningMessage } from '../../common/vscodeApis/windowApis'; import { IInterpreterService } from '../../interpreter/contracts'; +import { traceInfo } from '../../logging'; import { buildPythonExecInfo, PythonExecInfo } from '../../pythonEnvironments/exec'; import { ICodeExecutionService } from '../../terminals/types'; - @injectable() export class TerminalCodeExecutionProvider implements ICodeExecutionService { private hasRanOutsideCurrentDrive = false; @@ -27,6 +29,7 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService { @inject(IDisposableRegistry) protected readonly disposables: Disposable[], @inject(IPlatformService) protected readonly platformService: IPlatformService, @inject(IInterpreterService) protected readonly interpreterService: IInterpreterService, + @inject(ICommandManager) protected readonly commandManager: ICommandManager, ) {} public async executeFile(file: Uri, options?: { newTerminalPerFile: boolean }) { @@ -42,9 +45,17 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService { if (!code || code.trim().length === 0) { return; } - await this.initializeRepl(resource); - await this.getTerminalService(resource).sendText(code); + if (code == 'deprecated') { + // If user is trying to smart send deprecated code show warning + const selection = await showWarningMessage(Diagnostics.invalidSmartSendMessage, Repl.disableSmartSend); + traceInfo(`Selected file contains invalid Python or Deprecated Python 2 code`); + if (selection === Repl.disableSmartSend) { + this.configurationService.updateSetting('REPL.enableREPLSmartSend', false, resource); + } + } else { + await this.getTerminalService(resource).sendText(code); + } } public async initializeRepl(resource: Resource) { const terminalService = this.getTerminalService(resource); diff --git a/src/test/pythonFiles/terminalExec/sample_invalid_smart_selection.py b/src/test/pythonFiles/terminalExec/sample_invalid_smart_selection.py new file mode 100644 index 000000000000..73d9e0fba066 --- /dev/null +++ b/src/test/pythonFiles/terminalExec/sample_invalid_smart_selection.py @@ -0,0 +1,10 @@ +def beliebig(x, y, *mehr): + print "x=", x, ", x=", y + print "mehr: ", mehr + +list = [ +1, +2, +3, +] +print("Above is invalid");print("deprecated");print("show warning") diff --git a/src/test/terminals/codeExecution/smartSend.test.ts b/src/test/terminals/codeExecution/smartSend.test.ts index 8d70ab6e01e0..01f490e2b252 100644 --- a/src/test/terminals/codeExecution/smartSend.test.ts +++ b/src/test/terminals/codeExecution/smartSend.test.ts @@ -1,22 +1,28 @@ import * as TypeMoq from 'typemoq'; import * as path from 'path'; -import { TextEditor, Selection, Position, TextDocument } from 'vscode'; +import { TextEditor, Selection, Position, TextDocument, Uri } from 'vscode'; import * as fs from 'fs-extra'; import { SemVer } from 'semver'; import { assert, expect } from 'chai'; -import { IApplicationShell, ICommandManager, IDocumentManager } from '../../../client/common/application/types'; +import { + IActiveResourceService, + IApplicationShell, + ICommandManager, + IDocumentManager, +} from '../../../client/common/application/types'; import { IProcessService, IProcessServiceFactory } from '../../../client/common/process/types'; import { IInterpreterService } from '../../../client/interpreter/contracts'; -import { IConfigurationService, IExperimentService } from '../../../client/common/types'; +import { IConfigurationService, IExperimentService, IPythonSettings } from '../../../client/common/types'; import { CodeExecutionHelper } from '../../../client/terminals/codeExecution/helper'; import { IServiceContainer } from '../../../client/ioc/types'; import { ICodeExecutionHelper } from '../../../client/terminals/types'; import { EnableREPLSmartSend } from '../../../client/common/experiments/groups'; -import { EXTENSION_ROOT_DIR } from '../../../client/common/constants'; +import { Commands, EXTENSION_ROOT_DIR } from '../../../client/common/constants'; import { EnvironmentType, PythonEnvironment } from '../../../client/pythonEnvironments/info'; import { PYTHON_PATH } from '../../common'; import { Architecture } from '../../../client/common/utils/platform'; import { ProcessService } from '../../../client/common/process/proc'; +import { l10n } from '../../mocks/vsc'; const TEST_FILES_PATH = path.join(EXTENSION_ROOT_DIR, 'src', 'test', 'pythonFiles', 'terminalExec'); @@ -35,8 +41,11 @@ suite('REPL - Smart Send', () => { let experimentService: TypeMoq.IMock; let processService: TypeMoq.IMock; + let activeResourceService: TypeMoq.IMock; let document: TypeMoq.IMock; + let pythonSettings: TypeMoq.IMock; + const workingPython: PythonEnvironment = { path: PYTHON_PATH, version: new SemVer('3.6.6-final'), @@ -64,7 +73,9 @@ suite('REPL - Smart Send', () => { serviceContainer = TypeMoq.Mock.ofType(); experimentService = TypeMoq.Mock.ofType(); processService = TypeMoq.Mock.ofType(); - + activeResourceService = TypeMoq.Mock.ofType(); + pythonSettings = TypeMoq.Mock.ofType(); + const resource = Uri.parse('a'); // eslint-disable-next-line @typescript-eslint/no-explicit-any processService.setup((x: any) => x.then).returns(() => undefined); serviceContainer @@ -92,6 +103,14 @@ suite('REPL - Smart Send', () => { interpreterService .setup((i) => i.getActiveInterpreter(TypeMoq.It.isAny())) .returns(() => Promise.resolve(workingPython)); + serviceContainer + .setup((c) => c.get(TypeMoq.It.isValue(IActiveResourceService))) + .returns(() => activeResourceService.object); + activeResourceService.setup((a) => a.getActiveResource()).returns(() => resource); + + pythonSettings.setup((s) => s.REPL).returns(() => ({ enableREPLSmartSend: true, REPLSmartSend: true })); + + configurationService.setup((x) => x.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); codeExecutionHelper = new CodeExecutionHelper(serviceContainer.object); document = TypeMoq.Mock.ofType(); @@ -149,6 +168,16 @@ suite('REPL - Smart Send', () => { .setup((exp) => exp.inExperimentSync(TypeMoq.It.isValue(EnableREPLSmartSend.experiment))) .returns(() => true); + configurationService + .setup((c) => c.getSettings(TypeMoq.It.isAny())) + .returns({ + REPL: { + EnableREPLSmartSend: true, + REPLSmartSend: true, + }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + const activeEditor = TypeMoq.Mock.ofType(); const firstIndexPosition = new Position(0, 0); const selection = TypeMoq.Mock.ofType(); @@ -226,4 +255,58 @@ suite('REPL - Smart Send', () => { const expectedNonSmartResult = 'my_dict = {\n\n'; // Standard for previous normalization logic expect(actualNonSmartResult).to.be.equal(expectedNonSmartResult); }); + + test('Smart Send should provide warning when code is not valid', async () => { + experimentService + .setup((exp) => exp.inExperimentSync(TypeMoq.It.isValue(EnableREPLSmartSend.experiment))) + .returns(() => true); + + configurationService + .setup((c) => c.getSettings(TypeMoq.It.isAny())) + .returns({ + REPL: { + EnableREPLSmartSend: true, + REPLSmartSend: true, + }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + + const activeEditor = TypeMoq.Mock.ofType(); + const firstIndexPosition = new Position(0, 0); + const selection = TypeMoq.Mock.ofType(); + const wholeFileContent = await fs.readFile( + path.join(TEST_FILES_PATH, `sample_invalid_smart_selection.py`), + 'utf8', + ); + + selection.setup((s) => s.anchor).returns(() => firstIndexPosition); + selection.setup((s) => s.active).returns(() => firstIndexPosition); + selection.setup((s) => s.isEmpty).returns(() => true); + activeEditor.setup((e) => e.selection).returns(() => selection.object); + + documentManager.setup((d) => d.activeTextEditor).returns(() => activeEditor.object); + document.setup((d) => d.getText(TypeMoq.It.isAny())).returns(() => wholeFileContent); + const actualProcessService = new ProcessService(); + + const { execObservable } = actualProcessService; + + processService + .setup((p) => p.execObservable(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns((file, args, options) => execObservable.apply(actualProcessService, [file, args, options])); + + await codeExecutionHelper.normalizeLines('my_dict = {', wholeFileContent); + + applicationShell + .setup((a) => + a.showWarningMessage( + l10n.t( + `Python is unable to parse the code provided. Please + turn off Smart Send if you wish to always run line by line or explicitly select code + to force run. [logs](command:${Commands.ViewOutput}) for more details.`, + ), + 'Switch to line-by-line', + ), + ) + .verifiable(TypeMoq.Times.once()); + }); }); diff --git a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts index 9523f35a1040..93845e6189eb 100644 --- a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts +++ b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts @@ -84,6 +84,7 @@ suite('Terminal - Code Execution', () => { disposables, platform.object, interpreterService.object, + commandManager.object, ); break; } @@ -95,6 +96,7 @@ suite('Terminal - Code Execution', () => { disposables, platform.object, interpreterService.object, + commandManager.object, ); expectedTerminalTitle = 'REPL'; break;