Ensure method parameter tooltip/popup never occurs within strings or comments

news/2 Fixes/2057.md

@@ -0,0 +1 @@
+Fix bug where tooltips would popup whenever a comma is typed within a string.

src/client/providers/completionSource.ts

@@ -65,25 +65,18 @@ export class CompletionSource {
 
     private async getCompletionResult(document: vscode.TextDocument, position: vscode.Position, token: vscode.CancellationToken)
         : Promise<proxy.ICompletionResult | undefined> {
-        if (position.character <= 0) {
-            return undefined;
-        }
-        const filename = document.fileName;
-        const lineText = document.lineAt(position.line).text;
-        if (lineText.match(/^\s*\/\//)) {
-            return undefined;
-        }
-        // Suppress completion inside string and comments.
-        if (isPositionInsideStringOrComment(document, position)) {
+        if (position.character <= 0 ||
+            isPositionInsideStringOrComment(document, position)) {
             return undefined;
         }
+
         const type = proxy.CommandType.Completions;
         const columnIndex = position.character;
 
         const source = document.getText();
         const cmd: proxy.ICommand<proxy.ICommandResult> = {
             command: type,
-            fileName: filename,
+            fileName: document.fileName,
             columnIndex: columnIndex,
             lineIndex: position.line,
             source: source

src/client/providers/providerUtilities.ts

@@ -1,31 +1,28 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
-import * as vscode from 'vscode';
+import { Position, Range, TextDocument } from 'vscode';
 import { Tokenizer } from '../language/tokenizer';
 import { ITextRangeCollection, IToken, TokenizerMode, TokenType } from '../language/types';
 
-export function getDocumentTokens(document: vscode.TextDocument, tokenizeTo: vscode.Position, mode: TokenizerMode): ITextRangeCollection<IToken> {
-    const text = document.getText(new vscode.Range(new vscode.Position(0, 0), tokenizeTo));
+export function getDocumentTokens(document: TextDocument, tokenizeTo: Position, mode: TokenizerMode): ITextRangeCollection<IToken> {
+    const text = document.getText(new Range(new Position(0, 0), tokenizeTo));
     return new Tokenizer().tokenize(text, 0, text.length, mode);
 }
 
-export function isPositionInsideStringOrComment(document: vscode.TextDocument, position: vscode.Position): boolean {
+export function isPositionInsideStringOrComment(document: TextDocument, position: Position): boolean {
     const tokenizeTo = position.translate(1, 0);
     const tokens = getDocumentTokens(document, tokenizeTo, TokenizerMode.CommentsAndStrings);
     const offset = document.offsetAt(position);
-    let index = tokens.getItemContaining(offset);
+    const index = tokens.getItemContaining(offset - 1);
     if (index >= 0) {
         const token = tokens.getItemAt(index);
         return token.type === TokenType.String || token.type === TokenType.Comment;
     }
-    if (offset > 0) {
+    if (offset > 0 && index >= 0) {
         // In case position is at the every end of the comment or unterminated string
-        index = tokens.getItemContaining(offset - 1);
-        if (index >= 0) {
-            const token = tokens.getItemAt(index);
-            return token.end === offset && token.type === TokenType.Comment;
-        }
+        const token = tokens.getItemAt(index);
+        return token.end === offset && token.type === TokenType.Comment;
     }
     return false;
 }

src/client/providers/signatureProvider.ts

@@ -14,6 +14,7 @@ import { JediFactory } from '../languageServices/jediProxyFactory';
 import { captureTelemetry } from '../telemetry';
 import { SIGNATURE } from '../telemetry/constants';
 import * as proxy from './jediProxy';
+import { isPositionInsideStringOrComment } from './providerUtilities';
 
 const DOCSTRING_PARAM_PATTERNS = [
     '\\s*:type\\s*PARAMNAME:\\s*([^\\n, ]+)', // Sphinx
@@ -71,7 +72,7 @@ export class PythonSignatureProvider implements SignatureHelpProvider {
 
                 if (validParamInfo) {
                     const docLines = def.docstring.splitLines();
-                    label = docLines.shift().trim();
+                    label = docLines.shift()!.trim();
                     documentation = docLines.join(EOL).trim();
                 } else {
                     if (def.params && def.params.length > 0) {
@@ -111,6 +112,13 @@ export class PythonSignatureProvider implements SignatureHelpProvider {
     }
     @captureTelemetry(SIGNATURE)
     public provideSignatureHelp(document: TextDocument, position: Position, token: CancellationToken): Thenable<SignatureHelp> {
+        // early exit if we're in a string or comment (or in an undefined position)
+        if (position.character <= 0 ||
+            isPositionInsideStringOrComment(document, position))
+        {
+            return Promise.resolve(new SignatureHelp());
+        }
+
         const cmd: proxy.ICommand<proxy.IArgumentsResult> = {
             command: proxy.CommandType.Arguments,
             fileName: document.fileName,

src/test/providers/pythonSignatureProvider.unit.test.ts

@@ -0,0 +1,248 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+'use strict';
+
+// tslint:disable:max-func-body-length
+
+import { assert, expect, use } from 'chai';
+import * as chaipromise from 'chai-as-promised';
+import * as TypeMoq from 'typemoq';
+import { CancellationToken, Position, SignatureHelp,
+    TextDocument, TextLine, Uri } from 'vscode';
+import { JediFactory } from '../../client/languageServices/jediProxyFactory';
+import { IArgumentsResult, JediProxyHandler } from '../../client/providers/jediProxy';
+import { isPositionInsideStringOrComment } from '../../client/providers/providerUtilities';
+import { PythonSignatureProvider } from '../../client/providers/signatureProvider';
+
+use(chaipromise);
+
+suite('Signature Provider unit tests', () => {
+    let pySignatureProvider: PythonSignatureProvider;
+    let jediHandler: TypeMoq.IMock<JediProxyHandler<IArgumentsResult>>;
+    let argResultItems: IArgumentsResult;
+    setup(() => {
+        const jediFactory = TypeMoq.Mock.ofType(JediFactory);
+        jediHandler = TypeMoq.Mock.ofType<JediProxyHandler<IArgumentsResult>>();
+        jediFactory.setup(j => j.getJediProxyHandler(TypeMoq.It.isAny()))
+            .returns(() => jediHandler.object);
+        pySignatureProvider = new PythonSignatureProvider(jediFactory.object);
+        argResultItems = {
+            definitions: [
+                {
+                    description: 'The result',
+                    docstring: 'Some docstring goes here.',
+                    name: 'print',
+                    paramindex: 0,
+                    params: [
+                        {
+                            description: 'Some parameter',
+                            docstring: 'gimme docs',
+                            name: 'param',
+                            value: 'blah'
+                        }
+                    ]
+                }
+            ],
+            requestId: 1
+        };
+    });
+
+    function testSignatureReturns(source: string, pos: number): Thenable<SignatureHelp> {
+        const doc = TypeMoq.Mock.ofType<TextDocument>();
+        const position = new Position(0, pos);
+        const lineText = TypeMoq.Mock.ofType<TextLine>();
+        const argsResult = TypeMoq.Mock.ofType<IArgumentsResult>();
+        const cancelToken = TypeMoq.Mock.ofType<CancellationToken>();
+        cancelToken.setup(ct => ct.isCancellationRequested).returns(() => false);
+
+        doc.setup(d => d.fileName).returns(() => '');
+        doc.setup(d => d.getText(TypeMoq.It.isAny())).returns(() => source);
+        doc.setup(d => d.lineAt(TypeMoq.It.isAny())).returns(() => lineText.object);
+        doc.setup(d => d.offsetAt(TypeMoq.It.isAny())).returns(() => pos - 1); // pos is 1-based
+        const docUri = TypeMoq.Mock.ofType<Uri>();
+        docUri.setup(u => u.scheme).returns(() => 'http');
+        doc.setup(d => d.uri).returns(() => docUri.object);
+        lineText.setup(l => l.text).returns(() => source);
+        argsResult.setup(c => c.requestId).returns(() => 1);
+        argsResult.setup(c => c.definitions).returns(() => argResultItems[0].definitions);
+        jediHandler.setup(j => j.sendCommand(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => {
+            return Promise.resolve(argResultItems);
+        });
+
+        return pySignatureProvider.provideSignatureHelp(doc.object, position, cancelToken.object);
+    }
+
+    function testIsInsideStringOrComment(sourceLine: string, sourcePos: number) : boolean {
+        const textLine: TypeMoq.IMock<TextLine> = TypeMoq.Mock.ofType<TextLine>();
+        textLine.setup(t => t.text).returns(() => sourceLine);
+        const doc: TypeMoq.IMock<TextDocument> = TypeMoq.Mock.ofType<TextDocument>();
+        const pos: Position = new Position(1, sourcePos);
+
+        doc.setup(d => d.fileName).returns(() => '');
+        doc.setup(d => d.getText(TypeMoq.It.isAny())).returns(() => sourceLine);
+        doc.setup(d => d.lineAt(TypeMoq.It.isAny())).returns(() => textLine.object);
+        doc.setup(d => d.offsetAt(TypeMoq.It.isAny())).returns(() => sourcePos);
+
+        return isPositionInsideStringOrComment(doc.object, pos);
+    }
+
+    test('Ensure no signature is given within a string.', async () => {
+        const source = '  print(\'Python is awesome,\')\n';
+        const sigHelp: SignatureHelp = await testSignatureReturns(source, 27);
+        expect(sigHelp).to.not.be.equal(undefined, 'Expected to get a blank signature item back - did the pattern change here?');
+        expect(sigHelp.signatures.length).to.equal(0, 'Signature provided for symbols within a string?');
+    });
+    test('Ensure no signature is given within a line comment.', async () => {
+        const source = '#  print(\'Python is awesome,\')\n';
+        const sigHelp: SignatureHelp = await testSignatureReturns(source, 28);
+        expect(sigHelp).to.not.be.equal(undefined, 'Expected to get a blank signature item back - did the pattern change here?');
+        expect(sigHelp.signatures.length).to.equal(0, 'Signature provided for symbols within a full-line comment?');
+    });
+    test('Ensure no signature is given within a comment tailing a command.', async () => {
+        const source = '  print(\'Python\') # print(\'is awesome,\')\n';
+        const sigHelp: SignatureHelp = await testSignatureReturns(source, 38);
+        expect(sigHelp).to.not.be.equal(undefined, 'Expected to get a blank signature item back - did the pattern change here?');
+        expect(sigHelp.signatures.length).to.equal(0, 'Signature provided for symbols within a trailing comment?');
+    });
+    test('Ensure signature is given for built-in print command.', async () => {
+        const source = '  print(\'Python\',)\n';
+        let sigHelp: SignatureHelp;
+        try {
+            sigHelp = await testSignatureReturns(source, 18);
+            expect(sigHelp).to.not.equal(undefined, 'Expected to get a blank signature item back - did the pattern change here?');
+            expect(sigHelp.signatures.length).to.not.equal(0, 'Expected dummy argresult back from testing our print signature.');
+            expect(sigHelp.activeParameter).to.be.equal(0, 'Parameter for print should be the first member of the test argresult\'s params object.');
+            expect(sigHelp.activeSignature).to.be.equal(0, 'The signature for print should be the first member of the test argresult.');
+            expect(sigHelp.signatures[sigHelp.activeSignature].label).to.be.equal('print(param)', `Expected arg result calls for specific returned signature of \'print(param)\' but we got ${sigHelp.signatures[sigHelp.activeSignature].label}`);
+        } catch (error) {
+            assert(false, `Caught exception ${error}`);
+        }
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected.', () => {
+        const sourceLine: string = '  print(\'Hello world!\')\n';
+        const sourcePos: number = sourceLine.length - 1;
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.not.be.equal(true, [
+            `Position set to the end of ${sourceLine} but `,
+            'is reported as being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected at end of source.', () => {
+        const sourceLine: string = '  print(\'Hello world!\')\n';
+        const sourcePos: number = 0;
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.not.be.equal(true, [
+            `Position set to the end of ${sourceLine} but `,
+            'is reported as being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected at beginning of source.', () => {
+        const sourceLine: string = '  print(\'Hello world!\')\n';
+        const sourcePos: number = 0;
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.not.be.equal(true, [
+            `Position set to the beginning of ${sourceLine} but `,
+            'is reported as being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected within a string.', () => {
+        const sourceLine: string = '  print(\'Hello world!\')\n';
+        const sourcePos: number = 16;
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.be.equal(true, [
+            `Position set within the string in ${sourceLine} (position ${sourcePos}) but `,
+            'is reported as NOT being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected immediately before a string.', () => {
+        const sourceLine: string = '  print(\'Hello world!\')\n';
+        const sourcePos: number = 8;
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.be.equal(false, [
+            `Position set to just before the string in ${sourceLine} (position ${sourcePos}) but `,
+            'is reported as being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected immediately in a string.', () => {
+        const sourceLine: string = '  print(\'Hello world!\')\n';
+        const sourcePos: number = 9;
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.be.equal(true, [
+            `Position set to the start of the string in ${sourceLine} (position ${sourcePos}) but `,
+            'is reported as being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected within a comment.', () => {
+        const sourceLine: string = '#  print(\'Hello world!\')\n';
+        const sourcePos: number = 16;
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.be.equal(true, [
+            `Position set within a full line comment ${sourceLine} (position ${sourcePos}) but `,
+            'is reported as NOT being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected within a trailing comment.', () => {
+        const sourceLine: string = '  print(\'Hello world!\') # some comment...\n';
+        const sourcePos: number = 34;
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.be.equal(true, [
+            `Position set within a trailing line comment ${sourceLine} (position ${sourcePos}) but `,
+            'is reported as NOT being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected at the very end of a trailing comment.', () => {
+        const sourceLine: string = '  print(\'Hello world!\') # some comment...\n';
+        const sourcePos: number = sourceLine.length - 1;
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.be.equal(true, [
+            `Position set within a trailing line comment ${sourceLine} (position ${sourcePos}) but `,
+            'is reported as NOT being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected within a multiline string.', () => {
+        const sourceLine: string = '  stringVal = \'\'\'This is a multiline\nstring that you can use\nto test this stuff out with\neveryday!\'\'\'\n';
+        const sourcePos: number = 48;
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.be.equal(true, [
+            `Position set within a multi-line string ${sourceLine} (position ${sourcePos}) but `,
+            'is reported as NOT being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected at the very last quote on a multiline string.', () => {
+        const sourceLine: string = '  stringVal = \'\'\'This is a multiline\nstring that you can use\nto test this stuff out with\neveryday!\'\'\'\n';
+        const sourcePos: number = sourceLine.length - 2; // just at the last '
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.be.equal(true, [
+            `Position set within a multi-line string ${sourceLine} (position ${sourcePos}) but `,
+            'is reported as NOT being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected within a multiline string (double-quoted).', () => {
+        const sourceLine: string = '  stringVal = """This is a multiline\nstring that you can use\nto test this stuff out with\neveryday!"""\n';
+        const sourcePos: number = 48;
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.be.equal(true, [
+            `Position set within a multi-line string ${sourceLine} (position ${sourcePos}) but `,
+            'is reported as NOT being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected at the very last quote on a multiline string (double-quoted).', () => {
+        const sourceLine: string = '  stringVal = """This is a multiline\nstring that you can use\nto test this stuff out with\neveryday!"""\n';
+        const sourcePos: number = sourceLine.length - 2; // just at the last '
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.be.equal(true, [
+            `Position set within a multi-line string ${sourceLine} (position ${sourcePos}) but `,
+            'is reported as NOT being within a string or comment.'].join(''));
+    });
+    test('Ensure isPositionInsideStringOrComment is behaving as expected during construction of a multiline string (double-quoted).', () => {
+        const sourceLine: string = '  stringVal = """This is a multiline\nstring that you can use\nto test this stuff';
+        const sourcePos: number = sourceLine.length - 1; // just at the last position in the string before it's termination
+        const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
+
+        expect(isInsideStrComment).to.be.equal(true, [
+            `Position set within a multi-line string ${sourceLine} (position ${sourcePos}) but `,
+            'is reported as NOT being within a string or comment.'].join(''));
+    });
+});