diff --git a/news/2 Fixes/6813.md b/news/2 Fixes/6813.md new file mode 100644 index 000000000000..4c813edd73f6 --- /dev/null +++ b/news/2 Fixes/6813.md @@ -0,0 +1,2 @@ +Drop dedent-on-enter for "return" statements. It will be addressed in: + https://github.com/microsoft/vscode-python/issues/6564 diff --git a/src/client/common/utils/regexp.ts b/src/client/common/utils/regexp.ts new file mode 100644 index 000000000000..2d20b73e7f28 --- /dev/null +++ b/src/client/common/utils/regexp.ts @@ -0,0 +1,22 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +/* Generate a RegExp from a "verbose" pattern. + * + * All whitespace in the pattern is removed, including newlines. This + * allows the pattern to be much more readable by allowing it to span + * multiple lines and to separate tokens with insignificant whitespace. + * The functionality is similar to the VERBOSE ("x") flag in Python's + * regular expressions. + * + * Note that significant whitespace in the pattern must be explicitly + * indicated by "\s". Also, unlike with regular expression literals, + * backslashes must be escaped. Conversely, forward slashes do not + * need to be escaped. + */ +export function verboseRegExp(pattern: string): RegExp { + pattern = pattern.replace(/\s+?/g, ''); + return RegExp(pattern); +} diff --git a/src/client/extension.ts b/src/client/extension.ts index db56a5b24c66..67a6f9dd9507 100644 --- a/src/client/extension.ts +++ b/src/client/extension.ts @@ -40,7 +40,7 @@ import { registerTypes as appRegisterTypes } from './application/serviceRegistry import { IApplicationDiagnostics } from './application/types'; import { DebugService } from './common/application/debugService'; import { IApplicationShell, ICommandManager, IWorkspaceService } from './common/application/types'; -import { Commands, isTestExecution, PYTHON, STANDARD_OUTPUT_CHANNEL } from './common/constants'; +import { Commands, isTestExecution, PYTHON, PYTHON_LANGUAGE, STANDARD_OUTPUT_CHANNEL } from './common/constants'; import { registerTypes as registerDotNetTypes } from './common/dotnet/serviceRegistry'; import { registerTypes as installerRegisterTypes } from './common/installer/serviceRegistry'; import { traceError } from './common/logger'; @@ -85,7 +85,7 @@ import { registerTypes as interpretersRegisterTypes } from './interpreter/servic import { ServiceContainer } from './ioc/container'; import { ServiceManager } from './ioc/serviceManager'; import { IServiceContainer, IServiceManager } from './ioc/types'; -import { setLanguageConfiguration } from './language/languageConfiguration'; +import { getLanguageConfiguration } from './language/languageConfiguration'; import { LinterCommands } from './linters/linterCommands'; import { registerTypes as lintersRegisterTypes } from './linters/serviceRegistry'; import { ILintingEngine } from './linters/types'; @@ -176,7 +176,7 @@ async function activateUnsafe(context: ExtensionContext): Promise const linterProvider = new LinterProvider(context, serviceManager); context.subscriptions.push(linterProvider); - setLanguageConfiguration(); + languages.setLanguageConfiguration(PYTHON_LANGUAGE, getLanguageConfiguration()); if (pythonSettings && pythonSettings.formatting && pythonSettings.formatting.provider !== 'internalConsole') { const formatProvider = new PythonFormattingEditProvider(context, serviceContainer); diff --git a/src/client/language/languageConfiguration.ts b/src/client/language/languageConfiguration.ts index 19eebd8d0131..409371555210 100644 --- a/src/client/language/languageConfiguration.ts +++ b/src/client/language/languageConfiguration.ts @@ -2,33 +2,111 @@ // Licensed under the MIT License. 'use strict'; -import { IndentAction, languages } from 'vscode'; -import { PYTHON_LANGUAGE } from '../common/constants'; +import { IndentAction, LanguageConfiguration } from 'vscode'; +import { verboseRegExp } from '../common/utils/regexp'; -export const MULTILINE_SEPARATOR_INDENT_REGEX = /^(?!\s+\\)[^#\n]+\\$/; +// tslint:disable:no-multiline-string -export function setLanguageConfiguration() { - // Enable indentAction - languages.setLanguageConfiguration(PYTHON_LANGUAGE, { +// tslint:disable-next-line:max-func-body-length +export function getLanguageConfiguration(): LanguageConfiguration { + return { onEnterRules: [ + // multi-line separator { - beforeText: /^\s*(?:def|class|for|if|elif|else|while|try|with|finally|except|async)\b.*:\s*/, - action: { indentAction: IndentAction.Indent } - }, - { - beforeText: MULTILINE_SEPARATOR_INDENT_REGEX, - action: { indentAction: IndentAction.Indent } + beforeText: verboseRegExp(` + ^ + (?! \\s+ \\\\ ) + [^#\n]+ + \\\\ + $ + `), + action: { + indentAction: IndentAction.Indent + } }, + // continue comments { beforeText: /^\s*#.*/, afterText: /.+$/, - action: { indentAction: IndentAction.None, appendText: '# ' } + action: { + indentAction: IndentAction.None, + appendText: '# ' + } + }, + // indent on enter (block-beginning statements) + { + /** + * This does not handle all cases. However, it does handle nearly all usage. + * Here's what it does not cover: + * - the statement is split over multiple lines (and hence the ":" is on a different line) + * - the code block is inlined (after the ":") + * - there are multiple statements on the line (separated by semicolons) + * Also note that `lambda` is purposefully excluded. + */ + beforeText: verboseRegExp(` + ^ + \\s* + (?: + (?: + (?: + class | + def | + async \\s+ def | + except | + for | + if | + elif | + while | + with + ) + \\b .* + ) | + else | + try | + finally + ) + \\s* + [:] + \\s* + (?: [#] .* )? + $ + `), + action: { + indentAction: IndentAction.Indent + } }, + // outdent on enter (block-ending statements) { - beforeText: /^\s+(continue|break|return)\b.*/, - afterText: /\s+$/, - action: { indentAction: IndentAction.Outdent } + beforeText: verboseRegExp(` + ^ + (?: + (?: + \\s* + (?: + pass | + raise \\s+ [^#\\s] [^#]* + ) + ) | + (?: + \\s+ + (?: + raise | + break | + continue + ) + ) + ) + \\s* + (?: [#] .* )? + $ + `), + action: { + indentAction: IndentAction.Outdent + } } + // Note that we do not currently have an auto-dedent + // solution for "elif", "else", "except", and "finally". + // We had one but had to remove it (see issue #6886). ] - }); + }; } diff --git a/src/test/common/utils/regexp.unit.test.ts b/src/test/common/utils/regexp.unit.test.ts new file mode 100644 index 000000000000..9e8c8049bec6 --- /dev/null +++ b/src/test/common/utils/regexp.unit.test.ts @@ -0,0 +1,75 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +// tslint:disable:no-multiline-string + +import { expect } from 'chai'; + +import { + verboseRegExp +} from '../../../client/common/utils/regexp'; + +suite('Utils for regular expressions - verboseRegExp()', () => { + test('whitespace removed in multiline pattern (example of typical usage)', () => { + const regex = verboseRegExp(` + ^ + (?: + spam \\b .* + ) | + (?: + eggs \\b .* + ) + $ + `); + + expect(regex.source).to.equal('^(?:spam\\b.*)|(?:eggs\\b.*)$', 'mismatch'); + }); + + const whitespaceTests = [ + ['spam eggs', 'spameggs'], + [`spam + eggs`, 'spameggs'], + // empty + [' ', '(?:)'], + [` + `, '(?:)'] + ]; + for (const [pat, expected] of whitespaceTests) { + test(`whitespace removed ("${pat}")`, () => { + const regex = verboseRegExp(pat); + + expect(regex.source).to.equal(expected, 'mismatch'); + }); + } + + const noopPatterns = [ + '^(?:spam\\b.*)$', + 'spam', + '^spam$', + 'spam$', + '^spam' + ]; + for (const pat of noopPatterns) { + test(`pattern not changed ("${pat}")`, () => { + const regex = verboseRegExp(pat); + + expect(regex.source).to.equal(pat, 'mismatch'); + }); + } + + const emptyPatterns = [ + '', + ` + `, + ' ' + ]; + for (const pat of emptyPatterns) { + test(`no pattern ("${pat}")`, () => { + const regex = verboseRegExp(pat); + + expect(regex.source).to.equal('(?:)', 'mismatch'); + }); + } +}); diff --git a/src/test/language/languageConfiguration.unit.test.ts b/src/test/language/languageConfiguration.unit.test.ts index 1356995cfdf4..00ec4cd9a353 100644 --- a/src/test/language/languageConfiguration.unit.test.ts +++ b/src/test/language/languageConfiguration.unit.test.ts @@ -3,21 +3,253 @@ 'use strict'; +// tslint:disable:max-func-body-length + import { expect } from 'chai'; -import { MULTILINE_SEPARATOR_INDENT_REGEX } from '../../client/language/languageConfiguration'; +import { + getLanguageConfiguration +} from '../../client/language/languageConfiguration'; + +const NEEDS_INDENT = [ + /^break$/, + /^continue$/, + /^raise$/, // only re-raise + /^return\b/ +]; +const INDENT_ON_ENTER = [ // block-beginning statements + /^async\s+def\b/, + /^class\b/, + /^def\b/, + /^with\b/, + /^try\b/, + /^except\b/, + /^finally\b/, + /^while\b/, + /^for\b/, + /^if\b/, + /^elif\b/, + /^else\b/ +]; +const DEDENT_ON_ENTER = [ // block-ending statements + // For now we are ignoring "return" completely. See gh-6564. + ///^return\b/, + /^break$/, + /^continue$/, + /^raise\b/, + /^pass\b/ +]; + +function isMember(line: string, regexes: RegExp[]): boolean { + for (const regex of regexes) { + if (regex.test(line)) { + return true; + } + } + return false; +} + +function resolveExample( + base: string, + leading: string, + postKeyword: string, + preColon: string, + trailing: string +): [string | undefined, string | undefined, boolean] { + let invalid: string | undefined; + if (base.trim() === '') { + invalid = 'blank line'; + } else if (leading === '' && isMember(base, NEEDS_INDENT)) { + invalid = 'expected indent'; + } else if (leading.trim() !== '') { + invalid = 'look-alike - pre-keyword'; + } else if (postKeyword.trim() !== '') { + invalid = 'look-alike - post-keyword'; + } -suite('Language configuration regexes', () => { - test('Multiline separator indent regex should not pick up strings with no multiline separator', async () => { - const result = MULTILINE_SEPARATOR_INDENT_REGEX.test('a = "test"'); - expect (result).to.be.equal(false, 'Multiline separator indent regex for regular strings should not have matches'); + let resolvedBase = base; + if (postKeyword !== '') { + if (resolvedBase.includes(' ')) { + const kw = resolvedBase.split(' ', 1)[0]; + const remainder = resolvedBase.substring(kw.length); + resolvedBase = `${kw}${postKeyword} ${remainder}`; + } else { + if (resolvedBase.endsWith(':')) { + resolvedBase = `${resolvedBase.substring(0, resolvedBase.length - 1)}${postKeyword}:`; + } else { + resolvedBase = `${resolvedBase}${postKeyword}`; + } + } + } + if (preColon !== '') { + if (resolvedBase.endsWith(':')) { + resolvedBase = `${resolvedBase.substring(0, resolvedBase.length - 1)}${preColon}:`; + } else { + return [undefined, undefined, true]; + } + } + const example = `${leading}${resolvedBase}${trailing}`; + return [example, invalid, false]; +} + +suite('Language Configuration', () => { + const cfg = getLanguageConfiguration(); + + suite('"brackets"', () => { + test('brackets is not defined', () => { + expect(cfg.brackets).to.be.equal(undefined, 'missing tests'); + }); }); - test('Multiline separator indent regex should not pick up strings with escaped characters', async () => { - const result = MULTILINE_SEPARATOR_INDENT_REGEX.test('a = \'hello \\n\''); - expect (result).to.be.equal(false, 'Multiline separator indent regex for strings with escaped characters should not have matches'); + + suite('"comments"', () => { + test('comments is not defined', () => { + expect(cfg.comments).to.be.equal(undefined, 'missing tests'); + }); }); - test('Multiline separator indent regex should pick up strings ending with a multiline separator', async () => { - const result = MULTILINE_SEPARATOR_INDENT_REGEX.test('a = \'multiline \\'); - expect (result).to.be.equal(true, 'Multiline separator indent regex for strings with newline separator should have matches'); + + suite('"indentationRules"', () => { + test('indentationRules is not defined', () => { + expect(cfg.indentationRules).to.be.equal(undefined, 'missing tests'); + }); + }); + + suite('"onEnterRules"', () => { + const MULTILINE_SEPARATOR_INDENT_REGEX = cfg.onEnterRules![0].beforeText; + const INDENT_ONENTER_REGEX = cfg.onEnterRules![2].beforeText; + const OUTDENT_ONENTER_REGEX = cfg.onEnterRules![3].beforeText; + // To see the actual (non-verbose) regex patterns, un-comment + // the following lines: + //console.log(INDENT_ONENTER_REGEX.source); + //console.log(OUTDENT_ONENTER_REGEX.source); + + test('Multiline separator indent regex should not pick up strings with no multiline separator', async () => { + const result = MULTILINE_SEPARATOR_INDENT_REGEX.test('a = "test"'); + expect(result).to.be.equal(false, 'Multiline separator indent regex for regular strings should not have matches'); + }); + + test('Multiline separator indent regex should not pick up strings with escaped characters', async () => { + const result = MULTILINE_SEPARATOR_INDENT_REGEX.test('a = \'hello \\n\''); + expect(result).to.be.equal(false, 'Multiline separator indent regex for strings with escaped characters should not have matches'); + }); + + test('Multiline separator indent regex should pick up strings ending with a multiline separator', async () => { + const result = MULTILINE_SEPARATOR_INDENT_REGEX.test('a = \'multiline \\'); + expect(result).to.be.equal(true, 'Multiline separator indent regex for strings with newline separator should have matches'); + }); + + [ + // compound statements + 'async def test(self):', + 'async def :', + 'async :', + 'class Test:', + 'class Test(object):', + 'class :', + 'def spam():', + 'def spam(self, node, namespace=""):', + 'def :', + 'for item in items:', + 'for item in :', + 'for :', + 'if foo is None:', + 'if :', + 'try:', + 'while \'::\' in macaddress:', + 'while :', + 'with self.test:', + 'with :', + 'elif x < 5:', + 'elif :', + 'else:', + 'except TestError:', + 'except :', + 'finally:', + // simple statemenhts + 'pass', + 'raise Exception(msg)', + 'raise Exception', + 'raise', // re-raise + 'break', + 'continue', + 'return', + 'return True', + 'return (True, False, False)', + 'return [True, False, False]', + 'return {True, False, False}', + 'return (', + 'return [', + 'return {', + 'return', + // bogus + '', + ' ', + ' ' + ].forEach(base => { + [ + ['', '', '', ''], + // leading + [' ', '', '', ''], + [' ', '', '', ''], // unusual indent + ['\t\t', '', '', ''], + // pre-keyword + ['x', '', '', ''], + // post-keyword + ['', 'x', '', ''], + // pre-colon + ['', '', ' ', ''], + // trailing + ['', '', '', ' '], + ['', '', '', '# a comment'], + ['', '', '', ' # ...'] + ].forEach(whitespace => { + const [leading, postKeyword, preColon, trailing] = whitespace; + const [_example, invalid, ignored] = resolveExample(base, leading, postKeyword, preColon, trailing); + if (ignored) { + return; + } + const example = _example!; + + if (invalid) { + test(`Line "${example}" ignored (${invalid})`, () => { + let result: boolean; + + result = INDENT_ONENTER_REGEX.test(example); + expect(result).to.be.equal(false, 'unexpected match'); + + result = OUTDENT_ONENTER_REGEX.test(example); + expect(result).to.be.equal(false, 'unexpected match'); + }); + return; + } + + test(`Check indent-on-enter for line "${example}"`, () => { + let expected = false; + if (isMember(base, INDENT_ON_ENTER)) { + expected = true; + } + + const result = INDENT_ONENTER_REGEX.test(example); + + expect(result).to.be.equal(expected, 'unexpected result'); + }); + + test(`Check dedent-on-enter for line "${example}"`, () => { + let expected = false; + if (isMember(base, DEDENT_ON_ENTER)) { + expected = true; + } + + const result = OUTDENT_ONENTER_REGEX.test(example); + + expect(result).to.be.equal(expected, 'unexpected result'); + }); + }); + }); + }); + + suite('"wordPattern"', () => { + test('wordPattern is not defined', () => { + expect(cfg.wordPattern).to.be.equal(undefined, 'missing tests'); + }); }); }); diff --git a/src/test/mocks/vsc/index.ts b/src/test/mocks/vsc/index.ts index d9586c4a4d35..64cbc6721eb9 100644 --- a/src/test/mocks/vsc/index.ts +++ b/src/test/mocks/vsc/index.ts @@ -172,6 +172,12 @@ export namespace vscMock { Operator = 24, TypeParameter = 25 } + export enum IndentAction { + None = 0, + Indent = 1, + IndentOutdent = 2, + Outdent = 3 + } export class CodeActionKind { public static readonly Empty: CodeActionKind = new CodeActionKind('empty'); diff --git a/src/test/vscode-mock.ts b/src/test/vscode-mock.ts index cd99771644b4..647142bdaace 100644 --- a/src/test/vscode-mock.ts +++ b/src/test/vscode-mock.ts @@ -54,6 +54,7 @@ mockedVSCode.EventEmitter = vscodeMocks.vscMock.EventEmitter; mockedVSCode.CancellationTokenSource = vscodeMocks.vscMock.CancellationTokenSource; mockedVSCode.CompletionItemKind = vscodeMocks.vscMock.CompletionItemKind; mockedVSCode.SymbolKind = vscodeMocks.vscMock.SymbolKind; +mockedVSCode.IndentAction = vscodeMocks.vscMock.IndentAction; mockedVSCode.Uri = vscodeMocks.vscMock.Uri as any; mockedVSCode.Range = vscodeMocks.vscMockExtHostedTypes.Range; mockedVSCode.Position = vscodeMocks.vscMockExtHostedTypes.Position;