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: duplicate "undeclared name" errors following CL222237 #38136

Closed
myitcv opened this issue Mar 28, 2020 · 14 comments
Closed

x/tools/gopls: duplicate "undeclared name" errors following CL222237 #38136

myitcv opened this issue Mar 28, 2020 · 14 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Mar 28, 2020

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

$ go version
go version devel +2e918c3aab Tue Mar 17 06:38:32 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200328031815-3db5fc6bac03
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.8-0.20200328031815-3db5fc6bac03

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=""
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-build432849091=/tmp/go-build -gno-record-gcc-switches"

What did you do?

As pointed out by @findleyr in govim/govim#832 (comment), we are seeing duplicate "undeclared name" errors from gopls, as highlighted by the following govim test.

# Test for duplicate undefined errors

# Ensure we have two warnings
vim ex 'e main.go'
vimexprwait warnings.golden GOVIMTest_getqflist()

# Now create a single error and verify we see only that
vim ex 'e other.go'
vim call append '[6,"asd"]'
vimexprwait errors.golden GOVIMTest_getqflist()

-- go.mod --
module mod.com

go 1.12
-- main.go --
package main

import "fmt"

func main() {
	fmt.Printf("%v")
}
-- other.go --
package main

import "fmt"

func foo() {
	fmt.Printf("%v")
}
-- warnings.golden --
[
  {
    "bufname": "main.go",
    "col": 2,
    "lnum": 6,
    "module": "",
    "nr": 0,
    "pattern": "",
    "text": "Printf format %v reads arg #1, but call has 0 args",
    "type": "",
    "valid": 1,
    "vcol": 0
  },
  {
    "bufname": "other.go",
    "col": 2,
    "lnum": 6,
    "module": "",
    "nr": 0,
    "pattern": "",
    "text": "Printf format %v reads arg #1, but call has 0 args",
    "type": "",
    "valid": 1,
    "vcol": 0
  }
]
-- errors.golden --
[
  {
    "bufname": "other.go",
    "col": 1,
    "lnum": 7,
    "module": "",
    "nr": 0,
    "pattern": "",
    "text": "undeclared name: asd",
    "type": "",
    "valid": 1,
    "vcol": 0
  }
]

The output from the test is:

        # Test for duplicate undefined errors (0.497s)
        > vim ex 'e main.go'
        > vimexprwait warnings.golden GOVIMTest_getqflist()
        # open other.go and add a broken statement to get an error that masks the warnings (4.956s)
        > vim ex 'e other.go'
        > vim call append '[6,"asd"]'
        [stdout]
        0
        > vimexprwait errors.golden GOVIMTest_getqflist()
        [stderr]
         [
           {
             "bufname": "other.go",
             "col": 1,
             "lnum": 7,
             "module": "",
             "nr": 0,
             "pattern": "",
             "text": "undeclared name: asd",
             "type": "",
             "valid": 1,
             "vcol": 0
        -  },
        -  {
        -    "bufname": "other.go",
        -    "col": 1,
        -    "lnum": 7,
        -    "module": "",
        -    "nr": 0,
        -    "pattern": "",
        -    "text": "undeclared name: asd",
        -    "type": "",
        -    "valid": 1,
        -    "vcol": 0
           }
         ]
        [exit status 1]
        FAIL: testdata/scenario_default/test.txt:9: unexpected command failure

What did you expect to see?

The test passing

What did you see instead?

The test failed, as above.


cc @stamblerre @ridersofrohan

FYI @leitzler

@ridersofrohan
Copy link

@ridersofrohan ridersofrohan commented Mar 30, 2020

I can't seem to reproduce this using VSCode or running gopls tests. They only seem to return a single undeclared diagnostic. Are you able to see what the source of the diagnostic is (ex. "compiler", "undeclaredname",...)?

@myitcv
Copy link
Member Author

@myitcv myitcv commented Mar 30, 2020

Apologies, I forgot to attach the gopls log. Here it is: gopls.log

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 30, 2020

Change https://golang.org/cl/226417 mentions this issue: internal/lsp/regtest: create test for issue 38136

@ridersofrohan
Copy link

@ridersofrohan ridersofrohan commented Mar 30, 2020

@myitcv I created a test in https://golang.org/cl/226417 using Rob's regtest framework that does the same behavior as the vim test in question. It does not seem to be returning 2 diagnostics. From the gopls log you sent, it looks like the ranges are different. The way the new type error analyzers work is that there is a check to replace existing type errors with the analyzer error if they have the same range and message.

[Trace - 16:16:07.059 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {
  "uri":"file:///tmp/artefacts/govim/cmd/govim/scenario_default/script-signs_unload_load_buffer/other.go",
  "version":2,
  "diagnostics":[
    {
      "range":{"start":{"line":6,"character":0},"end":{"line":6,"character":3}},
      "severity":1,"source":"compiler","message":"undeclared name: asd"
    },
    {
       "range":{"start":{"line":6,"character":0},"end":{"line":7,"character":0}},
       "severity":2,"source":"undeclaredname","message":"undeclared name: asd"
    }
  ],
}

@stamblerre is this a gopls thing?

@myitcv
Copy link
Member Author

@myitcv myitcv commented Mar 30, 2020

@ridersofrohan - are the configurations identical, comparing your test with the govim test?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Mar 30, 2020

@ridersofrohan: It looks like the ranges aren't the same for these errors - is that causing the problem, maybe?

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 2, 2020

Change https://golang.org/cl/226777 mentions this issue: internal/lsp: add type error fixes to existing diagnostics

@stamblerre stamblerre closed this Apr 2, 2020
@myitcv
Copy link
Member Author

@myitcv myitcv commented Apr 2, 2020

What's the status here @stamblerre - did you close this by accident?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 2, 2020

Nope - the above CL fixes the test.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Apr 2, 2020

Ah sorry, I hadn't appreciated that. I saw:

Updates golang/go#38136

and assumed there were further CLs to follow.

@leitzler
Copy link
Contributor

@leitzler leitzler commented Apr 2, 2020

Isn’t it easier to track progress if issues are closed after the CL is merged?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 2, 2020

Ah, sorry I see what you meant now - I have a couple of analysis-related CLs in flight right now, and I forgot which one I had merged.

@stamblerre stamblerre reopened this Apr 2, 2020
@myitcv
Copy link
Member Author

@myitcv myitcv commented Apr 2, 2020

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 3, 2020

Ah, ok since this is fixed I will close the issue. We'll re-enable these analyzers with gopls/v0.5.0, so we'll make sure not to break this test again with that release.

@stamblerre stamblerre closed this Apr 3, 2020
gopherbot pushed a commit to golang/tools that referenced this issue Apr 8, 2020
This change is the first step in handling golang/go#38136. Instead of
creating multiple diagnostic reports for type error analyzers, we add
suggested fixes to the existing reports. To match the analyzers for
FindAnalysisError, we add an ErrorMatch function to source.Analyzer.

This is not an ideal solution, but it was the best one I could come up
with without modifying the go/analysis API. analysisinternal could be
used for this purpose, but it seemed to complicated to be worth it, and
this is fairly simple. I think that go/analysis itself might need to be
extended for type error analyzers, but these temporary measures will
help us understand the kinds of features we need for type error
analyzers.

A follow-up CL might be to not add reports for type error analyzers
until the end of source.Diagnostic, which would remove the need for the
look-up.

Fixes golang/go#38136

Change-Id: I25bc6396b09d49facecd918bf5591d2d5bdf1b3a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226777
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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