From 36a523fefd906b06076d90482f7b17e71633823d Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 5 Jan 2024 12:24:57 -0500 Subject: [PATCH 01/36] all: update codereview.cfg for gopls-release-branch.0.15 For golang/go#64973 Change-Id: Ifcee07059e7f5d60eb98babcfb39365c93a00c6c Reviewed-on: https://go-review.googlesource.com/c/tools/+/554318 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan Auto-Submit: Robert Findley --- codereview.cfg | 2 ++ 1 file changed, 2 insertions(+) diff --git a/codereview.cfg b/codereview.cfg index 3f8b14b64e8..6051232fb03 100644 --- a/codereview.cfg +++ b/codereview.cfg @@ -1 +1,3 @@ issuerepo: golang/go +branch: gopls-release-branch.0.15 +parent-branch: master From 50d32be0ffdaf5f1eb331d3d5da0a984e1830eb0 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 5 Jan 2024 12:40:36 -0500 Subject: [PATCH 02/36] gopls: update go.mod for v0.15.0-pre.1 Remove the replace directive and update x/tools. For golang/go#64973 Change-Id: I79c5d5aa7f1fe96641a7958d2f3c2cbb6d038be3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/554319 LUCI-TryBot-Result: Go LUCI Auto-Submit: Robert Findley Reviewed-by: Hyang-Ah Hana Kim --- gopls/go.mod | 4 +--- gopls/go.sum | 13 +++---------- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/gopls/go.mod b/gopls/go.mod index f22cef7577a..3eabeef21eb 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -10,7 +10,7 @@ require ( golang.org/x/sync v0.5.0 golang.org/x/telemetry v0.0.0-20231114163143-69313e640400 golang.org/x/text v0.14.0 - golang.org/x/tools v0.13.1-0.20230920233436-f9b8da7b22be + golang.org/x/tools v0.16.2-0.20240105173808-36a523fefd90 golang.org/x/vuln v1.0.1 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.4.5 @@ -26,5 +26,3 @@ require ( gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect ) - -replace golang.org/x/tools => ../ diff --git a/gopls/go.sum b/gopls/go.sum index 3fab4a46e44..1f982dd1dff 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -18,30 +18,23 @@ github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/rogpeppe/go-internal v1.8.1/go.mod h1:JeRgkft04UBgHMgCIwADu4Pn6Mtm5d4nPKWu0nJ5d+o= github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8= -github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= -golang.org/x/crypto v0.16.0/go.mod h1:gCAAfMLgwOJRpTjQ2zCCt2OcSfYMTeZVSRtQlPC7Nq4= golang.org/x/exp/typeparams v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y= golang.org/x/exp/typeparams v0.0.0-20221212164502-fae10dda9338/go.mod h1:AbB0pIl9nAr9wVwH+Z2ZpaocVmF5I4GyWCDIsVjR0bk= -golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.14.0 h1:dGoOF9QVLYng8IHTm7BAyWqCqSheQ5pYWGhzW00YJr0= golang.org/x/mod v0.14.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= -golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg= -golang.org/x/net v0.19.0/go.mod h1:CfAk/cbD4CthTvqiEl8NpboMuiuOYsAr/7NOjZJtv1U= golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.5.0 h1:60k92dhOjHxJkrqnwsfl8KuaHbn/5dl0lUPUklKo3qE= golang.org/x/sync v0.5.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= -golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.15.0 h1:h48lPFYpsTvQJZF4EKyI4aLHaev3CxivZmv7yZig9pc= golang.org/x/sys v0.15.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/telemetry v0.0.0-20231114163143-69313e640400 h1:brbkEFfGwNGAEkykUOcryE/JiHUMMJouzE0fWWmz/QU= golang.org/x/telemetry v0.0.0-20231114163143-69313e640400/go.mod h1:P6hMdmAcoG7FyATwqSr6R/U0n7yeXNP/QXeRlxb1szE= -golang.org/x/term v0.8.0/go.mod h1:xPskH00ivmX89bAKVGSKKtLOWNx2+17Eiy94tnKShWo= -golang.org/x/term v0.15.0/go.mod h1:BDl952bC7+uMoWR75FIrCDx79TPU9oHkTZ9yRbYOrX0= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= +golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= +golang.org/x/tools v0.16.2-0.20240105173808-36a523fefd90 h1:3dw2Y1Aes2o83VYvSZSPc+p70AuT9zvNCTBHCvUUZrE= +golang.org/x/tools v0.16.2-0.20240105173808-36a523fefd90/go.mod h1:kYVVN6I1mBNoB1OX+noeBjbRk4IUEPa7JJ+TJMEooJ0= golang.org/x/vuln v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= golang.org/x/vuln v1.0.1/go.mod h1:bb2hMwln/tqxg32BNY4CcxHWtHXuYa3SbIBmtsyjxtM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From 9f736d6502d4de0233d5cf069e69f249d2f04e4e Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 6 Feb 2024 21:00:45 +0000 Subject: [PATCH 03/36] gopls: update go.mod for v0.15.0-pre.2 Remove the replace directive and update x/tools. For golang/go#64973 Change-Id: Ia5ae53c8e1ed415b417bd2be125a0077d9a0c319 Reviewed-on: https://go-review.googlesource.com/c/tools/+/562038 Reviewed-by: Suzy Mueller Auto-Submit: Robert Findley LUCI-TryBot-Result: Go LUCI --- gopls/go.mod | 4 +--- gopls/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/gopls/go.mod b/gopls/go.mod index bafca530086..70171e9d9be 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -10,7 +10,7 @@ require ( golang.org/x/sync v0.6.0 golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509 golang.org/x/text v0.14.0 - golang.org/x/tools v0.17.0 + golang.org/x/tools v0.17.1-0.20240206204217-56dc8684fb5a golang.org/x/vuln v1.0.1 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.4.6 @@ -26,5 +26,3 @@ require ( gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect ) - -replace golang.org/x/tools => ../ diff --git a/gopls/go.sum b/gopls/go.sum index 89bac973aa4..0731e5eeb22 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -27,8 +27,8 @@ golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= -golang.org/x/tools v0.17.0 h1:FvmRgNOcs3kOa+T20R1uhfP9F6HgG2mfxDv1vrx1Htc= -golang.org/x/tools v0.17.0/go.mod h1:xsh6VxdV005rRVaS6SSAf9oiAqljS7UZUacMZ8Bnsps= +golang.org/x/tools v0.17.1-0.20240206204217-56dc8684fb5a h1:WL/PnseOKWuDtRbgfYKDpuAmMpy3OrX84rv7uQFZJK4= +golang.org/x/tools v0.17.1-0.20240206204217-56dc8684fb5a/go.mod h1:ngPhXSzq5toGRGmVELsotoOaTqvj1Ojf/H79cbjAGfw= golang.org/x/vuln v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= golang.org/x/vuln v1.0.1/go.mod h1:bb2hMwln/tqxg32BNY4CcxHWtHXuYa3SbIBmtsyjxtM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From 48c24dc7bda1a23126c61d992b803611b80e8a26 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 13 Feb 2024 15:11:03 +0000 Subject: [PATCH 04/36] gopls: update go.mod for v0.15.0-pre.3 Remove the replace directive and update x/tools. For golang/go#64973 Change-Id: I3ab6bef99bd12dd98ddd553309bc06b908d630c9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/563775 Auto-Submit: Robert Findley LUCI-TryBot-Result: Go LUCI Reviewed-by: Hyang-Ah Hana Kim --- gopls/go.mod | 4 +--- gopls/go.sum | 14 +++----------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/gopls/go.mod b/gopls/go.mod index fd6585120f0..a22bab2c0de 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -10,7 +10,7 @@ require ( golang.org/x/sync v0.6.0 golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 golang.org/x/text v0.14.0 - golang.org/x/tools v0.17.1-0.20240206204217-56dc8684fb5a + golang.org/x/tools v0.18.1-0.20240213144131-c2dba4487189 golang.org/x/vuln v1.0.1 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.4.6 @@ -26,5 +26,3 @@ require ( gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect ) - -replace golang.org/x/tools => ../ diff --git a/gopls/go.sum b/gopls/go.sum index c7ad7ba01e5..71c9f6c534c 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -13,30 +13,22 @@ github.com/jba/templatecheck v0.6.0/go.mod h1:/1k7EajoSErFI9GLHAsiIJEaNLt3ALKNw2 github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= -github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= -golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= golang.org/x/exp/typeparams v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y= golang.org/x/exp/typeparams v0.0.0-20221212164502-fae10dda9338/go.mod h1:AbB0pIl9nAr9wVwH+Z2ZpaocVmF5I4GyWCDIsVjR0bk= -golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.15.0 h1:SernR4v+D55NyBH2QiEQrlBAnj1ECL6AGrA5+dPaMY8= golang.org/x/mod v0.15.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= -golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg= -golang.org/x/net v0.21.0/go.mod h1:bIjVDfnllIU7BJ2DNgfnXvpSvtn8VRwhlsaeUTyUS44= golang.org/x/sync v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ= golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= -golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.17.0 h1:25cE3gD+tdBA7lp7QfhuV+rJiE9YXTcS3VG1SqssI/Y= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808/go.mod h1:KG1lNk5ZFNssSZLrpVb4sMXKMpGwGXOxSG3rnu2gZQQ= golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 h1:vcVnuftN4J4UKLRcgetjzfU9FjjgXUUYUc3JhFplgV4= golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78/go.mod h1:KG1lNk5ZFNssSZLrpVb4sMXKMpGwGXOxSG3rnu2gZQQ= -golang.org/x/term v0.8.0/go.mod h1:xPskH00ivmX89bAKVGSKKtLOWNx2+17Eiy94tnKShWo= -golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= +golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= +golang.org/x/tools v0.18.1-0.20240213144131-c2dba4487189 h1:K183SddK0FbcEISI7zBIVqmmAoGlsdJrVe//JvCHc4s= +golang.org/x/tools v0.18.1-0.20240213144131-c2dba4487189/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= golang.org/x/vuln v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= golang.org/x/vuln v1.0.1/go.mod h1:bb2hMwln/tqxg32BNY4CcxHWtHXuYa3SbIBmtsyjxtM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From fbcb99f4d7db994ce7fad6c664a7c5c68dd620d0 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 13 Feb 2024 16:35:15 -0500 Subject: [PATCH 05/36] [gopls-release-branch.0.15] gopls/internal/server: disambiguate diagnostics by OS,ARCH This change adds a disambiguating suffix such as " [windows,arm64]" to diagnostics that do not appear in the default build configuration. Fixes golang/go#65496 Change-Id: Ided7a7110ff630e57b7a96a311a240fef210ca93 Reviewed-on: https://go-review.googlesource.com/c/tools/+/563876 Reviewed-by: Robert Findley LUCI-TryBot-Result: Go LUCI (cherry picked from commit df9c1c7efa1a9c412265ae4eedf5e18b2f35f33f) Reviewed-on: https://go-review.googlesource.com/c/tools/+/564557 Reviewed-by: Alan Donovan --- gopls/internal/server/diagnostics.go | 80 +++++++++++++++---- .../testdata/diagnostics/osarch_suffix.txt | 46 +++++++++++ 2 files changed, 110 insertions(+), 16 deletions(-) create mode 100644 gopls/internal/test/marker/testdata/diagnostics/osarch_suffix.txt diff --git a/gopls/internal/server/diagnostics.go b/gopls/internal/server/diagnostics.go index 6aa01aba127..4d8e3aae4d1 100644 --- a/gopls/internal/server/diagnostics.go +++ b/gopls/internal/server/diagnostics.go @@ -11,6 +11,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "sort" "strings" "sync" @@ -61,7 +62,9 @@ type ( diagMap = map[protocol.DocumentURI][]*cache.Diagnostic ) -// hashDiagnostics computes a hash to identify a diagnostic. +// hashDiagnostic computes a hash to identify a diagnostic. +// The hash is for deduplicating within a file, +// so it need not incorporate d.URI. func hashDiagnostic(d *cache.Diagnostic) file.Hash { h := sha256.New() for _, t := range d.Tags { @@ -822,23 +825,25 @@ func (s *server) updateOrphanedFileDiagnostics(ctx context.Context, modID uint64 // // If the publication succeeds, it updates f.publishedHash and f.mustPublish. func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet, uri protocol.DocumentURI, version int32, f *fileDiagnostics) error { - // Check that the set of views is up-to-date, and de-dupe diagnostics - // across views. - var ( - diagHashes = make(map[file.Hash]unit) // unique diagnostic hashes - hash file.Hash // XOR of diagnostic hashes - unique []*cache.Diagnostic // unique diagnostics - ) - add := func(diag *cache.Diagnostic) { + + // We add a disambiguating suffix (e.g. " [darwin,arm64]") to + // each diagnostic that doesn't occur in the default view; + // see golang/go#65496. + type diagSuffix struct { + diag *cache.Diagnostic + suffix string // "" for default build (or orphans) + } + + // diagSuffixes records the set of view suffixes for a given diagnostic. + diagSuffixes := make(map[file.Hash][]diagSuffix) + add := func(diag *cache.Diagnostic, suffix string) { h := hashDiagnostic(diag) - if _, ok := diagHashes[h]; !ok { - diagHashes[h] = unit{} - unique = append(unique, diag) - hash.XORWith(h) - } + diagSuffixes[h] = append(diagSuffixes[h], diagSuffix{diag, suffix}) } + + // Construct the inverse mapping, from diagnostic (hash) to its suffixes (views). for _, diag := range f.orphanedFileDiagnostics { - add(diag) + add(diag, "") } for view, viewDiags := range f.byView { if _, ok := views[view]; !ok { @@ -848,10 +853,53 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet if viewDiags.version != version { continue // a payload of diagnostics applies to a specific file version } + + // Compute the view's suffix (e.g. " [darwin,arm64]"). + var suffix string + { + var words []string + if view.GOOS() != runtime.GOOS { + words = append(words, view.GOOS()) + } + if view.GOARCH() != runtime.GOARCH { + words = append(words, view.GOARCH()) + } + if len(words) > 0 { + suffix = fmt.Sprintf(" [%s]", strings.Join(words, ",")) + } + } + for _, diag := range viewDiags.diagnostics { - add(diag) + add(diag, suffix) } } + + // De-dup diagnostics across views by hash, and sort. + var ( + hash file.Hash + unique []*cache.Diagnostic + ) + for h, items := range diagSuffixes { + // Sort the items by ascending suffix, so that the + // default view (if present) is first. + // (The others are ordered arbitrarily.) + sort.Slice(items, func(i, j int) bool { + return items[i].suffix < items[j].suffix + }) + + // If the diagnostic was not present in + // the default view, add the view suffix. + first := items[0] + if first.suffix != "" { + diag2 := *first.diag // shallow copy + diag2.Message += first.suffix + first.diag = &diag2 + h = hashDiagnostic(&diag2) // update the hash + } + + hash.XORWith(h) + unique = append(unique, first.diag) + } sortDiagnostics(unique) // Publish, if necessary. diff --git a/gopls/internal/test/marker/testdata/diagnostics/osarch_suffix.txt b/gopls/internal/test/marker/testdata/diagnostics/osarch_suffix.txt new file mode 100644 index 00000000000..95336085b2f --- /dev/null +++ b/gopls/internal/test/marker/testdata/diagnostics/osarch_suffix.txt @@ -0,0 +1,46 @@ +This test verifies that we add an [os,arch] suffix to each diagnostic +that doesn't appear in the default build (=runtime.{GOOS,GOARCH}). + +See golang/go#65496. + +The two p/*.go files below are written to trigger the same diagnostic +(range, message, source, etc) but varying only by URI. + +In the q test, a single location in the common code q.go has two +diagnostics, one of which is tagged. + +This test would fail on openbsd/mips64 because it will be +the same as the default build, so we skip that platform. + +-- flags -- +-skip_goos=openbsd + +-- go.mod -- +module example.com + +-- p/p.go -- +package p + +var _ fmt.Stringer //@diag("fmt", re"unde.*: fmt$") + +-- p/p_openbsd_mips64.go -- +package p + +var _ fmt.Stringer //@diag("fmt", re"unde.*: fmt \\[openbsd,mips64\\]") + +-- q/q_default.go -- +//+build !openbsd && !mips64 + +package q + +func f(int) int + +-- q/q_openbsd_mips64.go -- +package q + +func f(string) int + +-- q/q.go -- +package q + +var _ = f() //@ diag(")", re`.*want \(string\) \[openbsd,mips64\]`), diag(")", re`.*want \(int\)$`) From e12fb60043d3bafe3d91ad7de506c6ba067f1cc6 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 16 Feb 2024 17:34:06 +0000 Subject: [PATCH 06/36] gopls: update go.mod for v0.15.0-pre.4 Update x/tools. For golang/go#64973 Change-Id: Icfc2f5327e0f2154e65fb366647078538bb4190f Reviewed-on: https://go-review.googlesource.com/c/tools/+/564656 Reviewed-by: Alan Donovan Auto-Submit: Robert Findley LUCI-TryBot-Result: Go LUCI --- gopls/go.mod | 2 +- gopls/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gopls/go.mod b/gopls/go.mod index a22bab2c0de..f8e7200836b 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -10,7 +10,7 @@ require ( golang.org/x/sync v0.6.0 golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 golang.org/x/text v0.14.0 - golang.org/x/tools v0.18.1-0.20240213144131-c2dba4487189 + golang.org/x/tools v0.18.1-0.20240216172611-fbcb99f4d7db golang.org/x/vuln v1.0.1 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.4.6 diff --git a/gopls/go.sum b/gopls/go.sum index 71c9f6c534c..75b5d1b0a80 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -27,8 +27,8 @@ golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= -golang.org/x/tools v0.18.1-0.20240213144131-c2dba4487189 h1:K183SddK0FbcEISI7zBIVqmmAoGlsdJrVe//JvCHc4s= -golang.org/x/tools v0.18.1-0.20240213144131-c2dba4487189/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= +golang.org/x/tools v0.18.1-0.20240216172611-fbcb99f4d7db h1:Eo6D0lMVoT72gzApl11ZrOJSUvohnhYq75BCfpVRHpc= +golang.org/x/tools v0.18.1-0.20240216172611-fbcb99f4d7db/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= golang.org/x/vuln v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= golang.org/x/vuln v1.0.1/go.mod h1:bb2hMwln/tqxg32BNY4CcxHWtHXuYa3SbIBmtsyjxtM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From 05962dddd84b05ae7993d27df98c6705ced63687 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 16 Feb 2024 21:26:22 +0000 Subject: [PATCH 07/36] [gopls-release-branch.0.15] gopls/internal/settings: default "includeReplaceInWorkspace" to false As described in golang/go#65762, treating locally replaced modules the same as workspace modules doesn't really work, since there will be missing go.sum entries. Fortunately, this functionality was guarded by the "includeReplaceInWorkspace" setting. Revert the default value for this setting. Fixes golang/go#65762 Change-Id: I521acb2863404cba7612887aa7730075dcfebd96 Reviewed-on: https://go-review.googlesource.com/c/tools/+/564558 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan (cherry picked from commit 3ac77cb1c1a6c81d09a674b2ecf89c6d01ca0415) Reviewed-on: https://go-review.googlesource.com/c/tools/+/564559 Auto-Submit: Robert Findley --- gopls/internal/cache/session_test.go | 17 ++++++++++++----- gopls/internal/settings/default.go | 2 +- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/gopls/internal/cache/session_test.go b/gopls/internal/cache/session_test.go index a3bd8ce5800..913c3bd1f27 100644 --- a/gopls/internal/cache/session_test.go +++ b/gopls/internal/cache/session_test.go @@ -20,6 +20,7 @@ import ( func TestZeroConfigAlgorithm(t *testing.T) { testenv.NeedsExec(t) // executes the Go command + t.Setenv("GOPACKAGESDRIVER", "off") type viewSummary struct { // fields exported for cmp.Diff @@ -33,6 +34,12 @@ func TestZeroConfigAlgorithm(t *testing.T) { options func(dir string) map[string]any // options may refer to the temp dir } + includeReplaceInWorkspace := func(string) map[string]any { + return map[string]any{ + "includeReplaceInWorkspace": true, + } + } + type test struct { name string files map[string]string // use a map rather than txtar as file content is tiny @@ -235,7 +242,7 @@ func TestZeroConfigAlgorithm(t *testing.T) { "b/go.mod": "module golang.org/b\ngo 1.18\n", "b/b.go": "package b", }, - []folderSummary{{dir: "."}}, + []folderSummary{{dir: ".", options: includeReplaceInWorkspace}}, []string{"a/a.go", "b/b.go"}, []viewSummary{{GoModView, ".", nil}}, }, @@ -247,7 +254,7 @@ func TestZeroConfigAlgorithm(t *testing.T) { "b/go.mod": "module golang.org/b\ngo 1.18\nrequire golang.org/a v1.2.3\nreplace golang.org/a => ../", "b/b.go": "package b", }, - []folderSummary{{dir: "."}}, + []folderSummary{{dir: ".", options: includeReplaceInWorkspace}}, []string{"a/a.go", "b/b.go"}, []viewSummary{{GoModView, ".", nil}, {GoModView, "b", nil}}, }, @@ -277,12 +284,12 @@ replace ( "d/go.mod": "module golang.org/d\ngo 1.18", "d/d.go": "package d", }, - []folderSummary{{dir: "."}}, + []folderSummary{{dir: ".", options: includeReplaceInWorkspace}}, []string{"b/b.go", "c/c.go", "d/d.go"}, []viewSummary{{GoModView, ".", nil}, {GoModView, "d", nil}}, }, { - "go.mod with many replace", + "go.mod with replace outside the workspace", map[string]string{ "go.mod": "module golang.org/a\ngo 1.18", "a.go": "package a", @@ -290,7 +297,7 @@ replace ( "b/b.go": "package b", }, []folderSummary{{dir: "b"}}, - []string{"a/a.go", "b/b.go"}, + []string{"a.go", "b/b.go"}, []viewSummary{{GoModView, "b", nil}}, }, { diff --git a/gopls/internal/settings/default.go b/gopls/internal/settings/default.go index 752effab3ef..f29b439428b 100644 --- a/gopls/internal/settings/default.go +++ b/gopls/internal/settings/default.go @@ -115,7 +115,7 @@ func DefaultOptions(overrides ...func(*Options)) *Options { ReportAnalysisProgressAfter: 5 * time.Second, TelemetryPrompt: false, LinkifyShowMessage: false, - IncludeReplaceInWorkspace: true, + IncludeReplaceInWorkspace: false, ZeroConfig: true, }, Hooks: Hooks{ From a6289fe0c3fb86e172d15e1a1ea262b3a041c980 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Tue, 20 Feb 2024 12:31:38 -0500 Subject: [PATCH 08/36] [gopls-release-branch.0.15] gopls/internal/cache: fix two bugs related to workspace packages Fix two bugs related to workspace packages that contributed to golang/go#65801. The first is that we didn't consider packages with open files to be workspace packages. This was pre-existing behavior, but in the past we simply relied on diagnoseChangedFiles to provide diagnostics for open packages, even though we didn't consider them to be part of the workspace. That led to races and inconsistent behavior: a change in one file would wipe out diagnostics in another, and so on. It's much simpler to say that if the package is open, it is part of the workspace. This leads to consistent behavior, no matter where diagnostics originate. The second bug is related to loading std and cmd. The new workspace package heuristics relied on go/packages.Package.Module to determine if a package is included in the workspace. For std and cmd, this field is nil (golang/go#65816), apparently due to limitations of `go list`. As a result, we were finding no workspace packages for std or cmd. Fix this by falling back to searching for the relevant go.mod file in the filesystem. Unfortunately this required reinstating the lockedSnapshot file source. These bugs revealed a rather large gap in our test coverage for std. Add a test that verifies that we compute std workspace packages. Fixes golang/go#65801 Change-Id: Ic454d4a43e34af10e1224755a09d6c94c728c97d Reviewed-on: https://go-review.googlesource.com/c/tools/+/565475 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI (cherry picked from commit 607b6641fe2fd4f4d22f30706e31719456a7e269) Reviewed-on: https://go-review.googlesource.com/c/tools/+/565457 --- gopls/internal/cache/load.go | 43 +++++- gopls/internal/cache/snapshot.go | 20 ++- .../test/integration/workspace/std_test.go | 73 ++++++++++ .../integration/workspace/workspace_test.go | 137 +++++++++++++++++- 4 files changed, 256 insertions(+), 17 deletions(-) create mode 100644 gopls/internal/test/integration/workspace/std_test.go diff --git a/gopls/internal/cache/load.go b/gopls/internal/cache/load.go index 4bbeb2d160a..74c90cd66f7 100644 --- a/gopls/internal/cache/load.go +++ b/gopls/internal/cache/load.go @@ -27,6 +27,7 @@ import ( "golang.org/x/tools/internal/event/tag" "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/packagesinternal" + "golang.org/x/tools/internal/xcontext" ) var loadID uint64 // atomic identifier for loads @@ -283,7 +284,7 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc event.Log(ctx, fmt.Sprintf("%s: updating metadata for %d packages", eventName, len(updates))) meta := s.meta.Update(updates) - workspacePackages := computeWorkspacePackagesLocked(s, meta) + workspacePackages := computeWorkspacePackagesLocked(ctx, s, meta) s.meta = meta s.workspacePackages = workspacePackages s.resetActivePackagesLocked() @@ -563,7 +564,7 @@ func computeLoadDiagnostics(ctx context.Context, snapshot *Snapshot, mp *metadat // heuristics. // // s.mu must be held while calling this function. -func isWorkspacePackageLocked(s *Snapshot, meta *metadata.Graph, pkg *metadata.Package) bool { +func isWorkspacePackageLocked(ctx context.Context, s *Snapshot, meta *metadata.Graph, pkg *metadata.Package) bool { if metadata.IsCommandLineArguments(pkg.ID) { // Ad-hoc command-line-arguments packages aren't workspace packages. // With zero-config gopls (golang/go#57979) they should be very rare, as @@ -587,6 +588,16 @@ func isWorkspacePackageLocked(s *Snapshot, meta *metadata.Graph, pkg *metadata.P return containsOpenFileLocked(s, pkg) } + // golang/go#65801: any (non command-line-arguments) open package is a + // workspace package. + // + // Otherwise, we'd display diagnostics for changes in an open package (due to + // the logic of diagnoseChangedFiles), but then erase those diagnostics when + // we do the final diagnostics pass. Diagnostics should be stable. + if containsOpenFileLocked(s, pkg) { + return true + } + // Apply filtering logic. // // Workspace packages must contain at least one non-filtered file. @@ -624,10 +635,24 @@ func isWorkspacePackageLocked(s *Snapshot, meta *metadata.Graph, pkg *metadata.P // In module mode, a workspace package must be contained in a workspace // module. if s.view.moduleMode() { - if pkg.Module == nil { - return false + var modURI protocol.DocumentURI + if pkg.Module != nil { + modURI = protocol.URIFromPath(pkg.Module.GoMod) + } else { + // golang/go#65816: for std and cmd, Module is nil. + // Fall back to an inferior heuristic. + if len(pkg.CompiledGoFiles) == 0 { + return false // need at least one file to guess the go.mod file + } + dir := pkg.CompiledGoFiles[0].Dir() + var err error + modURI, err = findRootPattern(ctx, dir, "go.mod", lockedSnapshot{s}) + if err != nil || modURI == "" { + // err != nil implies context cancellation, in which case the result of + // this query does not matter. + return false + } } - modURI := protocol.URIFromPath(pkg.Module.GoMod) _, ok := s.view.workspaceModFiles[modURI] return ok } @@ -662,10 +687,14 @@ func containsOpenFileLocked(s *Snapshot, mp *metadata.Package) bool { // contain intermediate test variants. // // s.mu must be held while calling this function. -func computeWorkspacePackagesLocked(s *Snapshot, meta *metadata.Graph) immutable.Map[PackageID, PackagePath] { +func computeWorkspacePackagesLocked(ctx context.Context, s *Snapshot, meta *metadata.Graph) immutable.Map[PackageID, PackagePath] { + // The provided context is used for reading snapshot files, which can only + // fail due to context cancellation. Don't let this happen as it could lead + // to inconsistent results. + ctx = xcontext.Detach(ctx) workspacePackages := make(map[PackageID]PackagePath) for _, mp := range meta.Packages { - if !isWorkspacePackageLocked(s, meta, mp) { + if !isWorkspacePackageLocked(ctx, s, meta, mp) { continue } diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go index 3d97ed47ccb..3c535b8d6a2 100644 --- a/gopls/internal/cache/snapshot.go +++ b/gopls/internal/cache/snapshot.go @@ -1273,14 +1273,26 @@ func (s *Snapshot) ReadFile(ctx context.Context, uri protocol.DocumentURI) (file s.mu.Lock() defer s.mu.Unlock() - fh, ok := s.files.get(uri) + return lockedSnapshot{s}.ReadFile(ctx, uri) +} + +// lockedSnapshot implements the file.Source interface, while holding s.mu. +// +// TODO(rfindley): This unfortunate type had been eliminated, but it had to be +// restored to fix golang/go#65801. We should endeavor to remove it again. +type lockedSnapshot struct { + s *Snapshot +} + +func (s lockedSnapshot) ReadFile(ctx context.Context, uri protocol.DocumentURI) (file.Handle, error) { + fh, ok := s.s.files.get(uri) if !ok { var err error - fh, err = s.view.fs.ReadFile(ctx, uri) + fh, err = s.s.view.fs.ReadFile(ctx, uri) if err != nil { return nil, err } - s.files.set(uri, fh) + s.s.files.set(uri, fh) } return fh, nil } @@ -2016,7 +2028,7 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f // Update workspace and active packages, if necessary. if result.meta != s.meta || anyFileOpenedOrClosed { needsDiagnosis = true - result.workspacePackages = computeWorkspacePackagesLocked(result, result.meta) + result.workspacePackages = computeWorkspacePackagesLocked(ctx, result, result.meta) result.resetActivePackagesLocked() } else { result.workspacePackages = s.workspacePackages diff --git a/gopls/internal/test/integration/workspace/std_test.go b/gopls/internal/test/integration/workspace/std_test.go new file mode 100644 index 00000000000..51c0603f014 --- /dev/null +++ b/gopls/internal/test/integration/workspace/std_test.go @@ -0,0 +1,73 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package workspace + +import ( + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + . "golang.org/x/tools/gopls/internal/test/integration" +) + +func TestStdWorkspace(t *testing.T) { + // This test checks that we actually load workspace packages when opening + // GOROOT. + // + // In golang/go#65801, we failed to do this because go/packages returns nil + // Module for std and cmd. + // + // Because this test loads std as a workspace, it may be slow on smaller + // builders. + if testing.Short() { + t.Skip("skipping with -short: loads GOROOT") + } + + // The test also fails on Windows because an absolute path does not match + // (likely a misspelling due to slashes). + // TODO(rfindley): investigate and fix this on windows. + if runtime.GOOS == "windows" { + t.Skip("skipping on windows: fails to to misspelled paths") + } + + // Query GOROOT. This is slightly more precise than e.g. runtime.GOROOT, as + // it queries the go command in the environment. + goroot, err := exec.Command("go", "env", "GOROOT").Output() + if err != nil { + t.Fatal(err) + } + stdDir := filepath.Join(strings.TrimSpace(string(goroot)), "src") + WithOptions( + Modes(Default), // This test may be slow. No reason to run it multiple times. + WorkspaceFolders(stdDir), + ).Run(t, "", func(t *testing.T, env *Env) { + // Find parser.ParseFile. Query with `'` to get an exact match. + syms := env.Symbol("'go/parser.ParseFile") + if len(syms) != 1 { + t.Fatalf("got %d symbols, want exactly 1. Symbols:\n%v", len(syms), syms) + } + parserPath := syms[0].Location.URI.Path() + env.OpenFile(parserPath) + + // Find the reference to ast.File from the signature of ParseFile. This + // helps guard against matching a comment. + astFile := env.RegexpSearch(parserPath, `func ParseFile\(.*ast\.(File)`) + refs := env.References(astFile) + + // If we've successfully loaded workspace packages for std, we should find + // a reference in go/types. + foundGoTypesReference := false + for _, ref := range refs { + if strings.Contains(string(ref.URI), "go/types") { + foundGoTypesReference = true + } + } + if !foundGoTypesReference { + t.Errorf("references(ast.File) did not return a go/types reference. Refs:\n%v", refs) + } + }) +} diff --git a/gopls/internal/test/integration/workspace/workspace_test.go b/gopls/internal/test/integration/workspace/workspace_test.go index 28b3978a8cd..e2819404dfa 100644 --- a/gopls/internal/test/integration/workspace/workspace_test.go +++ b/gopls/internal/test/integration/workspace/workspace_test.go @@ -7,9 +7,11 @@ package workspace import ( "context" "fmt" + "sort" "strings" "testing" + "github.com/google/go-cmp/cmp" "golang.org/x/tools/gopls/internal/hooks" "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/test/integration/fake" @@ -1123,16 +1125,139 @@ import ( env.AfterChange( Diagnostics(env.AtRegexp("a/main.go", "V"), WithMessage("not used")), ) + // Here, diagnostics are added because of zero-config gopls. + // In the past, they were added simply due to diagnosing changed files. + // (see TestClearNonWorkspaceDiagnostics_NoView below for a + // reimplementation of that test). + if got, want := len(env.Views()), 2; got != want { + t.Errorf("after opening a/main.go, got %d views, want %d", got, want) + } env.CloseBuffer("a/main.go") + env.AfterChange( + NoDiagnostics(ForFile("a/main.go")), + ) + if got, want := len(env.Views()), 1; got != want { + t.Errorf("after closing a/main.go, got %d views, want %d", got, want) + } + }) +} + +// This test is like TestClearNonWorkspaceDiagnostics, but bypasses the +// zero-config algorithm by opening a nested workspace folder. +// +// We should still compute diagnostics correctly for open packages. +func TestClearNonWorkspaceDiagnostics_NoView(t *testing.T) { + const ws = ` +-- a/go.mod -- +module example.com/a + +go 1.18 + +require example.com/b v1.2.3 + +replace example.com/b => ../b + +-- a/a.go -- +package a + +import "example.com/b" + +func _() { + V := b.B // unused +} + +-- b/go.mod -- +module b + +go 1.18 + +-- b/b.go -- +package b + +const B = 2 + +func _() { + var V int // unused +} + +-- b/b2.go -- +package b + +const B2 = B + +-- c/c.go -- +package main + +func main() { + var V int // unused +} +` + WithOptions( + WorkspaceFolders("a"), + ).Run(t, ws, func(t *testing.T, env *Env) { + env.OpenFile("a/a.go") + env.AfterChange( + Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")), + NoDiagnostics(ForFile("b/b.go")), + NoDiagnostics(ForFile("c/c.go")), + ) + env.OpenFile("b/b.go") + env.AfterChange( + Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")), + Diagnostics(env.AtRegexp("b/b.go", "V"), WithMessage("not used")), + NoDiagnostics(ForFile("c/c.go")), + ) - // Make an arbitrary edit because gopls explicitly diagnoses a/main.go - // whenever it is "changed". + // Opening b/b.go should not result in a new view, because b is not + // contained in a workspace folder. // - // TODO(rfindley): it should not be necessary to make another edit here. - // Gopls should be smart enough to avoid diagnosing a. - env.RegexpReplace("b/main.go", "package b", "package b // a package") + // Yet we should get diagnostics for b, because it is open. + if got, want := len(env.Views()), 1; got != want { + t.Errorf("after opening b/b.go, got %d views, want %d", got, want) + } + env.CloseBuffer("b/b.go") env.AfterChange( - NoDiagnostics(ForFile("a/main.go")), + Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")), + NoDiagnostics(ForFile("b/b.go")), + NoDiagnostics(ForFile("c/c.go")), + ) + + // We should get references in the b package. + bUse := env.RegexpSearch("a/a.go", `b\.(B)`) + refs := env.References(bUse) + wantRefs := []string{"a/a.go", "b/b.go", "b/b2.go"} + var gotRefs []string + for _, ref := range refs { + gotRefs = append(gotRefs, env.Sandbox.Workdir.URIToPath(ref.URI)) + } + sort.Strings(gotRefs) + if diff := cmp.Diff(wantRefs, gotRefs); diff != "" { + t.Errorf("references(b.B) mismatch (-want +got)\n%s", diff) + } + + // Opening c/c.go should also not result in a new view, yet we should get + // orphaned file diagnostics. + env.OpenFile("c/c.go") + env.AfterChange( + Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")), + NoDiagnostics(ForFile("b/b.go")), + Diagnostics(env.AtRegexp("c/c.go", "V"), WithMessage("not used")), + ) + if got, want := len(env.Views()), 1; got != want { + t.Errorf("after opening b/b.go, got %d views, want %d", got, want) + } + + env.CloseBuffer("c/c.go") + env.AfterChange( + Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")), + NoDiagnostics(ForFile("b/b.go")), + NoDiagnostics(ForFile("c/c.go")), + ) + env.CloseBuffer("a/a.go") + env.AfterChange( + Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")), + NoDiagnostics(ForFile("b/b.go")), + NoDiagnostics(ForFile("c/c.go")), ) }) } From a220b3b5ba60ebe7da61e2c142a885e611ccb757 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Tue, 20 Feb 2024 16:16:57 -0500 Subject: [PATCH 09/36] [gopls-release-branch.0.15] gopls/internal/cache: don't create Views for vendored modules We should not create Views for vendored modules, just as we don't create Views for modules in the module cache. With this change, gopls behaves similarly to gopls@v0.14.2 when navigating around the Kubernetes repo. Also add some test coverage that vendored packages are not workspace packages. Fixes golang/go#65830 Change-Id: If9883dc9616774952bd49c395e1c0d37ad3c2a6a Reviewed-on: https://go-review.googlesource.com/c/tools/+/565458 Reviewed-by: Alan Donovan Auto-Submit: Robert Findley LUCI-TryBot-Result: Go LUCI (cherry picked from commit a821e617f30094a078bfdc1d63623b93365b3130) Reviewed-on: https://go-review.googlesource.com/c/tools/+/565695 --- gopls/internal/cache/session.go | 12 +++- .../test/integration/misc/definition_test.go | 4 +- .../test/integration/misc/references_test.go | 6 +- .../test/integration/workspace/vendor_test.go | 67 +++++++++++++++++++ .../integration/workspace/zero_config_test.go | 56 ++++++++++++++++ 5 files changed, 136 insertions(+), 9 deletions(-) create mode 100644 gopls/internal/test/integration/workspace/vendor_test.go diff --git a/gopls/internal/cache/session.go b/gopls/internal/cache/session.go index 7d2bf43773f..3ad78336c5c 100644 --- a/gopls/internal/cache/session.go +++ b/gopls/internal/cache/session.go @@ -470,7 +470,17 @@ func selectViewDefs(ctx context.Context, fs file.Source, folders []*Folder, open folderForFile := func(uri protocol.DocumentURI) *Folder { var longest *Folder for _, folder := range folders { - if (longest == nil || len(folder.Dir) > len(longest.Dir)) && folder.Dir.Encloses(uri) { + // Check that this is a better match than longest, but not through a + // vendor directory. Count occurrences of "/vendor/" as a quick check + // that the vendor directory is between the folder and the file. Note the + // addition of a trailing "/" to handle the odd case where the folder is named + // vendor (which I hope is exceedingly rare in any case). + // + // Vendored packages are, by definition, part of an existing view. + if (longest == nil || len(folder.Dir) > len(longest.Dir)) && + folder.Dir.Encloses(uri) && + strings.Count(string(uri), "/vendor/") == strings.Count(string(folder.Dir)+"/", "/vendor/") { + longest = folder } } diff --git a/gopls/internal/test/integration/misc/definition_test.go b/gopls/internal/test/integration/misc/definition_test.go index b7394b8dd67..6b364e2e9d5 100644 --- a/gopls/internal/test/integration/misc/definition_test.go +++ b/gopls/internal/test/integration/misc/definition_test.go @@ -495,9 +495,7 @@ const _ = b.K } // Run 'go mod vendor' outside the editor. - if err := env.Sandbox.RunGoCommand(env.Ctx, ".", "mod", []string{"vendor"}, nil, true); err != nil { - t.Fatalf("go mod vendor: %v", err) - } + env.RunGoCommand("mod", "vendor") // Synchronize changes to watched files. env.Await(env.DoneWithChangeWatchedFiles()) diff --git a/gopls/internal/test/integration/misc/references_test.go b/gopls/internal/test/integration/misc/references_test.go index fcd72d85c68..73e4fffe3b8 100644 --- a/gopls/internal/test/integration/misc/references_test.go +++ b/gopls/internal/test/integration/misc/references_test.go @@ -360,8 +360,6 @@ func _() { // implementations in vendored modules were not found. The actual fix // was the same as for #55995; see TestVendoringInvalidatesMetadata. func TestImplementationsInVendor(t *testing.T) { - t.Skip("golang/go#56169: file watching does not capture vendor dirs") - const proxy = ` -- other.com/b@v1.0.0/go.mod -- module other.com/b @@ -415,9 +413,7 @@ var _ b.B checkVendor(env.Implementations(refLoc), false) // Run 'go mod vendor' outside the editor. - if err := env.Sandbox.RunGoCommand(env.Ctx, ".", "mod", []string{"vendor"}, nil, true); err != nil { - t.Fatalf("go mod vendor: %v", err) - } + env.RunGoCommand("mod", "vendor") // Synchronize changes to watched files. env.Await(env.DoneWithChangeWatchedFiles()) diff --git a/gopls/internal/test/integration/workspace/vendor_test.go b/gopls/internal/test/integration/workspace/vendor_test.go new file mode 100644 index 00000000000..f14cf539de0 --- /dev/null +++ b/gopls/internal/test/integration/workspace/vendor_test.go @@ -0,0 +1,67 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package workspace + +import ( + "testing" + + . "golang.org/x/tools/gopls/internal/test/integration" +) + +func TestWorkspacePackagesExcludesVendor(t *testing.T) { + // This test verifies that packages in the vendor directory are not workspace + // packages. This would be an easy mistake for gopls to make, since mod + // vendoring excludes go.mod files, and therefore the nearest go.mod file for + // vendored packages is often the workspace mod file. + const proxy = ` +-- other.com/b@v1.0.0/go.mod -- +module other.com/b + +go 1.18 + +-- other.com/b@v1.0.0/b.go -- +package b + +type B int + +func _() { + var V int // unused +} +` + const src = ` +-- go.mod -- +module example.com/a +go 1.14 +require other.com/b v1.0.0 + +-- go.sum -- +other.com/b v1.0.0 h1:ct1+0RPozzMvA2rSYnVvIfr/GDHcd7oVnw147okdi3g= +other.com/b v1.0.0/go.mod h1:bfTSZo/4ZtAQJWBYScopwW6n9Ctfsl2mi8nXsqjDXR8= + +-- a.go -- +package a + +import "other.com/b" + +var _ b.B + +` + WithOptions( + ProxyFiles(proxy), + Modes(Default), + ).Run(t, src, func(t *testing.T, env *Env) { + env.RunGoCommand("mod", "vendor") + // Uncomment for updated go.sum contents. + // env.DumpGoSum(".") + env.OpenFile("a.go") + env.AfterChange( + NoDiagnostics(), // as b is not a workspace package + ) + env.GoToDefinition(env.RegexpSearch("a.go", `b\.(B)`)) + env.AfterChange( + Diagnostics(env.AtRegexp("vendor/other.com/b/b.go", "V"), WithMessage("not used")), + ) + }) +} diff --git a/gopls/internal/test/integration/workspace/zero_config_test.go b/gopls/internal/test/integration/workspace/zero_config_test.go index 93c24886c9e..57498831a7d 100644 --- a/gopls/internal/test/integration/workspace/zero_config_test.go +++ b/gopls/internal/test/integration/workspace/zero_config_test.go @@ -5,6 +5,7 @@ package workspace import ( + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -268,3 +269,58 @@ package b } }) } + +func TestVendorExcluded(t *testing.T) { + // Test that we don't create Views for vendored modules. + // + // We construct the vendor directory manually here, as `go mod vendor` will + // omit the go.mod file. This synthesizes the setup of Kubernetes, where the + // entire module is vendored through a symlinked directory. + const src = ` +-- go.mod -- +module example.com/a + +go 1.18 + +require other.com/b v1.0.0 + +-- a.go -- +package a +import "other.com/b" +var _ b.B + +-- vendor/modules.txt -- +# other.com/b v1.0.0 +## explicit; go 1.14 +other.com/b + +-- vendor/other.com/b/go.mod -- +module other.com/b +go 1.14 + +-- vendor/other.com/b/b.go -- +package b +type B int + +func _() { + var V int // unused +} +` + WithOptions( + Modes(Default), + ).Run(t, src, func(t *testing.T, env *Env) { + env.OpenFile("a.go") + env.AfterChange(NoDiagnostics()) + loc := env.GoToDefinition(env.RegexpSearch("a.go", `b\.(B)`)) + if !strings.Contains(string(loc.URI), "/vendor/") { + t.Fatalf("Definition(b.B) = %v, want vendored location", loc.URI) + } + env.AfterChange( + Diagnostics(env.AtRegexp("vendor/other.com/b/b.go", "V"), WithMessage("not used")), + ) + + if views := env.Views(); len(views) != 1 { + t.Errorf("After opening /vendor/, got %d views, want 1. Views:\n%v", len(views), views) + } + }) +} From 50e6ff28fb47219a23c4d0ed12da704b94087f76 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Wed, 21 Feb 2024 10:06:40 -0500 Subject: [PATCH 10/36] gopls: update go.mod for v0.15.0-pre.5 For golang/go#64973 Change-Id: I5b15535ce2ce7f6edefb85c9da66fc4df0e24775 Reviewed-on: https://go-review.googlesource.com/c/tools/+/565715 LUCI-TryBot-Result: Go LUCI Auto-Submit: Robert Findley Reviewed-by: Alan Donovan --- gopls/go.mod | 2 +- gopls/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gopls/go.mod b/gopls/go.mod index f8e7200836b..97de14fbf61 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -10,7 +10,7 @@ require ( golang.org/x/sync v0.6.0 golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 golang.org/x/text v0.14.0 - golang.org/x/tools v0.18.1-0.20240216172611-fbcb99f4d7db + golang.org/x/tools v0.18.1-0.20240221145400-a220b3b5ba60 golang.org/x/vuln v1.0.1 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.4.6 diff --git a/gopls/go.sum b/gopls/go.sum index 75b5d1b0a80..402724e193b 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -27,8 +27,8 @@ golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= -golang.org/x/tools v0.18.1-0.20240216172611-fbcb99f4d7db h1:Eo6D0lMVoT72gzApl11ZrOJSUvohnhYq75BCfpVRHpc= -golang.org/x/tools v0.18.1-0.20240216172611-fbcb99f4d7db/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= +golang.org/x/tools v0.18.1-0.20240221145400-a220b3b5ba60 h1:2rj0Wb7+TXMtGEN3h6rrHno/8PmOJ/FVkLLTQSZeo4I= +golang.org/x/tools v0.18.1-0.20240221145400-a220b3b5ba60/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= golang.org/x/vuln v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= golang.org/x/vuln v1.0.1/go.mod h1:bb2hMwln/tqxg32BNY4CcxHWtHXuYa3SbIBmtsyjxtM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From a8f7013f917e91f7154a9da0fb67977da9b2f032 Mon Sep 17 00:00:00 2001 From: Patrick Pichler Date: Tue, 27 Feb 2024 11:23:51 +0000 Subject: [PATCH 11/36] [gopls-release-branch.0.15] gopls: add non nil if check around function result highlight The result of funcType will be nil, if a function does not return any values. This caused an SIGSEGV before. Fixes golang/go#65952 Change-Id: Ibf4ac3070744f42033504220f05b35a78c97d992 GitHub-Last-Rev: 74182b258d52620694f2d10c1d1518493dca907e GitHub-Pull-Request: golang/tools#480 Reviewed-on: https://go-review.googlesource.com/c/tools/+/567275 Reviewed-by: Mauri de Souza Meneguzzo Reviewed-by: Robert Findley Reviewed-by: Hyang-Ah Hana Kim LUCI-TryBot-Result: Go LUCI (cherry picked from commit c1f340a8c0b6c9fc336cdb1b6a58dde56d1b7e88) Reviewed-on: https://go-review.googlesource.com/c/tools/+/567415 Reviewed-by: Alan Donovan --- gopls/internal/golang/highlight.go | 70 +++++++++++++++--------------- 1 file changed, 36 insertions(+), 34 deletions(-) diff --git a/gopls/internal/golang/highlight.go b/gopls/internal/golang/highlight.go index f11426319cf..e9f1376a2c0 100644 --- a/gopls/internal/golang/highlight.go +++ b/gopls/internal/golang/highlight.go @@ -232,45 +232,47 @@ findEnclosingFunc: } } - // Scan fields, either adding highlights according to the highlightIndexes - // computed above, or accounting for the cursor position within the result - // list. - // (We do both at once to avoid repeating the cumbersome field traversal.) - i := 0 - findField: - for _, field := range funcType.Results.List { - for j, name := range field.Names { - if inNode(name) || highlightIndexes[i+j] { - result[posRange{name.Pos(), name.End()}] = unit{} - highlightIndexes[i+j] = true - break findField // found/highlighted the specific name - } - } - // If the cursor is in a field but not in a name (e.g. in the space, or - // the type), highlight the whole field. - // - // Note that this may not be ideal if we're at e.g. - // - // (x,‸y int, z int8) - // - // ...where it would make more sense to highlight only y. But we don't - // reach this function if not in a func, return, ident, or basiclit. - if inNode(field) || highlightIndexes[i] { - result[posRange{field.Pos(), field.End()}] = unit{} - highlightIndexes[i] = true - if inNode(field) { - for j := range field.Names { + if funcType.Results != nil { + // Scan fields, either adding highlights according to the highlightIndexes + // computed above, or accounting for the cursor position within the result + // list. + // (We do both at once to avoid repeating the cumbersome field traversal.) + i := 0 + findField: + for _, field := range funcType.Results.List { + for j, name := range field.Names { + if inNode(name) || highlightIndexes[i+j] { + result[posRange{name.Pos(), name.End()}] = unit{} highlightIndexes[i+j] = true + break findField // found/highlighted the specific name } } - break findField // found/highlighted the field - } + // If the cursor is in a field but not in a name (e.g. in the space, or + // the type), highlight the whole field. + // + // Note that this may not be ideal if we're at e.g. + // + // (x,‸y int, z int8) + // + // ...where it would make more sense to highlight only y. But we don't + // reach this function if not in a func, return, ident, or basiclit. + if inNode(field) || highlightIndexes[i] { + result[posRange{field.Pos(), field.End()}] = unit{} + highlightIndexes[i] = true + if inNode(field) { + for j := range field.Names { + highlightIndexes[i+j] = true + } + } + break findField // found/highlighted the field + } - n := len(field.Names) - if n == 0 { - n = 1 + n := len(field.Names) + if n == 0 { + n = 1 + } + i += n } - i += n } } } From 1c466157abc91275b53f487466e9c653a99b6cfa Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 27 Feb 2024 14:55:19 +0000 Subject: [PATCH 12/36] [gopls-release-branch.0.15] gopls/internal/test: add test for NPE in control flow highlighting Add a test for the fix in golang/go#65952: a nil pointer exception when highlighting a return value in a function returning no results. Also, merge tests related to control flow highlighting, since it is convenient to be able to run them together, and since there is nontrivial overhead to tiny tests. Updates golang/go#65952 Change-Id: Ibf8c7c6f0f4feed6dc7a283736bc038600a0bf04 Reviewed-on: https://go-review.googlesource.com/c/tools/+/567256 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI Auto-Submit: Robert Findley (cherry picked from commit fc70354354e63fa88b8cba95d89e3115bdd717dd) Reviewed-on: https://go-review.googlesource.com/c/tools/+/567417 --- .../marker/testdata/highlight/controlflow.txt | 71 +++++++++++++++++++ .../marker/testdata/highlight/issue60589.txt | 30 -------- .../marker/testdata/highlight/issue65516.txt | 7 -- 3 files changed, 71 insertions(+), 37 deletions(-) create mode 100644 gopls/internal/test/marker/testdata/highlight/controlflow.txt delete mode 100644 gopls/internal/test/marker/testdata/highlight/issue60589.txt delete mode 100644 gopls/internal/test/marker/testdata/highlight/issue65516.txt diff --git a/gopls/internal/test/marker/testdata/highlight/controlflow.txt b/gopls/internal/test/marker/testdata/highlight/controlflow.txt new file mode 100644 index 00000000000..25cc9394a47 --- /dev/null +++ b/gopls/internal/test/marker/testdata/highlight/controlflow.txt @@ -0,0 +1,71 @@ +This test verifies document highlighting for control flow. + +-- go.mod -- +module mod.com + +go 1.18 + +-- p.go -- +package p + +-- issue60589.go -- +package p + +// This test verifies that control flow lighlighting correctly +// accounts for multi-name result parameters. +// In golang/go#60589, it did not. + +func _() (foo int, bar, baz string) { //@ loc(func, "func"), loc(foo, "foo"), loc(fooint, "foo int"), loc(int, "int"), loc(bar, "bar"), loc(beforebaz, " baz"), loc(baz, "baz"), loc(barbazstring, "bar, baz string"), loc(beforestring, re`() string`), loc(string, "string") + return 0, "1", "2" //@ loc(return, `return 0, "1", "2"`), loc(l0, "0"), loc(l1, `"1"`), loc(l2, `"2"`) +} + +// Assertions, expressed here to avoid clutter above. +// Note that when the cursor is over the field type, there is some +// (likely harmless) redundancy. + +//@ highlight(func, func, return) +//@ highlight(foo, foo, l0) +//@ highlight(int, fooint, int, l0) +//@ highlight(bar, bar, l1) +//@ highlight(beforebaz) +//@ highlight(baz, baz, l2) +//@ highlight(beforestring, baz, l2) +//@ highlight(string, barbazstring, string, l1, l2) +//@ highlight(l0, foo, l0) +//@ highlight(l1, bar, l1) +//@ highlight(l2, baz, l2) + +// Check that duplicate result names do not cause +// inaccurate highlighting. + +func _() (x, x int32) { //@ loc(x1, re`\((x)`), loc(x2, re`(x) int`), diag(x1, re"redeclared"), diag(x2, re"redeclared") + return 1, 2 //@ loc(one, "1"), loc(two, "2") +} + +//@ highlight(one, one, x1) +//@ highlight(two, two, x2) +//@ highlight(x1, x1, one) +//@ highlight(x2, x2, two) + +-- issue65516.go -- +package p + +// This test checks that gopls doesn't crash while highlighting +// functions with no body (golang/go#65516). + +func Foo() (int, string) //@highlight("int", "int"), highlight("func", "func") + +-- issue65952.go -- +package p + +// This test checks that gopls doesn't crash while highlighting +// return values in functions with no results. + +func _() { + return 0 //@highlight("0", "0"), diag("0", re"too many return") +} + +func _() () { + // TODO(golang/go#65966): fix the triplicate diagnostics here. + return 0 //@highlight("0", "0"), diag("0", re"too many return"), diag("0", re"too many return"), diag("0", re"too many return") +} diff --git a/gopls/internal/test/marker/testdata/highlight/issue60589.txt b/gopls/internal/test/marker/testdata/highlight/issue60589.txt deleted file mode 100644 index afa4335a9c6..00000000000 --- a/gopls/internal/test/marker/testdata/highlight/issue60589.txt +++ /dev/null @@ -1,30 +0,0 @@ -This test verifies that control flow lighlighting correctly accounts for -multi-name result parameters. In golang/go#60589, it did not. - --- go.mod -- -module mod.com - -go 1.18 - --- p.go -- -package p - -func _() (foo int, bar, baz string) { //@ loc(func, "func"), loc(foo, "foo"), loc(fooint, "foo int"), loc(int, "int"), loc(bar, "bar"), loc(beforebaz, " baz"), loc(baz, "baz"), loc(barbazstring, "bar, baz string"), loc(beforestring, re`() string`), loc(string, "string") - return 0, "1", "2" //@ loc(return, `return 0, "1", "2"`), loc(l0, "0"), loc(l1, `"1"`), loc(l2, `"2"`) -} - -// Assertions, expressed here to avoid clutter above. -// Note that when the cursor is over the field type, there is some -// (likely harmless) redundancy. - -//@ highlight(func, func, return) -//@ highlight(foo, foo, l0) -//@ highlight(int, fooint, int, l0) -//@ highlight(bar, bar, l1) -//@ highlight(beforebaz) -//@ highlight(baz, baz, l2) -//@ highlight(beforestring, baz, l2) -//@ highlight(string, barbazstring, string, l1, l2) -//@ highlight(l0, foo, l0) -//@ highlight(l1, bar, l1) -//@ highlight(l2, baz, l2) diff --git a/gopls/internal/test/marker/testdata/highlight/issue65516.txt b/gopls/internal/test/marker/testdata/highlight/issue65516.txt deleted file mode 100644 index 3fc3be27416..00000000000 --- a/gopls/internal/test/marker/testdata/highlight/issue65516.txt +++ /dev/null @@ -1,7 +0,0 @@ -This test checks that gopls doesn't crash while highlighting functions with no -body (golang/go#65516). - --- p.go -- -package p - -func Foo() (int, string) //@highlight("int", "int"), highlight("func", "func") From e335b746546e6d85012916dbf85e830011b58951 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 27 Feb 2024 18:09:40 +0000 Subject: [PATCH 13/36] gopls: update go.mod for v0.15.1-pre.1 Update x/tools. For golang/go#65964 Change-Id: I0cebc18326d1b2c0949c0a8583d761bd22f737d0 Reviewed-on: https://go-review.googlesource.com/c/tools/+/567419 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan Auto-Submit: Robert Findley --- gopls/go.mod | 2 +- gopls/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gopls/go.mod b/gopls/go.mod index 97de14fbf61..17fb1ee6178 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -10,7 +10,7 @@ require ( golang.org/x/sync v0.6.0 golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 golang.org/x/text v0.14.0 - golang.org/x/tools v0.18.1-0.20240221145400-a220b3b5ba60 + golang.org/x/tools v0.18.1-0.20240227180630-1c466157abc9 golang.org/x/vuln v1.0.1 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.4.6 diff --git a/gopls/go.sum b/gopls/go.sum index 402724e193b..7289b6c5fa6 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -27,8 +27,8 @@ golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= -golang.org/x/tools v0.18.1-0.20240221145400-a220b3b5ba60 h1:2rj0Wb7+TXMtGEN3h6rrHno/8PmOJ/FVkLLTQSZeo4I= -golang.org/x/tools v0.18.1-0.20240221145400-a220b3b5ba60/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= +golang.org/x/tools v0.18.1-0.20240227180630-1c466157abc9 h1:DAFzI/OUTyNjVKs2nsM593aR8oHWjVh3ftZ7XQFEKXw= +golang.org/x/tools v0.18.1-0.20240227180630-1c466157abc9/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= golang.org/x/vuln v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= golang.org/x/vuln v1.0.1/go.mod h1:bb2hMwln/tqxg32BNY4CcxHWtHXuYa3SbIBmtsyjxtM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From 03e6d41c513ac085409a112bfc71ebfe02636ced Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 5 Mar 2024 18:09:53 +0000 Subject: [PATCH 14/36] [gopls-release-branch.0.15] gopls/internal/cache: prune broken edges to command-line-arguments pkgs Fix two bugs discovered during the investigation of golang/go#66109, which revealed the strange and broken intermediate test variant form "path/to/command/package [command-line-arguments.test]", referenced from the equally broken "command-line-arguments [command-line-arguments.test]". This latter package was *also* detected as an ITV, which is why we never tried to type check it in gopls@v0.14.2. - Snapshot.orphanedFileDiagnostics was not pruning intermediate test variants, causing it to be the one place where we were now type checking ITVs. - Fix the latent bug that caused gopls to record a dangling edge between the two ITVs. There is a third bug in go/packages, filed as golang/go#66126. Fixes golang/go#66109 Change-Id: Ie5795b6d5a4831bf2f73217c8eb22c6ba18e59cd Reviewed-on: https://go-review.googlesource.com/c/tools/+/569035 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan (cherry picked from commit caf59401b47d460e3f3d20cc3e209796f4753a46) Reviewed-on: https://go-review.googlesource.com/c/tools/+/569437 Auto-Submit: Robert Findley --- gopls/internal/cache/load.go | 25 +++++++++++++------ gopls/internal/cache/snapshot.go | 2 ++ .../marker/testdata/fixedbugs/issue66109.txt | 25 +++++++++++++++++++ 3 files changed, 45 insertions(+), 7 deletions(-) create mode 100644 gopls/internal/test/marker/testdata/fixedbugs/issue66109.txt diff --git a/gopls/internal/cache/load.go b/gopls/internal/cache/load.go index 74c90cd66f7..4661fd444c6 100644 --- a/gopls/internal/cache/load.go +++ b/gopls/internal/cache/load.go @@ -336,7 +336,11 @@ func (m *moduleErrorMap) Error() string { // buildMetadata populates the updates map with metadata updates to // apply, based on the given pkg. It recurs through pkg.Imports to ensure that // metadata exists for all dependencies. -func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Package, loadDir string, standalone bool) { +// +// Returns the metadata.Package that was built (or which was already present in +// updates), or nil if the package could not be built. Notably, the resulting +// metadata.Package may have an ID that differs from pkg.ID. +func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Package, loadDir string, standalone bool) *metadata.Package { // Allow for multiple ad-hoc packages in the workspace (see #47584). pkgPath := PackagePath(pkg.PkgPath) id := PackageID(pkg.ID) @@ -351,27 +355,27 @@ func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Packag // (Can this happen? #64557) if len(pkg.CompiledGoFiles) > 1 { bug.Reportf("unexpected files in command-line-arguments package: %v", pkg.CompiledGoFiles) - return + return nil } } else if len(pkg.IgnoredFiles) > 0 { // A file=empty.go query results in IgnoredFiles=[empty.go]. f = pkg.IgnoredFiles[0] } else { bug.Reportf("command-line-arguments package has neither CompiledGoFiles nor IgnoredFiles: %#v", "") //*pkg.Metadata) - return + return nil } id = PackageID(pkg.ID + f) pkgPath = PackagePath(pkg.PkgPath + f) } // Duplicate? - if _, ok := updates[id]; ok { + if existing, ok := updates[id]; ok { // A package was encountered twice due to shared // subgraphs (common) or cycles (rare). Although "go // list" usually breaks cycles, we don't rely on it. // breakImportCycles in metadataGraph.Clone takes care // of it later. - return + return existing } if pkg.TypesSizes == nil { @@ -492,15 +496,21 @@ func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Packag continue } - buildMetadata(updates, imported, loadDir, false) // only top level packages can be standalone + dep := buildMetadata(updates, imported, loadDir, false) // only top level packages can be standalone // Don't record edges to packages with no name, as they cause trouble for // the importer (golang/go#60952). // + // Also don't record edges to packages whose ID was modified (i.e. + // command-line-arguments packages), as encountered in golang/go#66109. In + // this case, we could theoretically keep the edge through dep.ID, but + // since this import doesn't make any sense in the first place, we instead + // choose to consider it invalid. + // // However, we do want to insert these packages into the update map // (buildMetadata above), so that we get type-checking diagnostics for the // invalid packages. - if imported.Name == "" { + if dep == nil || dep.ID != PackageID(imported.ID) || imported.Name == "" { depsByImpPath[importPath] = "" // missing continue } @@ -510,6 +520,7 @@ func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Packag } mp.DepsByImpPath = depsByImpPath mp.DepsByPkgPath = depsByPkgPath + return mp // m.Diagnostics is set later in the loading pass, using // computeLoadDiagnostics. diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go index 3c535b8d6a2..0a86fa2001c 100644 --- a/gopls/internal/cache/snapshot.go +++ b/gopls/internal/cache/snapshot.go @@ -1440,6 +1440,8 @@ searchOverlays: continue searchOverlays } } + metadata.RemoveIntermediateTestVariants(&mps) + // With zero-config gopls (golang/go#57979), orphaned file diagnostics // include diagnostics for orphaned files -- not just diagnostics relating // to the reason the files are opened. diff --git a/gopls/internal/test/marker/testdata/fixedbugs/issue66109.txt b/gopls/internal/test/marker/testdata/fixedbugs/issue66109.txt new file mode 100644 index 00000000000..c73390066ae --- /dev/null +++ b/gopls/internal/test/marker/testdata/fixedbugs/issue66109.txt @@ -0,0 +1,25 @@ +This test exercises the crash in golang/go#66109: a dangling reference due to +test variants of a command-line-arguments package. + +-- flags -- +-min_go=go1.22 + +-- go.mod -- +module example.com/tools + +go 1.22 + +-- tools_test.go -- +//go:build tools + +package tools //@diag("tools", re"No packages found") + +import ( + _ "example.com/tools/tool" +) + +-- tool/tool.go -- +package main + +func main() { +} From f5357f59bf231b3cc90abf1ed6b29287dbe86597 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Thu, 7 Mar 2024 17:56:59 +0000 Subject: [PATCH 15/36] [gopls-release-branch.0.15] gopls/internal/cache: fix spurious diagnostics in multi-root workspaces In golang/go#66145, users reported spurious import errors in multi-root workspaces. The problem was related to scenarios where module A had a local replace of module B, and the user opened a file F in module B that wasn't in the forward dependencies of module A. In this case, if the View of module A tried to load F, it would get real packages (not command-line-arguments), but due to module graph pruning the View of module A would not have access to the full set of dependencies for module B, resulting in the potential for import errors. Even this would not be a problem, as long as the package that module A loaded for F was not considered a 'workspace' package. Unfortunately a couple of incorrect heuristics in gopls added along with the zero-config work of gopls@v0.15.0 allowed us to diagnose these broken packages: 1. In resolveImportGraph, we called MetadataForFile for each overlay. As a result, the import graph optimization caused gopls to attempt loading packages for each open file, for each View. It was wrong for an optimization to have this side effect. 2. In golang/go#65801, we observed that it was inconsistent to diagnose changed packages independent of whether they're workspace packages. To fix that, I made all open packages workspace packages. It was probably wrong for the set of workspace packages to depend on open files. To summarize a rather long philosophical digression: open files should determine Views, not packages. Fixing either one of these incorrect heuristics would have prevented golang/go#66145. In this CL, we fix (2) by building the import graph based on existing metadata, without triggering an additional load. For (1), we check IsWorkspacePackage in diagnoseChangedFiles to enforce consistency in the set of diagnosed packages. It would be nice to also remove the heuristic that "all open packages are workspace packages", but we can't do that yet as it would mean no diagnostics for files outside the workspace, after e.g. jumping to definition. A TODO is left to address this another day, when we can be less conservative. Fixes golang/go#66145 Change-Id: Ic4cf2bbbb515b6ea0df24b8e6e46c725b82b4779 Reviewed-on: https://go-review.googlesource.com/c/tools/+/569836 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan (cherry picked from commit 93c0ca5673a16f730c2c886546bf80352dfd8908) Reviewed-on: https://go-review.googlesource.com/c/tools/+/569876 Auto-Submit: Robert Findley --- gopls/internal/cache/check.go | 14 +++- gopls/internal/cache/load.go | 33 ++++++-- gopls/internal/server/diagnostics.go | 7 +- .../workspace/multi_folder_test.go | 75 +++++++++++++++++++ 4 files changed, 120 insertions(+), 9 deletions(-) diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index 0af59655ab1..61eb98f9641 100644 --- a/gopls/internal/cache/check.go +++ b/gopls/internal/cache/check.go @@ -204,9 +204,17 @@ func (s *Snapshot) resolveImportGraph() (*importGraph, error) { openPackages := make(map[PackageID]bool) for _, fh := range s.Overlays() { - mps, err := s.MetadataForFile(ctx, fh.URI()) - if err != nil { - return nil, err + // golang/go#66145: don't call MetadataForFile here. This function, which + // builds a shared import graph, is an optimization. We don't want it to + // have the side effect of triggering a load. + // + // In the past, a call to MetadataForFile here caused a bunch of + // unnecessary loads in multi-root workspaces (and as a result, spurious + // diagnostics). + g := s.MetadataGraph() + var mps []*metadata.Package + for _, id := range g.IDs[fh.URI()] { + mps = append(mps, g.Packages[id]) } metadata.RemoveIntermediateTestVariants(&mps) for _, mp := range mps { diff --git a/gopls/internal/cache/load.go b/gopls/internal/cache/load.go index 4661fd444c6..bcc551099d0 100644 --- a/gopls/internal/cache/load.go +++ b/gopls/internal/cache/load.go @@ -564,6 +564,22 @@ func computeLoadDiagnostics(ctx context.Context, snapshot *Snapshot, mp *metadat return diags } +// IsWorkspacePackage reports whether id points to a workspace package in s. +// +// Currently, the result depends on the current set of loaded packages, and so +// is not guaranteed to be stable. +func (s *Snapshot) IsWorkspacePackage(ctx context.Context, id PackageID) bool { + s.mu.Lock() + defer s.mu.Unlock() + + mg := s.meta + m := mg.Packages[id] + if m == nil { + return false + } + return isWorkspacePackageLocked(ctx, s, mg, m) +} + // isWorkspacePackageLocked reports whether p is a workspace package for the // snapshot s. // @@ -575,6 +591,12 @@ func computeLoadDiagnostics(ctx context.Context, snapshot *Snapshot, mp *metadat // heuristics. // // s.mu must be held while calling this function. +// +// TODO(rfindley): remove 'meta' from this function signature. Whether or not a +// package is a workspace package should depend only on the package, view +// definition, and snapshot file source. While useful, the heuristic +// "allFilesHaveRealPackages" does not add that much value and is path +// dependent as it depends on the timing of loads. func isWorkspacePackageLocked(ctx context.Context, s *Snapshot, meta *metadata.Graph, pkg *metadata.Package) bool { if metadata.IsCommandLineArguments(pkg.ID) { // Ad-hoc command-line-arguments packages aren't workspace packages. @@ -599,12 +621,13 @@ func isWorkspacePackageLocked(ctx context.Context, s *Snapshot, meta *metadata.G return containsOpenFileLocked(s, pkg) } - // golang/go#65801: any (non command-line-arguments) open package is a - // workspace package. + // If a real package is open, consider it to be part of the workspace. // - // Otherwise, we'd display diagnostics for changes in an open package (due to - // the logic of diagnoseChangedFiles), but then erase those diagnostics when - // we do the final diagnostics pass. Diagnostics should be stable. + // TODO(rfindley): reconsider this. In golang/go#66145, we saw that even if a + // View sees a real package for a file, it doesn't mean that View is able to + // cleanly diagnose the package. Yet, we do want to show diagnostics for open + // packages outside the workspace. Is there a better way to ensure that only + // the 'best' View gets a workspace package for the open file? if containsOpenFileLocked(s, pkg) { return true } diff --git a/gopls/internal/server/diagnostics.go b/gopls/internal/server/diagnostics.go index 4d8e3aae4d1..fe5e5055503 100644 --- a/gopls/internal/server/diagnostics.go +++ b/gopls/internal/server/diagnostics.go @@ -276,7 +276,12 @@ func (s *server) diagnoseChangedFiles(ctx context.Context, snapshot *cache.Snaps // noisy to log (and we'll handle things later in the slow pass). continue } - toDiagnose[meta.ID] = meta + // golang/go#65801: only diagnose changes to workspace packages. Otherwise, + // diagnostics will be unstable, as the slow-path diagnostics will erase + // them. + if snapshot.IsWorkspacePackage(ctx, meta.ID) { + toDiagnose[meta.ID] = meta + } } diags, err := snapshot.PackageDiagnostics(ctx, maps.Keys(toDiagnose)...) if err != nil { diff --git a/gopls/internal/test/integration/workspace/multi_folder_test.go b/gopls/internal/test/integration/workspace/multi_folder_test.go index 3dace862c24..6adc1f8d5ce 100644 --- a/gopls/internal/test/integration/workspace/multi_folder_test.go +++ b/gopls/internal/test/integration/workspace/multi_folder_test.go @@ -51,3 +51,78 @@ func _() { ) }) } + +func TestMultiView_LocalReplace(t *testing.T) { + // This is a regression test for #66145, where gopls attempted to load a + // package in a locally replaced module as a workspace package, resulting in + // spurious import diagnostics because the module graph had been pruned. + + const proxy = ` +-- example.com/c@v1.2.3/go.mod -- +module example.com/c + +go 1.20 + +-- example.com/c@v1.2.3/c.go -- +package c + +const C = 3 + +` + // In the past, gopls would only diagnose one View at a time + // (the last to have changed). + // + // This test verifies that gopls can maintain diagnostics for multiple Views. + const files = ` +-- a/go.mod -- +module golang.org/lsptests/a + +go 1.20 + +require golang.org/lsptests/b v1.2.3 + +replace golang.org/lsptests/b => ../b + +-- a/a.go -- +package a + +import "golang.org/lsptests/b" + +const A = b.B - 1 + +-- b/go.mod -- +module golang.org/lsptests/b + +go 1.20 + +require example.com/c v1.2.3 + +-- b/go.sum -- +example.com/c v1.2.3 h1:hsOPhoHQLZPEn7l3kNya3fR3SfqW0/rafZMP8ave6fg= +example.com/c v1.2.3/go.mod h1:4uG6Y5qX88LrEd4KfRoiguHZIbdLKUEHD1wXqPyrHcA= +-- b/b.go -- +package b + +const B = 2 + +-- b/unrelated/u.go -- +package unrelated + +import "example.com/c" + +const U = c.C +` + + WithOptions( + WorkspaceFolders("a", "b"), + ProxyFiles(proxy), + ).Run(t, files, func(t *testing.T, env *Env) { + // Opening unrelated first ensures that when we compute workspace packages + // for the "a" workspace, it includes the unrelated package, which will be + // unloadable from a as there is no a/go.sum. + env.OpenFile("b/unrelated/u.go") + env.AfterChange() + env.OpenFile("a/a.go") + env.AfterChange(NoDiagnostics()) + }) +} From 68630b32f58b502bc9b3ac8d8ba4f5d5a9266deb Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 8 Mar 2024 10:54:34 -0500 Subject: [PATCH 16/36] [gopls-release-branch.0.15] gopls/internal/cache: avoid go/types panic on version "go1.2.3" This change fixes a gopls panic caused by giving go/types@go.1.20 a Go version string with three components, e.g. go1.2.3. Unfortunately this is hard to write a test for, since it requires building gopls with go1.20 and running it with a go1.21 toolchain. Fixes golang/go#66195 Change-Id: I09257e6ded69568812b367ee80cafea30add93d3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/570135 LUCI-TryBot-Result: Go LUCI Reviewed-by: Robert Findley (cherry picked from commit 9b64301ea6a8b54b65ae8822dda04d4800de9b53) Reviewed-on: https://go-review.googlesource.com/c/tools/+/569879 Reviewed-by: Alan Donovan Auto-Submit: Robert Findley --- gopls/internal/cache/check.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index 61eb98f9641..c159410a2f5 100644 --- a/gopls/internal/cache/check.go +++ b/gopls/internal/cache/check.go @@ -9,6 +9,7 @@ import ( "crypto/sha256" "fmt" "go/ast" + "go/build" "go/parser" "go/token" "go/types" @@ -29,6 +30,7 @@ import ( "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/util/bug" "golang.org/x/tools/gopls/internal/util/safetoken" + "golang.org/x/tools/gopls/internal/util/slices" "golang.org/x/tools/internal/analysisinternal" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/event/tag" @@ -1628,10 +1630,19 @@ func (b *typeCheckBatch) typesConfig(ctx context.Context, inputs typeCheckInputs if inputs.goVersion != "" { goVersion := "go" + inputs.goVersion + // types.NewChecker panics if GoVersion is invalid. An unparsable mod // file should probably stop us before we get here, but double check // just in case. - if goVersionRx.MatchString(goVersion) { + // + // Prior to go/types@go1.21 the precondition was stricter: + // no patch version. That's not a problem when also using go1.20 list, + // as it would reject go.mod files containing a patch version, but + // go/types@go1.20 will panic on go.mod versions that are returned + // by go1.21 list, hence the need for the extra check. + if goVersionRx.MatchString(goVersion) && + (slices.Contains(build.Default.ReleaseTags, "go1.21") || + strings.Count(goVersion, ".") < 2) { // no patch version typesinternal.SetGoVersion(cfg, goVersion) } } From 3f95539a745a68d799abd55e987923f96109423a Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 8 Mar 2024 11:40:54 -0500 Subject: [PATCH 17/36] [gopls-release-branch.0.15] gopls/internal/server: set -mod=readonly when checking for upgrades MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a vendor directory is present, we must explicitly use -mod=readonly to query upgrades with `go list`. This was broken by the fixes for workspace vendoring, which removed the `-mod` flag in most usage of the gocommand package. Fixes golang/go#66055 Change-Id: I29efb617a8fe56e9752dc088dc5ea884f1cefb86 Reviewed-on: https://go-review.googlesource.com/c/tools/+/569877 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan (cherry picked from commit c1789339c5a63378820d81e96b5eaf376ec913a4) Reviewed-on: https://go-review.googlesource.com/c/tools/+/569878 Reviewed-by: Nooras Saba‎ Auto-Submit: Robert Findley --- gopls/internal/server/command.go | 1 + .../integration/codelens/codelens_test.go | 71 +++++++++++++++++-- internal/gocommand/invoke.go | 3 + 3 files changed, 68 insertions(+), 7 deletions(-) diff --git a/gopls/internal/server/command.go b/gopls/internal/server/command.go index 8681db7e0ac..0ea51c4b2e1 100644 --- a/gopls/internal/server/command.go +++ b/gopls/internal/server/command.go @@ -745,6 +745,7 @@ func (s *server) getUpgrades(ctx context.Context, snapshot *cache.Snapshot, uri stdout, err := snapshot.RunGoCommandDirect(ctx, cache.Normal|cache.AllowNetwork, &gocommand.Invocation{ Verb: "list", Args: append([]string{"-m", "-u", "-json"}, modules...), + ModFlag: "readonly", // necessary when vendor is present (golang/go#66055) WorkingDir: filepath.Dir(uri.Path()), }) if err != nil { diff --git a/gopls/internal/test/integration/codelens/codelens_test.go b/gopls/internal/test/integration/codelens/codelens_test.go index 07ad3b9431b..c1c28dab803 100644 --- a/gopls/internal/test/integration/codelens/codelens_test.go +++ b/gopls/internal/test/integration/codelens/codelens_test.go @@ -73,13 +73,7 @@ const ( } } -// This test confirms the full functionality of the code lenses for updating -// dependencies in a go.mod file. It checks for the code lens that suggests -// an update and then executes the command associated with that code lens. A -// regression test for golang/go#39446. It also checks that these code lenses -// only affect the diagnostics and contents of the containing go.mod file. -func TestUpgradeCodelens(t *testing.T) { - const proxyWithLatest = ` +const proxyWithLatest = ` -- golang.org/x/hello@v1.3.3/go.mod -- module golang.org/x/hello @@ -98,6 +92,13 @@ package hi var Goodbye error ` +// This test confirms the full functionality of the code lenses for updating +// dependencies in a go.mod file, when using a go.work file. It checks for the +// code lens that suggests an update and then executes the command associated +// with that code lens. A regression test for golang/go#39446. It also checks +// that these code lenses only affect the diagnostics and contents of the +// containing go.mod file. +func TestUpgradeCodelens_Workspace(t *testing.T) { const shouldUpdateDep = ` -- go.work -- go 1.18 @@ -246,6 +247,62 @@ require golang.org/x/hello v1.2.3 } } +func TestUpgradeCodelens_ModVendor(t *testing.T) { + // This test checks the regression of golang/go#66055. The upgrade codelens + // should work in a mod vendor context (the test above using a go.work file + // was not broken). + testenv.NeedsGo1Point(t, 22) + const shouldUpdateDep = ` +-- go.mod -- +module mod.com/a + +go 1.22 + +require golang.org/x/hello v1.2.3 +-- go.sum -- +golang.org/x/hello v1.2.3 h1:7Wesfkx/uBd+eFgPrq0irYj/1XfmbvLV8jZ/W7C2Dwg= +golang.org/x/hello v1.2.3/go.mod h1:OgtlzsxVMUUdsdQCIDYgaauCTH47B8T8vofouNJfzgY= +-- main.go -- +package main + +import "golang.org/x/hello/hi" + +func main() { + _ = hi.Goodbye +} +` + + const wantGoModA = `module mod.com/a + +go 1.22 + +require golang.org/x/hello v1.3.3 +` + + WithOptions( + ProxyFiles(proxyWithLatest), + ).Run(t, shouldUpdateDep, func(t *testing.T, env *Env) { + env.RunGoCommand("mod", "vendor") + env.AfterChange() + env.OpenFile("go.mod") + + env.ExecuteCodeLensCommand("go.mod", command.CheckUpgrades, nil) + d := &protocol.PublishDiagnosticsParams{} + env.OnceMet( + CompletedWork(server.DiagnosticWorkTitle(server.FromCheckUpgrades), 1, true), + Diagnostics(env.AtRegexp("go.mod", `require`), WithMessage("can be upgraded")), + ReadDiagnostics("go.mod", d), + ) + + // Apply the diagnostics to a/go.mod. + env.ApplyQuickFixes("go.mod", d.Diagnostics) + env.AfterChange() + if got := env.BufferText("go.mod"); got != wantGoModA { + t.Fatalf("go.mod upgrade failed:\n%s", compare.Text(wantGoModA, got)) + } + }) +} + func TestUnusedDependenciesCodelens(t *testing.T) { const proxy = ` -- golang.org/x/hello@v1.0.0/go.mod -- diff --git a/internal/gocommand/invoke.go b/internal/gocommand/invoke.go index 55312522dc2..f7de3c8283b 100644 --- a/internal/gocommand/invoke.go +++ b/internal/gocommand/invoke.go @@ -158,12 +158,15 @@ type Invocation struct { BuildFlags []string // If ModFlag is set, the go command is invoked with -mod=ModFlag. + // TODO(rfindley): remove, in favor of Args. ModFlag string // If ModFile is set, the go command is invoked with -modfile=ModFile. + // TODO(rfindley): remove, in favor of Args. ModFile string // If Overlay is set, the go command is invoked with -overlay=Overlay. + // TODO(rfindley): remove, in favor of Args. Overlay string // If CleanEnv is set, the invocation will run only with the environment From 86c58d0f7ea30d435593491c2f10a8adc567aa8a Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 7 Mar 2024 17:40:33 -0500 Subject: [PATCH 18/36] [gopls-release-branch.0.15] gopls/internal/cache: add assertions for telemetry crash Updates golang/go#64547 Change-Id: I35b2477b8f6182bf6774095f18726104227a2fcd Reviewed-on: https://go-review.googlesource.com/c/tools/+/569935 Reviewed-by: Robert Findley LUCI-TryBot-Result: Go LUCI Auto-Submit: Alan Donovan (cherry picked from commit 31f056a488d85809d97136fb7c8f609c858aa93d) Reviewed-on: https://go-review.googlesource.com/c/tools/+/569880 Auto-Submit: Robert Findley Reviewed-by: Alan Donovan --- gopls/internal/cache/analysis.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index 0b7bc08d378..95802a047be 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -1289,6 +1289,15 @@ func (act *action) exec() (interface{}, *actionSummary, error) { if end == token.NoPos { end = start } + + // debugging #64547 + if start < token.Pos(tokFile.Base()) { + bug.Reportf("start < start of file") + } + if end > token.Pos(tokFile.Base()+tokFile.Size()+1) { + bug.Reportf("end > end of file + 1") + } + return p.PosLocation(start, end) } } From 3a44541b58244ddffc7e2610ba6eeaf6400ddb85 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 8 Mar 2024 12:46:05 -0500 Subject: [PATCH 19/36] gopls: update go.mod for v0.15.2-pre.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update x/tools. For golang/go#66197 Change-Id: I9322ee52e4ed40fde3c6c597df2305c3dd2dfff3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/569881 Reviewed-by: Nooras Saba‎ Auto-Submit: Robert Findley LUCI-TryBot-Result: Go LUCI --- gopls/go.mod | 2 +- gopls/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gopls/go.mod b/gopls/go.mod index 17fb1ee6178..cc414ad4a51 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -10,7 +10,7 @@ require ( golang.org/x/sync v0.6.0 golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 golang.org/x/text v0.14.0 - golang.org/x/tools v0.18.1-0.20240227180630-1c466157abc9 + golang.org/x/tools v0.18.1-0.20240308174224-86c58d0f7ea3 golang.org/x/vuln v1.0.1 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.4.6 diff --git a/gopls/go.sum b/gopls/go.sum index 7289b6c5fa6..581c8055edb 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -27,8 +27,8 @@ golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= -golang.org/x/tools v0.18.1-0.20240227180630-1c466157abc9 h1:DAFzI/OUTyNjVKs2nsM593aR8oHWjVh3ftZ7XQFEKXw= -golang.org/x/tools v0.18.1-0.20240227180630-1c466157abc9/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= +golang.org/x/tools v0.18.1-0.20240308174224-86c58d0f7ea3 h1:3lLoBCqE6WcuEgOBUEecCKmpMt/6Oaf9Sk/eP+sZZLw= +golang.org/x/tools v0.18.1-0.20240308174224-86c58d0f7ea3/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= golang.org/x/vuln v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= golang.org/x/vuln v1.0.1/go.mod h1:bb2hMwln/tqxg32BNY4CcxHWtHXuYa3SbIBmtsyjxtM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From ba252e896e66498160639824d25892a6b4e11c05 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Mon, 11 Mar 2024 16:56:18 +0000 Subject: [PATCH 20/36] [gopls-release-branch.0.15] gopls/internal/server: fix crash in SignatureHelp Fix a crash when Snapshot.fileOf fails, due to e.g. context cancellation. The defer of release should only occur after the error is checked. Unfortunately, this is very hard to test since it likely occurs only due to races. This is our first bug found completely via automated crash reporting. Fixes golang/go#66090 Change-Id: I7c22b5c2d1a0115718cd4bf2b84c31cc38a891ec Reviewed-on: https://go-review.googlesource.com/c/tools/+/570675 Auto-Submit: Robert Findley Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI (cherry picked from commit f89da538c87b608ed77d64d40a153bf4ec200efa) Reviewed-on: https://go-review.googlesource.com/c/tools/+/570596 --- gopls/internal/server/signature_help.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gopls/internal/server/signature_help.go b/gopls/internal/server/signature_help.go index 712c35b820c..a10aa56d848 100644 --- a/gopls/internal/server/signature_help.go +++ b/gopls/internal/server/signature_help.go @@ -19,10 +19,10 @@ func (s *server) SignatureHelp(ctx context.Context, params *protocol.SignatureHe defer done() fh, snapshot, release, err := s.fileOf(ctx, params.TextDocument.URI) - defer release() if err != nil { return nil, err } + defer release() if snapshot.FileKind(fh) != file.Go { return nil, nil // empty result From adfa453e7c20040afc2914e614903456196022d0 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Mon, 11 Mar 2024 17:32:30 +0000 Subject: [PATCH 21/36] [gopls-release-branch.0.15] gopls/internal/golang: fix crash in package references Fix a crash found via telemetry, which may be encountered when finding references for a package where one of the package files lacks a valid name. Fixes golang/go#66250 Change-Id: Ifca9b6b7ab2ab8181b70e97d343fa2b072e866eb Reviewed-on: https://go-review.googlesource.com/c/tools/+/570597 Auto-Submit: Robert Findley Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI --- gopls/internal/golang/references.go | 13 ++++++++----- .../marker/testdata/fixedbugs/issue66250.txt | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 5 deletions(-) create mode 100644 gopls/internal/test/marker/testdata/fixedbugs/issue66250.txt diff --git a/gopls/internal/golang/references.go b/gopls/internal/golang/references.go index c448830a1d0..87d1de5dd70 100644 --- a/gopls/internal/golang/references.go +++ b/gopls/internal/golang/references.go @@ -197,11 +197,14 @@ func packageReferences(ctx context.Context, snapshot *cache.Snapshot, uri protoc if err != nil { return nil, err } - refs = append(refs, reference{ - isDeclaration: true, // (one of many) - location: mustLocation(f, f.File.Name), - pkgPath: widest.PkgPath, - }) + // golang/go#66250: don't crash if the package file lacks a name. + if f.File.Name.Pos().IsValid() { + refs = append(refs, reference{ + isDeclaration: true, // (one of many) + location: mustLocation(f, f.File.Name), + pkgPath: widest.PkgPath, + }) + } } return refs, nil diff --git a/gopls/internal/test/marker/testdata/fixedbugs/issue66250.txt b/gopls/internal/test/marker/testdata/fixedbugs/issue66250.txt new file mode 100644 index 00000000000..748d19de6d4 --- /dev/null +++ b/gopls/internal/test/marker/testdata/fixedbugs/issue66250.txt @@ -0,0 +1,17 @@ +This bug checks the fix for golang/go#66250. Package references should not +crash when one package file lacks a package name. + +TODO(rfindley): the -ignore_extra_diags flag is only necessary because of +problems matching diagnostics in the broken file, likely due to poor parser +recovery. + +-- flags -- +-ignore_extra_diags + +-- a.go -- +package x //@refs("x", "x") + +-- b.go -- + +func _() { +} From 78fbdeb6184226700378cd8949b33a9ebd9f66a9 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Mon, 11 Mar 2024 18:30:24 +0000 Subject: [PATCH 22/36] [gopls-release-branch.0.15] gopls/internal/server: update telemetry prompt link Now that go.dev/doc/telemetry exists, use it as a better link for information about go telemetry. For golang/go#63883 Change-Id: Ibe561c435e648b324a5ac444a8aade953354e92b Reviewed-on: https://go-review.googlesource.com/c/tools/+/570598 Reviewed-by: Hyang-Ah Hana Kim LUCI-TryBot-Result: Go LUCI --- gopls/internal/server/prompt.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gopls/internal/server/prompt.go b/gopls/internal/server/prompt.go index 72c5113dc4e..3167136a2ee 100644 --- a/gopls/internal/server/prompt.go +++ b/gopls/internal/server/prompt.go @@ -191,12 +191,12 @@ func (s *server) maybePromptForTelemetry(ctx context.Context, enabled bool) { return } - var prompt = `Go telemetry helps us improve Go by periodically sending anonymous metrics and crash reports to the Go team. Learn more at https://telemetry.go.dev/privacy. + var prompt = `Go telemetry helps us improve Go by periodically sending anonymous metrics and crash reports to the Go team. Learn more at https://go.dev/doc/telemetry. Would you like to enable Go telemetry? ` if s.Options().LinkifyShowMessage { - prompt = `Go telemetry helps us improve Go by periodically sending anonymous metrics and crash reports to the Go team. Learn more at [telemetry.go.dev/privacy](https://telemetry.go.dev/privacy). + prompt = `Go telemetry helps us improve Go by periodically sending anonymous metrics and crash reports to the Go team. Learn more at [go.dev/doc/telemetry](https://go.dev/doc/telemetry). Would you like to enable Go telemetry? ` From 270fd83ec7a71b0911948e62a2397ff43ad83790 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Mon, 11 Mar 2024 20:17:53 +0000 Subject: [PATCH 23/36] gopls: update go.mod for v0.15.2-pre.2 Update x/tools. For golang/go#66197 Change-Id: I7143d1a371a2eed88186cb0c5cf7019a78931b5e Reviewed-on: https://go-review.googlesource.com/c/tools/+/570756 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan --- gopls/go.mod | 2 +- gopls/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gopls/go.mod b/gopls/go.mod index cc414ad4a51..dd54fc53aca 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -10,7 +10,7 @@ require ( golang.org/x/sync v0.6.0 golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 golang.org/x/text v0.14.0 - golang.org/x/tools v0.18.1-0.20240308174224-86c58d0f7ea3 + golang.org/x/tools v0.18.1-0.20240311201521-78fbdeb61842 golang.org/x/vuln v1.0.1 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.4.6 diff --git a/gopls/go.sum b/gopls/go.sum index 581c8055edb..da2e878a44b 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -27,8 +27,8 @@ golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= -golang.org/x/tools v0.18.1-0.20240308174224-86c58d0f7ea3 h1:3lLoBCqE6WcuEgOBUEecCKmpMt/6Oaf9Sk/eP+sZZLw= -golang.org/x/tools v0.18.1-0.20240308174224-86c58d0f7ea3/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= +golang.org/x/tools v0.18.1-0.20240311201521-78fbdeb61842 h1:No0LMXYFkp3j4oEsPdtY8LUQz33gu79Rm9DE+izMeGQ= +golang.org/x/tools v0.18.1-0.20240311201521-78fbdeb61842/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= golang.org/x/vuln v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= golang.org/x/vuln v1.0.1/go.mod h1:bb2hMwln/tqxg32BNY4CcxHWtHXuYa3SbIBmtsyjxtM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From 4b7ce7c70d1f702c17826b8e686ce3cfca018887 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Thu, 14 Mar 2024 17:09:46 -0400 Subject: [PATCH 24/36] [gopls-release-branch.0.15] gopls: fix test failures due to quoting of names in go/types errors Starting soon, go/types will quote some user defined names in error messages as `a'. Update tests to be tolerant of the new syntax. Notably, logic in the unusedvariable analyzer which extracts the variable name had to be updated. Change-Id: I091ab2af9b5ed82aa7eacad8f4a9405f34fcded7 Reviewed-on: https://go-review.googlesource.com/c/tools/+/571517 LUCI-TryBot-Result: Go LUCI Auto-Submit: Robert Findley Reviewed-by: Robert Griesemer (cherry picked from commit 67e856beabf185fd771e1ef618781c1e84e4f386) Reviewed-on: https://go-review.googlesource.com/c/tools/+/577258 Reviewed-by: Alan Donovan --- .../unusedvariable/testdata/src/assign/a.go | 34 +++++++++---------- .../testdata/src/assign/a.go.golden | 20 +++++------ .../unusedvariable/testdata/src/decl/a.go | 8 ++--- .../testdata/src/decl/a.go.golden | 2 +- .../analysis/unusedvariable/unusedvariable.go | 3 ++ .../test/marker/testdata/completion/bad.txt | 18 +++++----- .../test/marker/testdata/completion/testy.txt | 2 +- .../marker/testdata/diagnostics/generated.txt | 4 +-- .../testdata/diagnostics/issue56943.txt | 2 +- .../test/marker/testdata/format/format.txt | 4 +-- 10 files changed, 50 insertions(+), 47 deletions(-) diff --git a/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go b/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go index aa9f46e5b31..0eb74e98b8c 100644 --- a/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go +++ b/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go @@ -14,55 +14,55 @@ type A struct { } func singleAssignment() { - v := "s" // want `v declared (and|but) not used` + v := "s" // want `v.*declared (and|but) not used` - s := []int{ // want `s declared (and|but) not used` + s := []int{ // want `s.*declared (and|but) not used` 1, 2, } - a := func(s string) bool { // want `a declared (and|but) not used` + a := func(s string) bool { // want `a.*declared (and|but) not used` return false } if 1 == 1 { - s := "v" // want `s declared (and|but) not used` + s := "v" // want `s.*declared (and|but) not used` } panic("I should survive") } func noOtherStmtsInBlock() { - v := "s" // want `v declared (and|but) not used` + v := "s" // want `v.*declared (and|but) not used` } func partOfMultiAssignment() { - f, err := os.Open("file") // want `f declared (and|but) not used` + f, err := os.Open("file") // want `f.*declared (and|but) not used` panic(err) } func sideEffects(cBool chan bool, cInt chan int) { - b := <-c // want `b declared (and|but) not used` - s := fmt.Sprint("") // want `s declared (and|but) not used` - a := A{ // want `a declared (and|but) not used` + b := <-c // want `b.*declared (and|but) not used` + s := fmt.Sprint("") // want `s.*declared (and|but) not used` + a := A{ // want `a.*declared (and|but) not used` b: func() int { return 1 }(), } - c := A{<-cInt} // want `c declared (and|but) not used` - d := fInt() + <-cInt // want `d declared (and|but) not used` - e := fBool() && <-cBool // want `e declared (and|but) not used` - f := map[int]int{ // want `f declared (and|but) not used` + c := A{<-cInt} // want `c.*declared (and|but) not used` + d := fInt() + <-cInt // want `d.*declared (and|but) not used` + e := fBool() && <-cBool // want `e.*declared (and|but) not used` + f := map[int]int{ // want `f.*declared (and|but) not used` fInt(): <-cInt, } - g := []int{<-cInt} // want `g declared (and|but) not used` - h := func(s string) {} // want `h declared (and|but) not used` - i := func(s string) {}() // want `i declared (and|but) not used` + g := []int{<-cInt} // want `g.*declared (and|but) not used` + h := func(s string) {} // want `h.*declared (and|but) not used` + i := func(s string) {}() // want `i.*declared (and|but) not used` } func commentAbove() { // v is a variable - v := "s" // want `v declared (and|but) not used` + v := "s" // want `v.*declared (and|but) not used` } func fBool() bool { diff --git a/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go.golden b/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go.golden index 18173ce0bf9..fd45e2efe98 100644 --- a/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go.golden +++ b/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go.golden @@ -24,26 +24,26 @@ func noOtherStmtsInBlock() { } func partOfMultiAssignment() { - _, err := os.Open("file") // want `f declared (and|but) not used` + _, err := os.Open("file") // want `f.*declared (and|but) not used` panic(err) } func sideEffects(cBool chan bool, cInt chan int) { - <-c // want `b declared (and|but) not used` - fmt.Sprint("") // want `s declared (and|but) not used` - A{ // want `a declared (and|but) not used` + <-c // want `b.*declared (and|but) not used` + fmt.Sprint("") // want `s.*declared (and|but) not used` + A{ // want `a.*declared (and|but) not used` b: func() int { return 1 }(), } - A{<-cInt} // want `c declared (and|but) not used` - fInt() + <-cInt // want `d declared (and|but) not used` - fBool() && <-cBool // want `e declared (and|but) not used` - map[int]int{ // want `f declared (and|but) not used` + A{<-cInt} // want `c.*declared (and|but) not used` + fInt() + <-cInt // want `d.*declared (and|but) not used` + fBool() && <-cBool // want `e.*declared (and|but) not used` + map[int]int{ // want `f.*declared (and|but) not used` fInt(): <-cInt, } - []int{<-cInt} // want `g declared (and|but) not used` - func(s string) {}() // want `i declared (and|but) not used` + []int{<-cInt} // want `g.*declared (and|but) not used` + func(s string) {}() // want `i.*declared (and|but) not used` } func commentAbove() { diff --git a/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go b/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go index 8e843024a54..57cb4b2c972 100644 --- a/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go +++ b/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go @@ -5,17 +5,17 @@ package decl func a() { - var b, c bool // want `b declared (and|but) not used` + var b, c bool // want `b.*declared (and|but) not used` panic(c) if 1 == 1 { - var s string // want `s declared (and|but) not used` + var s string // want `s.*declared (and|but) not used` } } func b() { // b is a variable - var b bool // want `b declared (and|but) not used` + var b bool // want `b.*declared (and|but) not used` } func c() { @@ -23,7 +23,7 @@ func c() { d string // some comment for c - c bool // want `c declared (and|but) not used` + c bool // want `c.*declared (and|but) not used` ) panic(d) diff --git a/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go.golden b/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go.golden index 6ed97332eea..3fbabed18ac 100644 --- a/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go.golden +++ b/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go.golden @@ -5,7 +5,7 @@ package decl func a() { - var c bool // want `b declared (and|but) not used` + var c bool // want `b.*declared (and|but) not used` panic(c) if 1 == 1 { diff --git a/gopls/internal/analysis/unusedvariable/unusedvariable.go b/gopls/internal/analysis/unusedvariable/unusedvariable.go index f8a4db1d292..106e856fee8 100644 --- a/gopls/internal/analysis/unusedvariable/unusedvariable.go +++ b/gopls/internal/analysis/unusedvariable/unusedvariable.go @@ -37,6 +37,9 @@ func run(pass *analysis.Pass) (interface{}, error) { for _, suffix := range unusedVariableSuffixes { if strings.HasSuffix(typeErr.Msg, suffix) { varName := strings.TrimSuffix(typeErr.Msg, suffix) + // Beginning in Go 1.23, go/types began quoting vars as `v'. + varName = strings.Trim(varName, "'`'") + err := runForError(pass, typeErr, varName) if err != nil { return nil, err diff --git a/gopls/internal/test/marker/testdata/completion/bad.txt b/gopls/internal/test/marker/testdata/completion/bad.txt index 4da021ae322..30a96afb043 100644 --- a/gopls/internal/test/marker/testdata/completion/bad.txt +++ b/gopls/internal/test/marker/testdata/completion/bad.txt @@ -20,7 +20,7 @@ func stuff() { //@item(stuff, "stuff", "func()", "func") x := "heeeeyyyy" random2(x) //@diag("x", re"cannot use x \\(variable of type string\\) as int value in argument to random2") random2(1) //@complete("dom", random, random2, random3) - y := 3 //@diag("y", re"y declared (and|but) not used") + y := 3 //@diag("y", re"y.*declared (and|but) not used") } type bob struct { //@item(bob, "bob", "struct{...}", "struct") @@ -48,9 +48,9 @@ func random() int { //@item(random, "random", "func() int", "func") } func random2(y int) int { //@item(random2, "random2", "func(y int) int", "func"),item(bad_y_param, "y", "int", "var") - x := 6 //@item(x, "x", "int", "var"),diag("x", re"x declared (and|but) not used") - var q blah //@item(q, "q", "blah", "var"),diag("q", re"q declared (and|but) not used"),diag("blah", re"(undeclared name|undefined): blah") - var t **blob //@item(t, "t", "**blob", "var"),diag("t", re"t declared (and|but) not used"),diag("blob", re"(undeclared name|undefined): blob") + x := 6 //@item(x, "x", "int", "var"),diag("x", re"x.*declared (and|but) not used") + var q blah //@item(q, "q", "blah", "var"),diag("q", re"q.*declared (and|but) not used"),diag("blah", re"(undeclared name|undefined): blah") + var t **blob //@item(t, "t", "**blob", "var"),diag("t", re"t.*declared (and|but) not used"),diag("blob", re"(undeclared name|undefined): blob") //@complete("", q, t, x, bad_y_param, global_a, bob, random, random2, random3, stateFunc, stuff) return y @@ -59,10 +59,10 @@ func random2(y int) int { //@item(random2, "random2", "func(y int) int", "func") func random3(y ...int) { //@item(random3, "random3", "func(y ...int)", "func"),item(y_variadic_param, "y", "[]int", "var") //@complete("", y_variadic_param, global_a, bob, random, random2, random3, stateFunc, stuff) - var ch chan (favType1) //@item(ch, "ch", "chan (favType1)", "var"),diag("ch", re"ch declared (and|but) not used"),diag("favType1", re"(undeclared name|undefined): favType1") - var m map[keyType]int //@item(m, "m", "map[keyType]int", "var"),diag("m", re"m declared (and|but) not used"),diag("keyType", re"(undeclared name|undefined): keyType") - var arr []favType2 //@item(arr, "arr", "[]favType2", "var"),diag("arr", re"arr declared (and|but) not used"),diag("favType2", re"(undeclared name|undefined): favType2") - var fn1 func() badResult //@item(fn1, "fn1", "func() badResult", "var"),diag("fn1", re"fn1 declared (and|but) not used"),diag("badResult", re"(undeclared name|undefined): badResult") - var fn2 func(badParam) //@item(fn2, "fn2", "func(badParam)", "var"),diag("fn2", re"fn2 declared (and|but) not used"),diag("badParam", re"(undeclared name|undefined): badParam") + var ch chan (favType1) //@item(ch, "ch", "chan (favType1)", "var"),diag("ch", re"ch.*declared (and|but) not used"),diag("favType1", re"(undeclared name|undefined): favType1") + var m map[keyType]int //@item(m, "m", "map[keyType]int", "var"),diag("m", re"m.*declared (and|but) not used"),diag("keyType", re"(undeclared name|undefined): keyType") + var arr []favType2 //@item(arr, "arr", "[]favType2", "var"),diag("arr", re"arr.*declared (and|but) not used"),diag("favType2", re"(undeclared name|undefined): favType2") + var fn1 func() badResult //@item(fn1, "fn1", "func() badResult", "var"),diag("fn1", re"fn1.*declared (and|but) not used"),diag("badResult", re"(undeclared name|undefined): badResult") + var fn2 func(badParam) //@item(fn2, "fn2", "func(badParam)", "var"),diag("fn2", re"fn2.*declared (and|but) not used"),diag("badParam", re"(undeclared name|undefined): badParam") //@complete("", arr, ch, fn1, fn2, m, y_variadic_param, global_a, bob, random, random2, random3, stateFunc, stuff) } diff --git a/gopls/internal/test/marker/testdata/completion/testy.txt b/gopls/internal/test/marker/testdata/completion/testy.txt index 983fc09160b..a7a9e1ce36c 100644 --- a/gopls/internal/test/marker/testdata/completion/testy.txt +++ b/gopls/internal/test/marker/testdata/completion/testy.txt @@ -47,7 +47,7 @@ import ( ) func TestSomething(t *testing.T) { //@item(TestSomething, "TestSomething(t *testing.T)", "", "func") - var x int //@loc(testyX, "x"), diag("x", re"x declared (and|but) not used") + var x int //@loc(testyX, "x"), diag("x", re"x.*declared (and|but) not used") a() //@loc(testyA, "a") } diff --git a/gopls/internal/test/marker/testdata/diagnostics/generated.txt b/gopls/internal/test/marker/testdata/diagnostics/generated.txt index bae69b1cd3a..7352f13aa94 100644 --- a/gopls/internal/test/marker/testdata/diagnostics/generated.txt +++ b/gopls/internal/test/marker/testdata/diagnostics/generated.txt @@ -10,12 +10,12 @@ package generated // Code generated by generator.go. DO NOT EDIT. func _() { - var y int //@diag("y", re"y declared (and|but) not used") + var y int //@diag("y", re"y.*declared (and|but) not used") } -- generator.go -- package generated func _() { - var x int //@diag("x", re"x declared (and|but) not used") + var x int //@diag("x", re"x.*declared (and|but) not used") } diff --git a/gopls/internal/test/marker/testdata/diagnostics/issue56943.txt b/gopls/internal/test/marker/testdata/diagnostics/issue56943.txt index f0b114bc42b..9695c0db0a2 100644 --- a/gopls/internal/test/marker/testdata/diagnostics/issue56943.txt +++ b/gopls/internal/test/marker/testdata/diagnostics/issue56943.txt @@ -12,7 +12,7 @@ import ( ) func main() { - var a int //@diag(re"(a) int", re"a declared.*not used") + var a int //@diag(re"(a) int", re"a.*declared.*not used") var _ ast.Expr = node{} //@diag("node{}", re"missing.*exprNode") } diff --git a/gopls/internal/test/marker/testdata/format/format.txt b/gopls/internal/test/marker/testdata/format/format.txt index b1437386768..bd078468e14 100644 --- a/gopls/internal/test/marker/testdata/format/format.txt +++ b/gopls/internal/test/marker/testdata/format/format.txt @@ -39,7 +39,7 @@ func hello() { - var x int //@diag("x", re"x declared (and|but) not used") + var x int //@diag("x", re"x.*declared (and|but) not used") } func hi() { @@ -59,7 +59,7 @@ import ( func hello() { - var x int //@diag("x", re"x declared (and|but) not used") + var x int //@diag("x", re"x.*declared (and|but) not used") } func hi() { From cf2002638317adc33f1dbf77a92032cfd1e64539 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 8 Apr 2024 13:15:20 -0400 Subject: [PATCH 25/36] all: add replace directive to help stage the v0.15.3 release For golang/go#66730 Change-Id: I2ca97b64212e0bff853893c4fe48013ea76cf7ca Reviewed-on: https://go-review.googlesource.com/c/tools/+/577298 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI --- gopls/go.mod | 2 ++ gopls/go.sum | 14 +++++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/gopls/go.mod b/gopls/go.mod index dd54fc53aca..cc125732113 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -26,3 +26,5 @@ require ( gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect ) + +replace golang.org/x/tools => ../ diff --git a/gopls/go.sum b/gopls/go.sum index da2e878a44b..c7ad7ba01e5 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -13,22 +13,30 @@ github.com/jba/templatecheck v0.6.0/go.mod h1:/1k7EajoSErFI9GLHAsiIJEaNLt3ALKNw2 github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= +github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= +golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= golang.org/x/exp/typeparams v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y= golang.org/x/exp/typeparams v0.0.0-20221212164502-fae10dda9338/go.mod h1:AbB0pIl9nAr9wVwH+Z2ZpaocVmF5I4GyWCDIsVjR0bk= +golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.15.0 h1:SernR4v+D55NyBH2QiEQrlBAnj1ECL6AGrA5+dPaMY8= golang.org/x/mod v0.15.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= +golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg= +golang.org/x/net v0.21.0/go.mod h1:bIjVDfnllIU7BJ2DNgfnXvpSvtn8VRwhlsaeUTyUS44= golang.org/x/sync v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ= golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.17.0 h1:25cE3gD+tdBA7lp7QfhuV+rJiE9YXTcS3VG1SqssI/Y= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808/go.mod h1:KG1lNk5ZFNssSZLrpVb4sMXKMpGwGXOxSG3rnu2gZQQ= golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 h1:vcVnuftN4J4UKLRcgetjzfU9FjjgXUUYUc3JhFplgV4= golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78/go.mod h1:KG1lNk5ZFNssSZLrpVb4sMXKMpGwGXOxSG3rnu2gZQQ= +golang.org/x/term v0.8.0/go.mod h1:xPskH00ivmX89bAKVGSKKtLOWNx2+17Eiy94tnKShWo= +golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= +golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= -golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= -golang.org/x/tools v0.18.1-0.20240311201521-78fbdeb61842 h1:No0LMXYFkp3j4oEsPdtY8LUQz33gu79Rm9DE+izMeGQ= -golang.org/x/tools v0.18.1-0.20240311201521-78fbdeb61842/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= golang.org/x/vuln v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= golang.org/x/vuln v1.0.1/go.mod h1:bb2hMwln/tqxg32BNY4CcxHWtHXuYa3SbIBmtsyjxtM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From 5a4dc7e3310740e1b8aa0019ce093152dfdd4270 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 25 Mar 2024 12:21:47 -0400 Subject: [PATCH 26/36] [gopls-release-branch.0.15] internal/imports: fix two "nil pointer in interface" bugs CL 559635 changed newModuleResolver so that it can return (nil, err). That means it is no longer safe to unconditionally convert the first result to a Resolver interface, but we forgot to check in two places, causing a crash that was reported by telemetry. This change adds the two checks. Updates golang/go#66490 Updates golang/go#66730 Change-Id: I3f2b84ed792b1eea179fc0d4d5ee9843281506fc Reviewed-on: https://go-review.googlesource.com/c/tools/+/574136 Reviewed-by: Peter Weinberger LUCI-TryBot-Result: Go LUCI (cherry picked from commit 63b3b5af27310e1ad7822eb366ce29a17fe25699) Reviewed-on: https://go-review.googlesource.com/c/tools/+/577297 Reviewed-by: Alan Donovan --- internal/imports/fix.go | 4 +++- internal/imports/mod.go | 22 +++++++++++++--------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/internal/imports/fix.go b/internal/imports/fix.go index 6a18f63a44d..a9784f4252c 100644 --- a/internal/imports/fix.go +++ b/internal/imports/fix.go @@ -988,8 +988,10 @@ func (e *ProcessEnv) GetResolver() (Resolver, error) { // already know the view type. if len(e.Env["GOMOD"]) == 0 && len(e.Env["GOWORK"]) == 0 { e.resolver = newGopathResolver(e) + } else if r, err := newModuleResolver(e, e.ModCache); err != nil { + e.resolverErr = err } else { - e.resolver, e.resolverErr = newModuleResolver(e, e.ModCache) + e.resolver = Resolver(r) } } diff --git a/internal/imports/mod.go b/internal/imports/mod.go index 3d0f38f6c23..4a034828406 100644 --- a/internal/imports/mod.go +++ b/internal/imports/mod.go @@ -313,15 +313,19 @@ func (r *ModuleResolver) ClearForNewScan() Resolver { // TODO(rfindley): move this to a new env.go, consolidating ProcessEnv methods. func (e *ProcessEnv) ClearModuleInfo() { if r, ok := e.resolver.(*ModuleResolver); ok { - resolver, resolverErr := newModuleResolver(e, e.ModCache) - if resolverErr == nil { - <-r.scanSema // acquire (guards caches) - resolver.moduleCacheCache = r.moduleCacheCache - resolver.otherCache = r.otherCache - r.scanSema <- struct{}{} // release - } - e.resolver = resolver - e.resolverErr = resolverErr + resolver, err := newModuleResolver(e, e.ModCache) + if err != nil { + e.resolver = nil + e.resolverErr = err + return + } + + <-r.scanSema // acquire (guards caches) + resolver.moduleCacheCache = r.moduleCacheCache + resolver.otherCache = r.otherCache + r.scanSema <- struct{}{} // release + + e.UpdateResolver(resolver) } } From 3051f4fd646cade15426d4d8e4b48a7b61f92ee7 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 29 Mar 2024 17:29:20 -0400 Subject: [PATCH 27/36] [gopls-release-branch.0.15] gopls/internal/server: filter diagnostics to "best" views Filter diagnostics only to the "best" view for a file. This reduces the likelihood that we show spurious import diagnostics due to module graph pruning, as reported by golang/go#66425. Absent a reproducer this is hard to test, yet the change makes intuitive sense (arguably): it is confusing if diagnostics are inconsistent with other operations like jump-to-definition that find the "best" view. Fixes golang/go#66425 Updates golang/go#66730 Change-Id: Iadb1a01518a30cc3dad2d412b1ded612ab35d6cc Reviewed-on: https://go-review.googlesource.com/c/tools/+/574718 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI (cherry picked from commit f345449c09b356db134114ed7e407b4c1eedc55c) Reviewed-on: https://go-review.googlesource.com/c/tools/+/577261 Auto-Submit: Robert Findley --- gopls/internal/cache/session.go | 42 +++++++++++-------- .../internal/golang/completion/completion.go | 2 +- gopls/internal/server/diagnostics.go | 30 ++++++++++++- 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/gopls/internal/cache/session.go b/gopls/internal/cache/session.go index 3ad78336c5c..05ed0694148 100644 --- a/gopls/internal/cache/session.go +++ b/gopls/internal/cache/session.go @@ -533,27 +533,17 @@ checkFiles: // Views and viewDefinitions. type viewDefiner interface{ definition() *viewDefinition } -// bestView returns the best View or viewDefinition that contains the -// given file, or (nil, nil) if no matching view is found. -// -// bestView only returns an error in the event of context cancellation. -// -// Making this function generic is convenient so that we can avoid mapping view -// definitions back to views inside Session.DidModifyFiles, where performance -// matters. It is, however, not the cleanest application of generics. +// BestViews returns the most relevant subset of views for a given uri. // -// Note: keep this function in sync with defineView. -func bestView[V viewDefiner](ctx context.Context, fs file.Source, fh file.Handle, views []V) (V, error) { - var zero V - +// This may be used to filter diagnostics to the most relevant builds. +func BestViews[V viewDefiner](ctx context.Context, fs file.Source, uri protocol.DocumentURI, views []V) ([]V, error) { if len(views) == 0 { - return zero, nil // avoid the call to findRootPattern + return nil, nil // avoid the call to findRootPattern } - uri := fh.URI() dir := uri.Dir() modURI, err := findRootPattern(ctx, dir, "go.mod", fs) if err != nil { - return zero, err + return nil, err } // Prefer GoWork > GoMod > GOPATH > GoPackages > AdHoc. @@ -631,8 +621,26 @@ func bestView[V viewDefiner](ctx context.Context, fs file.Source, fh file.Handle bestViews = goPackagesViews case len(adHocViews) > 0: bestViews = adHocViews - default: - return zero, nil + } + + return bestViews, nil +} + +// bestView returns the best View or viewDefinition that contains the +// given file, or (nil, nil) if no matching view is found. +// +// bestView only returns an error in the event of context cancellation. +// +// Making this function generic is convenient so that we can avoid mapping view +// definitions back to views inside Session.DidModifyFiles, where performance +// matters. It is, however, not the cleanest application of generics. +// +// Note: keep this function in sync with defineView. +func bestView[V viewDefiner](ctx context.Context, fs file.Source, fh file.Handle, views []V) (V, error) { + var zero V + bestViews, err := BestViews(ctx, fs, fh.URI(), views) + if err != nil || len(bestViews) == 0 { + return zero, err } content, err := fh.Content() diff --git a/gopls/internal/golang/completion/completion.go b/gopls/internal/golang/completion/completion.go index b796de4932d..9c66199a76c 100644 --- a/gopls/internal/golang/completion/completion.go +++ b/gopls/internal/golang/completion/completion.go @@ -463,7 +463,7 @@ func Completion(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p items, surrounding, innerErr := packageClauseCompletions(ctx, snapshot, fh, protoPos) if innerErr != nil { // return the error for GetParsedFile since it's more relevant in this situation. - return nil, nil, fmt.Errorf("getting file %s for Completion: %w (package completions: %v)", fh.URI(), err, innerErr) + return nil, nil, fmt.Errorf("getting file %s for Completion: %v (package completions: %v)", fh.URI(), err, innerErr) } return items, surrounding, nil } diff --git a/gopls/internal/server/diagnostics.go b/gopls/internal/server/diagnostics.go index fe5e5055503..551b8f775f8 100644 --- a/gopls/internal/server/diagnostics.go +++ b/gopls/internal/server/diagnostics.go @@ -830,7 +830,6 @@ func (s *server) updateOrphanedFileDiagnostics(ctx context.Context, modID uint64 // // If the publication succeeds, it updates f.publishedHash and f.mustPublish. func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet, uri protocol.DocumentURI, version int32, f *fileDiagnostics) error { - // We add a disambiguating suffix (e.g. " [darwin,arm64]") to // each diagnostic that doesn't occur in the default view; // see golang/go#65496. @@ -850,6 +849,8 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet for _, diag := range f.orphanedFileDiagnostics { add(diag, "") } + + var allViews []*cache.View for view, viewDiags := range f.byView { if _, ok := views[view]; !ok { delete(f.byView, view) // view no longer exists @@ -858,7 +859,34 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet if viewDiags.version != version { continue // a payload of diagnostics applies to a specific file version } + allViews = append(allViews, view) + } + + // Only report diagnostics from the best views for a file. This avoids + // spurious import errors when a view has only a partial set of dependencies + // for a package (golang/go#66425). + // + // It's ok to use the session to derive the eligible views, because we + // publish diagnostics following any state change, so the set of best views + // is eventually consistent. + bestViews, err := cache.BestViews(ctx, s.session, uri, allViews) + if err != nil { + return err + } + + if len(bestViews) == 0 { + // If we have no preferred diagnostics for a given file (i.e., the file is + // not naturally nested within a view), then all diagnostics should be + // considered valid. + // + // This could arise if the user jumps to definition outside the workspace. + // There is no view that owns the file, so its diagnostics are valid from + // any view. + bestViews = allViews + } + for _, view := range bestViews { + viewDiags := f.byView[view] // Compute the view's suffix (e.g. " [darwin,arm64]"). var suffix string { From 1202974997fdabe6fead5aeb9d952ba03dc45412 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Wed, 3 Apr 2024 11:32:30 -0400 Subject: [PATCH 28/36] [gopls-release-branch.0.15] gopls/internal/cache: fix crash in snapshot.Analyze with patch versions Fix the same crash as golang/go#66195, this time in Analyze: don't set invalid Go versions on the types.Config. The largest part of this change was writing a realistic test, since the lack of a test for golang/go#66195 caused us to miss this additional location. It was possible to create a test that worked, by using a flag to select a go1.21 binary location. For this test, it was required to move a couple additional integration test preconditions into integration.Main (otherwise, the test would be skipped due to the modified environment). Updates golang/go#66636 Updates golang/go#66730 Change-Id: I24385474d4a6ebf6b7e9ae8f20948564bad3f55e Reviewed-on: https://go-review.googlesource.com/c/tools/+/576135 Auto-Submit: Robert Findley Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI (cherry picked from commit c623a2817b4c985e38252e1ce89b85a91814bcea) Reviewed-on: https://go-review.googlesource.com/c/tools/+/577301 --- gopls/internal/cache/analysis.go | 9 +-- gopls/internal/cache/check.go | 34 ++++++---- gopls/internal/test/integration/regtest.go | 14 +++++ gopls/internal/test/integration/runner.go | 20 +++--- .../integration/workspace/goversion_test.go | 62 +++++++++++++++++++ internal/testenv/testenv.go | 7 ++- 6 files changed, 118 insertions(+), 28 deletions(-) create mode 100644 gopls/internal/test/integration/workspace/goversion_test.go diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index 95802a047be..1d75d5abb7c 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -32,11 +32,11 @@ import ( "golang.org/x/sync/errgroup" "golang.org/x/tools/go/analysis" + "golang.org/x/tools/gopls/internal/cache/metadata" "golang.org/x/tools/gopls/internal/file" "golang.org/x/tools/gopls/internal/filecache" - "golang.org/x/tools/gopls/internal/cache/metadata" - "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/progress" + "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/settings" "golang.org/x/tools/gopls/internal/util/astutil" "golang.org/x/tools/gopls/internal/util/bug" @@ -1012,10 +1012,7 @@ func (an *analysisNode) typeCheck(parsed []*ParsedGoFile) *analysisPackage { // Set Go dialect. if mp.Module != nil && mp.Module.GoVersion != "" { goVersion := "go" + mp.Module.GoVersion - // types.NewChecker panics if GoVersion is invalid. - // An unparsable mod file should probably stop us - // before we get here, but double check just in case. - if goVersionRx.MatchString(goVersion) { + if validGoVersion(goVersion) { typesinternal.SetGoVersion(cfg, goVersion) } } diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index c159410a2f5..81485dc47a9 100644 --- a/gopls/internal/cache/check.go +++ b/gopls/internal/cache/check.go @@ -1630,19 +1630,7 @@ func (b *typeCheckBatch) typesConfig(ctx context.Context, inputs typeCheckInputs if inputs.goVersion != "" { goVersion := "go" + inputs.goVersion - - // types.NewChecker panics if GoVersion is invalid. An unparsable mod - // file should probably stop us before we get here, but double check - // just in case. - // - // Prior to go/types@go1.21 the precondition was stricter: - // no patch version. That's not a problem when also using go1.20 list, - // as it would reject go.mod files containing a patch version, but - // go/types@go1.20 will panic on go.mod versions that are returned - // by go1.21 list, hence the need for the extra check. - if goVersionRx.MatchString(goVersion) && - (slices.Contains(build.Default.ReleaseTags, "go1.21") || - strings.Count(goVersion, ".") < 2) { // no patch version + if validGoVersion(goVersion) { typesinternal.SetGoVersion(cfg, goVersion) } } @@ -1653,6 +1641,26 @@ func (b *typeCheckBatch) typesConfig(ctx context.Context, inputs typeCheckInputs return cfg } +// validGoVersion reports whether goVersion is a valid Go version for go/types. +// types.NewChecker panics if GoVersion is invalid. +// +// Note that, prior to go1.21, go/types required exactly two components to the +// version number. For example, go types would panic with the Go version +// go1.21.1. validGoVersion handles this case when built with go1.20 or earlier. +func validGoVersion(goVersion string) bool { + if !goVersionRx.MatchString(goVersion) { + return false // malformed version string + } + + // TODO(rfindley): remove once we no longer support building gopls with Go + // 1.20 or earlier. + if !slices.Contains(build.Default.ReleaseTags, "go1.21") && strings.Count(goVersion, ".") >= 2 { + return false // unsupported patch version + } + + return true +} + // depsErrors creates diagnostics for each metadata error (e.g. import cycle). // These may be attached to import declarations in the transitive source files // of pkg, or to 'requires' declarations in the package's go.mod file. diff --git a/gopls/internal/test/integration/regtest.go b/gopls/internal/test/integration/regtest.go index 4e26fd79d5b..c183cfde061 100644 --- a/gopls/internal/test/integration/regtest.go +++ b/gopls/internal/test/integration/regtest.go @@ -106,8 +106,12 @@ func DefaultModes() Mode { return modes } +var runFromMain = false // true if Main has been called + // Main sets up and tears down the shared integration test state. func Main(m *testing.M, hook func(*settings.Options)) { + runFromMain = true + // golang/go#54461: enable additional debugging around hanging Go commands. gocommand.DebugHangingGoCommands = true @@ -127,6 +131,16 @@ func Main(m *testing.M, hook func(*settings.Options)) { // Disable GOPACKAGESDRIVER, as it can cause spurious test failures. os.Setenv("GOPACKAGESDRIVER", "off") + if skipReason := checkBuilder(); skipReason != "" { + fmt.Printf("Skipping all tests: %s\n", skipReason) + os.Exit(0) + } + + if err := testenv.HasTool("go"); err != nil { + fmt.Println("Missing go command") + os.Exit(1) + } + flag.Parse() runner = &Runner{ diff --git a/gopls/internal/test/integration/runner.go b/gopls/internal/test/integration/runner.go index 7696b346b8f..caa2fc64d6c 100644 --- a/gopls/internal/test/integration/runner.go +++ b/gopls/internal/test/integration/runner.go @@ -135,9 +135,14 @@ type TestFunc func(t *testing.T, env *Env) func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOption) { // TODO(rfindley): this function has gotten overly complicated, and warrants // refactoring. - t.Helper() - checkBuilder(t) - testenv.NeedsGoPackages(t) + + if !runFromMain { + // Main performs various setup precondition checks. + // While it could theoretically be made OK for a Runner to be used outside + // of Main, it is simpler to enforce that we only use the Runner from + // integration test suites. + t.Fatal("integration.Runner.Run must be run from integration.Main") + } tests := []struct { name string @@ -271,16 +276,17 @@ var longBuilders = map[string]string{ "windows-arm-zx2c4": "", } -func checkBuilder(t *testing.T) { - t.Helper() +// TODO(rfindley): inline into Main. +func checkBuilder() string { builder := os.Getenv("GO_BUILDER_NAME") if reason, ok := longBuilders[builder]; ok && testing.Short() { if reason != "" { - t.Skipf("Skipping %s with -short due to %s", builder, reason) + return fmt.Sprintf("skipping %s with -short due to %s", builder, reason) } else { - t.Skipf("Skipping %s with -short", builder) + return fmt.Sprintf("skipping %s with -short", builder) } } + return "" } type loggingFramer struct { diff --git a/gopls/internal/test/integration/workspace/goversion_test.go b/gopls/internal/test/integration/workspace/goversion_test.go new file mode 100644 index 00000000000..b6604afe6b3 --- /dev/null +++ b/gopls/internal/test/integration/workspace/goversion_test.go @@ -0,0 +1,62 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package workspace + +import ( + "flag" + "os" + "runtime" + "testing" + + . "golang.org/x/tools/gopls/internal/test/integration" +) + +var go121bin = flag.String("go121bin", "", "bin directory containing go 1.21 or later") + +// TODO(golang/go#65917): delete this test once we no longer support building +// gopls with older Go versions. +func TestCanHandlePatchVersions(t *testing.T) { + // This test verifies the fixes for golang/go#66195 and golang/go#66636 -- + // that gopls does not crash when encountering a go version with a patch + // number in the go.mod file. + // + // This is tricky to test, because the regression requires that gopls is + // built with an older go version, and then the environment is upgraded to + // have a more recent go. To set up this scenario, the test requires a path + // to a bin directory containing go1.21 or later. + if *go121bin == "" { + t.Skip("-go121bin directory is not set") + } + + if runtime.GOOS != "linux" && runtime.GOOS != "darwin" { + t.Skip("requires linux or darwin") // for PATH separator + } + + path := os.Getenv("PATH") + t.Setenv("PATH", *go121bin+":"+path) + + const files = ` +-- go.mod -- +module example.com/bar + +go 1.21.1 + +-- p.go -- +package bar + +type I interface { string } +` + + WithOptions( + EnvVars{ + "PATH": path, + }, + ).Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("p.go") + env.AfterChange( + NoDiagnostics(ForFile("p.go")), + ) + }) +} diff --git a/internal/testenv/testenv.go b/internal/testenv/testenv.go index 511da9d7e85..477fa362f76 100644 --- a/internal/testenv/testenv.go +++ b/internal/testenv/testenv.go @@ -45,7 +45,10 @@ var checkGoBuild struct { err error } -func hasTool(tool string) error { +// HasTool reports an error if the required tool is not available in PATH. +// +// For certain tools, it checks that the tool executable is correct. +func HasTool(tool string) error { if tool == "cgo" { enabled, err := cgoEnabled(false) if err != nil { @@ -192,7 +195,7 @@ func allowMissingTool(tool string) bool { // NeedsTool skips t if the named tool is not present in the path. // As a special case, "cgo" means "go" is present and can compile cgo programs. func NeedsTool(t testing.TB, tool string) { - err := hasTool(tool) + err := HasTool(tool) if err == nil { return } From 4bdbdcaf9198ac1969785b865598de903ebd42d3 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 5 Apr 2024 10:43:44 -0400 Subject: [PATCH 29/36] [gopls-release-branch.0.15] internal/check: filter out too-new Go versions for type checking NOTE: Patched for the release branch to use versions.Compare rather than versions.Before. The type checker produces an error if the Go version is too new. When compiled with Go 1.21, this error is silently dropped on the floor and the type checked package is empty, due to golang/go##66525. Guard against this very problematic failure mode by filtering out Go versions that are too new. We should also produce a diagnostic, but that is more complicated and covered by golang/go#61673. Also: fix a bug where sandbox cleanup would fail due to being run with a non-local toolchain. Updates golang/go#66677 Updates golang/go#66730 Change-Id: Ia66f17c195382c9c55cf0ef883e898553ce950e3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/576678 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI (cherry picked from commit de6db989f5b9b77373e850b5848a5dbb2284728e) Reviewed-on: https://go-review.googlesource.com/c/tools/+/577302 --- gopls/internal/cache/check.go | 17 +++++ .../internal/test/integration/fake/sandbox.go | 4 +- .../integration/workspace/goversion_test.go | 64 +++++++++++++++++++ gopls/internal/test/marker/doc.go | 3 +- gopls/internal/test/marker/marker_test.go | 9 +++ 5 files changed, 95 insertions(+), 2 deletions(-) diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index 81485dc47a9..a3e455d4af0 100644 --- a/gopls/internal/cache/check.go +++ b/gopls/internal/cache/check.go @@ -1652,6 +1652,10 @@ func validGoVersion(goVersion string) bool { return false // malformed version string } + if relVer := releaseVersion(); relVer != "" && versions.Compare(relVer, goVersion) < 0 { + return false // 'go list' is too new for go/types + } + // TODO(rfindley): remove once we no longer support building gopls with Go // 1.20 or earlier. if !slices.Contains(build.Default.ReleaseTags, "go1.21") && strings.Count(goVersion, ".") >= 2 { @@ -1661,6 +1665,19 @@ func validGoVersion(goVersion string) bool { return true } +// releaseVersion reports the Go language version used to compile gopls, or "" +// if it cannot be determined. +func releaseVersion() string { + if len(build.Default.ReleaseTags) > 0 { + v := build.Default.ReleaseTags[len(build.Default.ReleaseTags)-1] + var dummy int + if _, err := fmt.Sscanf(v, "go1.%d", &dummy); err == nil { + return v + } + } + return "" +} + // depsErrors creates diagnostics for each metadata error (e.g. import cycle). // These may be attached to import declarations in the transitive source files // of pkg, or to 'requires' declarations in the package's go.mod file. diff --git a/gopls/internal/test/integration/fake/sandbox.go b/gopls/internal/test/integration/fake/sandbox.go index 8218bc15bc5..571258c49f9 100644 --- a/gopls/internal/test/integration/fake/sandbox.go +++ b/gopls/internal/test/integration/fake/sandbox.go @@ -289,7 +289,9 @@ func (sb *Sandbox) GoVersion(ctx context.Context) (int, error) { func (sb *Sandbox) Close() error { var goCleanErr error if sb.gopath != "" { - goCleanErr = sb.RunGoCommand(context.Background(), "", "clean", []string{"-modcache"}, nil, false) + // Important: run this command in RootDir so that it doesn't interact with + // any toolchain downloads that may occur + goCleanErr = sb.RunGoCommand(context.Background(), sb.RootDir(), "clean", []string{"-modcache"}, nil, false) } err := robustio.RemoveAll(sb.rootdir) if err != nil || goCleanErr != nil { diff --git a/gopls/internal/test/integration/workspace/goversion_test.go b/gopls/internal/test/integration/workspace/goversion_test.go index b6604afe6b3..0a2f91505c2 100644 --- a/gopls/internal/test/integration/workspace/goversion_test.go +++ b/gopls/internal/test/integration/workspace/goversion_test.go @@ -7,10 +7,13 @@ package workspace import ( "flag" "os" + "os/exec" "runtime" + "strings" "testing" . "golang.org/x/tools/gopls/internal/test/integration" + "golang.org/x/tools/internal/testenv" ) var go121bin = flag.String("go121bin", "", "bin directory containing go 1.21 or later") @@ -60,3 +63,64 @@ type I interface { string } ) }) } + +func TestTypeCheckingFutureVersions(t *testing.T) { + // This test checks the regression in golang/go#66677, where go/types fails + // silently when the language version is 1.22. + // + // It does this by recreating the scenario of a toolchain upgrade to 1.22, as + // reported in the issue. For this to work, the test must be able to download + // toolchains from proxy.golang.org. + // + // This is really only a problem for Go 1.21, because with Go 1.23, the bug + // is fixed, and starting with 1.23 we're going to *require* 1.23 to build + // gopls. + // + // TODO(golang/go#65917): delete this test after Go 1.23 is released and + // gopls requires the latest Go to build. + testenv.SkipAfterGo1Point(t, 21) + + if testing.Short() { + t.Skip("skipping with -short, as this test uses the network") + } + + // If go 1.22.2 is already available in the module cache, reuse it rather + // than downloading it anew. + out, err := exec.Command("go", "env", "GOPATH").Output() + if err != nil { + t.Fatal(err) + } + gopath := strings.TrimSpace(string(out)) // use the ambient 1.22.2 toolchain if available + + const files = ` +-- go.mod -- +module example.com/foo + +go 1.22.2 + +-- main.go -- +package main + +func main() { + x := 1 +} +` + + WithOptions( + Modes(Default), // slow test, only run in one mode + EnvVars{ + "GOPATH": gopath, + "GOTOOLCHAIN": "", // not local + "GOPROXY": "https://proxy.golang.org/", + "GOSUMDB": "sum.golang.org", + }, + ).Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("main.go") + env.AfterChange( + Diagnostics( + env.AtRegexp("main.go", "x"), + WithMessage("not used"), + ), + ) + }) +} diff --git a/gopls/internal/test/marker/doc.go b/gopls/internal/test/marker/doc.go index 212f09c78a7..69c132da1f1 100644 --- a/gopls/internal/test/marker/doc.go +++ b/gopls/internal/test/marker/doc.go @@ -43,7 +43,8 @@ treatment by the test runner: - "flags": this file is treated as a whitespace-separated list of flags that configure the MarkerTest instance. Supported flags: - -min_go=go1.20 sets the minimum Go version for the test; + -{min,max}_go=go1.20 sets the {min,max}imum Go version for the test + (inclusive) -cgo requires that CGO_ENABLED is set and the cgo tool is available -write_sumfile=a,b,c instructs the test runner to generate go.sum files in these directories before running the test. diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go index 1652eec7282..dee1b99e7aa 100644 --- a/gopls/internal/test/marker/marker_test.go +++ b/gopls/internal/test/marker/marker_test.go @@ -129,6 +129,13 @@ func Test(t *testing.T) { } testenv.NeedsGo1Point(t, go1point) } + if test.maxGoVersion != "" { + var go1point int + if _, err := fmt.Sscanf(test.maxGoVersion, "go1.%d", &go1point); err != nil { + t.Fatalf("parsing -max_go version: %v", err) + } + testenv.SkipAfterGo1Point(t, go1point) + } if test.cgo { testenv.NeedsTool(t, "cgo") } @@ -500,6 +507,7 @@ type markerTest struct { // Parsed flags values. minGoVersion string + maxGoVersion string cgo bool writeGoSum []string // comma separated dirs to write go sum for skipGOOS []string // comma separated GOOS values to skip @@ -513,6 +521,7 @@ type markerTest struct { func (t *markerTest) flagSet() *flag.FlagSet { flags := flag.NewFlagSet(t.name, flag.ContinueOnError) flags.StringVar(&t.minGoVersion, "min_go", "", "if set, the minimum go1.X version required for this test") + flags.StringVar(&t.maxGoVersion, "max_go", "", "if set, the maximum go1.X version required for this test") flags.BoolVar(&t.cgo, "cgo", false, "if set, requires cgo (both the cgo tool and CGO_ENABLED=1)") flags.Var((*stringListValue)(&t.writeGoSum), "write_sumfile", "if set, write the sumfile for these directories") flags.Var((*stringListValue)(&t.skipGOOS), "skip_goos", "if set, skip this test on these GOOS values") From dda1721e98423f70e988a866959945cf37f35149 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 8 Apr 2024 13:47:47 -0400 Subject: [PATCH 30/36] [gopls-release-branch.0.15] gopls/internal/cache: avoid panic when the primary diagnostic is broken If the primary diagnostic has invalid position information (due to a bug in the parser or AST fix logic), gopls may panics when linking primary and related information. Avoid this panic. Updates golang/go#66731 Updates golang/go#66730 Change-Id: Ie2f95d158a1c93d00603a7ce4d38d8097bd8cb08 Reviewed-on: https://go-review.googlesource.com/c/tools/+/577259 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan (cherry picked from commit f41d27ef3ace9e6edd63207b6a566cc2dc7b552c) Reviewed-on: https://go-review.googlesource.com/c/tools/+/577303 Auto-Submit: Robert Findley --- gopls/internal/cache/check.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index a3e455d4af0..169d8c1c5db 100644 --- a/gopls/internal/cache/check.go +++ b/gopls/internal/cache/check.go @@ -1936,7 +1936,10 @@ func typeErrorsToDiagnostics(pkg *syntaxPackage, errs []types.Error, linkTarget // This is because go/types assumes that errors are read top-down, such as // in the cycle error "A refers to...". The structure of the secondary // error set likely only makes sense for the primary error. - if i > 0 { + // + // NOTE: len(diags) == 0 if the primary diagnostic has invalid positions. + // See also golang/go#66731. + if i > 0 && len(diags) > 0 { primary := diags[0] primary.Related = append(primary.Related, protocol.DiagnosticRelatedInformation{ Location: protocol.Location{URI: diag.URI, Range: diag.Range}, From 91b49925f8410304d471dc9608b4a20bcf80e4bd Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 4 Apr 2024 15:14:52 -0400 Subject: [PATCH 31/36] [gopls-release-branch.0.15] gopls/internal/cache: analysis: repair start/end and refine bug report Poor parser error recovery may cause Node.End to be zero, or a small positive displacement from zero due to recursive Node.End computation (#66683). This change further refines the bug.Reports for such problems, and additionally repairs the values heuristically to avoid downstream bug.Reports after toGobDiagnostics (#64547). Updates golang/go#66683 Updates golang/go#64547 Updates golang/go#66730 Change-Id: I7c795622ec6b63574978d2953c82036fcc4425af Reviewed-on: https://go-review.googlesource.com/c/tools/+/576655 LUCI-TryBot-Result: Go LUCI Reviewed-by: Robert Findley (cherry picked from commit c7b6b8d02fd4e576956c6b61773995ccbfce783f) Reviewed-on: https://go-review.googlesource.com/c/tools/+/577595 Auto-Submit: Alan Donovan --- gopls/internal/cache/analysis.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index 1d75d5abb7c..d462a9e3dc6 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -1288,11 +1288,24 @@ func (act *action) exec() (interface{}, *actionSummary, error) { } // debugging #64547 - if start < token.Pos(tokFile.Base()) { + fileStart := token.Pos(tokFile.Base()) + fileEnd := fileStart + token.Pos(tokFile.Size()) + if start < fileStart { bug.Reportf("start < start of file") + start = fileStart } - if end > token.Pos(tokFile.Base()+tokFile.Size()+1) { + if end < start { + // This can happen if End is zero (#66683) + // or a small positive displacement from zero + // due to recursively Node.End() computation. + // This usually arises from poor parser recovery + // of an incomplete term at EOF. + bug.Reportf("end < start of file") + end = fileEnd + } + if end > fileEnd+1 { bug.Reportf("end > end of file + 1") + end = fileEnd } return p.PosLocation(start, end) From c69dfaffd78ea784616c6efc7ece5eea34eaa8c0 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 8 Apr 2024 16:03:41 -0400 Subject: [PATCH 32/36] [gopls-release-branch.0.15] gopls/internal/cache: add debug assertions to refine golang/go#66732 Also, use go1.19 generic helpers for atomics. Updates golang/go#66732 Updates golang/go#66730 Change-Id: I7aa447144353ff2ac5906ca746e2f98b115aa732 Reviewed-on: https://go-review.googlesource.com/c/tools/+/577435 LUCI-TryBot-Result: Go LUCI Reviewed-by: Robert Findley Auto-Submit: Alan Donovan (cherry picked from commit f6298eb1183ae4a636b2240a31cf359ade3e4f26) Reviewed-on: https://go-review.googlesource.com/c/tools/+/577596 --- gopls/internal/cache/analysis.go | 50 +++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index d462a9e3dc6..bb27142e779 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -41,6 +41,7 @@ import ( "golang.org/x/tools/gopls/internal/util/astutil" "golang.org/x/tools/gopls/internal/util/bug" "golang.org/x/tools/gopls/internal/util/frob" + "golang.org/x/tools/gopls/internal/util/maps" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/event/tag" "golang.org/x/tools/internal/facts" @@ -177,8 +178,6 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac var tagStr string // sorted comma-separated list of PackageIDs { - // TODO(adonovan): replace with a generic map[S]any -> string - // function in the tag package, and use maps.Keys + slices.Sort. keys := make([]string, 0, len(pkgs)) for id := range pkgs { keys = append(keys, string(id)) @@ -303,10 +302,10 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac } // Add edge from predecessor. if from != nil { - atomic.AddInt32(&from.unfinishedSuccs, 1) // TODO(adonovan): use generics + from.unfinishedSuccs.Add(+1) // incref an.preds = append(an.preds, from) } - atomic.AddInt32(&an.unfinishedPreds, 1) + an.unfinishedPreds.Add(+1) return an, nil } @@ -387,7 +386,7 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac // prevents workers from enqeuing, and thus finishing, and thus allowing the // group to make progress: deadlock. limiter := make(chan unit, runtime.GOMAXPROCS(0)) - var completed int64 + var completed atomic.Int64 var enqueue func(*analysisNode) enqueue = func(an *analysisNode) { @@ -399,13 +398,13 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac if err != nil { return err // cancelled, or failed to produce a package } - maybeReport(atomic.AddInt64(&completed, 1)) + maybeReport(completed.Add(1)) an.summary = summary // Notify each waiting predecessor, // and enqueue it when it becomes a leaf. for _, pred := range an.preds { - if atomic.AddInt32(&pred.unfinishedSuccs, -1) == 0 { + if pred.unfinishedSuccs.Add(-1) == 0 { // decref enqueue(pred) } } @@ -427,6 +426,18 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac return nil, err // cancelled, or failed to produce a package } + // Inv: all root nodes now have a summary (#66732). + // + // We know this is falsified empirically. This means either + // the summary was "successfully" set to nil (above), or there + // is a problem with the graph such the enqueuing leaves does + // not lead to completion of roots (or an error). + for _, root := range roots { + if root.summary == nil { + bug.Report("root analysisNode has nil summary") + } + } + // Report diagnostics only from enabled actions that succeeded. // Errors from creating or analyzing packages are ignored. // Diagnostics are reported in the order of the analyzers argument. @@ -458,6 +469,7 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac } // Inv: root.summary is the successful result of run (via runCached). + // TODO(adonovan): fix: root.summary is sometimes nil! (#66732). summary, ok := root.summary.Actions[stableNames[a]] if summary == nil { panic(fmt.Sprintf("analyzeSummary.Actions[%q] = (nil, %t); got %v (#60551)", @@ -475,7 +487,7 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac } func (an *analysisNode) decrefPreds() { - if atomic.AddInt32(&an.unfinishedPreds, -1) == 0 { + if an.unfinishedPreds.Add(-1) == 0 { an.summary.Actions = nil } } @@ -510,8 +522,8 @@ type analysisNode struct { analyzers []*analysis.Analyzer // set of analyzers to run preds []*analysisNode // graph edges: succs map[PackageID]*analysisNode // (preds -> self -> succs) - unfinishedSuccs int32 - unfinishedPreds int32 // effectively a summary.Actions refcount + unfinishedSuccs atomic.Int32 + unfinishedPreds atomic.Int32 // effectively a summary.Actions refcount allDeps map[PackagePath]*analysisNode // all dependencies including self exportDeps map[PackagePath]*analysisNode // subset of allDeps ref'd by export data (+self) summary *analyzeSummary // serializable result of analyzing this package @@ -664,6 +676,9 @@ func (an *analysisNode) runCached(ctx context.Context) (*analyzeSummary, error) if data, err := filecache.Get(cacheKind, key); err == nil { // cache hit analyzeSummaryCodec.Decode(data, &summary) + if summary == nil { // debugging #66732 + bug.Reportf("analyzeSummaryCodec.Decode yielded nil *analyzeSummary") + } } else if err != filecache.ErrNotFound { return nil, bug.Errorf("internal error reading shared cache: %v", err) } else { @@ -673,8 +688,11 @@ func (an *analysisNode) runCached(ctx context.Context) (*analyzeSummary, error) if err != nil { return nil, err } + if summary == nil { // debugging #66732 (can't happen) + bug.Reportf("analyzeNode.run returned nil *analyzeSummary") + } - atomic.AddInt32(&an.unfinishedPreds, +1) // incref + an.unfinishedPreds.Add(+1) // incref go func() { defer an.decrefPreds() //decref @@ -742,13 +760,11 @@ func (an *analysisNode) cacheKey() [sha256.Size]byte { } // vdeps, in PackageID order - depIDs := make([]string, 0, len(an.succs)) - for depID := range an.succs { - depIDs = append(depIDs, string(depID)) - } - sort.Strings(depIDs) // TODO(adonovan): avoid conversions by using slices.Sort[PackageID] + depIDs := maps.Keys(an.succs) + // TODO(adonovan): use go1.2x slices.Sort(depIDs). + sort.Slice(depIDs, func(i, j int) bool { return depIDs[i] < depIDs[j] }) for _, depID := range depIDs { - vdep := an.succs[PackageID(depID)] + vdep := an.succs[depID] fmt.Fprintf(hasher, "dep: %s\n", vdep.mp.PkgPath) fmt.Fprintf(hasher, "export: %s\n", vdep.summary.DeepExportHash) From bf3e474594ee0fd2a8145017f1c978631a676d8f Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Tue, 9 Apr 2024 14:38:03 -0400 Subject: [PATCH 33/36] gopls: update go.mod for v0.15.3-pre.1 Remove the replace directive and update x/tools. For golang/go#66730 Change-Id: Icb7444a1ebeeeb8e95cc6639c4e6038a17a56735 Reviewed-on: https://go-review.googlesource.com/c/tools/+/577656 Reviewed-by: Alan Donovan Auto-Submit: Robert Findley LUCI-TryBot-Result: Go LUCI --- gopls/go.mod | 4 +--- gopls/go.sum | 14 +++----------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/gopls/go.mod b/gopls/go.mod index cc125732113..a50abb19694 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -10,7 +10,7 @@ require ( golang.org/x/sync v0.6.0 golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 golang.org/x/text v0.14.0 - golang.org/x/tools v0.18.1-0.20240311201521-78fbdeb61842 + golang.org/x/tools v0.18.1-0.20240409144231-c69dfaffd78e golang.org/x/vuln v1.0.1 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.4.6 @@ -26,5 +26,3 @@ require ( gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect ) - -replace golang.org/x/tools => ../ diff --git a/gopls/go.sum b/gopls/go.sum index c7ad7ba01e5..800136c2c35 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -13,30 +13,22 @@ github.com/jba/templatecheck v0.6.0/go.mod h1:/1k7EajoSErFI9GLHAsiIJEaNLt3ALKNw2 github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= -github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= -golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= golang.org/x/exp/typeparams v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y= golang.org/x/exp/typeparams v0.0.0-20221212164502-fae10dda9338/go.mod h1:AbB0pIl9nAr9wVwH+Z2ZpaocVmF5I4GyWCDIsVjR0bk= -golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.15.0 h1:SernR4v+D55NyBH2QiEQrlBAnj1ECL6AGrA5+dPaMY8= golang.org/x/mod v0.15.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= -golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg= -golang.org/x/net v0.21.0/go.mod h1:bIjVDfnllIU7BJ2DNgfnXvpSvtn8VRwhlsaeUTyUS44= golang.org/x/sync v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ= golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= -golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.17.0 h1:25cE3gD+tdBA7lp7QfhuV+rJiE9YXTcS3VG1SqssI/Y= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808/go.mod h1:KG1lNk5ZFNssSZLrpVb4sMXKMpGwGXOxSG3rnu2gZQQ= golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 h1:vcVnuftN4J4UKLRcgetjzfU9FjjgXUUYUc3JhFplgV4= golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78/go.mod h1:KG1lNk5ZFNssSZLrpVb4sMXKMpGwGXOxSG3rnu2gZQQ= -golang.org/x/term v0.8.0/go.mod h1:xPskH00ivmX89bAKVGSKKtLOWNx2+17Eiy94tnKShWo= -golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= +golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= +golang.org/x/tools v0.18.1-0.20240409144231-c69dfaffd78e h1:coUXHif/q1tnVcUOt9L07DAmTk8HLuZq3RmEVNpj93A= +golang.org/x/tools v0.18.1-0.20240409144231-c69dfaffd78e/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= golang.org/x/vuln v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= golang.org/x/vuln v1.0.1/go.mod h1:bb2hMwln/tqxg32BNY4CcxHWtHXuYa3SbIBmtsyjxtM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From d92ae0781217e30c3ce6a623849fe9c197cd5141 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Thu, 11 Apr 2024 13:39:34 -0400 Subject: [PATCH 34/36] [gopls-release-branch.0.15] gopls/internal/server: don't reset views if configuration did not change In CL 538796, options were made immutable on the View, meaning any change to options required a new View. As a result, the minorOptionsChange heuristic, which avoided recreating views on certain options changes, was removed. Unfortunately, in golang/go#66647, it appears that certain clients may send frequent didChangeConfiguration notifications. Presumably the configuration is unchanged, and yet gopls still reinitializes the view. This may be a cause of significant performance regression for these clients. Fix this by making didChangeConfiguration a no op if nothing changed, using reflect.DeepEqual to compare Options. Since Hooks are not comparable due to the GofumptFormat func value, they are excluded from comparison. A subsequent CL will remove hooks altogether. For golang/go#66647 Updates golang/go#66730 Change-Id: I280059953d6b128461bef1001da3034f89ba3226 Reviewed-on: https://go-review.googlesource.com/c/tools/+/578037 LUCI-TryBot-Result: Go LUCI Auto-Submit: Robert Findley Reviewed-by: Alan Donovan (cherry picked from commit e388fff4f5b7c0b77b5fcd94d2bbb229a16dbdb7) Reviewed-on: https://go-review.googlesource.com/c/tools/+/578040 --- gopls/doc/commands.md | 1 + gopls/internal/cache/session_test.go | 2 +- gopls/internal/cache/view.go | 4 +- gopls/internal/protocol/command/interface.go | 1 + gopls/internal/server/command.go | 1 + gopls/internal/server/general.go | 26 +------ gopls/internal/server/workspace.go | 50 ++++++++++++-- gopls/internal/settings/api_json.go | 2 +- gopls/internal/settings/settings_test.go | 9 +++ .../integration/misc/configuration_test.go | 69 ++++++++++++++++++- .../integration/workspace/zero_config_test.go | 5 +- 11 files changed, 132 insertions(+), 38 deletions(-) diff --git a/gopls/doc/commands.md b/gopls/doc/commands.md index 6a651faf467..8a40164fd10 100644 --- a/gopls/doc/commands.md +++ b/gopls/doc/commands.md @@ -679,6 +679,7 @@ Result: ``` []{ + "ID": string, "Type": string, "Root": string, "Folder": string, diff --git a/gopls/internal/cache/session_test.go b/gopls/internal/cache/session_test.go index 913c3bd1f27..dd6a64a5ebd 100644 --- a/gopls/internal/cache/session_test.go +++ b/gopls/internal/cache/session_test.go @@ -351,7 +351,7 @@ replace ( Dir: toURI(f.dir), Name: path.Base(f.dir), Options: opts, - Env: env, + Env: *env, }) } diff --git a/gopls/internal/cache/view.go b/gopls/internal/cache/view.go index ed52646f31d..3a68ff6c1bb 100644 --- a/gopls/internal/cache/view.go +++ b/gopls/internal/cache/view.go @@ -51,7 +51,7 @@ type Folder struct { Dir protocol.DocumentURI Name string // decorative name for UI; not necessarily unique Options *settings.Options - Env *GoEnv + Env GoEnv } // GoEnv holds the environment variables and data from the Go command that is @@ -426,7 +426,7 @@ func viewEnv(v *View) string { v.root.Path(), strings.TrimRight(v.folder.Env.GoVersionOutput, "\n"), v.folder.Options.BuildFlags, - *v.folder.Env, + v.folder.Env, v.envOverlay, ) diff --git a/gopls/internal/protocol/command/interface.go b/gopls/internal/protocol/command/interface.go index b0dd06088bd..879da607958 100644 --- a/gopls/internal/protocol/command/interface.go +++ b/gopls/internal/protocol/command/interface.go @@ -521,6 +521,7 @@ type DiagnoseFilesArgs struct { // A View holds summary information about a cache.View. type View struct { + ID string // view ID (the index of this view among all views created) Type string // view type (via cache.ViewType.String) Root protocol.DocumentURI // root dir of the view (e.g. containing go.mod or go.work) Folder protocol.DocumentURI // workspace folder associated with the view diff --git a/gopls/internal/server/command.go b/gopls/internal/server/command.go index 0ea51c4b2e1..219f4514098 100644 --- a/gopls/internal/server/command.go +++ b/gopls/internal/server/command.go @@ -1329,6 +1329,7 @@ func (c *commandHandler) Views(ctx context.Context) ([]command.View, error) { var summaries []command.View for _, view := range c.s.session.Views() { summaries = append(summaries, command.View{ + ID: view.ID(), Type: view.Type().String(), Root: view.Root(), Folder: view.Folder().Dir, diff --git a/gopls/internal/server/general.go b/gopls/internal/server/general.go index cdaee1b973b..638a504880c 100644 --- a/gopls/internal/server/general.go +++ b/gopls/internal/server/general.go @@ -458,29 +458,7 @@ func (s *server) SetOptions(opts *settings.Options) { s.options = opts } -func (s *server) newFolder(ctx context.Context, folder protocol.DocumentURI, name string) (*cache.Folder, error) { - opts := s.Options() - if opts.ConfigurationSupported { - scope := string(folder) - configs, err := s.client.Configuration(ctx, &protocol.ParamConfiguration{ - Items: []protocol.ConfigurationItem{{ - ScopeURI: &scope, - Section: "gopls", - }}, - }, - ) - if err != nil { - return nil, fmt.Errorf("failed to get workspace configuration from client (%s): %v", folder, err) - } - - opts = opts.Clone() - for _, config := range configs { - if err := s.handleOptionResults(ctx, settings.SetOptions(opts, config)); err != nil { - return nil, err - } - } - } - +func (s *server) newFolder(ctx context.Context, folder protocol.DocumentURI, name string, opts *settings.Options) (*cache.Folder, error) { env, err := cache.FetchGoEnv(ctx, folder, opts) if err != nil { return nil, err @@ -489,7 +467,7 @@ func (s *server) newFolder(ctx context.Context, folder protocol.DocumentURI, nam Dir: folder, Name: name, Options: opts, - Env: env, + Env: *env, }, nil } diff --git a/gopls/internal/server/workspace.go b/gopls/internal/server/workspace.go index 1a3c0864d33..60c2105cee9 100644 --- a/gopls/internal/server/workspace.go +++ b/gopls/internal/server/workspace.go @@ -7,10 +7,12 @@ package server import ( "context" "fmt" + "reflect" "sync" "golang.org/x/tools/gopls/internal/cache" "golang.org/x/tools/gopls/internal/protocol" + "golang.org/x/tools/gopls/internal/settings" "golang.org/x/tools/internal/event" ) @@ -37,7 +39,11 @@ func (s *server) addView(ctx context.Context, name string, dir protocol.Document if state < serverInitialized { return nil, nil, fmt.Errorf("addView called before server initialized") } - folder, err := s.newFolder(ctx, dir, name) + opts, err := s.fetchFolderOptions(ctx, dir) + if err != nil { + return nil, nil, err + } + folder, err := s.newFolder(ctx, dir, name, opts) if err != nil { return nil, nil, err } @@ -68,15 +74,45 @@ func (s *server) DidChangeConfiguration(ctx context.Context, _ *protocol.DidChan s.SetOptions(options) // Collect options for all workspace folders. - seen := make(map[protocol.DocumentURI]bool) - var newFolders []*cache.Folder - for _, view := range s.session.Views() { + // If none have changed, this is a no op. + folderOpts := make(map[protocol.DocumentURI]*settings.Options) + changed := false + // The set of views is implicitly guarded by the fact that gopls processes + // didChange notifications synchronously. + // + // TODO(rfindley): investigate this assumption: perhaps we should hold viewMu + // here. + views := s.session.Views() + for _, view := range views { folder := view.Folder() - if seen[folder.Dir] { + if folderOpts[folder.Dir] != nil { continue } - seen[folder.Dir] = true - newFolder, err := s.newFolder(ctx, folder.Dir, folder.Name) + opts, err := s.fetchFolderOptions(ctx, folder.Dir) + if err != nil { + return err + } + + // Ignore hooks for the purposes of equality. + sameOptions := reflect.DeepEqual(folder.Options.ClientOptions, opts.ClientOptions) && + reflect.DeepEqual(folder.Options.ServerOptions, opts.ServerOptions) && + reflect.DeepEqual(folder.Options.UserOptions, opts.UserOptions) && + reflect.DeepEqual(folder.Options.InternalOptions, opts.InternalOptions) + + if !sameOptions { + changed = true + } + folderOpts[folder.Dir] = opts + } + if !changed { + return nil + } + + var newFolders []*cache.Folder + for _, view := range views { + folder := view.Folder() + opts := folderOpts[folder.Dir] + newFolder, err := s.newFolder(ctx, folder.Dir, folder.Name, opts) if err != nil { return err } diff --git a/gopls/internal/settings/api_json.go b/gopls/internal/settings/api_json.go index 9324992cc3a..9aff1f5f200 100644 --- a/gopls/internal/settings/api_json.go +++ b/gopls/internal/settings/api_json.go @@ -913,7 +913,7 @@ var GeneratedAPIJSON = &APIJSON{ Command: "gopls.views", Title: "List current Views on the server.", Doc: "This command is intended for use by gopls tests only.", - ResultDoc: "[]{\n\t\"Type\": string,\n\t\"Root\": string,\n\t\"Folder\": string,\n\t\"EnvOverlay\": []string,\n}", + ResultDoc: "[]{\n\t\"ID\": string,\n\t\"Type\": string,\n\t\"Root\": string,\n\t\"Folder\": string,\n\t\"EnvOverlay\": []string,\n}", }, { Command: "gopls.workspace_stats", diff --git a/gopls/internal/settings/settings_test.go b/gopls/internal/settings/settings_test.go index ebc4f2c41a8..ac0cb41ced7 100644 --- a/gopls/internal/settings/settings_test.go +++ b/gopls/internal/settings/settings_test.go @@ -5,10 +5,19 @@ package settings import ( + "reflect" "testing" "time" ) +func TestDefaultsEquivalence(t *testing.T) { + opts1 := DefaultOptions() + opts2 := DefaultOptions() + if !reflect.DeepEqual(opts1, opts2) { + t.Fatal("default options are not equivalent using reflect.DeepEqual") + } +} + func TestSetOption(t *testing.T) { tests := []struct { name string diff --git a/gopls/internal/test/integration/misc/configuration_test.go b/gopls/internal/test/integration/misc/configuration_test.go index c6ed18041ae..df6c19f632e 100644 --- a/gopls/internal/test/integration/misc/configuration_test.go +++ b/gopls/internal/test/integration/misc/configuration_test.go @@ -39,7 +39,7 @@ var FooErr = errors.New("foo") NoDiagnostics(ForFile("a/a.go")), ) cfg := env.Editor.Config() - cfg.Settings = map[string]interface{}{ + cfg.Settings = map[string]any{ "staticcheck": true, } env.ChangeConfiguration(cfg) @@ -49,6 +49,73 @@ var FooErr = errors.New("foo") }) } +func TestIdenticalConfiguration(t *testing.T) { + // This test checks that changing configuration does not cause views to be + // recreated if there is no configuration change. + const files = ` +-- a.go -- +package p + +func _() { + var x *int + y := *x + _ = y +} +` + Run(t, files, func(t *testing.T, env *Env) { + // Sanity check: before disabling the nilness analyzer, we should have a + // diagnostic for the nil dereference. + env.OpenFile("a.go") + env.AfterChange( + Diagnostics( + ForFile("a.go"), + WithMessage("nil dereference"), + ), + ) + + // Collect the view ID before changing configuration. + viewID := func() string { + t.Helper() + views := env.Views() + if len(views) != 1 { + t.Fatalf("got %d views, want 1", len(views)) + } + return views[0].ID + } + before := viewID() + + // Now disable the nilness analyzer. + cfg := env.Editor.Config() + cfg.Settings = map[string]any{ + "analyses": map[string]any{ + "nilness": false, + }, + } + + // This should cause the diagnostic to disappear... + env.ChangeConfiguration(cfg) + env.AfterChange( + NoDiagnostics(), + ) + // ...and we should be on the second view. + after := viewID() + if after == before { + t.Errorf("after configuration change, got view %q (same as before), want new view", after) + } + + // Now change configuration again, this time with the same configuration as + // before. We should still have no diagnostics... + env.ChangeConfiguration(cfg) + env.AfterChange( + NoDiagnostics(), + ) + // ...and we should still be on the second view. + if got := viewID(); got != after { + t.Errorf("after second configuration change, got view %q, want %q", got, after) + } + }) +} + // Test that clients can configure per-workspace configuration, which is // queried via the scopeURI of a workspace/configuration request. // (this was broken in golang/go#65519). diff --git a/gopls/internal/test/integration/workspace/zero_config_test.go b/gopls/internal/test/integration/workspace/zero_config_test.go index 57498831a7d..95906274b93 100644 --- a/gopls/internal/test/integration/workspace/zero_config_test.go +++ b/gopls/internal/test/integration/workspace/zero_config_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "golang.org/x/tools/gopls/internal/cache" "golang.org/x/tools/gopls/internal/protocol/command" @@ -53,7 +54,7 @@ func main() {} } checkViews := func(want ...command.View) { got := env.Views() - if diff := cmp.Diff(want, got); diff != "" { + if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(command.View{}, "ID")); diff != "" { t.Errorf("SummarizeViews() mismatch (-want +got):\n%s", diff) } } @@ -130,7 +131,7 @@ package a } checkViews := func(want ...command.View) { got := env.Views() - if diff := cmp.Diff(want, got); diff != "" { + if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(command.View{}, "ID")); diff != "" { t.Errorf("SummarizeViews() mismatch (-want +got):\n%s", diff) } } From cd70d50baa6daa949efa12e295e10829f3a7bd46 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 12 Apr 2024 18:39:21 +0000 Subject: [PATCH 35/36] gopls: update go.mod for v0.15.3-pre.2 Remove the replace directive and update x/tools. For golang/go#66730 Change-Id: I012605bf892ba8ca86b9c068832117c68fb68928 Reviewed-on: https://go-review.googlesource.com/c/tools/+/578321 Reviewed-by: Alan Donovan Auto-Submit: Robert Findley LUCI-TryBot-Result: Go LUCI --- gopls/go.mod | 2 +- gopls/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gopls/go.mod b/gopls/go.mod index a50abb19694..be8210739ee 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -10,7 +10,7 @@ require ( golang.org/x/sync v0.6.0 golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 golang.org/x/text v0.14.0 - golang.org/x/tools v0.18.1-0.20240409144231-c69dfaffd78e + golang.org/x/tools v0.18.1-0.20240412183611-d92ae0781217 golang.org/x/vuln v1.0.1 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.4.6 diff --git a/gopls/go.sum b/gopls/go.sum index 800136c2c35..633de11db41 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -27,8 +27,8 @@ golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= -golang.org/x/tools v0.18.1-0.20240409144231-c69dfaffd78e h1:coUXHif/q1tnVcUOt9L07DAmTk8HLuZq3RmEVNpj93A= -golang.org/x/tools v0.18.1-0.20240409144231-c69dfaffd78e/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= +golang.org/x/tools v0.18.1-0.20240412183611-d92ae0781217 h1:uH9jJYgeLCvblH0S+03kFO0qUDxRkbLRLFiKVVDl7ak= +golang.org/x/tools v0.18.1-0.20240412183611-d92ae0781217/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= golang.org/x/vuln v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= golang.org/x/vuln v1.0.1/go.mod h1:bb2hMwln/tqxg32BNY4CcxHWtHXuYa3SbIBmtsyjxtM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From 429c9f0c2c0d52afd4862e9734511a94fea841b7 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 19 Apr 2024 18:38:31 -0400 Subject: [PATCH 36/36] [gopls-release-branch.0.15] gopls/internal/cache: use language versions when validating Go version The check in CL 576678 was comparing the release version, which is a language version, with the contents of the go directive, which may be a toolchain version. As a result, gopls detects go1.22 < go1.22.2, and does not set types.Config.GoVersion. Normally this would be acceptable, since go/types falls back on allowing all language features. Unfortunately, gopls@v0.15.x lacks CL 567635, and so the loopclosure analyzer reports a diagnostic in this case. Fix by comparing language versions. Fixes golang/go#567635 Change-Id: I13747f19e48186105967b9c777de5ca34908545f Reviewed-on: https://go-review.googlesource.com/c/tools/+/579758 LUCI-TryBot-Result: Go LUCI Reviewed-by: Tim King --- gopls/internal/cache/check.go | 2 +- .../marker/testdata/fixedbugs/issue66876.txt | 27 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 gopls/internal/test/marker/testdata/fixedbugs/issue66876.txt diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index 169d8c1c5db..9800abee458 100644 --- a/gopls/internal/cache/check.go +++ b/gopls/internal/cache/check.go @@ -1652,7 +1652,7 @@ func validGoVersion(goVersion string) bool { return false // malformed version string } - if relVer := releaseVersion(); relVer != "" && versions.Compare(relVer, goVersion) < 0 { + if relVer := releaseVersion(); relVer != "" && versions.Compare(versions.Lang(relVer), versions.Lang(goVersion)) < 0 { return false // 'go list' is too new for go/types } diff --git a/gopls/internal/test/marker/testdata/fixedbugs/issue66876.txt b/gopls/internal/test/marker/testdata/fixedbugs/issue66876.txt new file mode 100644 index 00000000000..d6edcb57a18 --- /dev/null +++ b/gopls/internal/test/marker/testdata/fixedbugs/issue66876.txt @@ -0,0 +1,27 @@ +This test checks that gopls successfully suppresses loopclosure diagnostics +when the go.mod go version is set to a 1.22 toolchain version (1.22.x). + +In golang/go#66876, gopls failed to handle this correctly. + +-- flags -- +-min_go=go1.22 + +-- go.mod -- +module example.com/loopclosure + +go 1.22.0 + +-- p.go -- +package main + +var x int //@loc(x, "x") + +func main() { + // Verify that type checking actually succeeded by jumping to + // an arbitrary definition. + _ = x //@def("x", x) + + for i := range 10 { + go func() { println(i) }() + } +}