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

Incorrect import by auto-complete when pkgname != last part of pkg path #647

Closed
AlekSi opened this issue Nov 24, 2016 · 6 comments
Closed

Comments

@AlekSi
Copy link
Contributor

AlekSi commented Nov 24, 2016

That's a continuation of #613. Get code from https://github.com/AlekSi/vscode-bug and then edit main.go as shown:

2016-11-24_16-11-38

Two things are wrong after hitting Enter:

  1. It adds sarama.v1 to code. As I explained in Format removes imports of vendored packages in use #613, the package name is sarama, not sarama.v1. Package name != last part of import path.
  2. It adds vendor/gopkg.in/Shopify/sarama.v1 which is incorrect import path. vendor/ is treated differently by Go toolchain. goreturns does the right thing:
$ goreturns -d src/bugger/cmd/bugger/main.go
diff src/bugger/cmd/bugger/main.go gofmt/src/bugger/cmd/bugger/main.go
--- /var/folders/fl/c3xvc0tj1js__3jnrnbkjll40000gn/T/gofmt002523159	2016-11-24 16:22:09.000000000 +0300
+++ /var/folders/fl/c3xvc0tj1js__3jnrnbkjll40000gn/T/gofmt440158346	2016-11-24 16:22:09.000000000 +0300
@@ -1,5 +1,7 @@
 package main

+import sarama "gopkg.in/Shopify/sarama.v1"
+
 func main() {
 	sarama.V0_10_0_0
 }
@ramya-rao-a
Copy link
Contributor

@AlekSi There are 2 separate things at play here.

  • Auto-complete completes sara to sarama.v1 instead of sarama. This will happen to all packages from gopkg.in due to the versioning supported there.
  • vendor included in the import path. This will happen only when the vendor folder is the immediate child of $GOPATH/src.

For the first issue, I have a PR out: #659

For the second, I am curious. Is it very common to have the vendor folder right under #GOPATH/src ?
Most of the projects I have seen, have the vendor folder under the Go project.

@AlekSi
Copy link
Contributor Author

AlekSi commented Nov 29, 2016

Auto-complete completes sara to sarama.v1 instead of sarama. This will happen to all packages from gopkg.in due to the versioning supported there.

It has nothing to do with v1 and gopkg.in. For example, try with go-xerial-snappy:

2016-11-29_18-37-06

2016-11-29_18-37-19 2

As mentioned several times before, package name is defined by package clause in source code, not by last component of import path. cb0d2bc is incorrect.

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Nov 29, 2016

@AlekSi First off, thanks for your patience.

I now understand that the last element in the import path need not always be the package name. It is just convention for predictability's sake that most of the time the package name is the same as the last element in the import path.

The list of packages that appear in the auto-complete comes from the output of the gopkgs tool which simply returns the import paths to all the packages in $GOPATH/pkg folder. The extension tries to fetch the package name from these import paths and has no knowledge of the source code of the packages themselves.

go list all on the other hand can return package names along with import paths as well.
gopkgs was preferred over running go list all as the latter was slow in comparision

The difference of go list all and gopkgs is that go list all looks for go packages in your $GOPATH/src while gopkgs looks in your $GOPATH/pkg. As a result of this, only importable packages that have been installed (either by go get or go install) are listed by gopkgs.

We could try out a few options and compare performance

  • Use go list -f '{{ .Name }} {{ .ImportPath }}' all
  • Use gopkgs and feed the output to go list -f '{{ .Name }} {{ .ImportPath }}' space-separated-output-from-gopkgs
  • Update gopkgs to do second option above and return both name and import path

Until then, I would keep the fix in cb0d2bc because it atleast takes us one step closer to the ideal solution where versioned packages from gopkg.in will get the package name without the version number.

Thoughts?

mattetti added a commit to mattetti/vscode-go that referenced this issue Jan 11, 2017
* 'master' of github.com:mattetti/vscode-go: (128 commits)
  Add telemetry support for the Go extension
  Add dlv path to error msg when dlv not found
  Fixes microsoft#465 outline offset problem in No-English language (microsoft#699)
  fix coverage display when editor changes to undefined (microsoft#693)
  Add support for reading http.proxy setting (microsoft#639)
  Fixing flaky unit tests
  Refactor for corner cases
  Reduce noise in console log due to errors from hover
  Changelog for next update 0.6.51
  Remove space after brackets in snippet as per microsoft#628
  Fixes microsoft#647 Strip version from autocompleted pkgs from gopkg.in (microsoft#659)
  Fixes microsoft#647 Support vendor pkgs from root of GOPATH/src (microsoft#660)
  Fixes microsoft#640 Use spawn to handle big data from gocode (microsoft#661)
  Fixed wrong workspace when one gopath is the substring of another. (microsoft#658)
  Fix "gotests" when generates test for current function for type methods (microsoft#657)
  Add benchmark function snippet (microsoft#648)
  Fix "Go to Definition" when running Go HEAD (microsoft#655)
  Fixes microsoft#642 Only add -d once to formatflags (microsoft#644)
  Updating Changelog and package.json for 0.6.50 update
  Log errors to console
  ...
@ramya-rao-a ramya-rao-a reopened this Apr 17, 2017
@ramya-rao-a ramya-rao-a changed the title Incorrect import for vendored packages Incorrect import when pkgname != last part of pkg path May 4, 2017
@ramya-rao-a ramya-rao-a changed the title Incorrect import when pkgname != last part of pkg path Incorrect import by auto-complete when pkgname != last part of pkg path May 4, 2017
@ramya-rao-a ramya-rao-a added the bug label Jun 2, 2017
@AlekSi
Copy link
Contributor Author

AlekSi commented Jul 16, 2017

I think a long-term solution would be to import packages normally, without relying on gopkgs (which does not work correctly in this case), and keep them in memory in the long-running process (gocode? Go language server? something new?).

And thank you for all your work on this extenstion. I'm in love with it. ❤️

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Jul 16, 2017

Language server is a good idea here.

For now, I have a working solution where I am thinking of doing the below

  • go list -f {{.Name}};{{.ImportPath}} all runs once on extension activation (like when you open a new VS Code window)
  • Form mapping between package import path and package name from the above output.
  • Use this for the auto-completion

And thanks for your kind words. I'll try and keep up the good work :)

ramya-rao-a added a commit that referenced this issue Jul 17, 2017
* Use go list all to get pkgname to use in completions Fixes #647

* Check for version being null

* Wait for golist before testing
@ramya-rao-a
Copy link
Contributor

The fix for this issue is now out in the latest update (0.6.63) to the Go extension

Thanks for your patience @AlekSi, I know this took a while :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants