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.

Sign up for GitHub

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

Jump to bottom

x/tools/gopls: missing direct requirements also listed in go.mod diagnostics #40470

Closed
myitcv opened this issue Jul 29, 2020 · 5 comments
Closed

x/tools/gopls: missing direct requirements also listed in go.mod diagnostics #40470

myitcv opened this issue Jul 29, 2020 · 5 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.4.4

Comments

@myitcv
Copy link
Member

myitcv commented Jul 29, 2020

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

$ go version
go version devel +71218dbc40 Wed Jul 22 21:31:19 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200729041821-df70183b1872
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20200729041821-df70183b1872

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
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/myitcv/gostuff/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/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-build466676480=/tmp/go-build -gno-record-gcc-switches"

What did you do?

CL 243218 added diagnostics to go.mod file where indirect requirements are missing. Quoting from the commit message:

Some modules may need to be added to the go.mod without being associated
with an import statement. In such cases, we show the missing module
diagnostic on the whole go.mod file. This isn't ideal, but mapping to
the full require statement isn't that simple, and this is an easy enough
starting point. The code in mod_tidy.go is becoming more unwieldy - I
think I will clean it up in a follow-up.

However we are seeing non-indirect requirements being reported as go.mod diagnostics in the scenario_tempmodfile/modfile_changes govim test.

--- FAIL: TestScripts (1.54s)
    main_test.go:77: using gopls at "/tmp/gobin-gopls-installdir340246205/gopls"
    --- FAIL: TestScripts/scenario_tempmodfile (0.04s)
        --- FAIL: TestScripts/scenario_tempmodfile/modfile_changes (10.62s)
            testscript.go:382:
                # A simple test that verifies we get a diagnostic for imports
                # that are missing from our go.mod file (0.000s)
                # Open main.go and save to ensure we are actually doing something via gopls (0.339s)
                # Sleep because we don't have an event we are waiting for (5.000s)
                # Verify that there have been no changes to go.mod (0.000s)
                # Verify that we diagnostics for the import in main.go that can't
                # be satisfied by the go.mod on disk (4.939s)
                > vimexprwait errors.golden GOVIMTest_getqflist()
                [stderr]
                 [
                   {
                -    "bufname": "go.mod",
                -    "col": 1,
                -    "lnum": 1,
                -    "module": "",
                -    "nr": 0,
                -    "pattern": "",
                -    "text": "example.com/blah is not in your go.mod file",
                -    "type": "",
                -    "valid": 1,
                -    "vcol": 0
                -  },
                -  {
                     "bufname": "main.go",
                     "col": 8,
                     "lnum": 3,
                     "module": "",
                     "nr": 0,
                     "pattern": "",
                     "text": "example.com/blah is not in your go.mod file",
                     "type": "",
                     "valid": 1,
                     "vcol": 0
                   }
                 ]
                [exit status 1]
                FAIL: testdata/scenario_tempmodfile/modfile_changes.txt:20: unexpected command failure
FAIL

What did you expect to see?

Only missing indirect requirements to be listed as go.mod diagnostics.

What did you see instead?

Missing direct requirements being listed twice: once in the file where they are imported, and secondly in the go.mod file.

cc @stamblerre @findleyr

FYI @leitzler
@myitcv myitcv added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. labels Jul 29, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jul 29, 2020
@gopherbot gopherbot added this to the Unreleased milestone Jul 29, 2020
@myitcv myitcv mentioned this issue Jul 29, 2020
Merged
@stamblerre
Copy link
Contributor

Hm, this is actually my intention, but it wasn't supposed to happen in this CL. https://golang.org/cl/244610 is meant to add this behavior. I'll look into the bug.

@stamblerre stamblerre modified the milestones: Unreleased, gopls/v0.4.4 Jul 29, 2020
@myitcv
Copy link
Member Author

myitcv commented Jul 29, 2020

Ah if this is ultimately intended, perhaps not worth worrying about?

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/245537 mentions this issue: internal/lsp: don't show diagnostic in go.mod for direct dependencies

@stamblerre
Copy link
Contributor

It's going into the release so we should fix it before it does. But yes, in general, the plan is to show those diagnostics in the future.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/245541 mentions this issue: [gopls-release-branch.0.4] internal/lsp: don't show diagnostic in go.mod for direct dependencies

gopherbot pushed a commit to golang/tools that referenced this issue Jul 29, 2020
@stamblerre
[gopls-release-branch.0.4] internal/lsp: don't show diagnostic in go.…
8bac6fa 
…mod for direct dependencies

Fixes golang/go#40470.

Change-Id: I4831b837e6db5cc344c20480abde1e688ddab0dc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245537
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
(cherry picked from commit 8afe7e0)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245541
@golang golang locked and limited conversation to collaborators Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.4.4
Development

No branches or pull requests
3 participants
@myitcv @stamblerre @gopherbot