Use goimports fork for modules support

[heschi]

I've pushed a beta version of goimports that uses go/packages for module support. As usual, it's slower than we'd like it to be but should be functional. I think it's ready for use in the same circumstances as the godef and gocode forks.

It does have one slight difference in behavior vs. standard goimports, described in golang/go#28428, but that change should be merged soon.

[heschi]

@ramya-rao-a: need anything else from me?

[ramya-rao-a]

@heschik I have pushed the necessary changes, and this will be part of the next update

[coder543]

I'm not 100% sure, but this could be breaking things right now. goimports was seemingly never being run, and switching back to gofmt definitely worked in VS Code settings, so I started digging in the code here and found that it might be trying to call goimports-gomod which isn't in my path. So, I go get installed the fork, then copied it to goimports-gomod, and things started working.

However, I'm not sure this code change has actually been deployed to the extension store yet, so another possibility is that goimports (non-fork) simply doesn't work on module-enabled projects at this point, and that by installing the fork on top of goimports and goimports-gomod that I fixed things that way.

EDIT: the on save behavior is still strangely unreliable. I can run goreturns -w . from PowerShell in my directory, and it works fine, but if I set goreturns as the on save formatter, nothing ever happens when I save.

[ramya-rao-a]

I'm not sure this code change has actually been deployed to the extension store yet,

Yes, this code change has not been released yet. You can use our beta version which does have the changes

So, I go get installed the fork, then copied it to goimports-gomod, and things started working.

Am not sure how this would work as the changes to use goimports-gomod is not yet out. Were you using the extension by running it from source?

goimports (non-fork) simply doesn't work on module-enabled projects at this point,

That is true

but if I set goreturns as the on save formatter, nothing ever happens when I save.

How about when you manually run the formatter? Right click -> Format Document?

[coder543]

Under Output -> Log (Window), I found the problem:

Aborted format on save after 750ms

So, this explains why it was being finicky. I increased the Format On Save timeout and the issue went away.

[heschi]

@ramya-rao-a if this still hasn't been released it might make sense to roll it back. The fork is affected by golang/go#28645, and after some internal discussions it looks like we're going to try to get tip working with modules. (Similar to godef.)

If it has been released, no problem.

[ramya-rao-a]

This has not been released @heschik

after some internal discussions it looks like we're going to try to get tip working with modules. (Similar to godef.)

That's great news. In that case, I'll just revert the changes done as part of this issue.

[atombender]

@ramya-rao-a Until this is fixed, how do I make the github.com/heschik/goimports fork work with Go 1.11 and modules?

I installed the fork, and via ps I can see that it's being executed (goimports -srcdir <file>). If I run it manually on the command line, it works, and correctly adds missing imports. But inside VSCode, while the code is formatted correctly, imports are never automatically added.

I don't understand why the tool would behave differently in the shell, vs. inside VSCode. My environment should be identical.

[atombender]

...Looks like it's because VScode runs goimports from a different directory. goimports only works if invoked with the current working directory set to the root of the module. I bet this also applies to the official goimports tool, once fixed. (go get also works differently if invoked inside a module, vs. outside. I don't like that.)

Edit: I wrote a little wrapper script that sets the right $PWD, and I can confirm that it works.

[ramya-rao-a]

@atombender I dont sent any specific cwd when spawning the process for goimports. See https://github.com/Microsoft/vscode-go/blob/0.7.0/src/goFormat.ts#L64. That can be easily fixed.

goimports only works if invoked with the current working directory set to the root of the module

I can either set the cwd to the folder that was opened in VS Code, or the parent directory of the file that is being formatted. Would the latter work?

[atombender]

Yes, the parent directory of the file is the most appropriate. I've confirmed that it works correctly for subdirectories under a module.

This comment is relevant to the above. Not sure if goimports will be fixed, or if it will require the right working directory.

[ramya-rao-a]

@atombender Can you remove the wrapper and try the latest beta version of this extension? I have updated it to use the parent directory of the file as cwd.

[atombender]

Fix confirmed. 🎉 Thank you!

[ramya-rao-a]

Thanks! Since you already have the beta version now, you can also check out the upcoming features in the next update. Take a look at #2164

[ianthehat]

With regards to setting the CWD, it has to be set to a folder in which go env GOMOD would return the module file for the project. This probably means the directory that was opened, not the parent directory of the file.
This is a general rule for all tools in a module aware world. And is easiest to explain for tools that only examine rather than modify code.

For instance, if you jump to definition in one of the projects source files, it may take you to a file in the module cache. If you set the CWD to the parent of that file when invoking any other tool, the tool is going to fail because the file is not on your GOPATH or part of a true module. And even if it does work, it may produce inconsistent results because it has different module version results to that of your project.
If you run it with the original CWD though, it can use that module to understand the code and dependencies, and will still get the right answer, even though the file in question is not below that directory.
Only the editor can have enough context to pick the CWD correctly, which is why the tools just trust whatever CWD they are given rather than trying to guess one from the file= queries.
Hope that makes it clear?

[atombender]

I understand the problem, but is that relevant to goimports and formatting? If we only consider Go modules (not pre-modules GOPATH or vendor), I see two use cases:

1. You run goimports on a module within your own tree. In this case, go env GOMOD is always the next parent in the file system that has a go.mod, as I understand it.

2. You do a "jump to definition" and end up in a file outside your project, presumably inside the module cache. But you're not going to be running goimports on that code anyway. But if you did, it would resolve imports relative to that module, by finding the parent module, which would be correct. You wouldn't want that to resolve imports relative to the module in which you jumped from.

Are there other use cases?

[ianthehat]

It is less of a problem for goimports than almost any other tool, but we want one clear rule that works for all tools in all editors, and most other tools will care.
If a tool would be happy with using the parent dir of the file, the tool could do that itself, so why get all the editors to do it. If it wants to know the workspace and you have changed the directory to the file, it has no way to recover that information. Setting the CWD to the workspace allows the tool to make that decision for itself.

Extra cases to consider:

• multi-module repositories
• a module with a replace directive

Also, if you ran it in the module cache it might not do it relative to that module, because on disk it might not be a valid module, go env GOMOD may return the empty string, and it's certainly not on your GOPATH.
Also it may be reasonable to run it on all files, even in the module cache. For instance, goimports does not have to write the file, you might be running it to see if it would change and suggesting fixes before the file is saved, and not want to have to special case the module cache to turn that behavior off.

[ramya-rao-a]

So, in this case, are we saying that the editor should use the workspace as cwd or that goimports should figure out the right directory based on the file being imported?

[atombender]

The workspace will be the wrong directory if the file being formatted is in a sub-module. E.g. you have /go.mod and /src/sub/go.mod, and the file being formatted is /src/sub/main.go. In this case, /src/sub is the right directory.

In my opinion, from what I understood of @ianthehat's explanation, the directory of the current file will always work for us when formatting with goimports. This won't work if you open something in the module cache, or something outside of a module. But then goimports will still work, it will just not be able to automatically add new imports.