From 0d8758985ceab3457760e8fec2f30a94d7c32f8c Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 31 Jan 2024 16:37:19 -0500 Subject: [PATCH 01/23] go/types/typeutil: audit for types.Alias safety Updates golang/go#65294 Change-Id: I921517b9c722d03aaa7c3dc3e0c45364b3a1d53d Reviewed-on: https://go-review.googlesource.com/c/tools/+/559915 LUCI-TryBot-Result: Go LUCI Reviewed-by: Tim King --- go/types/typeutil/map.go | 7 +++++++ go/types/typeutil/map_test.go | 29 +++++++++++++++++++++++++++++ go/types/typeutil/methodsetcache.go | 6 ++++-- 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/go/types/typeutil/map.go b/go/types/typeutil/map.go index 544246dac1c..e154be0bd60 100644 --- a/go/types/typeutil/map.go +++ b/go/types/typeutil/map.go @@ -12,6 +12,7 @@ import ( "go/types" "reflect" + "golang.org/x/tools/internal/aliases" "golang.org/x/tools/internal/typeparams" ) @@ -259,6 +260,9 @@ func (h Hasher) hashFor(t types.Type) uint32 { case *types.Basic: return uint32(t.Kind()) + case *aliases.Alias: + return h.Hash(t.Underlying()) + case *types.Array: return 9043 + 2*uint32(t.Len()) + 3*h.Hash(t.Elem()) @@ -457,6 +461,9 @@ func (h Hasher) shallowHash(t types.Type) uint32 { // elements (mostly Slice, Pointer, Basic, Named), // so there's no need to optimize anything else. switch t := t.(type) { + case *aliases.Alias: + return h.shallowHash(t.Underlying()) + case *types.Signature: var hash uint32 = 604171 if t.Variadic() { diff --git a/go/types/typeutil/map_test.go b/go/types/typeutil/map_test.go index 4891f7687d5..2cc1de786dc 100644 --- a/go/types/typeutil/map_test.go +++ b/go/types/typeutil/map_test.go @@ -247,6 +247,15 @@ var Issue56048 = Issue56048_I.m type Issue56048_Ib interface{ m() chan []*interface { Issue56048_Ib } } var Issue56048b = Issue56048_Ib.m +// Non-generic alias +type NonAlias int +type Alias1 = NonAlias +type Alias2 = NonAlias + +// Generic alias (requires go1.23) +// type SetOfInt = map[int]bool +// type Set[T comparable] = map[K]bool +// type SetOfInt2 = Set[int] ` fset := token.NewFileSet() @@ -307,6 +316,16 @@ var Issue56048b = Issue56048_Ib.m Quux = scope.Lookup("Quux").Type() Issue56048 = scope.Lookup("Issue56048").Type() Issue56048b = scope.Lookup("Issue56048b").Type() + + // In go1.23 these will be *types.Alias; for now they are all int. + NonAlias = scope.Lookup("NonAlias").Type() + Alias1 = scope.Lookup("Alias1").Type() + Alias2 = scope.Lookup("Alias2").Type() + + // Requires go1.23. + // SetOfInt = scope.Lookup("SetOfInt").Type() + // Set = scope.Lookup("Set").Type().(*types.Alias) + // SetOfInt2 = scope.Lookup("SetOfInt2").Type() ) tmap := new(typeutil.Map) @@ -379,6 +398,16 @@ var Issue56048b = Issue56048_Ib.m {Issue56048, "Issue56048", true}, // (not actually about generics) {Issue56048b, "Issue56048b", true}, // (not actually about generics) + + // All three types are identical. + {NonAlias, "NonAlias", true}, + {Alias1, "Alias1", false}, + {Alias2, "Alias2", false}, + + // Generic aliases: requires go1.23. + // {SetOfInt, "SetOfInt", true}, + // {Set, "Set", false}, + // {SetOfInt2, "SetOfInt2", false}, } for _, step := range steps { diff --git a/go/types/typeutil/methodsetcache.go b/go/types/typeutil/methodsetcache.go index a5d9310830c..bd71aafaaa1 100644 --- a/go/types/typeutil/methodsetcache.go +++ b/go/types/typeutil/methodsetcache.go @@ -9,6 +9,8 @@ package typeutil import ( "go/types" "sync" + + "golang.org/x/tools/internal/aliases" ) // A MethodSetCache records the method set of each type T for which @@ -32,12 +34,12 @@ func (cache *MethodSetCache) MethodSet(T types.Type) *types.MethodSet { cache.mu.Lock() defer cache.mu.Unlock() - switch T := T.(type) { + switch T := aliases.Unalias(T).(type) { case *types.Named: return cache.lookupNamed(T).value case *types.Pointer: - if N, ok := T.Elem().(*types.Named); ok { + if N, ok := aliases.Unalias(T.Elem()).(*types.Named); ok { return cache.lookupNamed(N).pointer } } From d64ed6ae4ce26ac4a29f26dda999e24bf027f9b8 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 6 Feb 2024 15:57:13 -0500 Subject: [PATCH 02/23] internal/refactor/inline: audit for types.Alias safety Updates golang/go#65294 Change-Id: I14f7d06a0e41799238707b20a88205ae1bfc1ce8 Reviewed-on: https://go-review.googlesource.com/c/tools/+/562036 Auto-Submit: Alan Donovan LUCI-TryBot-Result: Go LUCI Reviewed-by: Robert Findley Commit-Queue: Alan Donovan --- internal/refactor/inline/escape.go | 2 +- internal/refactor/inline/falcon.go | 3 ++- internal/refactor/inline/inline.go | 2 +- internal/refactor/inline/util.go | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/refactor/inline/escape.go b/internal/refactor/inline/escape.go index 795ad4feab6..a3f5e555e9f 100644 --- a/internal/refactor/inline/escape.go +++ b/internal/refactor/inline/escape.go @@ -72,7 +72,7 @@ func escape(info *types.Info, root ast.Node, f func(v *types.Var, escapes bool)) if sel, ok := n.Fun.(*ast.SelectorExpr); ok { if seln, ok := info.Selections[sel]; ok && seln.Kind() == types.MethodVal && - isPointer(seln.Obj().Type().(*types.Signature).Recv().Type()) { + isPointer(seln.Obj().Type().Underlying().(*types.Signature).Recv().Type()) { tArg, indirect := effectiveReceiver(seln) if !indirect && !isPointer(tArg) { lvalue(sel.X, true) // &x.f diff --git a/internal/refactor/inline/falcon.go b/internal/refactor/inline/falcon.go index 9863e8dbcfb..1b2cb746ade 100644 --- a/internal/refactor/inline/falcon.go +++ b/internal/refactor/inline/falcon.go @@ -17,6 +17,7 @@ import ( "strings" "golang.org/x/tools/go/types/typeutil" + "golang.org/x/tools/internal/aliases" "golang.org/x/tools/internal/typeparams" ) @@ -446,7 +447,7 @@ func (st *falconState) expr(e ast.Expr) (res any) { // = types.TypeAndValue | as // - for an array or *array, use [n]int. // The last two entail progressively stronger index checks. var ct ast.Expr // type syntax for constraint - switch t := t.(type) { + switch t := aliases.Unalias(t).(type) { case *types.Map: if types.IsInterface(t.Key()) { ct = &ast.MapType{ diff --git a/internal/refactor/inline/inline.go b/internal/refactor/inline/inline.go index 7eaa6bff3f5..24ffb55fd1a 100644 --- a/internal/refactor/inline/inline.go +++ b/internal/refactor/inline/inline.go @@ -1154,7 +1154,7 @@ func arguments(caller *Caller, calleeDecl *ast.FuncDecl, assign1 func(*types.Var // Make * or & explicit. argIsPtr := isPointer(arg.typ) - paramIsPtr := isPointer(seln.Obj().Type().(*types.Signature).Recv().Type()) + paramIsPtr := isPointer(seln.Obj().Type().Underlying().(*types.Signature).Recv().Type()) if !argIsPtr && paramIsPtr { // &recv arg.expr = &ast.UnaryExpr{Op: token.AND, X: arg.expr} diff --git a/internal/refactor/inline/util.go b/internal/refactor/inline/util.go index 267ef745e32..7581ca29a50 100644 --- a/internal/refactor/inline/util.go +++ b/internal/refactor/inline/util.go @@ -132,7 +132,7 @@ func indirectSelection(seln *types.Selection) bool { return true } - tParam := seln.Obj().Type().(*types.Signature).Recv().Type() + tParam := seln.Obj().Type().Underlying().(*types.Signature).Recv().Type() return isPointer(tArg) && !isPointer(tParam) // implicit * } From ab6796120bb2527bf20ffc2bba24b689bbf9525a Mon Sep 17 00:00:00 2001 From: Gopher Robot Date: Tue, 6 Feb 2024 22:53:47 +0000 Subject: [PATCH 03/23] internal/imports: update stdlib index for Go 1.22.0 For golang/go#38706. Change-Id: Ib2e9ce03cc86dd74e2498bf462c168169ae97d7a Reviewed-on: https://go-review.googlesource.com/c/tools/+/562277 LUCI-TryBot-Result: Go LUCI Auto-Submit: Gopher Robot Reviewed-by: David Chase Reviewed-by: Michael Knyszek --- internal/imports/zstdlib.go | 61 +++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/internal/imports/zstdlib.go b/internal/imports/zstdlib.go index 9f992c2bec8..8db24df2ff4 100644 --- a/internal/imports/zstdlib.go +++ b/internal/imports/zstdlib.go @@ -151,6 +151,7 @@ var stdlib = map[string][]string{ "cmp": { "Compare", "Less", + "Or", "Ordered", }, "compress/bzip2": { @@ -632,6 +633,8 @@ var stdlib = map[string][]string{ "NameMismatch", "NewCertPool", "NotAuthorizedToSign", + "OID", + "OIDFromInts", "PEMCipher", "PEMCipher3DES", "PEMCipherAES128", @@ -706,6 +709,7 @@ var stdlib = map[string][]string{ "LevelWriteCommitted", "Named", "NamedArg", + "Null", "NullBool", "NullByte", "NullFloat64", @@ -1921,6 +1925,7 @@ var stdlib = map[string][]string{ "R_LARCH_32", "R_LARCH_32_PCREL", "R_LARCH_64", + "R_LARCH_64_PCREL", "R_LARCH_ABS64_HI12", "R_LARCH_ABS64_LO20", "R_LARCH_ABS_HI20", @@ -1928,12 +1933,17 @@ var stdlib = map[string][]string{ "R_LARCH_ADD16", "R_LARCH_ADD24", "R_LARCH_ADD32", + "R_LARCH_ADD6", "R_LARCH_ADD64", "R_LARCH_ADD8", + "R_LARCH_ADD_ULEB128", + "R_LARCH_ALIGN", "R_LARCH_B16", "R_LARCH_B21", "R_LARCH_B26", + "R_LARCH_CFA", "R_LARCH_COPY", + "R_LARCH_DELETE", "R_LARCH_GNU_VTENTRY", "R_LARCH_GNU_VTINHERIT", "R_LARCH_GOT64_HI12", @@ -1953,6 +1963,7 @@ var stdlib = map[string][]string{ "R_LARCH_PCALA64_LO20", "R_LARCH_PCALA_HI20", "R_LARCH_PCALA_LO12", + "R_LARCH_PCREL20_S2", "R_LARCH_RELATIVE", "R_LARCH_RELAX", "R_LARCH_SOP_ADD", @@ -1983,8 +1994,10 @@ var stdlib = map[string][]string{ "R_LARCH_SUB16", "R_LARCH_SUB24", "R_LARCH_SUB32", + "R_LARCH_SUB6", "R_LARCH_SUB64", "R_LARCH_SUB8", + "R_LARCH_SUB_ULEB128", "R_LARCH_TLS_DTPMOD32", "R_LARCH_TLS_DTPMOD64", "R_LARCH_TLS_DTPREL32", @@ -2035,6 +2048,7 @@ var stdlib = map[string][]string{ "R_MIPS_LO16", "R_MIPS_NONE", "R_MIPS_PC16", + "R_MIPS_PC32", "R_MIPS_PJUMP", "R_MIPS_REL16", "R_MIPS_REL32", @@ -2952,6 +2966,8 @@ var stdlib = map[string][]string{ "RegisterName", }, "encoding/hex": { + "AppendDecode", + "AppendEncode", "Decode", "DecodeString", "DecodedLen", @@ -3233,6 +3249,7 @@ var stdlib = map[string][]string{ "TypeSpec", "TypeSwitchStmt", "UnaryExpr", + "Unparen", "ValueSpec", "Var", "Visitor", @@ -3492,6 +3509,7 @@ var stdlib = map[string][]string{ "XOR_ASSIGN", }, "go/types": { + "Alias", "ArgumentError", "Array", "AssertableTo", @@ -3559,6 +3577,7 @@ var stdlib = map[string][]string{ "MethodVal", "MissingMethod", "Named", + "NewAlias", "NewArray", "NewChan", "NewChecker", @@ -3627,6 +3646,7 @@ var stdlib = map[string][]string{ "Uint64", "Uint8", "Uintptr", + "Unalias", "Union", "Universe", "Unsafe", @@ -3643,6 +3663,11 @@ var stdlib = map[string][]string{ "WriteSignature", "WriteType", }, + "go/version": { + "Compare", + "IsValid", + "Lang", + }, "hash": { "Hash", "Hash32", @@ -4078,6 +4103,7 @@ var stdlib = map[string][]string{ "NewTextHandler", "Record", "SetDefault", + "SetLogLoggerLevel", "Source", "SourceKey", "String", @@ -4367,6 +4393,35 @@ var stdlib = map[string][]string{ "Uint64", "Zipf", }, + "math/rand/v2": { + "ChaCha8", + "ExpFloat64", + "Float32", + "Float64", + "Int", + "Int32", + "Int32N", + "Int64", + "Int64N", + "IntN", + "N", + "New", + "NewChaCha8", + "NewPCG", + "NewZipf", + "NormFloat64", + "PCG", + "Perm", + "Rand", + "Shuffle", + "Source", + "Uint32", + "Uint32N", + "Uint64", + "Uint64N", + "UintN", + "Zipf", + }, "mime": { "AddExtensionType", "BEncoding", @@ -4540,6 +4595,7 @@ var stdlib = map[string][]string{ "FS", "File", "FileServer", + "FileServerFS", "FileSystem", "Flusher", "Get", @@ -4566,6 +4622,7 @@ var stdlib = map[string][]string{ "MethodPut", "MethodTrace", "NewFileTransport", + "NewFileTransportFS", "NewRequest", "NewRequestWithContext", "NewResponseController", @@ -4599,6 +4656,7 @@ var stdlib = map[string][]string{ "Serve", "ServeContent", "ServeFile", + "ServeFileFS", "ServeMux", "ServeTLS", "Server", @@ -5106,6 +5164,7 @@ var stdlib = map[string][]string{ "StructTag", "Swapper", "Type", + "TypeFor", "TypeOf", "Uint", "Uint16", @@ -5342,6 +5401,7 @@ var stdlib = map[string][]string{ "CompactFunc", "Compare", "CompareFunc", + "Concat", "Contains", "ContainsFunc", "Delete", @@ -10824,6 +10884,7 @@ var stdlib = map[string][]string{ "Value", }, "testing/slogtest": { + "Run", "TestHandler", }, "text/scanner": { From 76795ef9aad9f4ecb1fe04e559e4461be54d13fe Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 6 Feb 2024 16:00:03 -0500 Subject: [PATCH 04/23] gopls/doc: audit for types.Alias safety Updates golang/go#65294 Change-Id: Ie4a873d5953e495924fc5b6691dc8e43b6917bb0 Reviewed-on: https://go-review.googlesource.com/c/tools/+/562037 LUCI-TryBot-Result: Go LUCI Reviewed-by: Robert Findley Auto-Submit: Alan Donovan --- gopls/doc/generate.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gopls/doc/generate.go b/gopls/doc/generate.go index c7d12269838..595c19a2bdf 100644 --- a/gopls/doc/generate.go +++ b/gopls/doc/generate.go @@ -242,7 +242,7 @@ func loadOptions(category reflect.Value, optsType types.Object, pkg *packages.Pa name := lowerFirst(typesField.Name()) var enumKeys settings.EnumKeys - if m, ok := typesField.Type().(*types.Map); ok { + if m, ok := typesField.Type().Underlying().(*types.Map); ok { e, ok := enums[m.Key()] if ok { typ = strings.Replace(typ, m.Key().String(), m.Key().Underlying().String(), 1) @@ -313,7 +313,7 @@ func collectEnumKeys(name string, m *types.Map, reflectField reflect.Value, enum } // We can get default values for enum -> bool maps. var isEnumBoolMap bool - if basic, ok := m.Elem().(*types.Basic); ok && basic.Kind() == types.Bool { + if basic, ok := m.Elem().Underlying().(*types.Basic); ok && basic.Kind() == types.Bool { isEnumBoolMap = true } for _, v := range enumValues { From 7f80389ada37c050de245638c0da1e9e4f732645 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 31 Jan 2024 16:56:58 -0500 Subject: [PATCH 05/23] go/analysis/passes/stringintconv: audit for types.Alias safety Updates golang/go#65294 Change-Id: Idfb583f0d3ad3753b770946cb9b9360625670d0d Reviewed-on: https://go-review.googlesource.com/c/tools/+/559917 LUCI-TryBot-Result: Go LUCI Reviewed-by: Tim King --- go/analysis/passes/stringintconv/string.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/go/analysis/passes/stringintconv/string.go b/go/analysis/passes/stringintconv/string.go index b2591ccff55..005e2e54b7d 100644 --- a/go/analysis/passes/stringintconv/string.go +++ b/go/analysis/passes/stringintconv/string.go @@ -15,6 +15,7 @@ import ( "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/analysis/passes/internal/analysisutil" "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/internal/aliases" "golang.org/x/tools/internal/typeparams" ) @@ -194,16 +195,15 @@ func run(pass *analysis.Pass) (interface{}, error) { func structuralTypes(t types.Type) ([]types.Type, error) { var structuralTypes []types.Type - switch t := t.(type) { - case *types.TypeParam: - terms, err := typeparams.StructuralTerms(t) + if tp, ok := aliases.Unalias(t).(*types.TypeParam); ok { + terms, err := typeparams.StructuralTerms(tp) if err != nil { return nil, err } for _, term := range terms { structuralTypes = append(structuralTypes, term.Type()) } - default: + } else { structuralTypes = append(structuralTypes, t) } return structuralTypes, nil From c11269ccb0bf3687bf1f75fa7b2ae44c6806dcae Mon Sep 17 00:00:00 2001 From: Peter Weinberger Date: Sun, 4 Feb 2024 07:52:56 -0500 Subject: [PATCH 06/23] gopls/semantic: elide zero-length tokens vscode complains in the Window channel about zero-length semantic tokens. It may not matter but it seems cleaner to avoid the error and not send zero-length semantic tokens. Fixes: golang/go#65254 Change-Id: I470f8052a6288f5bf4b592794cf6cd54d55b5539 Reviewed-on: https://go-review.googlesource.com/c/tools/+/561155 TryBot-Result: Gopher Robot Run-TryBot: Peter Weinberger Reviewed-by: Robert Findley --- gopls/internal/golang/semtok.go | 3 ++ gopls/internal/template/implementations.go | 3 ++ .../integration/misc/semantictokens_test.go | 44 +++++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/gopls/internal/golang/semtok.go b/gopls/internal/golang/semtok.go index 360b3acd498..181b8ce33ab 100644 --- a/gopls/internal/golang/semtok.go +++ b/gopls/internal/golang/semtok.go @@ -132,6 +132,9 @@ func (tv *tokenVisitor) visit() { } func (tv *tokenVisitor) token(start token.Pos, leng int, typ semtok.TokenType, mods []string) { + if leng <= 0 { + return // vscode doesn't like 0-length Tokens + } if !start.IsValid() { // This is not worth reporting. TODO(pjw): does it still happen? return diff --git a/gopls/internal/template/implementations.go b/gopls/internal/template/implementations.go index 430a337f3ea..19a27620b57 100644 --- a/gopls/internal/template/implementations.go +++ b/gopls/internal/template/implementations.go @@ -169,6 +169,9 @@ func SemanticTokens(ctx context.Context, snapshot *cache.Snapshot, spn protocol. var items []semtok.Token add := func(line, start, len uint32) { + if len == 0 { + return // vscode doesn't like 0-length Tokens + } // TODO(adonovan): don't ignore the rng restriction, if any. items = append(items, semtok.Token{ Line: line, diff --git a/gopls/internal/test/integration/misc/semantictokens_test.go b/gopls/internal/test/integration/misc/semantictokens_test.go index 34a820093a3..96d35bf74f1 100644 --- a/gopls/internal/test/integration/misc/semantictokens_test.go +++ b/gopls/internal/test/integration/misc/semantictokens_test.go @@ -193,3 +193,47 @@ func bar() {} } }) } + +// Make sure no zero-length tokens occur +func TestSemantic_65254(t *testing.T) { + src := ` +-- go.mod -- +module example.com + +go 1.21 +-- main.go -- +package main + +/* a comment with an + +empty line +*/ + +const bad = ` + + src += "`foo" + ` + ` + "bar`" + want := []fake.SemanticToken{ + {Token: "package", TokenType: "keyword"}, + {Token: "main", TokenType: "namespace"}, + {Token: "/* a comment with an", TokenType: "comment"}, + // --- Note that the zero length line does not show up + {Token: "empty line", TokenType: "comment"}, + {Token: "*/", TokenType: "comment"}, + {Token: "const", TokenType: "keyword"}, + {Token: "bad", TokenType: "variable", Mod: "definition readonly"}, + {Token: "`foo", TokenType: "string"}, + // --- Note the zero length line does not show up + {Token: "\tbar`", TokenType: "string"}, + } + WithOptions( + Modes(Default), + Settings{"semanticTokens": true}, + ).Run(t, src, func(t *testing.T, env *Env) { + env.OpenFile("main.go") + seen := env.SemanticTokensFull("main.go") + if x := cmp.Diff(want, seen); x != "" { + t.Errorf("Semantic tokens do not match (-want +got):\n%s", x) + } + }) +} From acf07b3c9a983ef28a755b0ceb6b4186a4fb035f Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Mon, 5 Feb 2024 06:28:45 +0000 Subject: [PATCH 07/23] gopls/internal/utils/immutable: remove unused Map.Keys Change-Id: Ib2f4cea4310dc2ea3b3c70264e90535516b418aa GitHub-Last-Rev: 3f221482772c068f21a78bc973cc7872ec4cf93a GitHub-Pull-Request: golang/tools#473 Reviewed-on: https://go-review.googlesource.com/c/tools/+/561117 Reviewed-by: Robert Findley LUCI-TryBot-Result: Go LUCI Auto-Submit: Robert Findley Reviewed-by: David Chase --- gopls/internal/util/immutable/immutable.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/gopls/internal/util/immutable/immutable.go b/gopls/internal/util/immutable/immutable.go index b0b14c16404..a88133fe92f 100644 --- a/gopls/internal/util/immutable/immutable.go +++ b/gopls/internal/util/immutable/immutable.go @@ -20,15 +20,6 @@ func MapOf[K comparable, V any](m map[K]V) Map[K, V] { return Map[K, V]{m} } -// Keys returns all keys present in the map. -func (m Map[K, V]) Keys() []K { - var keys []K - for k := range m.m { - keys = append(keys, k) - } - return keys -} - // Value returns the mapped value for k. // It is equivalent to the commaok form of an ordinary go map, and returns // (zero, false) if the key is not present. From 76ef6b6ac18a5fd4ca8d303e7c98d19535b72aa8 Mon Sep 17 00:00:00 2001 From: Dan Kortschak Date: Thu, 8 Feb 2024 05:50:16 +1030 Subject: [PATCH 08/23] internal/jsonrpc2: export WireError type This makes it possible for users of the package to us the optional error data field in error construction and error handling logic. Closes golang/go#64882 Change-Id: I749c52c5ffc2d83922aeafc746ee13ea657d6551 Reviewed-on: https://go-review.googlesource.com/c/tools/+/562515 LUCI-TryBot-Result: Go LUCI Reviewed-by: Bryan Mills Reviewed-by: Alan Donovan --- internal/jsonrpc2/messages.go | 8 ++++---- internal/jsonrpc2/wire.go | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/jsonrpc2/messages.go b/internal/jsonrpc2/messages.go index 9ff47f3d1d5..721168fd4f2 100644 --- a/internal/jsonrpc2/messages.go +++ b/internal/jsonrpc2/messages.go @@ -157,17 +157,17 @@ func (r *Response) MarshalJSON() ([]byte, error) { return data, nil } -func toWireError(err error) *wireError { +func toWireError(err error) *WireError { if err == nil { // no error, the response is complete return nil } - if err, ok := err.(*wireError); ok { + if err, ok := err.(*WireError); ok { // already a wire error, just use it return err } - result := &wireError{Message: err.Error()} - var wrapped *wireError + result := &WireError{Message: err.Error()} + var wrapped *WireError if errors.As(err, &wrapped) { // if we wrapped a wire error, keep the code from the wrapped error // but the message from the outer error diff --git a/internal/jsonrpc2/wire.go b/internal/jsonrpc2/wire.go index ac39f1601f0..f2aa2d63e8c 100644 --- a/internal/jsonrpc2/wire.go +++ b/internal/jsonrpc2/wire.go @@ -57,7 +57,7 @@ type wireResponse struct { // Result is the response value, and is required on success. Result *json.RawMessage `json:"result,omitempty"` // Error is a structured error response if the call fails. - Error *wireError `json:"error,omitempty"` + Error *WireError `json:"error,omitempty"` // ID must be set and is the identifier of the Request this is a response to. ID *ID `json:"id,omitempty"` } @@ -70,11 +70,11 @@ type wireCombined struct { Method string `json:"method"` Params *json.RawMessage `json:"params,omitempty"` Result *json.RawMessage `json:"result,omitempty"` - Error *wireError `json:"error,omitempty"` + Error *WireError `json:"error,omitempty"` } -// wireError represents a structured error in a Response. -type wireError struct { +// WireError represents a structured error in a Response. +type WireError struct { // Code is an error code indicating the type of failure. Code int64 `json:"code"` // Message is a short description of the error. @@ -96,13 +96,13 @@ type ID struct { } func NewError(code int64, message string) error { - return &wireError{ + return &WireError{ Code: code, Message: message, } } -func (err *wireError) Error() string { +func (err *WireError) Error() string { return err.Message } From e3f11805648f901c0be286027fbc9d567d22dcf9 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 7 Feb 2024 12:51:00 -0500 Subject: [PATCH 09/23] gopls/internal/cache: fix crash in analysis The "missing Diagnostic.Code" assertion checks an invariant that isn't guaranteed. This change downgrades it to a bug.Reportf. Also, we add a similar assertion to analysistest.RunWithSuggestedFixes which has much better coverage of all the analyzer's different ways of constructing a SuggestedFix, unlike gopl's own superficial tests of each analyzer it carries. (This assertion may be enabled only for analyzers in x/tools, since the invariant is stricter than is required by the public API.) Also, fix a couple of places in unusedvariable where it could conceivably return a fix with no TextEdits; they are not intended to be lazy fixes. Fixes golang/go#65578 Change-Id: I5ed6301b028d184ea896988ca8f210fb8f3dd64f Reviewed-on: https://go-review.googlesource.com/c/tools/+/562397 LUCI-TryBot-Result: Go LUCI Auto-Submit: Alan Donovan Commit-Queue: Alan Donovan Reviewed-by: Robert Findley --- go/analysis/analysistest/analysistest.go | 26 ++++++++++++++++--- .../analysis/unusedvariable/unusedvariable.go | 12 +++++++-- gopls/internal/cache/errors.go | 8 +++--- 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go index 72b703f7bb4..95db20f4be3 100644 --- a/go/analysis/analysistest/analysistest.go +++ b/go/analysis/analysistest/analysistest.go @@ -15,6 +15,7 @@ import ( "os" "path/filepath" "regexp" + "runtime" "sort" "strconv" "strings" @@ -128,6 +129,19 @@ type Testing interface { func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns ...string) []*Result { r := Run(t, dir, a, patterns...) + // If the immediate caller of RunWithSuggestedFixes is in + // x/tools, we apply stricter checks as required by gopls. + inTools := false + { + var pcs [1]uintptr + n := runtime.Callers(1, pcs[:]) + frames := runtime.CallersFrames(pcs[:n]) + fr, _ := frames.Next() + if fr.Func != nil && strings.HasPrefix(fr.Func.Name(), "golang.org/x/tools/") { + inTools = true + } + } + // Process each result (package) separately, matching up the suggested // fixes into a diff, which we will compare to the .golden file. We have // to do this per-result in case a file appears in two packages, such as in @@ -145,8 +159,14 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns // Validate edits, prepare the fileEdits map and read the file contents. for _, diag := range act.Diagnostics { - for _, sf := range diag.SuggestedFixes { - for _, edit := range sf.TextEdits { + for _, fix := range diag.SuggestedFixes { + + // Assert that lazy fixes have a Category (#65578, #65087). + if inTools && len(fix.TextEdits) == 0 && diag.Category == "" { + t.Errorf("missing Diagnostic.Category for SuggestedFix without TextEdits (gopls requires the category for the name of the fix command") + } + + for _, edit := range fix.TextEdits { start, end := edit.Pos, edit.End if !end.IsValid() { end = start @@ -175,7 +195,7 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns if _, ok := fileEdits[file]; !ok { fileEdits[file] = make(map[string][]diff.Edit) } - fileEdits[file][sf.Message] = append(fileEdits[file][sf.Message], diff.Edit{ + fileEdits[file][fix.Message] = append(fileEdits[file][fix.Message], diff.Edit{ Start: file.Offset(start), End: file.Offset(end), New: string(edit.NewText), diff --git a/gopls/internal/analysis/unusedvariable/unusedvariable.go b/gopls/internal/analysis/unusedvariable/unusedvariable.go index 67866d0e111..f8a4db1d292 100644 --- a/gopls/internal/analysis/unusedvariable/unusedvariable.go +++ b/gopls/internal/analysis/unusedvariable/unusedvariable.go @@ -155,10 +155,14 @@ func removeVariableFromSpec(pass *analysis.Pass, path []ast.Node, stmt *ast.Valu // Find parent DeclStmt and delete it for _, node := range path { if declStmt, ok := node.(*ast.DeclStmt); ok { + edits := deleteStmtFromBlock(path, declStmt) + if len(edits) == 0 { + return nil // can this happen? + } return []analysis.SuggestedFix{ { Message: suggestedFixMessage(ident.Name), - TextEdits: deleteStmtFromBlock(path, declStmt), + TextEdits: edits, }, } } @@ -208,10 +212,14 @@ func removeVariableFromAssignment(path []ast.Node, stmt *ast.AssignStmt, ident * } // RHS does not have any side effects, delete the whole statement + edits := deleteStmtFromBlock(path, stmt) + if len(edits) == 0 { + return nil // can this happen? + } return []analysis.SuggestedFix{ { Message: suggestedFixMessage(ident.Name), - TextEdits: deleteStmtFromBlock(path, stmt), + TextEdits: edits, }, } } diff --git a/gopls/internal/cache/errors.go b/gopls/internal/cache/errors.go index 1ed193b5d61..83382b0aad2 100644 --- a/gopls/internal/cache/errors.go +++ b/gopls/internal/cache/errors.go @@ -21,10 +21,10 @@ import ( "strings" "golang.org/x/tools/go/packages" - "golang.org/x/tools/gopls/internal/file" "golang.org/x/tools/gopls/internal/cache/metadata" - "golang.org/x/tools/gopls/internal/protocol/command" + "golang.org/x/tools/gopls/internal/file" "golang.org/x/tools/gopls/internal/protocol" + "golang.org/x/tools/gopls/internal/protocol/command" "golang.org/x/tools/gopls/internal/settings" "golang.org/x/tools/gopls/internal/util/bug" "golang.org/x/tools/internal/typesinternal" @@ -345,8 +345,10 @@ func toSourceDiagnostic(srcAnalyzer *settings.Analyzer, gobDiag *gobDiagnostic) } // Ensure that the analyzer specifies a category for all its no-edit fixes. + // This is asserted by analysistest.RunWithSuggestedFixes, but there + // may be gaps in test coverage. if diag.Code == "" || diag.Code == "default" { - panic(fmt.Sprintf("missing Diagnostic.Code: %#v", *diag)) + bug.Reportf("missing Diagnostic.Code: %#v", *diag) } } } From 5fcc6273f47e89fc27e8e4adfe4dfc7c336ba637 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 31 Jan 2024 18:13:45 -0500 Subject: [PATCH 10/23] go/analysis/passes/nilness: handle type assertions too This change causes nilness to detect patterns of the form: ptr, ok := x.(T) if !ok { println(*ptr) // nil dereference } It found three bugs in k8s, e.g. // pkg/controller/volume/persistentvolume/pv_controller.go claim, ok = obj.(*v1.PersistentVolumeClaim) if !ok { return fmt.Errorf("cannot convert object from volume cache to volume %q!?: %#v", claim.Spec.VolumeName, obj) } Fixes golang/go#47938 Change-Id: Idb4588ce037cb898ccc3feb460ea0b67f65803df Reviewed-on: https://go-review.googlesource.com/c/tools/+/560075 LUCI-TryBot-Result: Go LUCI Reviewed-by: Tim King --- go/analysis/passes/nilness/nilness.go | 56 +++++++++++++++++++ .../passes/nilness/testdata/src/a/a.go | 26 +++++++++ 2 files changed, 82 insertions(+) diff --git a/go/analysis/passes/nilness/nilness.go b/go/analysis/passes/nilness/nilness.go index c4999f7a9db..5e14c096ab1 100644 --- a/go/analysis/passes/nilness/nilness.go +++ b/go/analysis/passes/nilness/nilness.go @@ -189,6 +189,42 @@ func runFunc(pass *analysis.Pass, fn *ssa.Function) { } } + // In code of the form: + // + // if ptr, ok := x.(*T); ok { ... } else { fsucc } + // + // the fsucc block learns that ptr == nil, + // since that's its zero value. + if If, ok := b.Instrs[len(b.Instrs)-1].(*ssa.If); ok { + // Handle "if ok" and "if !ok" variants. + cond, fsucc := If.Cond, b.Succs[1] + if unop, ok := cond.(*ssa.UnOp); ok && unop.Op == token.NOT { + cond, fsucc = unop.X, b.Succs[0] + } + + // Match pattern: + // t0 = typeassert (pointerlike) + // t1 = extract t0 #0 // ptr + // t2 = extract t0 #1 // ok + // if t2 goto tsucc, fsucc + if extract1, ok := cond.(*ssa.Extract); ok && extract1.Index == 1 { + if assert, ok := extract1.Tuple.(*ssa.TypeAssert); ok && + isNillable(assert.AssertedType) { + for _, pinstr := range *assert.Referrers() { + if extract0, ok := pinstr.(*ssa.Extract); ok && + extract0.Index == 0 && + extract0.Tuple == extract1.Tuple { + for _, d := range b.Dominees() { + if len(d.Preds) == 1 && d == fsucc { + visit(d, append(stack, fact{extract0, isnil})) + } + } + } + } + } + } + } + for _, d := range b.Dominees() { visit(d, stack) } @@ -360,3 +396,23 @@ func (ff facts) negate() facts { } return nn } + +func is[T any](x any) bool { + _, ok := x.(T) + return ok +} + +func isNillable(t types.Type) bool { + switch t := typeparams.CoreType(t).(type) { + case *types.Pointer, + *types.Map, + *types.Signature, + *types.Chan, + *types.Interface, + *types.Slice: + return true + case *types.Basic: + return t == types.Typ[types.UnsafePointer] + } + return false +} diff --git a/go/analysis/passes/nilness/testdata/src/a/a.go b/go/analysis/passes/nilness/testdata/src/a/a.go index 0629e08d89e..89bccbe65bd 100644 --- a/go/analysis/passes/nilness/testdata/src/a/a.go +++ b/go/analysis/passes/nilness/testdata/src/a/a.go @@ -216,3 +216,29 @@ func f14() { print(x) } } + +func f15(x any) { + ptr, ok := x.(*int) + if ok { + return + } + println(*ptr) // want "nil dereference in load" +} + +func f16(x any) { + ptr, ok := x.(*int) + if !ok { + println(*ptr) // want "nil dereference in load" + return + } + println(*ptr) +} + +func f18(x any) { + ptr, ok := x.(*int) + if ok { + println(ptr) + // falls through + } + println(*ptr) +} From f4fa7a75e44868caa1b37947e8b811b1bdb144e6 Mon Sep 17 00:00:00 2001 From: Gopher Robot Date: Wed, 7 Feb 2024 22:17:05 +0000 Subject: [PATCH 11/23] go.mod: update golang.org/x dependencies Update golang.org/x dependencies to their latest tagged versions. Change-Id: I3b8350542d43d4528d8c2cd5cdc111bb7548a100 Reviewed-on: https://go-review.googlesource.com/c/tools/+/562497 LUCI-TryBot-Result: Go LUCI Reviewed-by: Dmitri Shuralyov Auto-Submit: Gopher Robot Reviewed-by: Than McIntosh --- go.mod | 6 +++--- go.sum | 12 ++++++------ gopls/go.mod | 4 ++-- gopls/go.sum | 12 +++++++----- 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/go.mod b/go.mod index fb7bcfcd998..dca3c87f8c9 100644 --- a/go.mod +++ b/go.mod @@ -4,13 +4,13 @@ go 1.18 require ( github.com/yuin/goldmark v1.4.13 - golang.org/x/mod v0.14.0 - golang.org/x/net v0.20.0 + golang.org/x/mod v0.15.0 + golang.org/x/net v0.21.0 ) require golang.org/x/sync v0.6.0 require ( - golang.org/x/sys v0.16.0 // indirect + golang.org/x/sys v0.17.0 // indirect golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509 ) diff --git a/go.sum b/go.sum index f30cef77493..07210ace954 100644 --- a/go.sum +++ b/go.sum @@ -1,12 +1,12 @@ github.com/yuin/goldmark v1.4.13 h1:fVcFKWvrslecOb/tg+Cc05dkeYx540o0FuFt3nUVDoE= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= -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.20.0 h1:aCL9BSgETF1k+blQaYUBx9hJ9LOGP3gAVemcZlf1Kpo= -golang.org/x/net v0.20.0/go.mod h1:z8BVo6PvndSri0LbOE3hAn0apkU+1YvI6E70E9jsnvY= +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.21.0 h1:AQyQV4dYCvJ7vGmJyKki9+PBdyvhkSd8EIx/qb0AYv4= +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.16.0 h1:xWw16ngr6ZMtmxDyKyIgsE93KNKz5HKmMa3b8ALHidU= -golang.org/x/sys v0.16.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +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-20240201224847-0a1d30dda509 h1:Nr7eTQpQZ/ytesxDJpQgaf0t4sdLnnDtAbmtViTrSUo= golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509/go.mod h1:ZthVHHkOi8rlMEsfFr3Ie42Ym1NonbFNNRKW3ci0UrU= diff --git a/gopls/go.mod b/gopls/go.mod index bafca530086..9baf8f7f43b 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -6,7 +6,7 @@ require ( github.com/google/go-cmp v0.6.0 github.com/jba/printsrc v0.2.2 github.com/jba/templatecheck v0.6.0 - golang.org/x/mod v0.14.0 + golang.org/x/mod v0.15.0 golang.org/x/sync v0.6.0 golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509 golang.org/x/text v0.14.0 @@ -22,7 +22,7 @@ require ( github.com/BurntSushi/toml v1.2.1 // indirect github.com/google/safehtml v0.1.0 // indirect golang.org/x/exp/typeparams v0.0.0-20221212164502-fae10dda9338 // indirect - golang.org/x/sys v0.16.0 // indirect + golang.org/x/sys v0.17.0 // indirect gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect ) diff --git a/gopls/go.sum b/gopls/go.sum index df5d3a49162..bd932343d48 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -14,24 +14,26 @@ 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.18.0/go.mod h1:R0j02AL6hcrfOiy9T4ZYp/rcWeMxM3L6QYxlOuEG1mg= +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.14.0 h1:dGoOF9QVLYng8IHTm7BAyWqCqSheQ5pYWGhzW00YJr0= golang.org/x/mod v0.14.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= +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.20.0/go.mod h1:z8BVo6PvndSri0LbOE3hAn0apkU+1YvI6E70E9jsnvY= +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.16.0 h1:xWw16ngr6ZMtmxDyKyIgsE93KNKz5HKmMa3b8ALHidU= golang.org/x/sys v0.16.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +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-20240201224847-0a1d30dda509 h1:Nr7eTQpQZ/ytesxDJpQgaf0t4sdLnnDtAbmtViTrSUo= golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509/go.mod h1:ZthVHHkOi8rlMEsfFr3Ie42Ym1NonbFNNRKW3ci0UrU= golang.org/x/term v0.8.0/go.mod h1:xPskH00ivmX89bAKVGSKKtLOWNx2+17Eiy94tnKShWo= -golang.org/x/term v0.16.0/go.mod h1:yn7UURbUtPyrVJPGPq404EukNFxcm/foM+bV/bfcDsY= +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= From 314368ddf001d9629fdf2df9fce2b209f8890925 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 31 Jan 2024 16:54:05 -0500 Subject: [PATCH 12/23] go/analysis/passes/deepequalerrors: audit for types.Alias safety Updates golang/go#65294 Change-Id: I00543b00c830ff5a4fe442f1bcf6f21ab0b12d97 Reviewed-on: https://go-review.googlesource.com/c/tools/+/559916 Reviewed-by: Tim King Auto-Submit: Alan Donovan LUCI-TryBot-Result: Go LUCI --- go/analysis/passes/deepequalerrors/deepequalerrors.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/go/analysis/passes/deepequalerrors/deepequalerrors.go b/go/analysis/passes/deepequalerrors/deepequalerrors.go index 1a83bddbcec..5e17bd1ab90 100644 --- a/go/analysis/passes/deepequalerrors/deepequalerrors.go +++ b/go/analysis/passes/deepequalerrors/deepequalerrors.go @@ -15,6 +15,7 @@ import ( "golang.org/x/tools/go/analysis/passes/internal/analysisutil" "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/go/types/typeutil" + "golang.org/x/tools/internal/aliases" ) const Doc = `check for calls of reflect.DeepEqual on error values @@ -101,7 +102,8 @@ func containsError(typ types.Type) bool { return true } } - case *types.Named: + case *types.Named, + *aliases.Alias: return check(t.Underlying()) // We list the remaining valid type kinds for completeness. From a7407facde3f3a9b2736a7306823549d2c24efae Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 8 Feb 2024 18:15:23 -0500 Subject: [PATCH 13/23] gopls: update telemetry This brings in the new notation for stacks. Change-Id: Iff5e0053798fb3984a25f3109fd6ca3a468575e2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/562855 LUCI-TryBot-Result: Go LUCI Reviewed-by: Robert Findley --- go.mod | 2 +- go.sum | 2 ++ gopls/go.mod | 2 +- gopls/go.sum | 2 ++ 4 files changed, 6 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index dca3c87f8c9..54b8bd473c5 100644 --- a/go.mod +++ b/go.mod @@ -12,5 +12,5 @@ require golang.org/x/sync v0.6.0 require ( golang.org/x/sys v0.17.0 // indirect - golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509 + golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808 ) diff --git a/go.sum b/go.sum index 07210ace954..373ed3cdd52 100644 --- a/go.sum +++ b/go.sum @@ -10,3 +10,5 @@ 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-20240201224847-0a1d30dda509 h1:Nr7eTQpQZ/ytesxDJpQgaf0t4sdLnnDtAbmtViTrSUo= golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509/go.mod h1:ZthVHHkOi8rlMEsfFr3Ie42Ym1NonbFNNRKW3ci0UrU= +golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808 h1:+Kc94D8UVEVxJnLXp/+FMfqQARZtWHfVrcRtcG8aT3g= +golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808/go.mod h1:KG1lNk5ZFNssSZLrpVb4sMXKMpGwGXOxSG3rnu2gZQQ= diff --git a/gopls/go.mod b/gopls/go.mod index 9baf8f7f43b..35300455879 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -8,7 +8,7 @@ require ( github.com/jba/templatecheck v0.6.0 golang.org/x/mod v0.15.0 golang.org/x/sync v0.6.0 - golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509 + golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808 golang.org/x/text v0.14.0 golang.org/x/tools v0.17.0 golang.org/x/vuln v1.0.1 diff --git a/gopls/go.sum b/gopls/go.sum index bd932343d48..e741723465f 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -32,6 +32,8 @@ 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-20240201224847-0a1d30dda509 h1:Nr7eTQpQZ/ytesxDJpQgaf0t4sdLnnDtAbmtViTrSUo= golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509/go.mod h1:ZthVHHkOi8rlMEsfFr3Ie42Ym1NonbFNNRKW3ci0UrU= +golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808 h1:+Kc94D8UVEVxJnLXp/+FMfqQARZtWHfVrcRtcG8aT3g= +golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808/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= From a5af84e3f3e3a6f2652789eeba70253d6ea8cb4c Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 9 Feb 2024 14:56:01 +0000 Subject: [PATCH 14/23] gopls/internal/cache: check views on any on-disk change to go.mod files To prepare for the follow up change, where we honor local replaces in the view selection algorithm, treat go.mod files more like go.work files in a couple places: - Re-run the view selection algorithm whenever a go.mod file changes on disk. - Use the workspaceModFiles field in the bestView algorithm, rather than checking the gomod field directly. Apart from perhaps running the view selection algorithm more frequently, this change should have no observable impact. For golang/go#64888 Change-Id: I9d9a74b876791a77e284a1a1d5fcc7a691199901 Reviewed-on: https://go-review.googlesource.com/c/tools/+/562679 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI --- gopls/internal/cache/session.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/gopls/internal/cache/session.go b/gopls/internal/cache/session.go index 102a2261d25..a6802336dc9 100644 --- a/gopls/internal/cache/session.go +++ b/gopls/internal/cache/session.go @@ -590,7 +590,7 @@ func bestView[V viewDefiner](ctx context.Context, fs file.Source, fh file.Handle pushView(&workViews, view) } case GoModView: - if modURI == def.gomod { + if _, ok := def.workspaceModFiles[modURI]; ok { modViews = append(modViews, view) } case GOPATHView: @@ -746,18 +746,12 @@ func (s *Session) DidModifyFiles(ctx context.Context, modifications []file.Modif checkViews = true } - // Any on-disk change to a go.work file causes recomputing views. + // Any on-disk change to a go.work or go.mod file causes recomputing views. // // TODO(rfindley): go.work files need not be named "go.work" -- we need to // check each view's source to handle the case of an explicit GOWORK value. // Write a test that fails, and fix this. - if isGoWork(c.URI) && (c.Action == file.Save || c.OnDisk) { - checkViews = true - } - // Opening/Close/Create/Delete of go.mod files all trigger - // re-evaluation of Views. Changes do not as they can't affect the set of - // Views. - if isGoMod(c.URI) && c.Action != file.Change && c.Action != file.Save { + if (isGoWork(c.URI) || isGoMod(c.URI)) && (c.Action == file.Save || c.OnDisk) { checkViews = true } From 9619683231abea86b02c3e2af49a1f5ee9fc5d1a Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 9 Feb 2024 15:33:01 +0000 Subject: [PATCH 15/23] gopls/internal/cache: treat local replaces as workspace modules Update the view selection algorithm to consider replace directives, treating locally replaced modules in a go.mod file as workspace modules. This causes gopls to register watch patterns for these local replaces. Since this is a fundamental change in behavior, add a hidden off switch to revert to the old behavior: an "includeReplaceInWorkspace" setting. Fixes golang/go#64762 Fixes golang/go#64888 Change-Id: I0fea97a05299acd877a220982d51ba9b4d4070e3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/562680 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI --- gopls/internal/cache/session_test.go | 85 +++++++++++++++++++ gopls/internal/cache/view.go | 26 +++++- gopls/internal/cache/workspace.go | 56 ++++++++++-- gopls/internal/settings/default.go | 1 + gopls/internal/settings/settings.go | 10 +++ .../integration/workspace/zero_config_test.go | 49 +++++++++++ 6 files changed, 214 insertions(+), 13 deletions(-) diff --git a/gopls/internal/cache/session_test.go b/gopls/internal/cache/session_test.go index cc6f664060a..a3bd8ce5800 100644 --- a/gopls/internal/cache/session_test.go +++ b/gopls/internal/cache/session_test.go @@ -227,6 +227,91 @@ func TestZeroConfigAlgorithm(t *testing.T) { []string{"a/a.go", "b/b.go", "b/c/c.go"}, []viewSummary{{GoWorkView, ".", nil}, {GoModView, "b/c", []string{"GOWORK=off"}}}, }, + { + "go.mod with nested replace", + map[string]string{ + "go.mod": "module golang.org/a\n require golang.org/b v1.2.3\nreplace example.com/b => ./b", + "a.go": "package a", + "b/go.mod": "module golang.org/b\ngo 1.18\n", + "b/b.go": "package b", + }, + []folderSummary{{dir: "."}}, + []string{"a/a.go", "b/b.go"}, + []viewSummary{{GoModView, ".", nil}}, + }, + { + "go.mod with parent replace, parent folder", + map[string]string{ + "go.mod": "module golang.org/a", + "a.go": "package a", + "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: "."}}, + []string{"a/a.go", "b/b.go"}, + []viewSummary{{GoModView, ".", nil}, {GoModView, "b", nil}}, + }, + { + "go.mod with multiple replace", + map[string]string{ + "go.mod": ` +module golang.org/root + +require ( + golang.org/a v1.2.3 + golang.org/b v1.2.3 + golang.org/c v1.2.3 +) + +replace ( + golang.org/b => ./b + golang.org/c => ./c + // Note: d is not replaced +) +`, + "a.go": "package a", + "b/go.mod": "module golang.org/b\ngo 1.18", + "b/b.go": "package b", + "c/go.mod": "module golang.org/c\ngo 1.18", + "c/c.go": "package c", + "d/go.mod": "module golang.org/d\ngo 1.18", + "d/d.go": "package d", + }, + []folderSummary{{dir: "."}}, + []string{"b/b.go", "c/c.go", "d/d.go"}, + []viewSummary{{GoModView, ".", nil}, {GoModView, "d", nil}}, + }, + { + "go.mod with many replace", + map[string]string{ + "go.mod": "module golang.org/a\ngo 1.18", + "a.go": "package a", + "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: "b"}}, + []string{"a/a.go", "b/b.go"}, + []viewSummary{{GoModView, "b", nil}}, + }, + { + "go.mod with replace directive; workspace replace off", + map[string]string{ + "go.mod": "module golang.org/a\n require golang.org/b v1.2.3\nreplace example.com/b => ./b", + "a.go": "package a", + "b/go.mod": "module golang.org/b\ngo 1.18\n", + "b/b.go": "package b", + }, + []folderSummary{{ + dir: ".", + options: func(string) map[string]any { + return map[string]any{ + "includeReplaceInWorkspace": false, + } + }, + }}, + []string{"a/a.go", "b/b.go"}, + []viewSummary{{GoModView, ".", nil}, {GoModView, "b", nil}}, + }, } for _, test := range tests { diff --git a/gopls/internal/cache/view.go b/gopls/internal/cache/view.go index 78e85a18b0c..ed52646f31d 100644 --- a/gopls/internal/cache/view.go +++ b/gopls/internal/cache/view.go @@ -154,8 +154,10 @@ type viewDefinition struct { // workspaceModFiles holds the set of mod files active in this snapshot. // - // This is either empty, a single entry for the workspace go.mod file, or the - // set of mod files used by the workspace go.work file. + // For a go.work workspace, this is the set of workspace modfiles. For a + // go.mod workspace, this contains the go.mod file defining the workspace + // root, as well as any locally replaced modules (if + // "includeReplaceInWorkspace" is set). // // TODO(rfindley): should we just run `go list -m` to compute this set? workspaceModFiles map[protocol.DocumentURI]struct{} @@ -939,6 +941,22 @@ func defineView(ctx context.Context, fs file.Source, folder *Folder, forFile fil // But gopls is less strict, allowing GOPATH mode if GO111MODULE="", and // AdHoc views if no module is found. + // gomodWorkspace is a helper to compute the correct set of workspace + // modfiles for a go.mod file, based on folder options. + gomodWorkspace := func() map[protocol.DocumentURI]unit { + modFiles := map[protocol.DocumentURI]struct{}{def.gomod: {}} + if folder.Options.IncludeReplaceInWorkspace { + includingReplace, err := goModModules(ctx, def.gomod, fs) + if err == nil { + modFiles = includingReplace + } else { + // If the go.mod file fails to parse, we don't know anything about + // replace directives, so fall back to a view of just the root module. + } + } + return modFiles + } + // Prefer a go.work file if it is available and contains the module relevant // to forURI. if def.adjustedGO111MODULE() != "off" && folder.Env.GOWORK != "off" && def.gowork != "" { @@ -959,7 +977,7 @@ func defineView(ctx context.Context, fs file.Source, folder *Folder, forFile fil if _, ok := def.workspaceModFiles[def.gomod]; !ok { def.typ = GoModView def.root = def.gomod.Dir() - def.workspaceModFiles = map[protocol.DocumentURI]unit{def.gomod: {}} + def.workspaceModFiles = gomodWorkspace() if def.envOverlay == nil { def.envOverlay = make(map[string]string) } @@ -978,7 +996,7 @@ func defineView(ctx context.Context, fs file.Source, folder *Folder, forFile fil if def.adjustedGO111MODULE() != "off" && def.gomod != "" { def.typ = GoModView def.root = def.gomod.Dir() - def.workspaceModFiles = map[protocol.DocumentURI]struct{}{def.gomod: {}} + def.workspaceModFiles = gomodWorkspace() return def, nil } diff --git a/gopls/internal/cache/workspace.go b/gopls/internal/cache/workspace.go index 4f01ebf0364..07134b3da00 100644 --- a/gopls/internal/cache/workspace.go +++ b/gopls/internal/cache/workspace.go @@ -15,8 +15,10 @@ import ( "golang.org/x/tools/gopls/internal/protocol" ) -// TODO(rfindley): now that experimentalWorkspaceModule is gone, this file can -// be massively cleaned up and/or removed. +// isGoWork reports if uri is a go.work file. +func isGoWork(uri protocol.DocumentURI) bool { + return filepath.Base(uri.Path()) == "go.work" +} // goWorkModules returns the URIs of go.mod files named by the go.work file. func goWorkModules(ctx context.Context, gowork protocol.DocumentURI, fs file.Source) (map[protocol.DocumentURI]unit, error) { @@ -34,16 +36,28 @@ func goWorkModules(ctx context.Context, gowork protocol.DocumentURI, fs file.Sou if err != nil { return nil, fmt.Errorf("parsing go.work: %w", err) } - modFiles := make(map[protocol.DocumentURI]unit) + var usedDirs []string for _, use := range workFile.Use { - modDir := filepath.FromSlash(use.Path) + usedDirs = append(usedDirs, use.Path) + } + return localModFiles(dir, usedDirs), nil +} + +// localModFiles builds a set of local go.mod files referenced by +// goWorkOrModPaths, which is a slice of paths as contained in a go.work 'use' +// directive or go.mod 'replace' directive (and which therefore may use either +// '/' or '\' as a path separator). +func localModFiles(relativeTo string, goWorkOrModPaths []string) map[protocol.DocumentURI]unit { + modFiles := make(map[protocol.DocumentURI]unit) + for _, path := range goWorkOrModPaths { + modDir := filepath.FromSlash(path) if !filepath.IsAbs(modDir) { - modDir = filepath.Join(dir, modDir) + modDir = filepath.Join(relativeTo, modDir) } modURI := protocol.URIFromPath(filepath.Join(modDir, "go.mod")) modFiles[modURI] = unit{} } - return modFiles, nil + return modFiles } // isGoMod reports if uri is a go.mod file. @@ -51,9 +65,33 @@ func isGoMod(uri protocol.DocumentURI) bool { return filepath.Base(uri.Path()) == "go.mod" } -// isGoWork reports if uri is a go.work file. -func isGoWork(uri protocol.DocumentURI) bool { - return filepath.Base(uri.Path()) == "go.work" +// goModModules returns the URIs of "workspace" go.mod files defined by a +// go.mod file. This set is defined to be the given go.mod file itself, as well +// as the modfiles of any locally replaced modules in the go.mod file. +func goModModules(ctx context.Context, gomod protocol.DocumentURI, fs file.Source) (map[protocol.DocumentURI]unit, error) { + fh, err := fs.ReadFile(ctx, gomod) + if err != nil { + return nil, err // canceled + } + content, err := fh.Content() + if err != nil { + return nil, err + } + filename := gomod.Path() + dir := filepath.Dir(filename) + modFile, err := modfile.Parse(filename, content, nil) + if err != nil { + return nil, err + } + var localReplaces []string + for _, replace := range modFile.Replace { + if modfile.IsDirectoryPath(replace.New.Path) { + localReplaces = append(localReplaces, replace.New.Path) + } + } + modFiles := localModFiles(dir, localReplaces) + modFiles[gomod] = unit{} + return modFiles, nil } // fileExists reports whether the file has a Content (which may be empty). diff --git a/gopls/internal/settings/default.go b/gopls/internal/settings/default.go index 74b98c82555..bb248273d4d 100644 --- a/gopls/internal/settings/default.go +++ b/gopls/internal/settings/default.go @@ -115,6 +115,7 @@ func DefaultOptions(overrides ...func(*Options)) *Options { ReportAnalysisProgressAfter: 5 * time.Second, TelemetryPrompt: false, LinkifyShowMessage: false, + IncludeReplaceInWorkspace: true, }, Hooks: Hooks{ URLRegexp: urlRegexp(), diff --git a/gopls/internal/settings/settings.go b/gopls/internal/settings/settings.go index 0f1f1779982..8010dcd6e5a 100644 --- a/gopls/internal/settings/settings.go +++ b/gopls/internal/settings/settings.go @@ -553,6 +553,12 @@ type InternalOptions struct { // LinkifyShowMessage controls whether the client wants gopls // to linkify links in showMessage. e.g. [go.dev](https://go.dev). LinkifyShowMessage bool + + // IncludeReplaceInWorkspace controls whether locally replaced modules in a + // go.mod file are treated like workspace modules. + // Or in other words, if a go.mod file with local replaces behaves like a + // go.work file. + IncludeReplaceInWorkspace bool } type SubdirWatchPatterns string @@ -1146,9 +1152,13 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{}) case "telemetryPrompt": result.setBool(&o.TelemetryPrompt) + case "linkifyShowMessage": result.setBool(&o.LinkifyShowMessage) + case "includeReplaceInWorkspace": + result.setBool(&o.IncludeReplaceInWorkspace) + // Replaced settings. case "experimentalDisabledAnalyses": result.deprecated("analyses") diff --git a/gopls/internal/test/integration/workspace/zero_config_test.go b/gopls/internal/test/integration/workspace/zero_config_test.go index 51fd9a7dbd2..87c7c087844 100644 --- a/gopls/internal/test/integration/workspace/zero_config_test.go +++ b/gopls/internal/test/integration/workspace/zero_config_test.go @@ -185,3 +185,52 @@ const C = 0 ) }) } + +func TestGoModReplace(t *testing.T) { + // This test checks that we treat locally replaced modules as workspace + // modules, according to the "includeReplaceInWorkspace" setting. + const files = ` +-- moda/go.mod -- +module golang.org/a + +require golang.org/b v1.2.3 + +replace golang.org/b => ../modb + +go 1.20 + +-- moda/a.go -- +package a + +import "golang.org/b" + +const A = b.B + +-- modb/go.mod -- +module golang.org/b + +go 1.20 + +-- modb/b.go -- +package b + +const B = 1 +` + + for useReplace, expectation := range map[bool]Expectation{ + true: FileWatchMatching("modb"), + false: NoFileWatchMatching("modb"), + } { + WithOptions( + WorkspaceFolders("moda"), + Settings{ + "includeReplaceInWorkspace": useReplace, + }, + ).Run(t, files, func(t *testing.T, env *Env) { + env.OnceMet( + InitialWorkspaceLoad, + expectation, + ) + }) + } +} From 95f04f4ae85908c11fcb0f0cdb7d0e14ca318fe3 Mon Sep 17 00:00:00 2001 From: Suzy Mueller Date: Wed, 7 Feb 2024 14:51:19 -0800 Subject: [PATCH 16/23] gopls/internal/golang: add resolve support for inline refactorings Resolve edits for inline refactorings when the client supports it. For golang/go#64510 Change-Id: I67101ab19576054b1d03c46e3d318a4660088a63 Reviewed-on: https://go-review.googlesource.com/c/tools/+/562118 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan --- gopls/internal/golang/codeaction.go | 17 ++++----- .../marker/testdata/codeaction/inline.txt | 3 +- .../testdata/codeaction/inline_resolve.txt | 35 +++++++++++++++++++ 3 files changed, 44 insertions(+), 11 deletions(-) create mode 100644 gopls/internal/test/marker/testdata/codeaction/inline_resolve.txt diff --git a/gopls/internal/golang/codeaction.go b/gopls/internal/golang/codeaction.go index a9aae821be8..5c6163cef12 100644 --- a/gopls/internal/golang/codeaction.go +++ b/gopls/internal/golang/codeaction.go @@ -106,7 +106,7 @@ func CodeActions(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, } if want[protocol.RefactorInline] { - rewrites, err := getInlineCodeActions(pkg, pgf, rng) + rewrites, err := getInlineCodeActions(pkg, pgf, rng, snapshot.Options()) if err != nil { return nil, err } @@ -381,7 +381,7 @@ func canRemoveParameter(pkg *cache.Package, pgf *ParsedGoFile, rng protocol.Rang } // getInlineCodeActions returns refactor.inline actions available at the specified range. -func getInlineCodeActions(pkg *cache.Package, pgf *ParsedGoFile, rng protocol.Range) ([]protocol.CodeAction, error) { +func getInlineCodeActions(pkg *cache.Package, pgf *ParsedGoFile, rng protocol.Range, options *settings.Options) ([]protocol.CodeAction, error) { start, end, err := pgf.RangePos(rng) if err != nil { return nil, err @@ -391,9 +391,10 @@ func getInlineCodeActions(pkg *cache.Package, pgf *ParsedGoFile, rng protocol.Ra var commands []protocol.Command if _, fn, err := EnclosingStaticCall(pkg, pgf, start, end); err == nil { cmd, err := command.NewApplyFixCommand(fmt.Sprintf("Inline call to %s", fn.Name()), command.ApplyFixArgs{ - Fix: fixInlineCall, - URI: pgf.URI, - Range: rng, + Fix: fixInlineCall, + URI: pgf.URI, + Range: rng, + ResolveEdits: supportsResolveEdits(options), }) if err != nil { return nil, err @@ -404,11 +405,7 @@ func getInlineCodeActions(pkg *cache.Package, pgf *ParsedGoFile, rng protocol.Ra // Convert commands to actions. var actions []protocol.CodeAction for i := range commands { - actions = append(actions, protocol.CodeAction{ - Title: commands[i].Title, - Kind: protocol.RefactorInline, - Command: &commands[i], - }) + actions = append(actions, newCodeAction(commands[i].Title, protocol.RefactorInline, &commands[i], nil, options)) } return actions, nil } diff --git a/gopls/internal/test/marker/testdata/codeaction/inline.txt b/gopls/internal/test/marker/testdata/codeaction/inline.txt index 339a05b0c41..0c5bcb41658 100644 --- a/gopls/internal/test/marker/testdata/codeaction/inline.txt +++ b/gopls/internal/test/marker/testdata/codeaction/inline.txt @@ -1,4 +1,5 @@ -This is a minimal test of the refactor.inline code action. +This is a minimal test of the refactor.inline code action, without resolve support. +See inline_resolve.txt for same test with resolve support. -- go.mod -- module example.com/codeaction diff --git a/gopls/internal/test/marker/testdata/codeaction/inline_resolve.txt b/gopls/internal/test/marker/testdata/codeaction/inline_resolve.txt new file mode 100644 index 00000000000..02c27e6505b --- /dev/null +++ b/gopls/internal/test/marker/testdata/codeaction/inline_resolve.txt @@ -0,0 +1,35 @@ +This is a minimal test of the refactor.inline code actions, with resolve support. +See inline.txt for same test without resolve support. + +-- capabilities.json -- +{ + "textDocument": { + "codeAction": { + "dataSupport": true, + "resolveSupport": { + "properties": ["edit"] + } + } + } +} +-- go.mod -- +module example.com/codeaction +go 1.18 + +-- a/a.go -- +package a + +func _() { + println(add(1, 2)) //@codeaction("add", ")", "refactor.inline", inline) +} + +func add(x, y int) int { return x + y } + +-- @inline/a/a.go -- +package a + +func _() { + println(1 + 2) //@codeaction("add", ")", "refactor.inline", inline) +} + +func add(x, y int) int { return x + y } From 730dc3c170c3e54f0dc78e72a0943f7cd43bb425 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 9 Feb 2024 17:55:48 +0000 Subject: [PATCH 17/23] gopls/internal/settings: add a hidden option to disable zero config Fixes golang/go#65515 Change-Id: If9786c00eb7629ec8d488cf8483144514d28786b Reviewed-on: https://go-review.googlesource.com/c/tools/+/562856 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan --- gopls/internal/cache/session.go | 2 +- gopls/internal/settings/default.go | 1 + gopls/internal/settings/settings.go | 8 +++++ .../integration/workspace/zero_config_test.go | 34 +++++++++++++++++++ 4 files changed, 44 insertions(+), 1 deletion(-) diff --git a/gopls/internal/cache/session.go b/gopls/internal/cache/session.go index a6802336dc9..7d2bf43773f 100644 --- a/gopls/internal/cache/session.go +++ b/gopls/internal/cache/session.go @@ -480,7 +480,7 @@ func selectViewDefs(ctx context.Context, fs file.Source, folders []*Folder, open checkFiles: for _, uri := range openFiles { folder := folderForFile(uri) - if folder == nil { + if folder == nil || !folder.Options.ZeroConfig { continue // only guess views for open files } fh, err := fs.ReadFile(ctx, uri) diff --git a/gopls/internal/settings/default.go b/gopls/internal/settings/default.go index bb248273d4d..752effab3ef 100644 --- a/gopls/internal/settings/default.go +++ b/gopls/internal/settings/default.go @@ -116,6 +116,7 @@ func DefaultOptions(overrides ...func(*Options)) *Options { TelemetryPrompt: false, LinkifyShowMessage: false, IncludeReplaceInWorkspace: true, + ZeroConfig: true, }, Hooks: Hooks{ URLRegexp: urlRegexp(), diff --git a/gopls/internal/settings/settings.go b/gopls/internal/settings/settings.go index 8010dcd6e5a..2f080596846 100644 --- a/gopls/internal/settings/settings.go +++ b/gopls/internal/settings/settings.go @@ -559,6 +559,11 @@ type InternalOptions struct { // Or in other words, if a go.mod file with local replaces behaves like a // go.work file. IncludeReplaceInWorkspace bool + + // ZeroConfig enables the zero-config algorithm for workspace layout, + // dynamically creating build configurations for different modules, + // directories, and GOOS/GOARCH combinations to cover open files. + ZeroConfig bool } type SubdirWatchPatterns string @@ -1159,6 +1164,9 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{}) case "includeReplaceInWorkspace": result.setBool(&o.IncludeReplaceInWorkspace) + case "zeroConfig": + result.setBool(&o.ZeroConfig) + // Replaced settings. case "experimentalDisabledAnalyses": result.deprecated("analyses") diff --git a/gopls/internal/test/integration/workspace/zero_config_test.go b/gopls/internal/test/integration/workspace/zero_config_test.go index 87c7c087844..93c24886c9e 100644 --- a/gopls/internal/test/integration/workspace/zero_config_test.go +++ b/gopls/internal/test/integration/workspace/zero_config_test.go @@ -234,3 +234,37 @@ const B = 1 }) } } + +func TestDisableZeroConfig(t *testing.T) { + // This test checks that we treat locally replaced modules as workspace + // modules, according to the "includeReplaceInWorkspace" setting. + const files = ` +-- moda/go.mod -- +module golang.org/a + +go 1.20 + +-- moda/a.go -- +package a + +-- modb/go.mod -- +module golang.org/b + +go 1.20 + +-- modb/b.go -- +package b + +` + + WithOptions( + Settings{"zeroConfig": false}, + ).Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("moda/a.go") + env.OpenFile("modb/b.go") + env.AfterChange() + if got := env.Views(); len(got) != 1 || got[0].Type != cache.AdHocView.String() { + t.Errorf("Views: got %v, want one adhoc view", got) + } + }) +} From 8cf0a8e204a2438da431f503635abed925cbfb79 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 9 Feb 2024 14:47:52 -0500 Subject: [PATCH 18/23] gopls: record that v0.15 will be the last to support go1.18 ...and v0.14.2 is the last to do so without warnings. Change-Id: Ie8cfd5e0337632fb60900938667e0a2a682ecb65 Reviewed-on: https://go-review.googlesource.com/c/tools/+/562682 Reviewed-by: Robert Findley LUCI-TryBot-Result: Go LUCI --- gopls/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/gopls/README.md b/gopls/README.md index 396f86c0242..5c80965c153 100644 --- a/gopls/README.md +++ b/gopls/README.md @@ -94,6 +94,7 @@ version of gopls. | Go 1.12 | [gopls@v0.7.5](https://github.com/golang/tools/releases/tag/gopls%2Fv0.7.5) | | Go 1.15 | [gopls@v0.9.5](https://github.com/golang/tools/releases/tag/gopls%2Fv0.9.5) | | Go 1.17 | [gopls@v0.11.0](https://github.com/golang/tools/releases/tag/gopls%2Fv0.11.0) | +| Go 1.18 | [gopls@v0.14.2](https://github.com/golang/tools/releases/tag/gopls%2Fv0.14.2) | Our extended support is enforced via [continuous integration with older Go versions](doc/contributing.md#ci). This legacy Go CI may not block releases: From f0ef3c690241cae5119135077dee723e50c1278d Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 9 Feb 2024 15:26:45 -0500 Subject: [PATCH 19/23] gopls: update x/telemetry dependency to fix crash Update to pick up the fix in https://go.dev/cl/562857. Change-Id: I0708338068e69d863fcbe3d5b6e0505f27735426 Reviewed-on: https://go-review.googlesource.com/c/tools/+/562683 Auto-Submit: Robert Findley Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI Reviewed-by: Peter Weinberger --- gopls/go.mod | 2 +- gopls/go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/gopls/go.mod b/gopls/go.mod index 35300455879..f17fdcaa522 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -8,7 +8,7 @@ require ( github.com/jba/templatecheck v0.6.0 golang.org/x/mod v0.15.0 golang.org/x/sync v0.6.0 - golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808 + golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 golang.org/x/text v0.14.0 golang.org/x/tools v0.17.0 golang.org/x/vuln v1.0.1 diff --git a/gopls/go.sum b/gopls/go.sum index e741723465f..9ab4bd75926 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -34,6 +34,8 @@ golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509 h1:Nr7eTQpQZ/ytesxDJpQ golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509/go.mod h1:ZthVHHkOi8rlMEsfFr3Ie42Ym1NonbFNNRKW3ci0UrU= golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808 h1:+Kc94D8UVEVxJnLXp/+FMfqQARZtWHfVrcRtcG8aT3g= 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= From 50b4f1b124a3a03ca655a70817a9b95eedab863d Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Sat, 10 Feb 2024 17:57:29 -0500 Subject: [PATCH 20/23] gopls/internal/golang: close open file Found while thinking about typestate. Change-Id: Ia98ace4782be34d1201f0ce5c8e536314fc494d7 Reviewed-on: https://go-review.googlesource.com/c/tools/+/563155 Reviewed-by: Robert Findley LUCI-TryBot-Result: Go LUCI Commit-Queue: Alan Donovan Auto-Submit: Alan Donovan --- gopls/internal/golang/gc_annotations.go | 1 + 1 file changed, 1 insertion(+) diff --git a/gopls/internal/golang/gc_annotations.go b/gopls/internal/golang/gc_annotations.go index 8bcf402cf1f..1ff866122ca 100644 --- a/gopls/internal/golang/gc_annotations.go +++ b/gopls/internal/golang/gc_annotations.go @@ -34,6 +34,7 @@ func GCOptimizationDetails(ctx context.Context, snapshot *cache.Snapshot, mp *me if err != nil { return nil, err } + tmpFile.Close() // ignore error defer os.Remove(tmpFile.Name()) outDirURI := protocol.URIFromPath(outDir) From c5643e9baf7fed6936d70e3abf925f86fa895ca1 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 12 Feb 2024 12:43:03 -0500 Subject: [PATCH 21/23] gopls/internal/server: fix two bugs related to dynamic configuration Fix two bugs related to dynamic configuration, that combine to prevent several clients from correctly configuring gopls, as reported in golang/go#65519 (Eglot), slack (lsp-mode), and team chat (Helix). The first bug has existed ~forever: when we make a workspace/configuration request in response to a didChangeConfiguration notification, we attempt to fetch the "global" settings by passing "scopeURI": "". The LSP spec defines "scopeURI" as a nullable URI: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#configurationItem Apparently, Javascript-based clients such as VS Code and coc.nvim (the two clients I regularly test with) will happily accept "" as a falsy value, and so this global query works. However, this query fails in Eglot (and likely other clients), causing didChangeConfiguration not to work. The second bug is new: When adding a new workspace folder we were failing to overwrite opts with the correct value (:= vs =, alas). This initial query had been masking the bug described above in Emacs, whereas in VS Code the (incorrectly) successful workspace/configuration request above masked the new bug. Since they both fail in Eglot, they are revealed. The fake editor is updated to reject the "" scope, highlighting the first bug. A new integration test is added to exercise the second bug, by way of a new integration test option to add per-folder configuration. Additionally, a marker test is added to exercise static configuration, which is when the client does not support the configuration capability at all. This wasn't actually broken, as first suspected, but the test is useful to include anyway, as we had no tests for this client behavior. Fixes golang/go#65519 Change-Id: Ie7170e3a26001546d4e334b83e6e73cd4ade10d8 Reviewed-on: https://go-review.googlesource.com/c/tools/+/563475 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI Auto-Submit: Robert Findley --- gopls/internal/server/general.go | 14 +++++-- .../internal/test/integration/fake/client.go | 5 ++- .../internal/test/integration/fake/editor.go | 36 ++++++++++++++-- .../integration/misc/configuration_test.go | 42 +++++++++++++++++++ gopls/internal/test/integration/options.go | 18 ++++++++ .../marker/testdata/configuration/static.txt | 41 ++++++++++++++++++ 6 files changed, 148 insertions(+), 8 deletions(-) create mode 100644 gopls/internal/test/marker/testdata/configuration/static.txt diff --git a/gopls/internal/server/general.go b/gopls/internal/server/general.go index 9f5e2b098b8..cdaee1b973b 100644 --- a/gopls/internal/server/general.go +++ b/gopls/internal/server/general.go @@ -473,7 +473,7 @@ func (s *server) newFolder(ctx context.Context, folder protocol.DocumentURI, nam return nil, fmt.Errorf("failed to get workspace configuration from client (%s): %v", folder, err) } - opts := opts.Clone() + opts = opts.Clone() for _, config := range configs { if err := s.handleOptionResults(ctx, settings.SetOptions(opts, config)); err != nil { return nil, err @@ -493,15 +493,23 @@ func (s *server) newFolder(ctx context.Context, folder protocol.DocumentURI, nam }, nil } +// fetchFolderOptions makes a workspace/configuration request for the given +// folder, and populates options with the result. +// +// If folder is "", fetchFolderOptions makes an unscoped request. func (s *server) fetchFolderOptions(ctx context.Context, folder protocol.DocumentURI) (*settings.Options, error) { opts := s.Options() if !opts.ConfigurationSupported { return opts, nil } - scope := string(folder) + var scopeURI *string + if folder != "" { + scope := string(folder) + scopeURI = &scope + } configs, err := s.client.Configuration(ctx, &protocol.ParamConfiguration{ Items: []protocol.ConfigurationItem{{ - ScopeURI: &scope, + ScopeURI: scopeURI, Section: "gopls", }}, }, diff --git a/gopls/internal/test/integration/fake/client.go b/gopls/internal/test/integration/fake/client.go index 2859d9b4047..f940821eefe 100644 --- a/gopls/internal/test/integration/fake/client.go +++ b/gopls/internal/test/integration/fake/client.go @@ -99,9 +99,12 @@ func (c *Client) WorkspaceFolders(context.Context) ([]protocol.WorkspaceFolder, func (c *Client) Configuration(_ context.Context, p *protocol.ParamConfiguration) ([]interface{}, error) { results := make([]interface{}, len(p.Items)) for i, item := range p.Items { + if item.ScopeURI != nil && *item.ScopeURI == "" { + return nil, fmt.Errorf(`malformed ScopeURI ""`) + } if item.Section == "gopls" { config := c.editor.Config() - results[i] = makeSettings(c.editor.sandbox, config) + results[i] = makeSettings(c.editor.sandbox, config, item.ScopeURI) } } return results, nil diff --git a/gopls/internal/test/integration/fake/editor.go b/gopls/internal/test/integration/fake/editor.go index e2eec07f51d..7c15106603f 100644 --- a/gopls/internal/test/integration/fake/editor.go +++ b/gopls/internal/test/integration/fake/editor.go @@ -110,7 +110,13 @@ type EditorConfig struct { FileAssociations map[string]string // Settings holds user-provided configuration for the LSP server. - Settings map[string]interface{} + Settings map[string]any + + // FolderSettings holds user-provided per-folder configuration, if any. + // + // It maps each folder (as a relative path to the sandbox workdir) to its + // configuration mapping (like Settings). + FolderSettings map[string]map[string]any // CapabilitiesJSON holds JSON client capabilities to overlay over the // editor's default client capabilities. @@ -216,7 +222,7 @@ func (e *Editor) Client() *Client { } // makeSettings builds the settings map for use in LSP settings RPCs. -func makeSettings(sandbox *Sandbox, config EditorConfig) map[string]interface{} { +func makeSettings(sandbox *Sandbox, config EditorConfig, scopeURI *protocol.URI) map[string]any { env := make(map[string]string) for k, v := range sandbox.GoEnv() { env[k] = v @@ -229,7 +235,7 @@ func makeSettings(sandbox *Sandbox, config EditorConfig) map[string]interface{} env[k] = v } - settings := map[string]interface{}{ + settings := map[string]any{ "env": env, // Use verbose progress reporting so that integration tests can assert on @@ -248,6 +254,28 @@ func makeSettings(sandbox *Sandbox, config EditorConfig) map[string]interface{} settings[k] = v } + // If the server is requesting configuration for a specific scope, apply + // settings for the nearest folder that has customized settings, if any. + if scopeURI != nil { + var ( + scopePath = protocol.DocumentURI(*scopeURI).Path() + closestDir string // longest dir with settings containing the scope, if any + closestSettings map[string]any // settings for that dir, if any + ) + for relPath, settings := range config.FolderSettings { + dir := sandbox.Workdir.AbsPath(relPath) + if strings.HasPrefix(scopePath+string(filepath.Separator), dir+string(filepath.Separator)) && len(dir) > len(closestDir) { + closestDir = dir + closestSettings = settings + } + } + if closestSettings != nil { + for k, v := range closestSettings { + settings[k] = v + } + } + } + return settings } @@ -261,7 +289,7 @@ func (e *Editor) initialize(ctx context.Context) error { Version: "v1.0.0", } } - params.InitializationOptions = makeSettings(e.sandbox, config) + params.InitializationOptions = makeSettings(e.sandbox, config, nil) params.WorkspaceFolders = makeWorkspaceFolders(e.sandbox, config.WorkspaceFolders) capabilities, err := clientCapabilities(config) diff --git a/gopls/internal/test/integration/misc/configuration_test.go b/gopls/internal/test/integration/misc/configuration_test.go index 2d39dc9fad1..fff4576bc5b 100644 --- a/gopls/internal/test/integration/misc/configuration_test.go +++ b/gopls/internal/test/integration/misc/configuration_test.go @@ -49,6 +49,48 @@ var FooErr = errors.New("foo") }) } +// 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). +func TestWorkspaceConfiguration(t *testing.T) { + const files = ` +-- go.mod -- +module example.com/config + +go 1.18 + +-- a/a.go -- +package a + +import "example.com/config/b" + +func _() { + _ = b.B{2} +} + +-- b/b.go -- +package b + +type B struct { + F int +} +` + + WithOptions( + WorkspaceFolders("a"), + FolderSettings(map[string]Settings{ + "a": { + "analyses": map[string]bool{ + "composites": false, + }, + }, + }), + ).Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("a/a.go") + env.AfterChange(NoDiagnostics()) + }) +} + // TestMajorOptionsChange is like TestChangeConfiguration, but modifies an // an open buffer before making a major (but inconsequential) change that // causes gopls to recreate the view. diff --git a/gopls/internal/test/integration/options.go b/gopls/internal/test/integration/options.go index 274b9e4ac6c..a6c394e3467 100644 --- a/gopls/internal/test/integration/options.go +++ b/gopls/internal/test/integration/options.go @@ -104,11 +104,29 @@ func WorkspaceFolders(relFolders ...string) RunOption { // Use an empty non-nil slice to signal explicitly no folders. relFolders = []string{} } + return optionSetter(func(opts *runConfig) { opts.editor.WorkspaceFolders = relFolders }) } +// FolderSettings defines per-folder workspace settings, keyed by relative path +// to the folder. +// +// Use in conjunction with WorkspaceFolders to have different settings for +// different folders. +func FolderSettings(folderSettings map[string]Settings) RunOption { + // Re-use the Settings type, for symmetry, but translate back into maps for + // the editor config. + folders := make(map[string]map[string]any) + for k, v := range folderSettings { + folders[k] = v + } + return optionSetter(func(opts *runConfig) { + opts.editor.FolderSettings = folders + }) +} + // EnvVars sets environment variables for the LSP session. When applying these // variables to the session, the special string $SANDBOX_WORKDIR is replaced by // the absolute path to the sandbox working directory. diff --git a/gopls/internal/test/marker/testdata/configuration/static.txt b/gopls/internal/test/marker/testdata/configuration/static.txt new file mode 100644 index 00000000000..c84b55db117 --- /dev/null +++ b/gopls/internal/test/marker/testdata/configuration/static.txt @@ -0,0 +1,41 @@ +This test confirms that gopls honors configuration even if the client does not +support dynamic configuration. + +-- capabilities.json -- +{ + "configuration": false +} + +-- settings.json -- +{ + "usePlaceholders": true, + "analyses": { + "composites": false + } +} + +-- go.mod -- +module example.com/config + +go 1.18 + +-- a/a.go -- +package a + +import "example.com/config/b" + +func Identity[P ~int](p P) P { //@item(Identity, "Identity", "", "") + return p +} + +func _() { + _ = b.B{2} + _ = Identi //@snippet(" //", Identity, "Identity(${1:p P})"), diag("Ident", re"(undefined|undeclared)") +} + +-- b/b.go -- +package b + +type B struct { + F int +} From afd84280124f1e3c3d100fd40838306752d28c40 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 12 Feb 2024 17:01:14 -0500 Subject: [PATCH 22/23] gopls/internal/test/integration: slightly more ergonomic FolderSettings After setting CL 563475 for autosubmit, I realized we should use a slightly more ergonomic pattern for configuring folder settings. Change-Id: I3a4e9e3dca3f4865c3bca02258cd81ff2b7024b3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/563402 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI Auto-Submit: Robert Findley --- .../test/integration/misc/configuration_test.go | 4 ++-- gopls/internal/test/integration/options.go | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/gopls/internal/test/integration/misc/configuration_test.go b/gopls/internal/test/integration/misc/configuration_test.go index fff4576bc5b..c6ed18041ae 100644 --- a/gopls/internal/test/integration/misc/configuration_test.go +++ b/gopls/internal/test/integration/misc/configuration_test.go @@ -78,13 +78,13 @@ type B struct { WithOptions( WorkspaceFolders("a"), - FolderSettings(map[string]Settings{ + FolderSettings{ "a": { "analyses": map[string]bool{ "composites": false, }, }, - }), + }, ).Run(t, files, func(t *testing.T, env *Env) { env.OpenFile("a/a.go") env.AfterChange(NoDiagnostics()) diff --git a/gopls/internal/test/integration/options.go b/gopls/internal/test/integration/options.go index a6c394e3467..f1d5425d807 100644 --- a/gopls/internal/test/integration/options.go +++ b/gopls/internal/test/integration/options.go @@ -115,16 +115,16 @@ func WorkspaceFolders(relFolders ...string) RunOption { // // Use in conjunction with WorkspaceFolders to have different settings for // different folders. -func FolderSettings(folderSettings map[string]Settings) RunOption { +type FolderSettings map[string]Settings + +func (fs FolderSettings) set(opts *runConfig) { // Re-use the Settings type, for symmetry, but translate back into maps for // the editor config. folders := make(map[string]map[string]any) - for k, v := range folderSettings { + for k, v := range fs { folders[k] = v } - return optionSetter(func(opts *runConfig) { - opts.editor.FolderSettings = folders - }) + opts.editor.FolderSettings = folders } // EnvVars sets environment variables for the LSP session. When applying these From 1b39a8b6a9ba38b140eb9b84196ddcb4e8f53ce2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Tue, 13 Feb 2024 13:41:59 +0000 Subject: [PATCH 23/23] internal/testenv: always Cleanup to appease go vet vet from Go 1.22 complains about cancelCtx being potentially leaked: exec.go:140:4: the cancelCtx function is not used on all paths (possible context leak) exec.go:192:2: this return statement may be reached without using the cancelCtx var defined on line 140 The code was fine with that, and in practice the leak does not happen, but having the vet warning around is still distracting. Thankfully, Go 1.14 is very old at this point, and per go.mod, this module now requires Go 1.18 or later, so we can clean this up. Change-Id: I4c402ad233e9b33a6d51c98ba0833c67fe01b209 Reviewed-on: https://go-review.googlesource.com/c/tools/+/563675 Reviewed-by: Than McIntosh Auto-Submit: Robert Findley Reviewed-by: Robert Findley LUCI-TryBot-Result: Go LUCI --- internal/testenv/exec.go | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/internal/testenv/exec.go b/internal/testenv/exec.go index 43aad5899fc..3b0cb6fcfa2 100644 --- a/internal/testenv/exec.go +++ b/internal/testenv/exec.go @@ -92,8 +92,7 @@ func NeedsExec(t testing.TB) { // for an arbitrary grace period before the test's deadline expires, // - if Cmd has the Cancel field, fails the test if the command is canceled // due to the test's deadline, and -// - if supported, sets a Cleanup function that verifies that the test did not -// leak a subprocess. +// - sets a Cleanup function that verifies that the test did not leak a subprocess. func CommandContext(t testing.TB, ctx context.Context, name string, args ...string) *exec.Cmd { t.Helper() NeedsExec(t) @@ -173,21 +172,14 @@ func CommandContext(t testing.TB, ctx context.Context, name string, args ...stri rWaitDelay.Set(reflect.ValueOf(gracePeriod)) } - // t.Cleanup was added in Go 1.14; for earlier Go versions, - // we just let the Context leak. - type Cleanupper interface { - Cleanup(func()) - } - if ct, ok := t.(Cleanupper); ok { - ct.Cleanup(func() { - if cancelCtx != nil { - cancelCtx() - } - if cmd.Process != nil && cmd.ProcessState == nil { - t.Errorf("command was started, but test did not wait for it to complete: %v", cmd) - } - }) - } + t.Cleanup(func() { + if cancelCtx != nil { + cancelCtx() + } + if cmd.Process != nil && cmd.ProcessState == nil { + t.Errorf("command was started, but test did not wait for it to complete: %v", cmd) + } + }) return cmd }