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

x/tools/cmd/gopls: should support modules in rootURI subdirectories #30841

Closed
vanackere opened this issue Mar 14, 2019 · 13 comments
Closed

x/tools/cmd/gopls: should support modules in rootURI subdirectories #30841

vanackere opened this issue Mar 14, 2019 · 13 comments

Comments

@vanackere
Copy link
Contributor

@vanackere vanackere commented Mar 14, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/vince/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/vince"
GOPROXY=""
GORACE=""
GOROOT="/home/vince/go"
GOTMPDIR=""
GOTOOLDIR="/home/vince/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/vince/dev/go.mod"

What did you do?

I am using vscode to edit a project consisting of several subdirectories, one of them being the root of my go module.

- doc
- resources
...
- src
    go.mod
    go.sum

What did you expect to see?

The root directory /home/vince/dev is opened in vscode, the go module is at /home/vince/dev/src/go.mod. When opening src/admin/file.go I'd like to have working completion / hover / ...

What did you see instead?

gopls does not work: the newColumnMap function returns an error "no file information for file:///home/vince/dev/src/admin/file.go"
(tok := f.GetToken(ctx) returns nil)

Manually updating the packages.Config.Dir path in server.go (line 108) works around the problem (Dir: rootPath + "/src") but of course this is not a proper fix

