goreturns erroneously disabled

[galileo-pkm]

When using modules vscode returns this message:
"goreturns doesnt support auto-importing missing imports when using Go modules yet. So updating the "formatTool" setting to goimports for this workspace."

That is false, goreturns works perfectly fine with go modules as can be seen from this example:

[galileo@sol server]$ echo $GO111MODULE
on
[galileo@sol server]$ go env 
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/galileo/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/galileo/ws/vsc/go/test_go_path/go:/home/galileo/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/golang"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/galileo/ws/vsc/go/test_go_path/project/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build621340973=/tmp/go-build -gno-record-gcc-switches"
[galileo@sol server]$ 
[galileo@sol server]$ 
[galileo@sol server]$ cat server.go 
package main

import (
        "net/http"
)

func main() {
        test := model.CU()
        _ = test

        e := echo.New()
        e.GET("/", func(c echo.Context) error {
                return c.String(http.StatusOK, "Hello, World!")
        })
        e.Logger.Fatal(e.Start(":1323"))
}
[galileo@sol server]$ 
[galileo@sol server]$ 
[galileo@sol server]$ 
[galileo@sol server]$ 
[galileo@sol server]$ 
[galileo@sol server]$ 
[galileo@sol server]$ goreturns server.go 
package main

import (
        "net/http"

        "github.com/galileo-pkm/go-mod-test/model"
        "github.com/labstack/echo/v4"
)

func main() {
        test := model.CU()
        _ = test

        e := echo.New()
        e.GET("/", func(c echo.Context) error {
                return c.String(http.StatusOK, "Hello, World!")
        })
        e.Logger.Fatal(e.Start(":1323"))
}

To make matters worse vscode changes the format tool to goimports which brakes the code
with versioned imports:

[galileo@sol server]$ cat server.go 
package main

import (
        "net/http"

        "github.com/galileo-pkm/go-mod-test/model"
        "github.com/labstack/echo/v4"
)

func main() {
        test := model.CU()
        _ = test

        e := echo.New()
        e.GET("/", func(c echo.Context) error {
                return c.String(http.StatusOK, "Hello, World!")
        })
        e.Logger.Fatal(e.Start(":1323"))
}
[galileo@sol server]$ 
[galileo@sol server]$ goimports server.go 
package main

import (
        "net/http"

        "github.com/galileo-pkm/go-mod-test/model"
)

func main() {
        test := model.CU()
        _ = test

        e := echo.New()
        e.GET("/", func(c echo.Context) error {
                return c.String(http.StatusOK, "Hello, World!")
        })
        e.Logger.Fatal(e.Start(":1323"))
}

Forcing a (broken in this case) choice on a user is newer the right to do. If I'm about to shoot myself in the foot, by all means display a warning (preferably once, not repeatedly) but let me proceed.
Hackish "fix" is to remove the write permissions from the settings file, vscode displays a warning but works as expected.
This is with the latest version (1.35.1).

[galileo-pkm]

Nothing?

[ramya-rao-a]

@galileo-pkm Thanks for reporting and your patience.

As per golang/go#24661, the issue that tracks Go module support in various Go tools in the eco system, goreturns has not been updated yet to support modules. We had received many reports of the same, and eventually goimports did support modules, thus the decision to change the default format tool when we detect that modules are being used without the language server

@stamblerre, @ianthehat Did something change here that goreturns is now able to work as expected i.e add missing imports in this case? If so, then I can update the extension to stop prompting the change.

[galileo-pkm]

I have been using vscode with my "fix" for a while now and the only issue is the annoying warnings.
True issue here is that vscode forces a broken choice on a user, if a haven't thought of that "fix" I would have been forced to either stop using vscode or stop using modules, as goimports breaks the code with versioned imports.

IMHO that kind of behaviour may be acceptable for software that caters to a non technical audience but in a developer tool, it is definitely a wrong thing to do.
I understand that this probably is not the best place to discuss the issue but maybe it's a good place to start?

[ramya-rao-a]

@galileo-pkm
I have pushed a fix to allow dismissal of the warning with an option of not ever showing it again. Can you please try out the latest beta version of this extension and let me know if you still see the warnings?

@ianthehat, @stamblerre What would be the best place for @galileo-pkm to log a bug against goimports for the issue of versioned imports getting added?

[galileo-pkm]

@galileo-pkm
I have pushed a fix to allow dismissal of the warning with an option of not ever showing it again. Can you please try out the latest beta version of this extension and let me know if you still see the warnings?

Just tried it, it works without issues.
As usual you have been most helpful Ramya, thank you.

@ianthehat, @stamblerre What would be the best place for @galileo-pkm to log a bug against goimports for the issue of versioned imports getting added?

An issue has been posted by multiple people almost a year ago, no one cares:
golang/go#26882

Two other minor issues if it matters:

