Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gocode: switch to https://github.com/mdempsky/gocode #1853

Merged
merged 3 commits into from
Aug 27, 2018
Merged

Conversation

bhcleek
Copy link
Collaborator

@bhcleek bhcleek commented Jun 7, 2018

nsf/gocode is not working well with Go 1.10. I just encountered it
myself again. The author of original gocode also states and recommends
the mdempsky/gocode fork over nsf/gocode:
https://github.com/nsf/gocode#an-autocompletion-daemon-for-the-go-programming-language

We actually moved once, but
because we didn't removed the obsolote settings, we weren't prepared for
issues that were
reported, and hence
reverted it back

This PR also removes all settings. The reason is that the new gocode
doesn't support settings anymore. The author decided not the support it.

  • For autobuild, this was removed because the author believe it's not
    worth tackling it inside gocode: auto install doesn't always work mdempsky/gocode#13
  • For propose builtins, this is actually still supported but disabled in
    the source code, we need to add it back again in the future:
    mdempsky/gocode@787cfae
  • For unimported packages: this was just removed and I couldn't find
    upcoming plans for it

Going forward this package seems to be more stable and is a better bet.
Most important, as I stated nfs/gocode is not 1.10 compatible and the
author has no intentions to fix it.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Jun 7, 2018

Continuation of #1851

@bhcleek
Copy link
Collaborator Author

bhcleek commented Jun 7, 2018

@fatih I can't get this to work at all. I never get autocompletion results and type info is never displayed when g:go_info_mode='gocode'. nsf/gocode works fine for me, though.

Without the autobuild option, I wonder if we don't need to go install -i sometimes so that users will get accurate results 🤔

As a reminder to myself (and anyone else that tests): when testing, it's important to make sure to kill any running gocode processes before trying the other gocode fork. With nsf/gocode, the command is gocode close. For mdempsky/gocode, the command is gocode exit.

Here's the basic test procedure:

  1. stop any running gocode server processes
  2. set g:go_info_mode='gocode'
  3. set g:go_auto_type_info=1
  4. open a Go file
  5. position the cursor over an identifier and verify that the command-line display the type information for the identifer.
  6. Test autocompletion.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Jun 7, 2018

I'm getting inconsistent results. Sometimes autocomplete with mdempsky/gocode works, and sometimes it doesn't; I'll try to create some reproducible failures in a simple file.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Jun 7, 2018

The problem I was experiencing was that I hadn't done go install with one of the packages that is a dependency of the file in which I was trying to use autocompletion. With this PR as-is, because mdempsky/gocode doesn't support autobuild, users will have to go install all dependencies to get autocompletion to work correctly. That seems like a pretty big step backwards.

Without the autobuild feature, the utility of gocode goes way down. Interestingly, all the complaints in nsf/gocode about Go's package cache seem to be irrelevant when autobuild is enabled. Others seem to be reporting the same thing: nsf/gocode#500 (comment)

I see four possibilities to resolve these concerns:

  1. close this PR without merging; I have yet to see any problems with nsf/gocode when autobuild is enabled.
  2. wait to merge this PR until after mdempsky/gocode's cache branch is merged.
  3. use mdempsky/gocode's -source flag so that autocompletion doesn't need to packages to be installed to work. From a purist standpoint, I like this option the best, but performance of mdempsky/gocode's source importer is currently poor: autocompletion takes about 1.5 seconds 😞 .
  4. Implement our own mechanism for installing out-of-date packages and their dependencies.

Technically, this PR lgtm, but I don't think we should merge it due to the usability concerns.

@fatih
Copy link
Owner

fatih commented Jun 7, 2018

I see that autobuild is a very useful feature. I'm not happy myself either. However the bigger issue is that nfs/gocode straight up refuses to work anymore. I've tried everything, installing a new version, purging GOPATH/pkg, etc.. It just doesn't work anymore.

I could live with that if the author (nfs) had any intentions to fix the issue, but it's not. If we go this way, we'll end up having a crippled, not maintained autocompletion daemon. It will hurt more in the long term.

I totally understand that missing autobuild or other features sucks, but at least the project is active. The Go team also actively works on improving the current loaders and tools. Which means things like --source will eventually be very fast. Once that is the case, we'll just add that support (we can add it even today, but it wouldn't make sense due the slowness).

It's a double edged sword, but I think it should be merged because nfs/gocode is broken, doesn't work and going forward will be not fixed in any case.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Jun 7, 2018

Let's merge it then and deal with the problems it introduces. Before you do that, though, I only have one quick question: have you tried running the nsf/gocode server with the -debug flag to see what's going wrong?

@fatih
Copy link
Owner

fatih commented Jun 8, 2018

It's weird. After installing nfs/godoce again, deleting the old binary and starting with gocode -debug -s it started working again. I wonder what was the issue here :/

@ramya-rao-a
Copy link