Should gopls try to find the appropriate go module for each given file by itself ? Or should the vscode-go plugin be responsible for it ? (In this case this bug should be reassigned to https://github.com/Microsoft/vscode-go)

@gopherbot gopherbot added this to the Unreleased milestone Mar 14, 2019
@stamblerre stamblerre added the gopls label Mar 22, 2019
@broady
Copy link
Member

@broady broady commented Mar 25, 2019

I found this, too.

go list (and therefore go/packages) only seems to work properly when its pwd is inside the module being resolved.

~/issue30841 $ tree
.
└── a
    ├── b
    │   ├── go.mod
    │   └── main.go
    ├── c
    │   └── main.go
    ├── go.mod
    └── main.go

3 directories, 5 files

Works:

~/issue30841/a $ go list $(pwd)
github.com/broady/issue30841/a

~/issue30841/a $ go list $(pwd)/c
github.com/broady/issue30841/a/c

~/issue30841/a/c $ go list $(pwd)
github.com/broady/issue30841/a/c

~/issue30841/a/b $ go list $(pwd)
github.com/broady/issue30841/a/b

Does not work (pwd is outside module):

~/issue30841 $ go list $(pwd)/a
go: cannot find main module; see 'go help modules'
Exit code 1

~/issue30841/a $ go list $(pwd)/b
can't load package: package github.com/broady/issue30841/a/b: unknown import path "github.com/broady/issue30841/a/b": cannot find module providing package github.com/broady/issue30841/a/b

I found this diff seems to load everything properly in vscode, but I'm not sure if it's the correct fix, since some tests fail:

diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index 2082b47d..7a912bd4 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -99,6 +99,7 @@ func (v *View) checkMetadata(ctx context.Context, f *File) ([]packages.Error, er
        if v.reparseImports(ctx, f, filename) {
                cfg := v.Config
                cfg.Mode = packages.LoadImports
+               cfg.Dir = filepath.Dir(filename)
                pkgs, err := packages.Load(&cfg, fmt.Sprintf("file=%s", filename))
                if len(pkgs) == 0 {
                        if err == nil {
@vanackere
Copy link
Contributor Author

@vanackere vanackere commented Mar 26, 2019

@broady : I applied your change locally and it makes the language server work properly for me at least

@kofj
Copy link

@kofj kofj commented Mar 29, 2019

@broady thx~

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Mar 29, 2019

This issue is a duplicate of #29174, and as the discussion there suggests, @broady's fix will not actually work. We are trying to figure out a good approach for fixing this problem, but it's difficult because LSP sends a rootURI when it is initialized, so we can't just use the current working directory.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Mar 29, 2019

Duplicate of #29174

@stamblerre stamblerre marked this as a duplicate of #29174 Mar 29, 2019
@movie-travel-code
Copy link

@movie-travel-code movie-travel-code commented Apr 8, 2019

We are trying to figure out a good approach for fixing this problem, but it's difficult because LSP sends a rootURI when it is initialized, so we can't just use the current working directory.

Encounter this problem too, based on @broady's fix, I use the following approach for the time being. Looking forward to the official fix!

	originDir := cfg.Dir

	fdir := filepath.Dir(f.filename)
	if !strings.HasPrefix(fdir, filepath.Join(os.Getenv("GOPATH"), "pkg")) {
		cfg.Dir = fdir
	}
	pkgs, err := packages.Load(&cfg, fmt.Sprintf("file=%s", f.filename))

	cfg.Dir = originDir
movie-travel-code added a commit to movie-travel-code/tools that referenced this issue Apr 8, 2019
…folders.

Since the `go list` command, which is a necessary command to resolve the
packages, is running at the rootURI folder, we have to run server at
rootURI too, see golang/go#30841.
@ianthehat
Copy link

@ianthehat ianthehat commented Apr 8, 2019

The official fix is already in, we support multiple workspace folders now.
The job of finding the go.mod files that people care about and adding their folders to the workspace is up to either the user or the editor/client, but once that has happened, gopls will correctly work in those directories.

@vanackere
Copy link
Contributor Author

@vanackere vanackere commented Apr 8, 2019

The job of finding the go.mod files that people care about and adding their folders to the workspace is up to either the user or the editor/client, but once that has happened, gopls will correctly work in those directories.

@ianthehat :

  • is there already some support in the vscode-go plugin for this ? I had a look but see nothing related in the changelog
  • is there a reason why the gopls server couldn't find the proper go.mod file by itself (by scanning the rootURI subdirectories for example), I don't understand why every editor/client should have to implement this by itself
@ianthehat
Copy link

@ianthehat ianthehat commented Apr 8, 2019

No, there is no automatic support, just add the directories to your workspace by hand.
The trouble as discussed in the other bugs is that there is no correct answer.
If you think through the edge cases of nested modules and replace directives with differing versions of dependancies there is no reasonable way to pick the right target of a cross module jump without understanding the flow that got the user to that point.
This means it is a question of intent and style, and thus cannot (and should not be) be answered automatically by gopls.

@movie-travel-code
Copy link

@movie-travel-code movie-travel-code commented Apr 9, 2019

Thanks, @ianthehat! You are right, the editor should provide the whole picture of the project, in contrast, the server should focus on the functionality.

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 12, 2019

@ianthehat, is there anything remaining to be done for this issue?

@vanackere
Copy link
Contributor Author

@vanackere vanackere commented Apr 15, 2019

No, there is no automatic support, just add the directories to your workspace by hand.
The trouble as discussed in the other bugs is that there is no correct answer.
If you think through the edge cases of nested modules and replace directives with differing versions of dependancies there is no reasonable way to pick the right target of a cross module jump without understanding the flow that got the user to that point.
This means it is a question of intent and style, and thus cannot (and should not be) be answered automatically by gopls.

@ianthehat : how should editors implement the necessary support ? Right now this is the most annoying issue with the language server, with no specific editor support for this, having to manually open the root directory of each module does not make for a great user experience at all.
As a sidenote: I also believe that although there are probably many edge cases that can't be handled automatically, there should at least be some simple fallback in the language server that would make it work for the vast majority of cases that are not corner cases if the editor did not provide the required information.

@ianthehat
Copy link

@ianthehat ianthehat commented May 6, 2019

The last piece for gopls of this is tracked in #31635 and fixed in https://go-review.googlesource.com/c/tools/+/175477
A better error message is being tracked in #31668

I don't want to dictate the right approach to editors, but I would suggest adding a workspace folder for all go.mod files seen at the earliest opportunity would cover the 99% case.

@ianthehat ianthehat closed this May 6, 2019
@golang golang locked and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.