From d27f00cc5598deef42152253be59b14928c7042f Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 1 Jun 2018 16:09:28 -0700 Subject: [PATCH] Improve detection if '=' is in function or lambda arguments (#1824) * Undo changes * Test fixes * Increase timeout * Remove double event listening * Remove test * Revert "Remove test" This reverts commit e240c3fd117c38b9e6fdcbdd1ba2715789fefe48. * Revert "Remove double event listening" This reverts commit af573be27372a79d5589e2134002cc753bb54f2a. * #1096 The if statement is automatically formatted incorrectly * Merge fix * Add more tests * More tests * Typo * Test * Also better handle multiline arguments * Add a couple missing periods [skip ci] * Undo changes * Test fixes * Increase timeout * Remove double event listening * Remove test * Revert "Remove test" This reverts commit e240c3fd117c38b9e6fdcbdd1ba2715789fefe48. * Revert "Remove double event listening" This reverts commit af573be27372a79d5589e2134002cc753bb54f2a. * Merge fix * #1257 On type formatting errors for args and kwargs * Handle f-strings * Stop importing from test code * #1308 Single line statements leading to an indentation on the next line * #726 editing python after inline if statement invalid indent * Undo change * Move constant * Harden LS startup error checks * #1364 Intellisense doesn't work after specific const string * Telemetry for the analysis enging * PR feedback * Fix typo * Test baseline update * Jedi 0.12 * Priority to goto_defition * News * Replace unzip * Linux flavors + test * Grammar check * Grammar test * Test baselines * Add news * Pin dependency [skip ci] * Specify markdown as preferable format * Improve function argument detection * Specify markdown * Pythia setting * Baseline updates * Baseline update * Improve startup * Handle missing interpreter better * Handle interpreter change * Delete old file * Fix LS startup time reporting * Remove Async suffix from IFileSystem * Remove Pythia * Remove pre-packaged MSIL * Exe name on Unix * Plain linux * Fix casing * Fix message * Update PTVS engine activation steps * Type formatter eats space in from . * fIX CASING * Remove flag * Don't wait for LS * Small test fixes * Update hover baselines * Rename the engine * Formatting 1 * Add support for 'rf' strings * Add two spaces before comment per PEP * Fix @ operator spacing * Handle module and unary ops * Type hints * Fix typo * Trailing comma * Require space after if * underscore numbers * Update list of keywords * Function arguments * Function arguments * PR feedback * Handle lambdas * News --- news/2 Fixes/1796.md | 1 + src/client/formatters/lineFormatter.ts | 157 +++++++++++++----- .../format/extension.lineFormatter.test.ts | 144 +++++++++------- 3 files changed, 202 insertions(+), 100 deletions(-) create mode 100644 news/2 Fixes/1796.md diff --git a/news/2 Fixes/1796.md b/news/2 Fixes/1796.md new file mode 100644 index 000000000000..41751c65168a --- /dev/null +++ b/news/2 Fixes/1796.md @@ -0,0 +1 @@ +`editor.formatOnType` now better handles multiline function arguments diff --git a/src/client/formatters/lineFormatter.ts b/src/client/formatters/lineFormatter.ts index b7a6a13aa29b..4a5142d873ce 100644 --- a/src/client/formatters/lineFormatter.ts +++ b/src/client/formatters/lineFormatter.ts @@ -3,7 +3,7 @@ // tslint:disable-next-line:import-name import Char from 'typescript-char'; -import { TextDocument } from 'vscode'; +import { Position, Range, TextDocument } from 'vscode'; import { BraceCounter } from '../language/braceCounter'; import { TextBuilder } from '../language/textBuilder'; import { TextRangeCollection } from '../language/textRangeCollection'; @@ -255,13 +255,11 @@ export class LineFormatter { // tslint:disable-next-line:cyclomatic-complexity private isEqualsInsideArguments(index: number): boolean { - // Since we don't have complete statement, this is mostly heuristics. - // Therefore the code may not be handling all possible ways of the - // argument list continuation. if (index < 1) { return false; } + // We are looking for IDENT = ? const prev = this.tokens.getItemAt(index - 1); if (prev.type !== TokenType.Identifier) { return false; @@ -271,41 +269,7 @@ export class LineFormatter { return false; // Type hint should have spaces around like foo(x: int = 1) per PEP 8 } - const first = this.tokens.getItemAt(0); - if (first.type === TokenType.Comma) { - return true; // Line starts with commma - } - - const last = this.tokens.getItemAt(this.tokens.count - 1); - if (last.type === TokenType.Comma) { - return true; // Line ends in comma - } - - if (last.type === TokenType.Comment && this.tokens.count > 1 && this.tokens.getItemAt(this.tokens.count - 2).type === TokenType.Comma) { - return true; // Line ends in comma and then comment - } - - if (this.document) { - const prevLine = this.lineNumber > 0 ? this.document.lineAt(this.lineNumber - 1).text : ''; - const prevLineTokens = new Tokenizer().tokenize(prevLine); - if (prevLineTokens.count > 0) { - const lastOnPrevLine = prevLineTokens.getItemAt(prevLineTokens.count - 1); - if (lastOnPrevLine.type === TokenType.Comma) { - return true; // Previous line ends in comma - } - if (lastOnPrevLine.type === TokenType.Comment && prevLineTokens.count > 1 && prevLineTokens.getItemAt(prevLineTokens.count - 2).type === TokenType.Comma) { - return true; // Previous line ends in comma and then comment - } - } - } - - for (let i = 0; i < index; i += 1) { - const t = this.tokens.getItemAt(i); - if (this.isKeyword(t, 'lambda')) { - return true; - } - } - return this.braceCounter.isOpened(TokenType.OpenBrace); + return this.isInsideFunctionArguments(this.tokens.getItemAt(index).start); } private isOpenBraceType(type: TokenType): boolean { @@ -317,6 +281,7 @@ export class LineFormatter { private isBraceType(type: TokenType): boolean { return this.isOpenBraceType(type) || this.isCloseBraceType(type); } + private isMultipleStatements(index: number): boolean { for (let i = index; i >= 0; i -= 1) { if (this.tokens.getItemAt(i).type === TokenType.Semicolon) { @@ -332,4 +297,118 @@ export class LineFormatter { private isKeyword(t: IToken, keyword: string): boolean { return t.type === TokenType.Identifier && t.length === keyword.length && this.text.substr(t.start, t.length) === keyword; } + + // tslint:disable-next-line:cyclomatic-complexity + private isInsideFunctionArguments(position: number): boolean { + if (!this.document) { + return false; // unable to determine + } + + // Walk up until beginning of the document or line with 'def IDENT(' or line ending with : + // IDENT( by itself is not reliable since they can be nested in IDENT(IDENT(a), x=1) + let start = new Position(0, 0); + for (let i = this.lineNumber; i >= 0; i -= 1) { + const line = this.document.lineAt(i); + const lineTokens = new Tokenizer().tokenize(line.text); + if (lineTokens.count === 0) { + continue; + } + // 'def IDENT(' + const first = lineTokens.getItemAt(0); + if (lineTokens.count >= 3 && + first.length === 3 && line.text.substr(first.start, first.length) === 'def' && + lineTokens.getItemAt(1).type === TokenType.Identifier && + lineTokens.getItemAt(2).type === TokenType.OpenBrace) { + start = line.range.start; + break; + } + + if (lineTokens.count > 0 && i < this.lineNumber) { + // One of previous lines ends with : + const last = lineTokens.getItemAt(lineTokens.count - 1); + if (last.type === TokenType.Colon) { + start = this.document.lineAt(i + 1).range.start; + break; + } else if (lineTokens.count > 1) { + const beforeLast = lineTokens.getItemAt(lineTokens.count - 2); + if (beforeLast.type === TokenType.Colon && last.type === TokenType.Comment) { + start = this.document.lineAt(i + 1).range.start; + break; + } + } + } + } + + // Now tokenize from the nearest reasonable point + const currentLine = this.document.lineAt(this.lineNumber); + const text = this.document.getText(new Range(start, currentLine.range.end)); + const tokens = new Tokenizer().tokenize(text); + + // Translate position in the line being formatted to the position in the tokenized block + position = this.document.offsetAt(currentLine.range.start) + position - this.document.offsetAt(start); + + // Walk tokens locating narrowest function signature as in IDENT( | ) + let funcCallStartIndex = -1; + let funcCallEndIndex = -1; + for (let i = 0; i < tokens.count - 1; i += 1) { + const t = tokens.getItemAt(i); + if (t.type === TokenType.Identifier) { + const next = tokens.getItemAt(i + 1); + if (next.type === TokenType.OpenBrace && !this.isKeywordWithSpaceBeforeBrace(text.substr(t.start, t.length))) { + // We are at IDENT(, try and locate the closing brace + let closeBraceIndex = this.findClosingBrace(tokens, i + 1); + // Closing brace is not required in case construct is not yet terminated + closeBraceIndex = closeBraceIndex > 0 ? closeBraceIndex : tokens.count - 1; + // Are we in range? + if (position > next.start && position < tokens.getItemAt(closeBraceIndex).start) { + funcCallStartIndex = i; + funcCallEndIndex = closeBraceIndex; + } + } + } + } + // Did we find anything? + if (funcCallStartIndex < 0) { + // No? See if we are between 'lambda' and ':' + for (let i = 0; i < tokens.count; i += 1) { + const t = tokens.getItemAt(i); + if (t.type === TokenType.Identifier && text.substr(t.start, t.length) === 'lambda') { + if (position < t.start) { + break; // Position is before the nearest 'lambda' + } + let colonIndex = this.findNearestColon(tokens, i + 1); + // Closing : is not required in case construct is not yet terminated + colonIndex = colonIndex > 0 ? colonIndex : tokens.count - 1; + if (position > t.start && position < tokens.getItemAt(colonIndex).start) { + funcCallStartIndex = i; + funcCallEndIndex = colonIndex; + } + } + } + } + return funcCallStartIndex >= 0 && funcCallEndIndex > 0; + } + + private findNearestColon(tokens: ITextRangeCollection, index: number): number { + for (let i = index; i < tokens.count; i += 1) { + if (tokens.getItemAt(i).type === TokenType.Colon) { + return i; + } + } + return -1; + } + + private findClosingBrace(tokens: ITextRangeCollection, index: number): number { + const braceCounter = new BraceCounter(); + for (let i = index; i < tokens.count; i += 1) { + const t = tokens.getItemAt(i); + if (t.type === TokenType.OpenBrace || t.type === TokenType.CloseBrace) { + braceCounter.countBrace(t); + } + if (braceCounter.count === 0) { + return i; + } + } + return -1; + } } diff --git a/src/test/format/extension.lineFormatter.test.ts b/src/test/format/extension.lineFormatter.test.ts index acb86d2c5715..2332aa52d7d8 100644 --- a/src/test/format/extension.lineFormatter.test.ts +++ b/src/test/format/extension.lineFormatter.test.ts @@ -6,7 +6,7 @@ import * as assert from 'assert'; import * as fs from 'fs'; import * as path from 'path'; import * as TypeMoq from 'typemoq'; -import { TextDocument, TextLine } from 'vscode'; +import { Position, Range, TextDocument, TextLine } from 'vscode'; import '../../client/common/extensions'; import { LineFormatter } from '../../client/formatters/lineFormatter'; @@ -22,15 +22,10 @@ suite('Formatting - line formatter', () => { testFormatLine('( x +1 )*y/ 3', '(x + 1) * y / 3'); }); test('Braces spacing', () => { - testFormatLine('foo =(0 ,)', 'foo = (0,)'); - }); - test('Function arguments', () => { - testFormatLine('z=foo (0 , x= 1, (3+7) , y , z )', - 'z = foo(0, x=1, (3 + 7), y, z)'); + testFormatMultiline('foo =(0 ,)', 0, 'foo = (0,)'); }); test('Colon regular', () => { - testFormatLine('if x == 4 : print x,y; x,y= y, x', - 'if x == 4: print x, y; x, y = y, x'); + testFormatMultiline('if x == 4 : print x,y; x,y= y, x', 0, 'if x == 4: print x, y; x, y = y, x'); }); test('Colon slices', () => { testFormatLine('x[1: 30]', 'x[1:30]'); @@ -52,14 +47,13 @@ suite('Formatting - line formatter', () => { 'ham[:upper_fn(x):step_fn(x)], ham[::step_fn(x)]'); }); test('Colon in for loop', () => { - testFormatLine('for index in range( len(fruits) ): ', - 'for index in range(len(fruits)):'); + testFormatLine('for index in range( len(fruits) ): ', 'for index in range(len(fruits)):'); }); test('Nested braces', () => { testFormatLine('[ 1 :[2: (x,),y]]{1}', '[1:[2:(x,), y]]{1}'); }); test('Trailing comment', () => { - testFormatLine('x=1 # comment', 'x = 1 # comment'); + testFormatMultiline('x=1 # comment', 0, 'x = 1 # comment'); }); test('Single comment', () => { testFormatLine('# comment', '# comment'); @@ -67,21 +61,6 @@ suite('Formatting - line formatter', () => { test('Comment with leading whitespace', () => { testFormatLine(' # comment', ' # comment'); }); - test('Equals in first argument', () => { - testFormatLine('foo(x =0)', 'foo(x=0)'); - }); - test('Equals in second argument', () => { - testFormatLine('foo(x,y= \"a\",', 'foo(x, y=\"a\",'); - }); - test('Equals in multiline arguments', () => { - testFormatLine2('foo(a,', 'x = 1,y =-2)', 'x=1, y=-2)'); - }); - test('Equals in multiline arguments starting comma', () => { - testFormatLine(',x = 1,y =m)', ', x=1, y=m)'); - }); - test('Equals in multiline arguments ending comma', () => { - testFormatLine('x = 1,y =m,', 'x=1, y=m,'); - }); test('Operators without following space', () => { testFormatLine('foo( *a, ** b, ! c)', 'foo(*a, **b, !c)'); }); @@ -109,13 +88,13 @@ suite('Formatting - line formatter', () => { testFormatLine('lambda * args, :0', 'lambda *args,: 0'); }); test('Comma expression', () => { - testFormatLine('x=1,2,3', 'x = 1, 2, 3'); + testFormatMultiline('x=1,2,3', 0, 'x = 1, 2, 3'); }); test('is exression', () => { testFormatLine('a( (False is 2) is 3)', 'a((False is 2) is 3)'); }); test('Function returning tuple', () => { - testFormatLine('x,y=f(a)', 'x, y = f(a)'); + testFormatMultiline('x,y=f(a)', 0, 'x, y = f(a)'); }); test('from. import A', () => { testFormatLine('from. import A', 'from . import A'); @@ -127,24 +106,24 @@ suite('Formatting - line formatter', () => { testFormatLine('from..x import', 'from ..x import'); }); test('Raw strings', () => { - testFormatLine('z=r""', 'z = r""'); - testFormatLine('z=rf""', 'z = rf""'); - testFormatLine('z=R""', 'z = R""'); - testFormatLine('z=RF""', 'z = RF""'); + testFormatMultiline('z=r""', 0, 'z = r""'); + testFormatMultiline('z=rf""', 0, 'z = rf""'); + testFormatMultiline('z=R""', 0, 'z = R""'); + testFormatMultiline('z=RF""', 0, 'z = RF""'); }); test('Binary @', () => { testFormatLine('a@ b', 'a @ b'); }); test('Unary operators', () => { - testFormatLine('x= - y', 'x = -y'); - testFormatLine('x= + y', 'x = +y'); - testFormatLine('x= ~ y', 'x = ~y'); - testFormatLine('x=-1', 'x = -1'); - testFormatLine('x= +1', 'x = +1'); - testFormatLine('x= ~1 ', 'x = ~1'); + testFormatMultiline('x= - y', 0, 'x = -y'); + testFormatMultiline('x= + y', 0, 'x = +y'); + testFormatMultiline('x= ~ y', 0, 'x = ~y'); + testFormatMultiline('x=-1', 0, 'x = -1'); + testFormatMultiline('x= +1', 0, 'x = +1'); + testFormatMultiline('x= ~1 ', 0, 'x = ~1'); }); test('Equals with type hints', () => { - testFormatLine('def foo(x:int=3,x=100.)', 'def foo(x: int = 3, x=100.)'); + testFormatMultiline('def foo(x:int=3,x=100.)', 0, 'def foo(x: int = 3, x=100.)'); }); test('Trailing comma', () => { testFormatLine('a, =[1]', 'a, = [1]'); @@ -152,15 +131,35 @@ suite('Formatting - line formatter', () => { test('if()', () => { testFormatLine('if(True) :', 'if (True):'); }); + test('lambda arguments', () => { + testFormatMultiline('l4= lambda x =lambda y =lambda z= 1: z: y(): x()', 0, 'l4 = lambda x=lambda y=lambda z=1: z: y(): x()'); + }); + + test('Multiline function call', () => { + testFormatMultiline('def foo(x = 1)', 0, 'def foo(x=1)'); + testFormatMultiline('def foo(a\n, x = 1)', 1, ', x=1)'); + testFormatMultiline('foo(a ,b,\n x = 1)', 1, ' x=1)'); + testFormatMultiline('if True:\n if False:\n foo(a , bar(\n x = 1)', 3, ' x=1)'); + testFormatMultiline('z=foo (0 , x= 1, (3+7) , y , z )', 0, 'z = foo(0, x=1, (3 + 7), y, z)'); + testFormatMultiline('foo (0,\n x= 1,', 1, ' x=1,'); + testFormatMultiline( +// tslint:disable-next-line:no-multiline-string +`async def fetch(): + async with aiohttp.ClientSession() as session: + async with session.ws_connect( + "http://127.0.0.1:8000/", headers = cookie) as ws: # add unwanted spaces`, 3, + ' "http://127.0.0.1:8000/", headers=cookie) as ws: # add unwanted spaces'); + testFormatMultiline('def pos0key1(*, key): return key\npos0key1(key= 100)', 1, 'pos0key1(key=100)'); + testFormatMultiline('def test_string_literals(self):\n x= 1; y =2; self.assertTrue(len(x) == 0 and x == y)', 1, + ' x = 1; y = 2; self.assertTrue(len(x) == 0 and x == y)'); + }); test('Grammar file', () => { const content = fs.readFileSync(grammarFile).toString('utf8'); const lines = content.splitLines({ trim: false, removeEmptyEntries: false }); - let prevLine = ''; for (let i = 0; i < lines.length; i += 1) { const line = lines[i]; - const actual = formatLine2(prevLine, line); + const actual = formatMultiline(content, i); assert.equal(actual, line, `Line ${i + 1} changed: '${line.trim()}' to '${actual.trim()}'`); - prevLine = line; } }); @@ -169,32 +168,55 @@ suite('Formatting - line formatter', () => { assert.equal(actual, expected); } - function formatLine(text: string): string { - const line = TypeMoq.Mock.ofType(); - line.setup(x => x.text).returns(() => text); + function testFormatMultiline(content: string, lineNumber: number, expected: string): void { + const actual = formatMultiline(content, lineNumber); + assert.equal(actual, expected); + } + + function formatMultiline(content: string, lineNumber: number): string { + const lines = content.splitLines({ trim: false, removeEmptyEntries: false }); const document = TypeMoq.Mock.ofType(); - document.setup(x => x.lineAt(TypeMoq.It.isAnyNumber())).returns(() => line.object); + document.setup(x => x.lineAt(TypeMoq.It.isAnyNumber())).returns(n => { + const line = TypeMoq.Mock.ofType(); + line.setup(x => x.text).returns(() => lines[n]); + line.setup(x => x.range).returns(() => new Range(new Position(n, 0), new Position(n, lines[n].length))); + return line.object; + }); + document.setup(x => x.getText(TypeMoq.It.isAny())).returns(o => { + const r = o as Range; + const bits: string[] = []; - return formatter.formatLine(document.object, 0); - } + if (r.start.line === r.end.line) { + return lines[r.start.line].substring(r.start.character, r.end.character); + } - function formatLine2(prevLineText: string, lineText: string): string { - const thisLine = TypeMoq.Mock.ofType(); - thisLine.setup(x => x.text).returns(() => lineText); + bits.push(lines[r.start.line].substr(r.start.character)); + for (let i = r.start.line + 1; i < r.end.line; i += 1) { + bits.push(lines[i]); + } + bits.push(lines[r.end.line].substring(0, r.end.character)); + return bits.join('\n'); + }); + document.setup(x => x.offsetAt(TypeMoq.It.isAny())).returns(o => { + const p = o as Position; + let offset = 0; + for (let i = 0; i < p.line; i += 1) { + offset += lines[i].length + 1; // Accounting for the line break + } + return offset + p.character; + }); - const prevLine = TypeMoq.Mock.ofType(); - prevLine.setup(x => x.text).returns(() => prevLineText); + return formatter.formatLine(document.object, lineNumber); + } - const document = TypeMoq.Mock.ofType(); - document.setup(x => x.lineAt(0)).returns(() => prevLine.object); - document.setup(x => x.lineAt(1)).returns(() => thisLine.object); + function formatLine(text: string): string { + const line = TypeMoq.Mock.ofType(); + line.setup(x => x.text).returns(() => text); - return formatter.formatLine(document.object, 1); - } + const document = TypeMoq.Mock.ofType(); + document.setup(x => x.lineAt(TypeMoq.It.isAnyNumber())).returns(() => line.object); - function testFormatLine2(prevLineText: string, lineText: string, expected: string): void { - const actual = formatLine2(prevLineText, lineText); - assert.equal(actual, expected); + return formatter.formatLine(document.object, 0); } });