Skip to content

Commit c976b6c

Browse files
d3r3kkd3r3kk
authored
Ensure method parameter tooltip/popup never occurs within strings or comments (microsoft#2072)
- stop function signature popups in comments - Add tests for signature provider - incorporate Don's vscMock for SignatureHelp - previously expecting signatures within a string...?
1 parent cda34d4 commit c976b6c

File tree

7 files changed

+276
-28
lines changed

7 files changed

+276
-28
lines changed

‎news/2 Fixes/2057.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix bug where tooltips would popup whenever a comma is typed within a string.

‎src/client/providers/completionSource.ts‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -65,25 +65,18 @@ export class CompletionSource {
6565

6666
private async getCompletionResult(document: vscode.TextDocument, position: vscode.Position, token: vscode.CancellationToken)
6767
: Promise<proxy.ICompletionResult | undefined> {
68-
if (position.character <= 0) {
69-
return undefined;
70-
}
71-
const filename = document.fileName;
72-
const lineText = document.lineAt(position.line).text;
73-
if (lineText.match(/^\s*\/\//)) {
74-
return undefined;
75-
}
76-
// Suppress completion inside string and comments.
77-
if (isPositionInsideStringOrComment(document, position)) {
68+
if (position.character <= 0 ||
69+
isPositionInsideStringOrComment(document, position)) {
7870
return undefined;
7971
}
72+
8073
const type = proxy.CommandType.Completions;
8174
const columnIndex = position.character;
8275

8376
const source = document.getText();
8477
const cmd: proxy.ICommand<proxy.ICommandResult> = {
8578
command: type,
86-
fileName: filename,
79+
fileName: document.fileName,
8780
columnIndex: columnIndex,
8881
lineIndex: position.line,
8982
source: source

‎src/client/providers/providerUtilities.ts‎

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

4-
import * as vscode from 'vscode';
4+
import { Position, Range, TextDocument } from 'vscode';
55
import { Tokenizer } from '../language/tokenizer';
66
import { ITextRangeCollection, IToken, TokenizerMode, TokenType } from '../language/types';
77

8-
export function getDocumentTokens(document: vscode.TextDocument, tokenizeTo: vscode.Position, mode: TokenizerMode): ITextRangeCollection<IToken> {
9-
const text = document.getText(new vscode.Range(new vscode.Position(0, 0), tokenizeTo));
8+
export function getDocumentTokens(document: TextDocument, tokenizeTo: Position, mode: TokenizerMode): ITextRangeCollection<IToken> {
9+
const text = document.getText(new Range(new Position(0, 0), tokenizeTo));
1010
return new Tokenizer().tokenize(text, 0, text.length, mode);
1111
}
1212

13-
export function isPositionInsideStringOrComment(document: vscode.TextDocument, position: vscode.Position): boolean {
13+
export function isPositionInsideStringOrComment(document: TextDocument, position: Position): boolean {
1414
const tokenizeTo = position.translate(1, 0);
1515
const tokens = getDocumentTokens(document, tokenizeTo, TokenizerMode.CommentsAndStrings);
1616
const offset = document.offsetAt(position);
17-
let index = tokens.getItemContaining(offset);
17+
const index = tokens.getItemContaining(offset - 1);
1818
if (index >= 0) {
1919
const token = tokens.getItemAt(index);
2020
return token.type === TokenType.String || token.type === TokenType.Comment;
2121
}
22-
if (offset > 0) {
22+
if (offset > 0 && index >= 0) {
2323
// In case position is at the every end of the comment or unterminated string
24-
index = tokens.getItemContaining(offset - 1);
25-
if (index >= 0) {
26-
const token = tokens.getItemAt(index);
27-
return token.end === offset && token.type === TokenType.Comment;
28-
}
24+
const token = tokens.getItemAt(index);
25+
return token.end === offset && token.type === TokenType.Comment;
2926
}
3027
return false;
3128
}

‎src/client/providers/signatureProvider.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { JediFactory } from '../languageServices/jediProxyFactory';
1414
import { captureTelemetry } from '../telemetry';
1515
import { SIGNATURE } from '../telemetry/constants';
1616
import * as proxy from './jediProxy';
17+
import { isPositionInsideStringOrComment } from './providerUtilities';
1718

1819
const DOCSTRING_PARAM_PATTERNS = [
1920
'\\s*:type\\s*PARAMNAME:\\s*([^\\n, ]+)', // Sphinx
@@ -71,7 +72,7 @@ export class PythonSignatureProvider implements SignatureHelpProvider {
7172

7273
if (validParamInfo) {
7374
const docLines = def.docstring.splitLines();
74-
label = docLines.shift().trim();
75+
label = docLines.shift()!.trim();
7576
documentation = docLines.join(EOL).trim();
7677
} else {
7778
if (def.params && def.params.length > 0) {
@@ -111,6 +112,13 @@ export class PythonSignatureProvider implements SignatureHelpProvider {
111112
}
112113
@captureTelemetry(SIGNATURE)
113114
public provideSignatureHelp(document: TextDocument, position: Position, token: CancellationToken): Thenable<SignatureHelp> {
115+
// early exit if we're in a string or comment (or in an undefined position)
116+
if (position.character <= 0 ||
117+
isPositionInsideStringOrComment(document, position))
118+
{
119+
return Promise.resolve(new SignatureHelp());
120+
}
121+
114122
const cmd: proxy.ICommand<proxy.IArgumentsResult> = {
115123
command: proxy.CommandType.Arguments,
116124
fileName: document.fileName,

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

Lines changed: 248 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,248 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
'use strict';
5+
6+
// tslint:disable:max-func-body-length
7+
8+
import { assert, expect, use } from 'chai';
9+
import * as chaipromise from 'chai-as-promised';
10+
import * as TypeMoq from 'typemoq';
11+
import { CancellationToken, Position, SignatureHelp,
12+
TextDocument, TextLine, Uri } from 'vscode';
13+
import { JediFactory } from '../../client/languageServices/jediProxyFactory';
14+
import { IArgumentsResult, JediProxyHandler } from '../../client/providers/jediProxy';
15+
import { isPositionInsideStringOrComment } from '../../client/providers/providerUtilities';
16+
import { PythonSignatureProvider } from '../../client/providers/signatureProvider';
17+
18+
use(chaipromise);
19+
20+
suite('Signature Provider unit tests', () => {
21+
let pySignatureProvider: PythonSignatureProvider;
22+
let jediHandler: TypeMoq.IMock<JediProxyHandler<IArgumentsResult>>;
23+
let argResultItems: IArgumentsResult;
24+
setup(() => {
25+
const jediFactory = TypeMoq.Mock.ofType(JediFactory);
26+
jediHandler = TypeMoq.Mock.ofType<JediProxyHandler<IArgumentsResult>>();
27+
jediFactory.setup(j => j.getJediProxyHandler(TypeMoq.It.isAny()))
28+
.returns(() => jediHandler.object);
29+
pySignatureProvider = new PythonSignatureProvider(jediFactory.object);
30+
argResultItems = {
31+
definitions: [
32+
{
33+
description: 'The result',
34+
docstring: 'Some docstring goes here.',
35+
name: 'print',
36+
paramindex: 0,
37+
params: [
38+
{
39+
description: 'Some parameter',
40+
docstring: 'gimme docs',
41+
name: 'param',
42+
value: 'blah'
43+
}
44+
]
45+
}
46+
],
47+
requestId: 1
48+
};
49+
});
50+
51+
function testSignatureReturns(source: string, pos: number): Thenable<SignatureHelp> {
52+
const doc = TypeMoq.Mock.ofType<TextDocument>();
53+
const position = new Position(0, pos);
54+
const lineText = TypeMoq.Mock.ofType<TextLine>();
55+
const argsResult = TypeMoq.Mock.ofType<IArgumentsResult>();
56+
const cancelToken = TypeMoq.Mock.ofType<CancellationToken>();
57+
cancelToken.setup(ct => ct.isCancellationRequested).returns(() => false);
58+
59+
doc.setup(d => d.fileName).returns(() => '');
60+
doc.setup(d => d.getText(TypeMoq.It.isAny())).returns(() => source);
61+
doc.setup(d => d.lineAt(TypeMoq.It.isAny())).returns(() => lineText.object);
62+
doc.setup(d => d.offsetAt(TypeMoq.It.isAny())).returns(() => pos - 1); // pos is 1-based
63+
const docUri = TypeMoq.Mock.ofType<Uri>();
64+
docUri.setup(u => u.scheme).returns(() => 'http');
65+
doc.setup(d => d.uri).returns(() => docUri.object);
66+
lineText.setup(l => l.text).returns(() => source);
67+
argsResult.setup(c => c.requestId).returns(() => 1);
68+
argsResult.setup(c => c.definitions).returns(() => argResultItems[0].definitions);
69+
jediHandler.setup(j => j.sendCommand(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => {
70+
return Promise.resolve(argResultItems);
71+
});
72+
73+
return pySignatureProvider.provideSignatureHelp(doc.object, position, cancelToken.object);
74+
}
75+
76+
function testIsInsideStringOrComment(sourceLine: string, sourcePos: number) : boolean {
77+
const textLine: TypeMoq.IMock<TextLine> = TypeMoq.Mock.ofType<TextLine>();
78+
textLine.setup(t => t.text).returns(() => sourceLine);
79+
const doc: TypeMoq.IMock<TextDocument> = TypeMoq.Mock.ofType<TextDocument>();
80+
const pos: Position = new Position(1, sourcePos);
81+
82+
doc.setup(d => d.fileName).returns(() => '');
83+
doc.setup(d => d.getText(TypeMoq.It.isAny())).returns(() => sourceLine);
84+
doc.setup(d => d.lineAt(TypeMoq.It.isAny())).returns(() => textLine.object);
85+
doc.setup(d => d.offsetAt(TypeMoq.It.isAny())).returns(() => sourcePos);
86+
87+
return isPositionInsideStringOrComment(doc.object, pos);
88+
}
89+
90+
test('Ensure no signature is given within a string.', async () => {
91+
const source = ' print(\'Python is awesome,\')\n';
92+
const sigHelp: SignatureHelp = await testSignatureReturns(source, 27);
93+
expect(sigHelp).to.not.be.equal(undefined, 'Expected to get a blank signature item back - did the pattern change here?');
94+
expect(sigHelp.signatures.length).to.equal(0, 'Signature provided for symbols within a string?');
95+
});
96+
test('Ensure no signature is given within a line comment.', async () => {
97+
const source = '# print(\'Python is awesome,\')\n';
98+
const sigHelp: SignatureHelp = await testSignatureReturns(source, 28);
99+
expect(sigHelp).to.not.be.equal(undefined, 'Expected to get a blank signature item back - did the pattern change here?');
100+
expect(sigHelp.signatures.length).to.equal(0, 'Signature provided for symbols within a full-line comment?');
101+
});
102+
test('Ensure no signature is given within a comment tailing a command.', async () => {
103+
const source = ' print(\'Python\') # print(\'is awesome,\')\n';
104+
const sigHelp: SignatureHelp = await testSignatureReturns(source, 38);
105+
expect(sigHelp).to.not.be.equal(undefined, 'Expected to get a blank signature item back - did the pattern change here?');
106+
expect(sigHelp.signatures.length).to.equal(0, 'Signature provided for symbols within a trailing comment?');
107+
});
108+
test('Ensure signature is given for built-in print command.', async () => {
109+
const source = ' print(\'Python\',)\n';
110+
let sigHelp: SignatureHelp;
111+
try {
112+
sigHelp = await testSignatureReturns(source, 18);
113+
expect(sigHelp).to.not.equal(undefined, 'Expected to get a blank signature item back - did the pattern change here?');
114+
expect(sigHelp.signatures.length).to.not.equal(0, 'Expected dummy argresult back from testing our print signature.');
115+
expect(sigHelp.activeParameter).to.be.equal(0, 'Parameter for print should be the first member of the test argresult\'s params object.');
116+
expect(sigHelp.activeSignature).to.be.equal(0, 'The signature for print should be the first member of the test argresult.');
117+
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}`);
118+
} catch (error) {
119+
assert(false, `Caught exception ${error}`);
120+
}
121+
});
122+
test('Ensure isPositionInsideStringOrComment is behaving as expected.', () => {
123+
const sourceLine: string = ' print(\'Hello world!\')\n';
124+
const sourcePos: number = sourceLine.length - 1;
125+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
126+
127+
expect(isInsideStrComment).to.not.be.equal(true, [
128+
`Position set to the end of ${sourceLine} but `,
129+
'is reported as being within a string or comment.'].join(''));
130+
});
131+
test('Ensure isPositionInsideStringOrComment is behaving as expected at end of source.', () => {
132+
const sourceLine: string = ' print(\'Hello world!\')\n';
133+
const sourcePos: number = 0;
134+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
135+
136+
expect(isInsideStrComment).to.not.be.equal(true, [
137+
`Position set to the end of ${sourceLine} but `,
138+
'is reported as being within a string or comment.'].join(''));
139+
});
140+
test('Ensure isPositionInsideStringOrComment is behaving as expected at beginning of source.', () => {
141+
const sourceLine: string = ' print(\'Hello world!\')\n';
142+
const sourcePos: number = 0;
143+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
144+
145+
expect(isInsideStrComment).to.not.be.equal(true, [
146+
`Position set to the beginning of ${sourceLine} but `,
147+
'is reported as being within a string or comment.'].join(''));
148+
});
149+
test('Ensure isPositionInsideStringOrComment is behaving as expected within a string.', () => {
150+
const sourceLine: string = ' print(\'Hello world!\')\n';
151+
const sourcePos: number = 16;
152+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
153+
154+
expect(isInsideStrComment).to.be.equal(true, [
155+
`Position set within the string in ${sourceLine} (position ${sourcePos}) but `,
156+
'is reported as NOT being within a string or comment.'].join(''));
157+
});
158+
test('Ensure isPositionInsideStringOrComment is behaving as expected immediately before a string.', () => {
159+
const sourceLine: string = ' print(\'Hello world!\')\n';
160+
const sourcePos: number = 8;
161+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
162+
163+
expect(isInsideStrComment).to.be.equal(false, [
164+
`Position set to just before the string in ${sourceLine} (position ${sourcePos}) but `,
165+
'is reported as being within a string or comment.'].join(''));
166+
});
167+
test('Ensure isPositionInsideStringOrComment is behaving as expected immediately in a string.', () => {
168+
const sourceLine: string = ' print(\'Hello world!\')\n';
169+
const sourcePos: number = 9;
170+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
171+
172+
expect(isInsideStrComment).to.be.equal(true, [
173+
`Position set to the start of the string in ${sourceLine} (position ${sourcePos}) but `,
174+
'is reported as being within a string or comment.'].join(''));
175+
});
176+
test('Ensure isPositionInsideStringOrComment is behaving as expected within a comment.', () => {
177+
const sourceLine: string = '# print(\'Hello world!\')\n';
178+
const sourcePos: number = 16;
179+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
180+
181+
expect(isInsideStrComment).to.be.equal(true, [
182+
`Position set within a full line comment ${sourceLine} (position ${sourcePos}) but `,
183+
'is reported as NOT being within a string or comment.'].join(''));
184+
});
185+
test('Ensure isPositionInsideStringOrComment is behaving as expected within a trailing comment.', () => {
186+
const sourceLine: string = ' print(\'Hello world!\') # some comment...\n';
187+
const sourcePos: number = 34;
188+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
189+
190+
expect(isInsideStrComment).to.be.equal(true, [
191+
`Position set within a trailing line comment ${sourceLine} (position ${sourcePos}) but `,
192+
'is reported as NOT being within a string or comment.'].join(''));
193+
});
194+
test('Ensure isPositionInsideStringOrComment is behaving as expected at the very end of a trailing comment.', () => {
195+
const sourceLine: string = ' print(\'Hello world!\') # some comment...\n';
196+
const sourcePos: number = sourceLine.length - 1;
197+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
198+
199+
expect(isInsideStrComment).to.be.equal(true, [
200+
`Position set within a trailing line comment ${sourceLine} (position ${sourcePos}) but `,
201+
'is reported as NOT being within a string or comment.'].join(''));
202+
});
203+
test('Ensure isPositionInsideStringOrComment is behaving as expected within a multiline string.', () => {
204+
const sourceLine: string = ' stringVal = \'\'\'This is a multiline\nstring that you can use\nto test this stuff out with\neveryday!\'\'\'\n';
205+
const sourcePos: number = 48;
206+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
207+
208+
expect(isInsideStrComment).to.be.equal(true, [
209+
`Position set within a multi-line string ${sourceLine} (position ${sourcePos}) but `,
210+
'is reported as NOT being within a string or comment.'].join(''));
211+
});
212+
test('Ensure isPositionInsideStringOrComment is behaving as expected at the very last quote on a multiline string.', () => {
213+
const sourceLine: string = ' stringVal = \'\'\'This is a multiline\nstring that you can use\nto test this stuff out with\neveryday!\'\'\'\n';
214+
const sourcePos: number = sourceLine.length - 2; // just at the last '
215+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
216+
217+
expect(isInsideStrComment).to.be.equal(true, [
218+
`Position set within a multi-line string ${sourceLine} (position ${sourcePos}) but `,
219+
'is reported as NOT being within a string or comment.'].join(''));
220+
});
221+
test('Ensure isPositionInsideStringOrComment is behaving as expected within a multiline string (double-quoted).', () => {
222+
const sourceLine: string = ' stringVal = """This is a multiline\nstring that you can use\nto test this stuff out with\neveryday!"""\n';
223+
const sourcePos: number = 48;
224+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
225+
226+
expect(isInsideStrComment).to.be.equal(true, [
227+
`Position set within a multi-line string ${sourceLine} (position ${sourcePos}) but `,
228+
'is reported as NOT being within a string or comment.'].join(''));
229+
});
230+
test('Ensure isPositionInsideStringOrComment is behaving as expected at the very last quote on a multiline string (double-quoted).', () => {
231+
const sourceLine: string = ' stringVal = """This is a multiline\nstring that you can use\nto test this stuff out with\neveryday!"""\n';
232+
const sourcePos: number = sourceLine.length - 2; // just at the last '
233+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
234+
235+
expect(isInsideStrComment).to.be.equal(true, [
236+
`Position set within a multi-line string ${sourceLine} (position ${sourcePos}) but `,
237+
'is reported as NOT being within a string or comment.'].join(''));
238+
});
239+
test('Ensure isPositionInsideStringOrComment is behaving as expected during construction of a multiline string (double-quoted).', () => {
240+
const sourceLine: string = ' stringVal = """This is a multiline\nstring that you can use\nto test this stuff';
241+
const sourcePos: number = sourceLine.length - 1; // just at the last position in the string before it's termination
242+
const isInsideStrComment: boolean = testIsInsideStringOrComment(sourceLine, sourcePos);
243+
244+
expect(isInsideStrComment).to.be.equal(true, [
245+
`Position set within a multi-line string ${sourceLine} (position ${sourcePos}) but `,
246+
'is reported as NOT being within a string or comment.'].join(''));
247+
});
248+
});

0 commit comments

Comments
 (0)