Skip to content

Commit

Permalink
deps: use temporary fork of x/tools and gopls (#689)
Browse files Browse the repository at this point in the history
In d80f0e2 we upgraded to the latest x/tools and gopls. But as is
documented in golang.org/issue/36601 there is a bug whereby reverting a
file to an earlier state results in no diagnostics where they are in
fact expected.

Fix this by using a temporary fork of x/tools and gopls where CL 214586
is partially reverted.

Also add tests to ensure the various permutations of transitions to/from
error state and back work as expected.
  • Loading branch information
myitcv committed Jan 19, 2020
1 parent d80f0e2 commit c7a6cc3
Show file tree
Hide file tree
Showing 11 changed files with 212 additions and 30 deletions.
7 changes: 6 additions & 1 deletion cmd/govim/internal/golang_org_x_tools/lsp/cache/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package cache
import (
"bytes"
"context"
"fmt"
"go/scanner"
"go/token"
"go/types"
Expand Down Expand Up @@ -104,8 +105,12 @@ func sourceError(ctx context.Context, fset *token.FileSet, pkg *pkg, e interface
if err != nil {
return nil, err
}
ph, err := pkg.File(spn.URI())
if err != nil {
return nil, fmt.Errorf("finding file for error %q: %v", msg, err)
}
return &source.Error{
URI: spn.URI(),
File: ph.File().Identity(),
Range: rng,
Message: msg,
Kind: kind,
Expand Down
11 changes: 10 additions & 1 deletion cmd/govim/internal/golang_org_x_tools/lsp/diagnostics.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,20 @@ func (s *Server) publishReports(ctx context.Context, snapshot source.Snapshot, r
s.deliveredMu.Lock()
defer s.deliveredMu.Unlock()

for fileID, diagnostics := range reports {
for identity, diagnostics := range reports {
// Don't deliver diagnostics if the context has already been canceled.
if ctx.Err() != nil {
break
}
// Rather than using the identity provided in the report,
// get the FileHandle directly through the snapshot.
// This prevents us from using cached file versions.
fh, err := snapshot.GetFile(identity.URI)
if err != nil {
log.Error(ctx, "publishReports: failed to get FileHandle", err, telemetry.File.Of(identity.URI))
continue
}
fileID := fh.Identity()

// Pre-sort diagnostics to avoid extra work when we compare them.
source.SortDiagnostics(diagnostics)
Expand Down
24 changes: 6 additions & 18 deletions cmd/govim/internal/golang_org_x_tools/lsp/source/diagnostics.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,18 +78,14 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, ph PackageHandle, withA
continue
}
// If no file is associated with the error, pick an open file from the package.
if e.URI.Filename() == "" {
if e.File.URI.Filename() == "" {
for _, ph := range pkg.CompiledGoFiles() {
if snapshot.View().Session().IsOpen(ph.File().Identity().URI) {
e.URI = ph.File().Identity().URI
e.File = ph.File().Identity()
}
}
}
fh, err := snapshot.GetFile(e.URI)
if err != nil {
return nil, warningMsg, err
}
clearReports(snapshot, reports, fh.Identity())
clearReports(snapshot, reports, e.File)
}
// Run diagnostics for the package that this URI belongs to.
hadDiagnostics, err := diagnostics(ctx, snapshot, reports, pkg)
Expand Down Expand Up @@ -149,14 +145,10 @@ func diagnostics(ctx context.Context, snapshot Snapshot, reports map[FileIdentit
Range: e.Range,
Severity: protocol.SeverityError,
}
fh, err := snapshot.GetFile(e.URI)
if err != nil {
return false, err
}
set, ok := diagSets[fh.Identity()]
set, ok := diagSets[e.File]
if !ok {
set = &diagnosticSet{}
diagSets[fh.Identity()] = set
diagSets[e.File] = set
}
switch e.Kind {
case ParseError:
Expand Down Expand Up @@ -210,11 +202,7 @@ func analyses(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][
if onlyDeletions(e.SuggestedFixes) {
tags = append(tags, protocol.Unnecessary)
}
fh, err := snapshot.GetFile(e.URI)
if err != nil {
return err
}
addReports(ctx, snapshot, reports, fh.Identity(), &Diagnostic{
addReports(ctx, snapshot, reports, e.File, &Diagnostic{
Range: e.Range,
Message: e.Message,
Source: e.Category,
Expand Down
4 changes: 2 additions & 2 deletions cmd/govim/internal/golang_org_x_tools/lsp/source/view.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ type Package interface {
}

type Error struct {
URI span.URI
File FileIdentity
Range protocol.Range
Kind ErrorKind
Message string
Expand All @@ -359,5 +359,5 @@ const (
)

func (e *Error) Error() string {
return fmt.Sprintf("%s:%s: %s", e.URI, e.Range, e.Message)
return fmt.Sprintf("%s:%s: %s", e.File, e.Range, e.Message)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# A test that ensures we have a fix for golang.org/issue/36601. Specifically
# that transitions between various states leave diagnostics in the expect state
# when we revert back to the original file contents.
#
# This test moves from error -> error -> error, i.e. we are making a change
# that should not alter the diagnostics, then undoing that change.

[short] skip 'Skip short because we sleep for GOVIM_ERRLOGMATCH_WAIT to ensure we don''t have any errors'

# Expect the initial state
vim ex 'e main.go'
vimexprwait errors.golden getqflist()

# Make a change that shouldn't alter the diagnostics
vim call append '[4,"\t"]'
sleep $GOVIM_ERRLOGMATCH_WAIT
vimexprwait errors.golden getqflist()

# Undo that change and ensure we still have the diagnostics
vim ex 5delete
sleep $GOVIM_ERRLOGMATCH_WAIT
vimexprwait errors.golden getqflist()

-- go.mod --
module mod.com

go 1.12
-- main.go --
package main

func main() {
x := 123
}
-- errors.golden --
[
{
"bufnr": 1,
"col": 2,
"lnum": 4,
"module": "",
"nr": 0,
"pattern": "",
"text": "x declared but not used",
"type": "",
"valid": 1,
"vcol": 0
}
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# A test that ensures we have a fix for golang.org/issue/36601. Specifically
# that transitions between various states leave diagnostics in the expect state
# when we revert back to the original file contents.
#
# This test moves from error -> no error -> error, i.e. we are making a change
# that should remove diagnostics, then undoing that change to bring them back.

# Expect the initial state
vim ex 'e main.go'
vimexprwait errors.golden getqflist()

# Make a change that removes diagnostics
vim call append '[4,"\tprintln(x)"]'
vimexprwait empty.golden getqflist()

# Undo that change and ensure we have the original diagnostics
vim ex 5delete
vimexprwait errors.golden getqflist()

-- go.mod --
module mod.com

go 1.12
-- main.go --
package main

func main() {
x := 123
}
-- errors.golden --
[
{
"bufnr": 1,
"col": 2,
"lnum": 4,
"module": "",
"nr": 0,
"pattern": "",
"text": "x declared but not used",
"type": "",
"valid": 1,
"vcol": 0
}
]
-- empty.golden --
[]
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# A test that ensures we have a fix for golang.org/issue/36601. Specifically
# that transitions between various states leave diagnostics in the expect state
# when we revert back to the original file contents.
#
# This test moves from no error -> error -> no error, i.e. we are making a change
# that should add diagnostics, then undoing that change to remove them.

[short] skip 'Skip short because we sleep for GOVIM_ERRLOGMATCH_WAIT to ensure we don''t have any errors'

# Expect the initial state
vim ex 'e main.go'
sleep $GOVIM_ERRLOGMATCH_WAIT
vimexprwait empty.golden getqflist()

# Make a change that adds diagnostics
vim call append '[4,"\tx := 123"]'
vimexprwait errors.golden getqflist()

# Undo that change and ensure we still zero diagnostics again
vim ex 5delete
sleep $GOVIM_ERRLOGMATCH_WAIT
vimexprwait empty.golden getqflist()

-- go.mod --
module mod.com

go 1.12
-- main.go --
package main

func main() {
//
}
-- errors.golden --
[
{
"bufnr": 1,
"col": 2,
"lnum": 5,
"module": "",
"nr": 0,
"pattern": "",
"text": "x declared but not used",
"type": "",
"valid": 1,
"vcol": 0
}
]
-- empty.golden --
[]
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# A test that ensures we have a fix for golang.org/issue/36601. Specifically
# that transitions between various states leave diagnostics in the expect state
# when we revert back to the original file contents.
#
# This test moves from no error -> no error -> no error, i.e. we are making a
# change that should not alter the diagnostics, then undoing that change.

[short] skip 'Skip short because we sleep for GOVIM_ERRLOGMATCH_WAIT to ensure we don''t have any errors'

# Expect the initial state
vim ex 'e main.go'
sleep $GOVIM_ERRLOGMATCH_WAIT
vimexprwait empty.golden getqflist()

# Make a change that shouldn't alter the diagnostics
vim call append '[4,"\t"]'
sleep $GOVIM_ERRLOGMATCH_WAIT
vimexprwait empty.golden getqflist()

# Undo that change and ensure we still have the diagnostics
vim ex 5delete
sleep $GOVIM_ERRLOGMATCH_WAIT
vimexprwait empty.golden getqflist()

-- go.mod --
module mod.com

go 1.12
-- main.go --
package main

func main() {
//
}
-- empty.golden --
[]
4 changes: 4 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,7 @@ require (
gopkg.in/tomb.v2 v2.0.0-20161208151619-d5d1b5820637
honnef.co/go/tools v0.0.1-2019.2.3
)

replace golang.org/x/tools => github.com/myitcvforks/tools v0.0.0-20200119092928-0fd5434cd1ba

replace golang.org/x/tools/gopls => github.com/myitcvforks/tools/gopls v0.0.0-20200119092928-0fd5434cd1ba
12 changes: 4 additions & 8 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
github.com/myitcv/vbash v0.0.4 h1:T8t40BBZPPeB/Y3sapguk9OGD1aDyjgKiTTgUrtRRTI=
github.com/myitcv/vbash v0.0.4/go.mod h1:8R1aaW5pjRay73Apauo2tzbNXizT9rViXl1jl5+/Xps=
github.com/myitcvforks/tools v0.0.0-20200119092928-0fd5434cd1ba h1:Clj7LkdGld4YomomMwCXpDWgxmbfpRkoFBZUy5XwD6I=
github.com/myitcvforks/tools v0.0.0-20200119092928-0fd5434cd1ba/go.mod h1:TB2adYChydJhpapKDTa4BR/hXlZSLoq2Wpct/0txZ28=
github.com/myitcvforks/tools/gopls v0.0.0-20200119092928-0fd5434cd1ba h1:+xeaqfrKKYdHLf6zxsI7ZaKSNOPc2ute6F6m2wYAkIE=
github.com/myitcvforks/tools/gopls v0.0.0-20200119092928-0fd5434cd1ba/go.mod h1:gl6R36ojRXGBQy36p7BYYZBu495D+W3pYAX3UYwDTpM=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/rogpeppe/clock v0.0.0-20190514195947-2896927a307a h1:3QH7VyOaaiUHNrA9Se4YQIRkDTCw1EJls9xTUCaCeRM=
Expand All @@ -44,7 +48,6 @@ golang.org/x/mod v0.0.0-20190513183733-4bf6d317e70e h1:JgcxKXxCjrA2tyDP/aNU9K0Ck
golang.org/x/mod v0.0.0-20190513183733-4bf6d317e70e/go.mod h1:mXi4GBBbnImb6dmsKGUJ2LatrhH/nqhxcFungHvyanc=
golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee h1:WG0RUwxtNT4qqaXX3DPA8zHFNm/D9xaBpxzHt1WcA/E=
golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee/go.mod h1:QqPTAvyqsEbceGzBzNggFXnrqF1CaUcvgkdR5Ot7KZg=
golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
golang.org/x/net v0.0.0-20190620200207-3b0461eec859 h1:R/3boaszxrf1GEUWTVDzSKVwLmSJpwZ1yqXm8j0v2QI=
golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
Expand All @@ -57,13 +60,6 @@ golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7w
golang.org/x/sys v0.0.0-20190429190828-d89cdac9e872 h1:cGjJzUd8RgBw428LXP65YXni0aiGNA4Bl+ls8SmLOm8=
golang.org/x/sys v0.0.0-20190429190828-d89cdac9e872/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/tools v0.0.0-20190621195816-6e04913cbbac/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc=
golang.org/x/tools v0.0.0-20191017151554-a3bc800455d5/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
golang.org/x/tools v0.0.0-20200117220505-0cba7a3a9ee9 h1:KOkk4e2xd5OeCDJGwacvr75ICCbCsShrHiqPEdsA9hg=
golang.org/x/tools v0.0.0-20200117220505-0cba7a3a9ee9/go.mod h1:TB2adYChydJhpapKDTa4BR/hXlZSLoq2Wpct/0txZ28=
golang.org/x/tools/gopls v0.1.8-0.20200117220505-0cba7a3a9ee9 h1:nzvuLbmeJ+AcY2QpRizKDzip5dbZHStOlteBZR29PH8=
golang.org/x/tools/gopls v0.1.8-0.20200117220505-0cba7a3a9ee9/go.mod h1:gl6R36ojRXGBQy36p7BYYZBu495D+W3pYAX3UYwDTpM=
golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898 h1:/atklqdjdhuosWIl6AIbOeHJjicWYPqR9bpxqxYG2pA=
golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
Expand Down

0 comments on commit c7a6cc3

Please sign in to comment.