Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Don`t process function signatures on comment lines #2496

Merged
merged 7 commits into from
May 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 8 additions & 2 deletions src/goSignature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import vscode = require('vscode');
import { SignatureHelpProvider, SignatureHelp, SignatureInformation, ParameterInformation, TextDocument, Position, CancellationToken, WorkspaceConfiguration } from 'vscode';
import { definitionLocation } from './goDeclaration';
import { getParametersAndReturnType, isPositionInString } from './util';
import { getParametersAndReturnType, isPositionInString, isPositionInComment } from './util';

export class GoSignatureHelpProvider implements SignatureHelpProvider {

Expand Down Expand Up @@ -94,12 +94,18 @@ export class GoSignatureHelpProvider implements SignatureHelpProvider {
for (let lineNr = position.line; lineNr >= 0 && maxLookupLines >= 0; lineNr-- , maxLookupLines--) {

const line = document.lineAt(lineNr);

// Stop processing if we're inside a comment
if (isPositionInComment(document, position)) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only catch the case where the entire line starts with the comment.
What about the below case?

x, err := regexp.Match("abc", data) // I have some comment here,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the most basic case of course. Either way, I'm noticing the bug isn't completely dependent on comments presence so there is still something else in it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to reuse the same function made for suggestions logic on this one

}

// if its current line, get the text until the position given, otherwise get the full line.
const [currentLine, characterPosition] = lineNr === position.line
? [line.text.substring(0, position.character), position.character]
: [line.text, line.text.length - 1];

for (let char = characterPosition - 1; char >= 0; char--) {
for (let char = characterPosition; char >= 0; char--) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometime during the last year I introduced this regression with the belief this would prevent an issue in the future, and while even though the code path shouldn't have caused any issues I remained none the wiser 🤦‍♂️

Sorry for that. This should fix it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem @lggomez, thanks for the fix!

switch (currentLine[char]) {
case '(':
parenBalance--;
Expand Down
17 changes: 1 addition & 16 deletions src/goSuggest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import path = require('path');
import vscode = require('vscode');
import cp = require('child_process');
import { getCurrentGoPath, getBinPath, getParametersAndReturnType, parseFilePrelude, isPositionInString, goKeywords, getToolsEnvVars, guessPackageNameFromFile, goBuiltinTypes, byteOffsetAt, runGodoc } from './util';
import { getCurrentGoPath, getBinPath, getParametersAndReturnType, parseFilePrelude, isPositionInString, goKeywords, getToolsEnvVars, guessPackageNameFromFile, goBuiltinTypes, byteOffsetAt, runGodoc, isPositionInComment } from './util';
import { getCurrentGoWorkspaceFromGOPATH } from './goPath';
import { promptForMissingTool, promptForUpdatingTool } from './goInstallTools';
import { getTextEditForAddImport } from './goImport';
Expand Down Expand Up @@ -497,21 +497,6 @@ function getCommentCompletion(document: vscode.TextDocument, position: vscode.Po
}
}

function isPositionInComment(document: vscode.TextDocument, position: vscode.Position): boolean {
const lineText = document.lineAt(position.line).text;

// prevent completion when typing in a line comment that doesnt start from the beginning of the line
const commentIndex = lineText.indexOf('//');

if (commentIndex >= 0 && position.character > commentIndex) {
const commentPosition = new vscode.Position(position.line, commentIndex);
const isCommentInString = isPositionInString(document, commentPosition);

return !isCommentInString;
}
return false;
}

function getCurrentWord(document: vscode.TextDocument, position: vscode.Position): string {
// get current word
const wordAtPosition = document.getWordRangeAtPosition(position);
Expand Down
18 changes: 18 additions & 0 deletions src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -954,3 +954,21 @@ export function runGodoc(cwd: string, packagePath: string, receiver: string, sym
});
});
}

/**
* Returns a boolean whether the current position lies within a comment or not
* @param document
* @param position
*/
export function isPositionInComment(document: vscode.TextDocument, position: vscode.Position): boolean {
const lineText = document.lineAt(position.line).text;
const commentIndex = lineText.indexOf('//');

if (commentIndex >= 0 && position.character > commentIndex) {
const commentPosition = new vscode.Position(position.line, commentIndex);
const isCommentInString = isPositionInString(document, commentPosition);

return !isCommentInString;
}
return false;
}