diff --git a/news/2 Fixes/2057.md b/news/2 Fixes/2057.md new file mode 100644 index 000000000000..34db4fed4207 --- /dev/null +++ b/news/2 Fixes/2057.md @@ -0,0 +1 @@ +Fix bug where tooltips would popup whenever a comma is typed within a string. diff --git a/src/client/providers/completionSource.ts b/src/client/providers/completionSource.ts index 764f9e570cfd..c287d4fa858c 100644 --- a/src/client/providers/completionSource.ts +++ b/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 { - 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 = { command: type, - fileName: filename, + fileName: document.fileName, columnIndex: columnIndex, lineIndex: position.line, source: source diff --git a/src/client/providers/providerUtilities.ts b/src/client/providers/providerUtilities.ts index 0a4f8274144d..7ee45ab8e25a 100644 --- a/src/client/providers/providerUtilities.ts +++ b/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 { - const text = document.getText(new vscode.Range(new vscode.Position(0, 0), tokenizeTo)); +export function getDocumentTokens(document: TextDocument, tokenizeTo: Position, mode: TokenizerMode): ITextRangeCollection { + 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; } diff --git a/src/client/providers/signatureProvider.ts b/src/client/providers/signatureProvider.ts index cf1014296519..f6ea0d65fd6e 100644 --- a/src/client/providers/signatureProvider.ts +++ b/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 { + // 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 = { command: proxy.CommandType.Arguments, fileName: document.fileName, diff --git a/src/test/providers/pythonSignatureProvider.unit.test.ts b/src/test/providers/pythonSignatureProvider.unit.test.ts new file mode 100644 index 000000000000..490eff19e9f9 --- /dev/null +++ b/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>; + let argResultItems: IArgumentsResult; + setup(() => { + const jediFactory = TypeMoq.Mock.ofType(JediFactory); + jediHandler = TypeMoq.Mock.ofType>(); + 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 { + const doc = TypeMoq.Mock.ofType(); + const position = new Position(0, pos); + const lineText = TypeMoq.Mock.ofType(); + const argsResult = TypeMoq.Mock.ofType(); + const cancelToken = TypeMoq.Mock.ofType(); + 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(); + 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 = TypeMoq.Mock.ofType(); + textLine.setup(t => t.text).returns(() => sourceLine); + const doc: TypeMoq.IMock = TypeMoq.Mock.ofType(); + 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('')); + }); +}); diff --git a/src/test/signature/signature.jedi.test.ts b/src/test/signature/signature.jedi.test.ts index 1c1d27f57a15..5805f7b56b04 100644 --- a/src/test/signature/signature.jedi.test.ts +++ b/src/test/signature/signature.jedi.test.ts @@ -50,11 +50,11 @@ suite('Signatures (Jedi)', () => { const expected = [ new SignatureHelpResult(5, 11, 0, 0, null), new SignatureHelpResult(5, 12, 1, 0, 'name'), - new SignatureHelpResult(5, 13, 1, 0, 'name'), - new SignatureHelpResult(5, 14, 1, 0, 'name'), - new SignatureHelpResult(5, 15, 1, 0, 'name'), - new SignatureHelpResult(5, 16, 1, 0, 'name'), - new SignatureHelpResult(5, 17, 1, 0, 'name'), + new SignatureHelpResult(5, 13, 0, 0, null), + new SignatureHelpResult(5, 14, 0, 0, null), + new SignatureHelpResult(5, 15, 0, 0, null), + new SignatureHelpResult(5, 16, 0, 0, null), + new SignatureHelpResult(5, 17, 0, 0, null), new SignatureHelpResult(5, 18, 1, 1, 'age'), new SignatureHelpResult(5, 19, 1, 1, 'age'), new SignatureHelpResult(5, 20, 0, 0, null) diff --git a/src/test/vscode-mock.ts b/src/test/vscode-mock.ts index eb401d8a4d54..1500ca249eae 100644 --- a/src/test/vscode-mock.ts +++ b/src/test/vscode-mock.ts @@ -63,6 +63,7 @@ mockedVSCode.SnippetString = vscodeMocks.vscMockExtHostedTypes.SnippetString; mockedVSCode.EventEmitter = vscodeMocks.vscMock.EventEmitter; mockedVSCode.ConfigurationTarget = vscodeMocks.vscMockExtHostedTypes.ConfigurationTarget; mockedVSCode.StatusBarAlignment = vscodeMocks.vscMockExtHostedTypes.StatusBarAlignment; +mockedVSCode.SignatureHelp = vscodeMocks.vscMockExtHostedTypes.SignatureHelp; // This API is used in src/client/telemetry/telemetry.ts const extensions = TypeMoq.Mock.ofType();