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

automatic import adder should not separate first import from its comment #1701

Closed
mbenkmann opened this issue May 31, 2018 · 9 comments · Fixed by #3045
Closed

automatic import adder should not separate first import from its comment #1701

mbenkmann opened this issue May 31, 2018 · 9 comments · Fixed by #3045
Labels
help wanted needs-decision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@mbenkmann
Copy link
Contributor

mbenkmann commented May 31, 2018

  1. Relevant Settings:

    "go.autocompleteUnimportedPackages": true
    
  2. Type the following program

package main

//#include <stdlib.h>
import "C"

func main() {
    fmt
}
  1. Place cursor behind "fmt"
  2. Type "." The Intellisense window should pop up with suggestions.
  3. Accept any suggestion (e.g. "Errorf")
  4. The import statement will be rewritten to
//#include <stdlib.h>
import (
	"fmt"
	"C"
)

However this breaks the magic of import "C" which uses the preceding comment.

When converting import lines to a parenthesized import block, vscode-go should insert the "import (" before the comment lines immediately preceding (i.e. with no empty lines in between) the first import (if there are any).

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented May 31, 2018

Can you share what happens when you just type out fmt.Println("hello") and then format the file? The formatter adds missing imports as well. I am curious to see how that is handled.

Also, do you know of other cases like this where the import statement should not be updated?

@mbenkmann
Copy link
Contributor Author

If vscode-go is supposed to add imports on "Format Document", then it doesn't seem to be working. Only if I set the formatTool to "goimports" does "Format Document" add imports. As for goimports' behaviour, it simply adds another import line without switching to the parentheses form, so it does not break the code.

@mbenkmann
Copy link
Contributor Author

I am not aware of any other magic packages aside from "C".

@mbenkmann
Copy link
Contributor Author

Having a brief a look at the code. The culprit seems to be src/goImport.ts:getTextEditForAddImport. As the code is currently written it simply inserts "import (" before the first import and then replaces "import" with "\t". In a case like this

import "foo"

//...
import "C"

import "bar"

that process works fine. It's only when the "C" is the first import that this breaks because it inserts the "import (" between the "C" and the comment. That's the actual issue. If the first package is preceded by a comment line, then the "import (" must be inserted before the comment. I don't even see this as a special case for "C". If someone writes code like this

// Use package "foo" instead of "newfoo" because the latter is still too buggy
import "foo"

converting it to

// Use package "foo" instead of "newfoo" because the latter is still too buggy

import (
  "foo"
  "fmt"
)

also breaks the association between the package and the comment.

So the solution would look something like this:

Current code:

edits.push(vscode.TextEdit.insert(new vscode.Position(imports[0].start, 0), 'import (\n\t"' + arg + '"\n'));

Fixed code (pseudo-code as I don't know vscode-go well enough):

insertBeforeLine = imports[0].start;
while (insertBeforeLine > 0) && textOfLine(insertBeforeLine-1).matches(/^\s*\/\//) {
  insertBeforeLine = insertBeforeLine - 1 ;
}
edits.push(vscode.TextEdit.insert(new vscode.Position(insertBeforeLine, 0), 'import (\n\t"' + arg + '"\n'));

This only handles // comments but I think that's fine. All of the examples in the documentation of cgo use these comments, so I'm sure 99% of programmers do that, too.

@mbenkmann mbenkmann changed the title automatic import adder should leave import "C" alone automatic import adder should not separate first import from its comment Jun 1, 2018
@stamblerre
Copy link
Contributor

This is a feature request for goimports - VS Code Go should not modify the output produced by goimports.

@heschik: Would you support a change like this or is this working as intended?

@stamblerre stamblerre added needs-decision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed bug labels Feb 12, 2020
@heschi
Copy link

heschi commented Feb 12, 2020

Seems someone got to it in the intervening 2 years.

$ cat foo.go
package main

//#include <stdlib.h>
import "C"

func main() {
    fmt.Print()
}
$ goimports foo.go
package main

//#include <stdlib.h>
import "C"
import "fmt"

func main() {
	fmt.Print()
}

@stamblerre
Copy link
Contributor

Great! Closing this issue since it is now fixed.

@stamblerre
Copy link
Contributor

Ah, my mistake - I didn't realize that VS Code had extra code for handling goimports, so while this is fixed in goimports, it's not in VS Code Go. As an alternative, @mbenkmann, you can try using the language server, which does not have this problem.

@stamblerre stamblerre reopened this Feb 12, 2020
hyangah added a commit to hyangah/vscode-go-old that referenced this issue Feb 13, 2020
getTextEditForAddImport tries to insert a newly added package
into an existing import group. If no import group exists, it
tries to merge single line import statements and create a group.

In this process, import statements like

  import "C"

is a pseudo import and shouldn't be selected for import grouping.
This CL changes parseFilePrelude to detect such pseudo imports
and excludes such imports from the grouping logic.

Fixes microsoft#1701
hyangah added a commit to hyangah/vscode-go-old that referenced this issue Feb 13, 2020
getTextEditForAddImport tries to insert a newly added package
into an existing import group. If no import group exists, it
tries to merge single line import statements and create a group.

In this process, import statements like

  import "C"

are pseudo imports and shouldn't be grouped with other imports.
This CL changes parseFilePrelude to detect such pseudo imports
and excludes such imports from the grouping logic.

Fixes microsoft#1701
hyangah added a commit to hyangah/vscode-go-old that referenced this issue Apr 20, 2020
getTextEditForAddImport tries to insert a newly added package
into an existing import group. If no import group exists, it
tries to merge single line import statements and create a group.

In this process, import statements like

  import "C"

are pseudo imports and shouldn't be grouped with other imports.
This CL changes parseFilePrelude to detect such pseudo imports
and excludes such imports from the grouping logic.

Fixes microsoft#1701
ramya-rao-a pushed a commit that referenced this issue Apr 27, 2020
* goImports: avoid collapsing new imports into pseudo import line

getTextEditForAddImport tries to insert a newly added package
into an existing import group. If no import group exists, it
tries to merge single line import statements and create a group.

In this process, import statements like

  import "C"

are pseudo imports and shouldn't be grouped with other imports.
This CL changes parseFilePrelude to detect such pseudo imports
and excludes such imports from the grouping logic.

Fixes #1701

* correct the file name

mac and vsccode ignored casing, but linux and git were unhappy.
@ramya-rao-a
Copy link
Contributor

The latest update 0.14.2 has the fix for this, Thanks @hyangah
And thanks for reporting @mbenkmann

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted needs-decision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants