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/gopls: build mod diagnostics with GOWORK=off #50862

Open
findleyr opened this issue Jan 27, 2022 · 6 comments
Open

x/tools/gopls: build mod diagnostics with GOWORK=off #50862

findleyr opened this issue Jan 27, 2022 · 6 comments
Assignees
Labels
gopls modules Tools
Milestone

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Jan 27, 2022

gopls relies on errors produced by the go command to suggest fixes to our users, but when using go.work, we don't get all of these errors. We can work around this by running go commands with -workfile=off to build our diagnostics.

Example (based on gopls/internal/regtest/workspace.TestUseGoWork):

[workspace]> ls -R
.:
go.work  moda  modb  tags

./moda:
a

./moda/a:
a.go  go.mod  go.sum

./modb:
b.go  go.mod  go.sum
[workspace]> cat ./modb/go.mod
module b.com

require golang.org/x/tools v0.1.9
[workspace]> cat ./moda/a/go.mod
module a.com

require b.com v1.2.3
[workspace]> cd modb
[modb]> GOPROXY=off go list all
../moda/a/a.go:4:2: b.com@v1.2.3: module lookup disabled by GOPROXY=off
[modb]> GOPROXY=off go list -workfile=off all
go: golang.org/x/tools@v0.1.9: module lookup disabled by GOPROXY=off
go: golang.org/x/tools@v0.1.9: module lookup disabled by GOPROXY=off

CC @matloob

@gopherbot gopherbot added Tools gopls labels Jan 27, 2022
@gopherbot gopherbot added this to the Unreleased milestone Jan 27, 2022
@findleyr findleyr removed this from the Unreleased milestone Jan 27, 2022
@findleyr findleyr added this to the gopls/v0.8.0 milestone Jan 27, 2022
@findleyr findleyr assigned matloob and findleyr and unassigned matloob Jan 27, 2022
@bcmills
Copy link
Member

@bcmills bcmills commented Jan 27, 2022

b.com@v1.2.3: module lookup disabled by GOPROXY=off

That error message looks deeply wrong to me. module lookup disabled should imply that we are looking up an imported package, but b.com@v1.2.3 implies that the module containing the package is already known. We should either get a module@version in the error or a module lookup disabled, but never both in the same error.

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 27, 2022

(That is: I agree that there is at least one bug here, but it may be more fundamental than workspace mode. 😅)

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Jan 27, 2022

Some more interesting behavior! For some reason -e -json behaves quite differently with -workfile=off.

[modb]> GOPROXY=off go list -e -json b.com/...
{
        "Dir": "/home/rfindley/tmp/workspace/modb",
        "ImportPath": "b.com",
        "Name": "b",
        "Root": "/home/rfindley/tmp/workspace/modb",
        "Module": {
                "Path": "b.com",
                "Main": true,
                "Dir": "/home/rfindley/tmp/workspace/modb",
                "GoMod": "/home/rfindley/tmp/workspace/modb/go.mod"
        },
        "Match": [
                "b.com/..."
        ],
        "Stale": true,
        "StaleReason": "build ID mismatch",
        "GoFiles": [
                "b.go"
        ]
}
{
        "ImportPath": "b.com/...",
        "Match": [
                "b.com/..."
        ],
        "Incomplete": true,
        "Error": {
                "ImportStack": [],
                "Pos": "",
                "Err": "b.com@v1.2.3: module lookup disabled by GOPROXY=off"
        }
}
[modb]> GOPROXY=off go list -e -json -workfile=off b.com/...
go: golang.org/x/tools@v0.1.9: module lookup disabled by GOPROXY=off
go: golang.org/x/tools@v0.1.9: module lookup disabled by GOPROXY=off

@findleyr findleyr removed this from the gopls/v0.8.0 milestone Feb 17, 2022
@findleyr findleyr added this to the gopls/v0.8.1 milestone Feb 17, 2022
@findleyr findleyr changed the title x/tools/gopls: build mod diagnostics with -workfile=off x/tools/gopls: build mod diagnostics with GOWORK=off Mar 12, 2022
@findleyr findleyr removed this from the gopls/v0.8.1 milestone Mar 14, 2022
@findleyr findleyr added this to the gopls/v0.8.2 milestone Mar 14, 2022
@findleyr findleyr removed this from the gopls/v0.8.2 milestone Mar 31, 2022
@findleyr findleyr added this to the gopls/v0.8.3 milestone Mar 31, 2022
@findleyr findleyr removed this from the gopls/v0.8.3 milestone Apr 5, 2022
@findleyr findleyr added this to the gopls/v0.8.4 milestone Apr 5, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented May 10, 2022

Change https://go.dev/cl/405259 mentions this issue: internal/lsp/cache: extract module load errors when go.work is used

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented May 11, 2022

@bcmills @matloob is it valid to depend on this behavior from go list, that errors will be embedded in synthetic package results corresponding to the query string?

i.e. this result:

{
        "ImportPath": "b.com/...",
        "Match": [
                "b.com/..."
        ],
        "Incomplete": true,
        "Error": {
                "ImportStack": [],
                "Pos": "",
                "Err": "b.com@v1.2.3: module lookup disabled by GOPROXY=off"
        }
}

It works, and solves our problem, but feel rather incorrect.

@bcmills
Copy link
Member

@bcmills bcmills commented May 12, 2022

Hmm. In general if we can't determine the import path(s) for a pattern we should report an error for that pattern, so that should be safe to rely on.

I agree that it feels a bit iffy, but I think the only better alternative might be to omit the ImportPath field entirely for the error, and I'm not sure that's any better for tools. 😅

@findleyr findleyr removed this from the gopls/v0.8.4 milestone May 18, 2022
@findleyr findleyr added this to the gopls/v0.9.0 milestone May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls modules Tools
Projects
None yet
Development

No branches or pull requests

4 participants