With 0.11.1-beta.1 I get this notification:
"vscode-icons has detected an Angular project. Select 'Restart' to enable the Angular icons."
I don't do any JS, so I guess there is something in the plugin that triggered vscode icons?

go.inferGopath: There is no technical reason to disable inferGopath when using modules. I usually set up GOPATH in the folder settings, and in this case my GOPATH is the same as if it was setup by go.inferGopath and everything works without issues. Maybe something to look into also?
My current folder settings:

[galileo@sol ~]$ cat /home/galileo/ws/vsc/go/ff_mod/.vscode/settings.json
{
    "go.inferGopath": false,
    "go.toolsGopath": "~/go",
    "go.gopath": "/home/galileo/ws/vsc/go/ff_mod:/home/galileo/go",    
    "go.formatTool": "goreturns"
}

[stamblerre]

@galileo-pkm: Are you using the latest version of goimports (go get -u golang.org/x/tools/cmd/goimports)? Looking at your repro case, I don't think that should happen, and it might be different from the bug you linked. If this reproduces with the latest version of goimports, please file a bug with the repro case on the Go issue tracker: https://github.com/golang/go/issues.

Also, @ramya-rao-a , neither Ian nor I would know much about goreturns, since that is a community driven tool. Looking at its code, it looks like it just calls functions defined in golang.org/x/tools/cmd/imports, so it should support modules fine.

[ramya-rao-a]

Looking at its code, it looks like it just calls functions defined in golang.org/x/tools/cmd/imports, so it should support modules fine

Ah! So, goreturns never had to do anything special to get modules working as long as the underlying imports package worked? @sqs Can you confirm this? If this indeed the case, then we should update the check mark next to goreturns in golang/go#24661

@galileo-pkm,

• The notification for Angular icons seems to be coming from the vscode-icons extension, please log an issue for it. The Go extension does nothing with icons.
• Enabling go.inferGopath setting makes the extension go up the folder path trying to find a "src" folder and then treat that folder's parent as the GOPATH. Since when working outside of GOPATH, this is un-necessary, I initially added this logic.
  • What is the reason for you to set the go.gopath when you are working with modules?
  • Do you have a src, pkg and bin right under /home/galileo/ws/vsc/go/ff_mod?

[stamblerre]

Oh you know I may have actually spoken too soon. I assumed goreturns only parsed, but looks like it does type-check files as well. In that case, it doesn't seem to use modules yet as I don't see it using go/packages. My mistake.

[sqs]

Yes, goreturns must typecheck because it needs to know if (eg) return foo() is a multiple return or a single return. That’s a rare enough use case that I probably should make it not try to fix those cases, which would eliminate the need for typechecking. But it is needed now.

[ramya-rao-a]

@sqs, So what would you say the state of modules support by goreturns is?

[galileo-pkm]

@galileo-pkm: Are you using the latest version of goimports (go get -u golang.org/x/tools/cmd/goimports)? Looking at your repro case, I don't think that should happen, and it might be different from the bug you linked. If this reproduces with the latest version of goimports, please file a bug with the repro case on the Go issue tracker: https://github.com/golang/go/issues.

The issue seems to be fixed with the latest (v0.0.0-20190628222527-fb37f6ba8261) version of go imports. I forgot the check what was the version that I had but I did the "Go:Install/Update tools" via VSCode about 2 weeks ago so it must have been fixed in the meantime.

[galileo-pkm]

@galileo-pkm,

* The notification for Angular icons seems to be coming from the vscode-icons extension, please log an issue for it. The Go extension does nothing with icons.

It popped up after I updated to the beta version that you provided so I presumed it was related, its just icons, it doesn't really matter.

* Enabling `go.inferGopath` setting makes the extension go up the folder path trying to find a "src" folder and then treat that folder's parent as the GOPATH. Since when working outside of GOPATH, this is un-necessary, I initially added this logic.
  
  * What is the reason for you to set the `go.gopath` when you are working with modules?
  * Do you have a `src`, `pkg` and `bin` right under /home/galileo/ws/vsc/go/ff_mod?

The reason for setting up a per project gopath is the same as when I was using go.inferGopath:
I do not want to clutter my ~/go folder with junk. I use different storage backends and backup methods for my home folder and workspaces so it is mainly driver by that.
I usually setup the traditional (src, bin, pkg) structure for new projects but not always.

Whether something is necessary or not is not the point. The main point here is overriding user configuration. That is newer a good decision. Imagine if Apache, BIND and the rest of the core internet software authors decided that it was a good idea that those services modify their own configuration and force their choice instead of the users. Half of the internet services would have been broken and there would be pitchfork mobs after them ;)

[ramya-rao-a]

Since goimports has the fix and the notification for using it in place of goreturns is now dismissable, l am closing this issue.

@galileo-pkm Can you log a new issue with the proposal for not disabling go.inferGopath?

[ramya-rao-a]

The latest update (0.11.1) to the Go extension now allows dismissing of the notification