Handle line formatting when line is in a middle or at the end of a multiline string

news/2 Fixes/2323.md

@@ -0,0 +1 @@
+Fixed incorrect on type formatting when line was in a middle or contained end of a multiline string.

news/2 Fixes/2405.md

@@ -1 +1,2 @@
-Added setting to control language server log output. Default is now 'error' so there should be much less noise in the output.
+Added setting to control language server log output through the `python.analysis.logLevel` setting (the default is now 'Error').
+

src/client/formatters/lineFormatter.ts

@@ -3,12 +3,12 @@
 
 // tslint:disable-next-line:import-name
 import Char from 'typescript-char';
-import { Position, Range, TextDocument } from 'vscode';
+import { Position, Range, TextDocument, TextLine } from 'vscode';
 import { BraceCounter } from '../language/braceCounter';
 import { TextBuilder } from '../language/textBuilder';
 import { TextRangeCollection } from '../language/textRangeCollection';
 import { Tokenizer } from '../language/tokenizer';
-import { ITextRangeCollection, IToken, TokenType } from '../language/types';
+import { ITextRangeCollection, IToken, Token, TokenizerMode, TokenType } from '../language/types';
 
 const keywordsWithSpaceBeforeBrace = [
     'and', 'as', 'assert', 'await',
@@ -35,10 +35,13 @@ export class LineFormatter {
 
     // tslint:disable-next-line:cyclomatic-complexity
     public formatLine(document: TextDocument, lineNumber: number): string {
+        const line = document.lineAt(lineNumber);
+
         this.document = document;
         this.lineNumber = lineNumber;
-        this.text = document.lineAt(lineNumber).text;
-        this.tokens = new Tokenizer().tokenize(this.text);
+        this.text = line.text;
+        this.tokens = this.getLineTokens(document, line);
+
         this.builder = new TextBuilder();
         this.braceCounter = new BraceCounter();
 
@@ -182,7 +185,7 @@ export class LineFormatter {
         this.builder.softAppendSpace();
     }
 
-    private handleStarOperator(current: IToken, prev: IToken): boolean {
+    private handleStarOperator(current: IToken, prev: IToken | undefined): boolean {
         if (this.text.charCodeAt(current.start) === Char.Asterisk && this.text.charCodeAt(current.start + 1) === Char.Asterisk) {
             if (!prev || (prev.type !== TokenType.Identifier && prev.type !== TokenType.Number)) {
                 this.builder.append('**');
@@ -438,4 +441,54 @@ export class LineFormatter {
         const line = this.document.lineAt(this.lineNumber - 1);
         return new Tokenizer().tokenize(line.text);
     }
+
+    private getLineStartToken(document: TextDocument, position: Position): IToken | undefined {
+        const tokenizeTo = position.translate(1, 0);
+        const text = document.getText(new Range(new Position(0, 0), tokenizeTo));
+        const tokens = new Tokenizer().tokenize(text, 0, text.length, TokenizerMode.CommentsAndStrings);
+        const offset = document.offsetAt(position);
+        const index = tokens.getItemContaining(offset - 1);
+        return index >= 0 ? tokens.getItemAt(index) : undefined;
+    }
+
+    private getLineTokens(document: TextDocument, line: TextLine): ITextRangeCollection<IToken> {
+        // Check if line start is inside a multiline string.
+        // If so, make sure first token is string type so we don't
+        // tokenize content of the multiline string as a legal code.
+        const lineStart = document.offsetAt(line.range.start);
+        const startToken = this.getLineStartToken(document, line.range.start);
+        if (startToken && startToken.type === TokenType.String) {
+            if (startToken.length >= 6 && startToken.start < lineStart && startToken.end > lineStart) {
+                // Line start is in a multiline string. Get quotes and
+                // insert them in the beginning of the line so the fist token
+                // will be a correct string.
+                const startQuotePosition = document.positionAt(startToken.start);
+                const endQuotePosition = document.positionAt(startToken.start + 3);
+                const quoteString = document.getText(new Range(startQuotePosition, endQuotePosition));
+                const tokens = new Tokenizer().tokenize(`${quoteString}${this.text}`);
+                let adjusted: IToken[] = [];
+                if (tokens.count > 1) {
+                    // There is something that follows the end of the multiline string as in 'text """ + 1'
+                    // so when we inserted triple quote at the start we ended up with more than one token.
+                    // Fix up token positions so they match the actual document: first token must be string
+                    // with the remaining token follow. In the example above, String -> Operator -> Number.
+                    adjusted = [new Token(TokenType.String, 0, startToken.end - lineStart)];
+                    for (let i = 1; i < tokens.count; i += 1) {
+                        const t = tokens.getItemAt(i);
+                        adjusted.push(new Token(t.type, t.start - quoteString.length, t.length));
+                    }
+                } else {
+                    // The entire line is inside the multiline string such as in
+                    // """
+                    // text
+                    // """
+                    // or it is a string with a terminator but has trailing whitespace as in
+                    // text"""<whitespace>
+                    adjusted = [new Token(TokenType.String, 0, line.text.trimRight().length)];
+                }
+                return new TextRangeCollection(adjusted);
+            }
+        }
+        return new Tokenizer().tokenize(this.text);
+    }
 }

src/client/language/tokenizer.ts

@@ -4,10 +4,10 @@
 
 // tslint:disable-next-line:import-name
 import Char from 'typescript-char';
-import { isBinary, isDecimal, isHex, isIdentifierChar, isIdentifierStartChar, isOctal, isWhiteSpace } from './characters';
+import { isBinary, isDecimal, isHex, isIdentifierChar, isIdentifierStartChar, isOctal } from './characters';
 import { CharacterStream } from './characterStream';
 import { TextRangeCollection } from './textRangeCollection';
-import { ICharacterStream, ITextRangeCollection, IToken, ITokenizer, TextRange, TokenizerMode, TokenType } from './types';
+import { ICharacterStream, ITextRangeCollection, IToken, ITokenizer, Token, TokenizerMode, TokenType } from './types';
 
 enum QuoteType {
     None,
@@ -17,15 +17,6 @@ enum QuoteType {
     TripleDouble
 }
 
-class Token extends TextRange implements IToken {
-    public readonly type: TokenType;
-
-    constructor(type: TokenType, start: number, length: number) {
-        super(start, length);
-        this.type = type;
-    }
-}
-
 export class Tokenizer implements ITokenizer {
     private cs: ICharacterStream = new CharacterStream('');
     private tokens: IToken[] = [];

src/client/language/types.ts

@@ -94,6 +94,14 @@ export interface IToken extends ITextRange {
     readonly type: TokenType;
 }
 
+export class Token extends TextRange implements IToken {
+    public readonly type: TokenType;
+
+    constructor(type: TokenType, start: number, length: number) {
+        super(start, length);
+        this.type = type;
+    }
+}
 export enum TokenizerMode {
     CommentsAndStrings,
     Full

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

@@ -8,6 +8,7 @@ import * as path from 'path';
 import * as TypeMoq from 'typemoq';
 import { Position, Range, TextDocument, TextLine } from 'vscode';
 import '../../client/common/extensions';
+import { createDeferred } from '../../client/common/helpers';
 import { LineFormatter } from '../../client/formatters/lineFormatter';
 
 const formatFilesPath = path.join(__dirname, '..', '..', '..', 'src', 'test', 'pythonFiles', 'formatting');
@@ -17,7 +18,18 @@ const grammarFile = path.join(formatFilesPath, 'pythonGrammar.py');
 // tslint:disable-next-line:max-func-body-length
 suite('Formatting - line formatter', () => {
     const formatter = new LineFormatter();
+    let grammarContent = '';
+    let grammarLines: string[] = [];
 
+    suiteSetup(async () => {
+        const d = createDeferred();
+        fs.readFile(grammarFile, {}, (err, data) => {
+            grammarContent = data.toString('utf-8');
+            grammarLines = grammarContent.splitLines({ trim: false, removeEmptyEntries: false });
+            d.resolve();
+        });
+        await d.promise;
+    });
     test('Operator spacing', () => {
         testFormatLine('( x  +1 )*y/ 3', '(x + 1) * y / 3');
     });
@@ -151,8 +163,8 @@ suite('Formatting - line formatter', () => {
         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():
+            // 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,
@@ -161,14 +173,35 @@ suite('Formatting - line formatter', () => {
         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 });
-        for (let i = 0; i < lines.length; i += 1) {
-            const line = lines[i];
-            const actual = formatMultiline(content, i);
-            assert.equal(actual, line, `Line ${i + 1} changed: '${line.trim()}' to '${actual.trim()}'`);
-        }
+    test('End of multiline string', () => {
+        testFormatLine('"""', '"""');
+        testFormatMultiline('"""\ntext "quoted"\n', 1, 'text "quoted"');
+        testFormatMultiline('"""string 1\nstring2""" ', 1, 'string2"""');
+        testFormatMultiline('"""string 1\nstring2""" +1 ', 1, 'string2""" + 1');
+    });
+    test('Grammar file part 1', () => {
+        testLines(grammarContent, grammarLines, 0, 200);
+    });
+    test('Grammar file part 2', async () => {
+        testLines(grammarContent, grammarLines, 201, 200);
+    });
+    test('Grammar file part 3', async () => {
+        testLines(grammarContent, grammarLines, 401, 200);
+    });
+    test('Grammar file part 4', async () => {
+        testLines(grammarContent, grammarLines, 601, 200);
+    });
+    test('Grammar file part 5', async () => {
+        testLines(grammarContent, grammarLines, 801, 200);
+    });
+    test('Grammar file part 6', async () => {
+        testLines(grammarContent, grammarLines, 1001, 200);
+    });
+    test('Grammar file part 7', async () => {
+        testLines(grammarContent, grammarLines, 1201, 200);
+    });
+    test('Grammar file part 8', async () => {
+        testLines(grammarContent, grammarLines, 1401);
     });
 
     function testFormatLine(text: string, expected: string): void {
@@ -181,14 +214,24 @@ suite('Formatting - line formatter', () => {
         assert.equal(actual, expected);
     }
 
+    function testLines(content: string, lines: string[], startLine: number, count?: number) {
+        count = count ? count : lines.length - startLine;
+        for (let i = startLine; i < startLine + count; i += 1) {
+            const line = lines[i];
+            const actual = formatMultiline(content, i);
+            assert.equal(actual, line, `Line ${i + 1} changed: '${line.trim()}' to '${actual.trim()}'`);
+        }
+    }
+
     function formatMultiline(content: string, lineNumber: number): string {
         const lines = content.splitLines({ trim: false, removeEmptyEntries: false });
 
         const document = TypeMoq.Mock.ofType<TextDocument>();
         document.setup(x => x.lineAt(TypeMoq.It.isAnyNumber())).returns(n => {
             const line = TypeMoq.Mock.ofType<TextLine>();
             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)));
+            const range = new Range(new Position(n, 0), new Position(n, lines[n].length));
+            line.setup(x => x.range).returns(() => range);
             return line.object;
         });
         document.setup(x => x.getText(TypeMoq.It.isAny())).returns(o => {
@@ -203,7 +246,9 @@ suite('Formatting - line formatter', () => {
             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));
+            if (r.end.line < lines.length) {
+                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 => {
@@ -214,6 +259,18 @@ suite('Formatting - line formatter', () => {
             }
             return offset + p.character;
         });
+        document.setup(x => x.positionAt(TypeMoq.It.isAnyNumber())).returns(n => {
+            let start = 0;
+            let end = 0;
+            for (let i = 0; i < lines.length; i += 1) {
+                end = start + lines[i].length;
+                if (n >= start && n < end + 1) {
+                    return new Position(i, n - start);
+                }
+                start = end + 1; // Accounting for the line break
+            }
+            throw new Error('Document position is out of range.');
+        });
 
         return formatter.formatLine(document.object, lineNumber);
     }
@@ -222,8 +279,12 @@ suite('Formatting - line formatter', () => {
         const line = TypeMoq.Mock.ofType<TextLine>();
         line.setup(x => x.text).returns(() => text);
 
+        const range = new Range(new Position(0, 0), new Position(0, text.length));
+        line.setup(x => x.range).returns(() => range);
+
         const document = TypeMoq.Mock.ofType<TextDocument>();
         document.setup(x => x.lineAt(TypeMoq.It.isAnyNumber())).returns(() => line.object);
+        document.setup(x => x.getText(TypeMoq.It.isAny())).returns(() => text);
 
         return formatter.formatLine(document.object, 0);
     }