Hello @fatih and @bhcleek,

I am very glad to have found this thread. I want to share some of my findings from the Go plugin to VS Code's perspective

  • We found that using the autoBuild option of nsf/gocode was slowing things down. We disabled this option (user could enable it if they wanted to) last November. We were able to do this as we use the -i flag when the code is built on save. See 0.6.67 makes typing in a medium-sized project too laggy microsoft/vscode-go#1323 (comment) and 0.6.67 makes typing in a medium-sized project too laggy microsoft/vscode-go#1323 (comment). I am curious, did you ever have perf issues by enabling this option?
  • We moved to using mdempsky/gocode this June when we got complaints on completions just not working on 1.10 and above. I didnt make the connection between autoBuild feature of gocode and this issue before, and so the only option I had was to make the move to using the fork
  • We use the -builtin flag when using mdempsky/gocode which is the counterpart to nsf/gocode's propose builtin
  • We also found that using -source slows things down significantly and so we dont use it as of now and are also hoping that the cache branch gets merged soon
  • We also have users who are using nsf/gocode with Go 1.10 and above who dont see any issues.

In conclusion, I do wish we had a unified solution here, but there isnt seem to be any. I am gate crashing this thread in hopes of being notified of further discussions in this space.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Aug 4, 2018

I am curious, did you ever have perf issues by enabling this option?
I've never noticed any performance issues specifically related to using autobuild, but I can certainly imagine that there could be performance issues when large packages have to be rebuilt.

Although we didn't see performance issues specifically related to autobuild, we did see some performance issues with gocode, especially when calls to gocode had to be executed synchronously. Those issues were mostly resolved in #1697.

We also found that using -source slows things down significantly and so we dont use it as of now and are also hoping that the cache branch gets merged soon

Yes, the -source flag is too slow without caching. Once the caching branch is merged and is stable, we will likely switch to using mdempsky/gocode.

@joshuarubin
Copy link
Contributor

FWIW, I've had to switch to github.com/visualfc/gocode as it's the only version that supports the upcoming Go modules.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Aug 6, 2018

@joshuarubin thank you for the tip. We've been discussing the work that needs to be done to vim-go to support modules.

@bhcleek bhcleek mentioned this pull request Aug 6, 2018
4 tasks
@isaachess
Copy link

I'm currently experiencing a severe slowdown in performance from using autobuild. I'm using vim 8.1, go1.10.3 linux/amd64. If I don't disable gocode's autobuild, on some files I experience a full 5-to-10-second delay between pressing the . and seeing the autocomplete results. When I disable autobuild the results appear instantaneously (though I assume will eventually get out of date).

For now I've disabled autobuild, otherwise gocode has become completely unusable for me.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Aug 26, 2018

Builtins are now supported again, so I've added that option back in.

The source mode is slow on big projects, but is easier to work with, because packages don't have to be installed manually. I've added an option to use it source mode, but it's disabled by default so that users can opt-in.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Aug 26, 2018

@fatih Given that nsf/gocode panics on Go 1.11 packages due to changes in the export format, I think it's time to merge this. WDYT?

fatih and others added 3 commits August 26, 2018 10:42
nsf/gocode is not working well with Go 1.10. I just encountered it
myself again. The author of original gocode also states and recommends
the mdempsky/gocode fork over nsf/gocode:
https://github.com/nsf/gocode#an-autocompletion-daemon-for-the-go-programming-language

We actually [moved once](#1814), but
because we didn't removed the obsolote settings, we weren't prepared for
issues that were
[reported](#1817), and hence
[reverted it back](#1823)

This PR also removes all settings. The reason is that the new gocode
doesn't support settings anymore. The author decided not the support it.

* For `autobuild`, this was removed because the author believe it's not
worth tackling it inside `gocode`: mdempsky/gocode#13
* For `propose builtins`, this is actually still supported but disabled in
the source code, we need to add it back again in the future:
mdempsky/gocode@787cfae
* For `unimported packages`: this was just removed and I couldn't find
upcoming plans for it

Going forward this package seems to be more stable and is a better bet.
Most important, as I stated nfs/gocode is not 1.10 compatible and the
author has no intentions to fix it.
@fatih
Copy link
Owner

fatih commented Aug 27, 2018

lgtm

@bhcleek let's do it. nfs is a dead end. There is not much we can do here as we know. Let's continue with mdempsky's fork.

@bhcleek bhcleek merged commit 4fdc590 into master Aug 27, 2018
@bhcleek bhcleek deleted the gocode-change branch August 27, 2018 15:28
bhcleek added a commit that referenced this pull request Aug 27, 2018
@ramya-rao-a
Copy link

awesome! Thats 3 out of 3. Now vim-go, go-plus and vscode-go all use mdempsky's fork. Meaning we all will see the same issues and can look into finding solutions.

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

Successfully merging this pull request may close these issues.

5 participants