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

Fix #1072 Avoid completions in block comments #1116

Closed
wants to merge 1 commit into from
Closed

Fix #1072 Avoid completions in block comments #1116

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 14 additions & 0 deletions src/goSuggest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,14 @@ export class GoCompletionItemProvider implements vscode.CompletionItemProvider {
return this.provideCompletionItemsInternal(document, position, token, vscode.workspace.getConfiguration('go'));
}

private isInBlockComment(document: vscode.TextDocument, position: vscode.Position): boolean {
let text = document.getText();
let currentOffset = document.offsetAt(position);
let lastBlockCommentStartIndex = text.lastIndexOf('/*', currentOffset);
let lastBlockCommentEndIndex = text.lastIndexOf('*/', currentOffset);
return lastBlockCommentStartIndex > lastBlockCommentEndIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few cases where this will give a false positive
One example is

	hi := "/*"

	bye := "*/"

You won't get completions between the above 2 lines

OR in between the below 2 lines

hi := "/*" 
	
	// This is a comment */

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing to think about is that we don't want completions inside multi line strings as well. Whatever approach we end up using for multi line comments can be used in multi line strings as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch, I indeed missed that, thanks for pointing this out.

Those cases are IMHO rather difficult to handle with plain string magic - perhaps a better
approach would be to use the go tooling for this.
Perhaps we could leverage the AST
derived via go Parser to find those single line / block comment as well as multiline string literals.

But I don't know how well the parser can deal with incomplete source code...
WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would definitely be nice to have the AST in hand during completions

Apart from stopping completions inside multi line comments and multi line strings, it would also help in #168

But I don't know how well the parser can deal with incomplete source code...

I dont know much here either.

We currently use https://github.com/ramya-rao-a/go-outline to get the file outline which uses the go parser. You can try and see if we can re-use that here

Copy link
Contributor Author

@thomasdarimont thomasdarimont Jul 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Just gave this a spin - it seems that I need to adjust go-outline to also return the comments.

Would it be possible to change the output of go-outline?
For instance currently go-outline returns an array of "Symbol"-Declarations in a file.
How about returning a "CompilationUnit" that contains those declarations and some additional comments?

With my branch I can get the comments with their respective positions.

E.g. for the go file hello.go:

package main

import (
	"fmt"
)

func hello() string {

	// single line comment

	// single line comment line1
	// single line comment line2

	/* block comment in single line */

	/*
		multi line block comment
	*/

	slString := "single line string"
	mlSlString := `multiline single line string`
	mlString := `
		multiline
		string
	`

	commentStart := "/*"
	// foo
	commentEnd := "*/"

	fmt.Println(slString, mlSlString, mlString, commentStart, commentEnd)

	return "hello"
}

I get the following json representation:

{
  "declarations": [
    {
      "label": "main",
      "type": "package",
      "start": 1,
      "end": 476,
      "children": [
        {
          "label": "\"fmt\"",
          "type": "import",
          "start": 25,
          "end": 30
        },
        {
          "label": "hello",
          "type": "function",
          "start": 34,
          "end": 476
        }
      ]
    }
  ],
  "comments": [
    {
      "start": 58,
      "end": 80,
      "text": "// single line comment"
    },
    {
      "start": 83,
      "end": 111,
      "text": "// single line comment line1"
    },
    {
      "start": 113,
      "end": 141,
      "text": "// single line comment line2"
    },
    {
      "start": 144,
      "end": 178,
      "text": "/* block comment in single line */"
    },
    {
      "start": 181,
      "end": 214,
      "text": "/*\n\t\tmulti line block comment\n\t*/"
    },
    {
      "start": 359,
      "end": 365,
      "text": "// foo"
    }
  ]
}

I could also try to add the comments to the respective declarations where they
belong to... but I thought a flat list might be easier to work with.

I wonder whether it would make sense to transform the full go AST to JSON instead of
adapting the "minimal" representation for every new feature.

wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I need some time to digest this. Give me a few days?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure no problem! Thanks for having a look into it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@thomasdarimont So I finally had time to look at your changes to go-outline.

Am not very comfortable changing the structure of the output, we will have to maintain 2 different ways of consuming the output, one for people still on the old version of go-outline, the other for the new.

Why not re-use the Declaration struct for comments. You can have type as comments, label as the comment text, Children would be empty array.

This way the final output structure would be the same as before.
An outer declaration for package whose children would declarations who in turn can be functions, types, variables, constants and comments

We can have a flag -include-comments. So that existing users of this tool will see no change in the usage even if they got the latest. We would return the comments, if this flag has been provided.

Copy link

Choose a reason for hiding this comment

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

Hi @ramya-rao-a, I've created a separate PR here (ramya-rao-a/go-outline#7) to push this fix forward.

}

public provideCompletionItemsInternal(document: vscode.TextDocument, position: vscode.Position, token: vscode.CancellationToken, config: vscode.WorkspaceConfiguration): Thenable<vscode.CompletionItem[]> {
return this.ensureGoCodeConfigured().then(() => {
return new Promise<vscode.CompletionItem[]>((resolve, reject) => {
Expand All @@ -52,10 +60,16 @@ export class GoCompletionItemProvider implements vscode.CompletionItemProvider {
let lineTillCurrentPosition = lineText.substr(0, position.character);
let autocompleteUnimportedPackages = config['autocompleteUnimportedPackages'] === true && !lineText.match(/^(\s)*(import|package)(\s)+/);

// no completions within single line comment
if (lineText.match(/^\s*\/\//)) {
return resolve([]);
}

// no completions within block comment
if (this.isInBlockComment(document, position)) {
return resolve([]);
}

let inString = isPositionInString(document, position);
if (!inString && lineTillCurrentPosition.endsWith('\"')) {
return resolve([]);
Expand Down