Skip to content

Commit f7e4529

Browse files
author
Mikhail Arkhipov
authored
Improve detection of the function argument list in autoformat (#1508)
* Undo changes * Test fixes * Increase timeout * Remove double event listening * Remove test * Revert "Remove test" This reverts commit e240c3f. * Revert "Remove double event listening" This reverts commit af573be. * #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 e240c3f. * Revert "Remove double event listening" This reverts commit af573be. * 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 * Improve function argument detection
1 parent 15f2a14 commit f7e4529

File tree

4 files changed

+123
-94
lines changed

4 files changed

+123
-94
lines changed

‎src/client/formatters/lineFormatter.ts

Lines changed: 33 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
// tslint:disable-next-line:import-name
55
import Char from 'typescript-char';
6+
import { TextDocument } from 'vscode';
67
import { BraceCounter } from '../language/braceCounter';
78
import { TextBuilder } from '../language/textBuilder';
89
import { TextRangeCollection } from '../language/textRangeCollection';
@@ -14,11 +15,15 @@ export class LineFormatter {
1415
private tokens: ITextRangeCollection<IToken> = new TextRangeCollection<IToken>([]);
1516
private braceCounter = new BraceCounter();
1617
private text = '';
18+
private document?: TextDocument;
19+
private lineNumber = 0;
1720

1821
// tslint:disable-next-line:cyclomatic-complexity
19-
public formatLine(text: string): string {
20-
this.tokens = new Tokenizer().tokenize(text);
21-
this.text = text;
22+
public formatLine(document: TextDocument, lineNumber: number): string {
23+
this.document = document;
24+
this.lineNumber = lineNumber;
25+
this.text = document.lineAt(lineNumber).text;
26+
this.tokens = new Tokenizer().tokenize(this.text);
2227
this.builder = new TextBuilder();
2328
this.braceCounter = new BraceCounter();
2429

@@ -107,7 +112,7 @@ export class LineFormatter {
107112
this.builder.append(this.text[t.start]);
108113
return;
109114
case Char.Asterisk:
110-
if (prev && prev.type === TokenType.Identifier && prev.length === 6 && this.text.substr(prev.start, prev.length) === 'lambda') {
115+
if (prev && this.isKeyword(prev, 'lambda')) {
111116
this.builder.softAppendSpace();
112117
this.builder.append('*');
113118
return;
@@ -122,7 +127,7 @@ export class LineFormatter {
122127
this.builder.append('**');
123128
return;
124129
}
125-
if (prev && prev.type === TokenType.Identifier && prev.length === 6 && this.text.substr(prev.start, prev.length) === 'lambda') {
130+
if (prev && this.isKeyword(prev, 'lambda')) {
126131
this.builder.softAppendSpace();
127132
this.builder.append('**');
128133
return;
@@ -194,6 +199,8 @@ export class LineFormatter {
194199
this.builder.softAppendSpace();
195200
}
196201
}
202+
203+
// tslint:disable-next-line:cyclomatic-complexity
197204
private isEqualsInsideArguments(index: number): boolean {
198205
// Since we don't have complete statement, this is mostly heuristics.
199206
// Therefore the code may not be handling all possible ways of the
@@ -217,28 +224,31 @@ export class LineFormatter {
217224
return true; // Line ends in comma
218225
}
219226

220-
if (index >= 2) {
221-
// (x=1 or ,x=1
222-
const prevPrev = this.tokens.getItemAt(index - 2);
223-
return prevPrev.type === TokenType.Comma || prevPrev.type === TokenType.OpenBrace;
227+
if (last.type === TokenType.Comment && this.tokens.count > 1 && this.tokens.getItemAt(this.tokens.count - 2).type === TokenType.Comma) {
228+
return true; // Line ends in comma and then comment
224229
}
225230

226-
if (index >= this.tokens.count - 2) {
227-
return false;
231+
if (this.document) {
232+
const prevLine = this.lineNumber > 0 ? this.document.lineAt(this.lineNumber - 1).text : '';
233+
const prevLineTokens = new Tokenizer().tokenize(prevLine);
234+
if (prevLineTokens.count > 0) {
235+
const lastOnPrevLine = prevLineTokens.getItemAt(prevLineTokens.count - 1);
236+
if (lastOnPrevLine.type === TokenType.Comma) {
237+
return true; // Previous line ends in comma
238+
}
239+
if (lastOnPrevLine.type === TokenType.Comment && prevLineTokens.count > 1 && prevLineTokens.getItemAt(prevLineTokens.count - 2).type === TokenType.Comma) {
240+
return true; // Previous line ends in comma and then comment
241+
}
242+
}
228243
}
229244

230-
const next = this.tokens.getItemAt(index + 1);
231-
const nextNext = this.tokens.getItemAt(index + 2);
232-
// x=1, or x=1)
233-
if (this.isValueType(next.type)) {
234-
if (nextNext.type === TokenType.CloseBrace) {
245+
for (let i = 0; i < index; i += 1) {
246+
const t = this.tokens.getItemAt(i);
247+
if (this.isKeyword(t, 'lambda')) {
235248
return true;
236249
}
237-
if (nextNext.type === TokenType.Comma) {
238-
return last.type === TokenType.CloseBrace;
239-
}
240250
}
241-
return false;
251+
return this.braceCounter.isOpened(TokenType.OpenBrace);
242252
}
243253

244254
private isOpenBraceType(type: TokenType): boolean {
@@ -250,10 +260,6 @@ export class LineFormatter {
250260
private isBraceType(type: TokenType): boolean {
251261
return this.isOpenBraceType(type) || this.isCloseBraceType(type);
252262
}
253-
private isValueType(type: TokenType): boolean {
254-
return type === TokenType.Identifier || type === TokenType.Unknown ||
255-
type === TokenType.Number || type === TokenType.String;
256-
}
257263
private isMultipleStatements(index: number): boolean {
258264
for (let i = index; i >= 0; i -= 1) {
259265
if (this.tokens.getItemAt(i).type === TokenType.Semicolon) {
@@ -268,4 +274,7 @@ export class LineFormatter {
268274
s === 'import' || s === 'except' || s === 'for' ||
269275
s === 'as' || s === 'is';
270276
}
277+
private isKeyword(t: IToken, keyword: string): boolean {
278+
return t.type === TokenType.Identifier && t.length === keyword.length && this.text.substr(t.start, t.length) === keyword;
279+
}
271280
}

‎src/client/typeFormatters/onEnterFormatter.ts

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4-
import * as vscode from 'vscode';
4+
import { CancellationToken, FormattingOptions, OnTypeFormattingEditProvider, Position, TextDocument, TextEdit } from 'vscode';
55
import { LineFormatter } from '../formatters/lineFormatter';
66
import { TokenizerMode, TokenType } from '../language/types';
77
import { getDocumentTokens } from '../providers/providerUtilities';
88

9-
export class OnEnterFormatter implements vscode.OnTypeFormattingEditProvider {
9+
export class OnEnterFormatter implements OnTypeFormattingEditProvider {
1010
private readonly formatter = new LineFormatter();
1111

1212
public provideOnTypeFormattingEdits(
13-
document: vscode.TextDocument,
14-
position: vscode.Position,
13+
document: TextDocument,
14+
position: Position,
1515
ch: string,
16-
options: vscode.FormattingOptions,
17-
cancellationToken: vscode.CancellationToken): vscode.TextEdit[] {
16+
options: FormattingOptions,
17+
cancellationToken: CancellationToken): TextEdit[] {
1818
if (position.line === 0) {
1919
return [];
2020
}
@@ -30,10 +30,10 @@ export class OnEnterFormatter implements vscode.OnTypeFormattingEditProvider {
3030
return [];
3131
}
3232
}
33-
const formatted = this.formatter.formatLine(prevLine.text);
33+
const formatted = this.formatter.formatLine(document, prevLine.lineNumber);
3434
if (formatted === prevLine.text) {
3535
return [];
3636
}
37-
return [new vscode.TextEdit(prevLine.range, formatted)];
37+
return [new TextEdit(prevLine.range, formatted)];
3838
}
3939
}

‎src/test/format/extension.lineFormatter.test.ts

Lines changed: 78 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
import * as assert from 'assert';
66
import * as fs from 'fs';
77
import * as path from 'path';
8+
import * as TypeMoq from 'typemoq';
9+
import { TextDocument, TextLine } from 'vscode';
810
import '../../client/common/extensions';
911
import { LineFormatter } from '../../client/formatters/lineFormatter';
1012

@@ -17,124 +19,142 @@ suite('Formatting - line formatter', () => {
1719
const formatter = new LineFormatter();
1820

1921
test('Operator spacing', () => {
20-
const actual = formatter.formatLine('( x +1 )*y/ 3');
21-
assert.equal(actual, '(x + 1) * y / 3');
22+
testFormatLine('( x +1 )*y/ 3', '(x + 1) * y / 3');
2223
});
2324
test('Braces spacing', () => {
24-
const actual = formatter.formatLine('foo =(0 ,)');
25-
assert.equal(actual, 'foo = (0,)');
25+
testFormatLine('foo =(0 ,)', 'foo = (0,)');
2626
});
2727
test('Function arguments', () => {
28-
const actual = formatter.formatLine('foo (0 , x= 1, (3+7) , y , z )');
29-
assert.equal(actual, 'foo(0, x=1, (3 + 7), y, z)');
28+
testFormatLine('z=foo (0 , x= 1, (3+7) , y , z )',
29+
'z = foo(0, x=1, (3 + 7), y, z)');
3030
});
3131
test('Colon regular', () => {
32-
const actual = formatter.formatLine('if x == 4 : print x,y; x,y= y, x');
33-
assert.equal(actual, 'if x == 4: print x, y; x, y = y, x');
32+
testFormatLine('if x == 4 : print x,y; x,y= y, x',
33+
'if x == 4: print x, y; x, y = y, x');
3434
});
3535
test('Colon slices', () => {
36-
const actual = formatter.formatLine('x[1: 30]');
37-
assert.equal(actual, 'x[1:30]');
36+
testFormatLine('x[1: 30]', 'x[1:30]');
3837
});
3938
test('Colon slices in arguments', () => {
40-
const actual = formatter.formatLine('spam ( ham[ 1 :3], {eggs : 2})');
41-
assert.equal(actual, 'spam(ham[1:3], {eggs: 2})');
39+
testFormatLine('spam ( ham[ 1 :3], {eggs : 2})',
40+
'spam(ham[1:3], {eggs: 2})');
4241
});
4342
test('Colon slices with double colon', () => {
44-
const actual = formatter.formatLine('ham [1:9 ], ham[ 1: 9: 3], ham[: 9 :3], ham[1: :3], ham [ 1: 9:]');
45-
assert.equal(actual, 'ham[1:9], ham[1:9:3], ham[:9:3], ham[1::3], ham[1:9:]');
43+
testFormatLine('ham [1:9 ], ham[ 1: 9: 3], ham[: 9 :3], ham[1: :3], ham [ 1: 9:]',
44+
'ham[1:9], ham[1:9:3], ham[:9:3], ham[1::3], ham[1:9:]');
4645
});
4746
test('Colon slices with operators', () => {
48-
const actual = formatter.formatLine('ham [lower+ offset :upper+offset]');
49-
assert.equal(actual, 'ham[lower + offset:upper + offset]');
47+
testFormatLine('ham [lower+ offset :upper+offset]',
48+
'ham[lower + offset:upper + offset]');
5049
});
5150
test('Colon slices with functions', () => {
52-
const actual = formatter.formatLine('ham[ : upper_fn ( x) : step_fn(x )], ham[ :: step_fn(x)]');
53-
assert.equal(actual, 'ham[:upper_fn(x):step_fn(x)], ham[::step_fn(x)]');
51+
testFormatLine('ham[ : upper_fn ( x) : step_fn(x )], ham[ :: step_fn(x)]',
52+
'ham[:upper_fn(x):step_fn(x)], ham[::step_fn(x)]');
5453
});
5554
test('Colon in for loop', () => {
56-
const actual = formatter.formatLine('for index in range( len(fruits) ): ');
57-
assert.equal(actual, 'for index in range(len(fruits)):');
55+
testFormatLine('for index in range( len(fruits) ): ',
56+
'for index in range(len(fruits)):');
5857
});
5958
test('Nested braces', () => {
60-
const actual = formatter.formatLine('[ 1 :[2: (x,),y]]{1}');
61-
assert.equal(actual, '[1:[2:(x,), y]]{1}');
59+
testFormatLine('[ 1 :[2: (x,),y]]{1}', '[1:[2:(x,), y]]{1}');
6260
});
6361
test('Trailing comment', () => {
64-
const actual = formatter.formatLine('x=1 # comment');
65-
assert.equal(actual, 'x = 1 # comment');
62+
testFormatLine('x=1 # comment', 'x = 1 # comment');
6663
});
6764
test('Single comment', () => {
68-
const actual = formatter.formatLine('# comment');
69-
assert.equal(actual, '# comment');
65+
testFormatLine('# comment', '# comment');
7066
});
7167
test('Comment with leading whitespace', () => {
72-
const actual = formatter.formatLine(' # comment');
73-
assert.equal(actual, ' # comment');
68+
testFormatLine(' # comment', ' # comment');
7469
});
7570
test('Equals in first argument', () => {
76-
const actual = formatter.formatLine('foo(x =0)');
77-
assert.equal(actual, 'foo(x=0)');
71+
testFormatLine('foo(x =0)', 'foo(x=0)');
7872
});
7973
test('Equals in second argument', () => {
80-
const actual = formatter.formatLine('foo(x,y= \"a\",');
81-
assert.equal(actual, 'foo(x, y=\"a\",');
74+
testFormatLine('foo(x,y= \"a\",', 'foo(x, y=\"a\",');
8275
});
8376
test('Equals in multiline arguments', () => {
84-
const actual = formatter.formatLine('x = 1,y =-2)');
85-
assert.equal(actual, 'x=1, y=-2)');
77+
testFormatLine2('foo(a,', 'x = 1,y =-2)', 'x=1, y=-2)');
8678
});
8779
test('Equals in multiline arguments starting comma', () => {
88-
const actual = formatter.formatLine(',x = 1,y =m)');
89-
assert.equal(actual, ', x=1, y=m)');
80+
testFormatLine(',x = 1,y =m)', ', x=1, y=m)');
9081
});
9182
test('Equals in multiline arguments ending comma', () => {
92-
const actual = formatter.formatLine('x = 1,y =m,');
93-
assert.equal(actual, 'x=1, y=m,');
83+
testFormatLine('x = 1,y =m,', 'x=1, y=m,');
9484
});
9585
test('Operators without following space', () => {
96-
const actual = formatter.formatLine('foo( *a, ** b, ! c)');
97-
assert.equal(actual, 'foo(*a, **b, !c)');
86+
testFormatLine('foo( *a, ** b, ! c)', 'foo(*a, **b, !c)');
9887
});
9988
test('Brace after keyword', () => {
100-
const actual = formatter.formatLine('for x in(1,2,3)');
101-
assert.equal(actual, 'for x in (1, 2, 3)');
89+
testFormatLine('for x in(1,2,3)', 'for x in (1, 2, 3)');
10290
});
10391
test('Dot operator', () => {
104-
const actual = formatter.formatLine('x.y');
105-
assert.equal(actual, 'x.y');
92+
testFormatLine('x.y', 'x.y');
10693
});
10794
test('Unknown tokens no space', () => {
108-
const actual = formatter.formatLine('abc\\n\\');
109-
assert.equal(actual, 'abc\\n\\');
95+
testFormatLine('abc\\n\\', 'abc\\n\\');
11096
});
11197
test('Unknown tokens with space', () => {
112-
const actual = formatter.formatLine('abc \\n \\');
113-
assert.equal(actual, 'abc \\n \\');
98+
testFormatLine('abc \\n \\', 'abc \\n \\');
11499
});
115100
test('Double asterisk', () => {
116-
const actual = formatter.formatLine('a**2, ** k');
117-
assert.equal(actual, 'a ** 2, **k');
101+
testFormatLine('a**2, ** k', 'a ** 2, **k');
118102
});
119103
test('Lambda', () => {
120-
const actual = formatter.formatLine('lambda * args, :0');
121-
assert.equal(actual, 'lambda *args,: 0');
104+
testFormatLine('lambda * args, :0', 'lambda *args,: 0');
122105
});
123106
test('Comma expression', () => {
124-
const actual = formatter.formatLine('x=1,2,3');
125-
assert.equal(actual, 'x = 1, 2, 3');
107+
testFormatLine('x=1,2,3', 'x = 1, 2, 3');
126108
});
127109
test('is exression', () => {
128-
const actual = formatter.formatLine('a( (False is 2) is 3)');
129-
assert.equal(actual, 'a((False is 2) is 3)');
110+
testFormatLine('a( (False is 2) is 3)', 'a((False is 2) is 3)');
111+
});
112+
test('Function returning tuple', () => {
113+
testFormatLine('x,y=f(a)', 'x, y = f(a)');
130114
});
131115
test('Grammar file', () => {
132116
const content = fs.readFileSync(grammarFile).toString('utf8');
133117
const lines = content.splitLines({ trim: false, removeEmptyEntries: false });
118+
let prevLine = '';
134119
for (let i = 0; i < lines.length; i += 1) {
135120
const line = lines[i];
136-
const actual = formatter.formatLine(line);
137-
assert.equal(actual, line, `Line ${i + 1} changed: '${line}' to '${actual}'`);
121+
const actual = formatLine2(prevLine, line);
122+
assert.equal(actual, line, `Line ${i + 1} changed: '${line.trim()}' to '${actual.trim()}'`);
123+
prevLine = line;
138124
}
139125
});
126+
127+
function testFormatLine(text: string, expected: string): void {
128+
const actual = formatLine(text);
129+
assert.equal(actual, expected);
130+
}
131+
132+
function formatLine(text: string): string {
133+
const line = TypeMoq.Mock.ofType<TextLine>();
134+
line.setup(x => x.text).returns(() => text);
135+
136+
const document = TypeMoq.Mock.ofType<TextDocument>();
137+
document.setup(x => x.lineAt(TypeMoq.It.isAnyNumber())).returns(() => line.object);
138+
139+
return formatter.formatLine(document.object, 0);
140+
}
141+
142+
function formatLine2(prevLineText: string, lineText: string): string {
143+
const thisLine = TypeMoq.Mock.ofType<TextLine>();
144+
thisLine.setup(x => x.text).returns(() => lineText);
145+
146+
const prevLine = TypeMoq.Mock.ofType<TextLine>();
147+
prevLine.setup(x => x.text).returns(() => prevLineText);
148+
149+
const document = TypeMoq.Mock.ofType<TextDocument>();
150+
document.setup(x => x.lineAt(0)).returns(() => prevLine.object);
151+
document.setup(x => x.lineAt(1)).returns(() => thisLine.object);
152+
153+
return formatter.formatLine(document.object, 1);
154+
}
155+
156+
function testFormatLine2(prevLineText: string, lineText: string, expected: string): void {
157+
const actual = formatLine2(prevLineText, lineText);
158+
assert.equal(actual, expected);
159+
}
140160
});

‎src/test/pythonFiles/formatting/pythonGrammar.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -646,7 +646,7 @@ def test_lambdef(self):
646646
l2 = lambda: a[d] # XXX just testing the expression
647647
l3 = lambda: [2 < x for x in [-1, 3, 0]]
648648
self.assertEqual(l3(), [0, 1, 0])
649-
l4 = lambda x = lambda y = lambda z = 1: z: y(): x()
649+
l4 = lambda x=lambda y=lambda z=1: z: y(): x()
650650
self.assertEqual(l4(), 1)
651651
l5 = lambda x, y, z=2: x + y + z
652652
self.assertEqual(l5(1, 2), 5)
@@ -696,8 +696,8 @@ def test_expr_stmt(self):
696696
x = 1
697697
x = 1, 2, 3
698698
x = y = z = 1, 2, 3
699-
x, y, z=1, 2, 3
700-
abc=a, b, c=x, y, z=xyz = 1, 2, (3, 4)
699+
x, y, z = 1, 2, 3
700+
abc = a, b, c = x, y, z = xyz = 1, 2, (3, 4)
701701

702702
check_syntax_error(self, "x + 1 = 1")
703703
check_syntax_error(self, "a + 1 = b + 2")
@@ -730,7 +730,7 @@ def test_former_statements_refer_to_builtins(self):
730730
def test_del_stmt(self):
731731
# 'del' exprlist
732732
abc = [1, 2, 3]
733-
x, y, z=abc
733+
x, y, z = abc
734734
xyz = x, y, z
735735

736736
del abc

0 commit comments

Comments
 (0)