diff --git a/codereview.cfg b/codereview.cfg index 3f8b14b64e8..ad2f9ad84e6 100644 --- a/codereview.cfg +++ b/codereview.cfg @@ -1 +1,3 @@ issuerepo: golang/go +branch: gopls-release-branch.0.16 +parent-branch: master diff --git a/go/analysis/passes/loopclosure/loopclosure_test.go b/go/analysis/passes/loopclosure/loopclosure_test.go index 03b810700ab..5e6bc5f3be5 100644 --- a/go/analysis/passes/loopclosure/loopclosure_test.go +++ b/go/analysis/passes/loopclosure/loopclosure_test.go @@ -24,6 +24,7 @@ func Test(t *testing.T) { } func TestVersions22(t *testing.T) { + t.Skip("Disabled for golang.org/cl/603895. Fix and re-enable.") testenv.NeedsGo1Point(t, 22) txtar := filepath.Join(analysistest.TestData(), "src", "versions", "go22.txtar") @@ -32,6 +33,7 @@ func TestVersions22(t *testing.T) { } func TestVersions18(t *testing.T) { + t.Skip("Disabled for golang.org/cl/603895. Fix and re-enable.") txtar := filepath.Join(analysistest.TestData(), "src", "versions", "go18.txtar") dir := testfiles.ExtractTxtarFileToTmp(t, txtar) analysistest.Run(t, dir, loopclosure.Analyzer, "golang.org/fake/versions") diff --git a/go/analysis/passes/printf/printf.go b/go/analysis/passes/printf/printf.go index 32350192583..e096100c187 100644 --- a/go/analysis/passes/printf/printf.go +++ b/go/analysis/passes/printf/printf.go @@ -159,10 +159,11 @@ func maybePrintfWrapper(info *types.Info, decl ast.Decl) *printfWrapper { params := sig.Params() nparams := params.Len() // variadic => nonzero + // Check final parameter is "args ...interface{}". args := params.At(nparams - 1) - iface, ok := args.Type().(*types.Slice).Elem().(*types.Interface) + iface, ok := aliases.Unalias(args.Type().(*types.Slice).Elem()).(*types.Interface) if !ok || !iface.Empty() { - return nil // final (args) param is not ...interface{} + return nil } // Is second last param 'format string'? diff --git a/go/analysis/passes/printf/printf_test.go b/go/analysis/passes/printf/printf_test.go index 853d8619b25..0963c361be2 100644 --- a/go/analysis/passes/printf/printf_test.go +++ b/go/analysis/passes/printf/printf_test.go @@ -15,5 +15,5 @@ func Test(t *testing.T) { testdata := analysistest.TestData() printf.Analyzer.Flags.Set("funcs", "Warn,Warnf") - analysistest.Run(t, testdata, printf.Analyzer, "a", "b", "nofmt", "typeparams") + analysistest.Run(t, testdata, printf.Analyzer, "a", "b", "nofmt", "typeparams", "issue68744") } diff --git a/go/analysis/passes/printf/testdata/src/issue68744/issue68744.go b/go/analysis/passes/printf/testdata/src/issue68744/issue68744.go new file mode 100644 index 00000000000..79922ffbaaa --- /dev/null +++ b/go/analysis/passes/printf/testdata/src/issue68744/issue68744.go @@ -0,0 +1,13 @@ +package issue68744 + +import "fmt" + +// The use of "any" here is crucial to exercise the bug. +// (None of our earlier tests covered this vital detail!) +func wrapf(format string, args ...any) { // want wrapf:"printfWrapper" + fmt.Printf(format, args...) +} + +func _() { + wrapf("%s", 123) // want `issue68744.wrapf format %s has arg 123 of wrong type int` +} diff --git a/go/analysis/passes/stdversion/stdversion_test.go b/go/analysis/passes/stdversion/stdversion_test.go index 7b2f72de81b..e1f71fac3f5 100644 --- a/go/analysis/passes/stdversion/stdversion_test.go +++ b/go/analysis/passes/stdversion/stdversion_test.go @@ -15,6 +15,7 @@ import ( ) func Test(t *testing.T) { + t.Skip("Disabled for golang.org/cl/603895. Fix and re-enable.") // The test relies on go1.21 std symbols, but the analyzer // itself requires the go1.22 implementation of versions.FileVersions. testenv.NeedsGo1Point(t, 22) diff --git a/go/callgraph/vta/testdata/src/callgraph_type_aliases.go b/go/callgraph/vta/testdata/src/callgraph_type_aliases.go index ded3158b874..9b32109a828 100644 --- a/go/callgraph/vta/testdata/src/callgraph_type_aliases.go +++ b/go/callgraph/vta/testdata/src/callgraph_type_aliases.go @@ -6,6 +6,7 @@ // This file is the same as callgraph_interfaces.go except for // types J, X, Y, and Z aliasing types I, A, B, and C, resp. +// This test requires GODEBUG=gotypesalias=1 (the default in go1.23). package testdata @@ -57,11 +58,11 @@ func Baz(b bool) { // func Do(b bool) I: // ... -// t1 = (C).Foo(struct{}{}:C) +// t1 = (C).Foo(struct{}{}:Z) // t2 = NewY() // t3 = make I <- B (t2) // return t3 // WANT: // Baz: Do(b) -> Do; invoke t0.Foo() -> A.Foo, B.Foo -// Do: (C).Foo(struct{}{}:C) -> C.Foo; NewY() -> NewY +// Do: (C).Foo(struct{}{}:Z) -> C.Foo; NewY() -> NewY diff --git a/go/callgraph/vta/vta_test.go b/go/callgraph/vta/vta_test.go index b190149edcc..67db1302afd 100644 --- a/go/callgraph/vta/vta_test.go +++ b/go/callgraph/vta/vta_test.go @@ -5,14 +5,17 @@ package vta import ( + "strings" "testing" + "github.com/google/go-cmp/cmp" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/analysistest" "golang.org/x/tools/go/analysis/passes/buildssa" "golang.org/x/tools/go/callgraph/cha" "golang.org/x/tools/go/ssa" "golang.org/x/tools/go/ssa/ssautil" + "golang.org/x/tools/internal/aliases" ) func TestVTACallGraph(t *testing.T) { @@ -30,6 +33,11 @@ func TestVTACallGraph(t *testing.T) { "testdata/src/callgraph_type_aliases.go", } { t.Run(file, func(t *testing.T) { + // https://github.com/golang/go/issues/68799 + if !aliases.Enabled() && file == "testdata/src/callgraph_type_aliases.go" { + t.Skip("callgraph_type_aliases.go requires gotypesalias=1") + } + prog, want, err := testProg(file, ssa.BuilderMode(0)) if err != nil { t.Fatalf("couldn't load test file '%s': %s", file, err) @@ -40,8 +48,12 @@ func TestVTACallGraph(t *testing.T) { g := CallGraph(ssautil.AllFunctions(prog), cha.CallGraph(prog)) got := callGraphStr(g) - if diff := setdiff(want, got); len(diff) > 0 { - t.Errorf("computed callgraph %v\nshould contain\n%v\n(diff: %v)", got, want, diff) + if missing := setdiff(want, got); len(missing) > 0 { + t.Errorf("got:\n%s\n\nwant:\n%s\n\nmissing:\n%s\n\ndiff:\n%s", + strings.Join(got, "\n"), + strings.Join(want, "\n"), + strings.Join(missing, "\n"), + cmp.Diff(got, want)) // to aid debugging } }) } diff --git a/go/ssa/interp/interp_test.go b/go/ssa/interp/interp_test.go index 2ad6a9a0982..c55fe36c425 100644 --- a/go/ssa/interp/interp_test.go +++ b/go/ssa/interp/interp_test.go @@ -117,7 +117,7 @@ var testdataTests = []string{ "deepequal.go", "defer.go", "fieldprom.go", - "forvarlifetime_old.go", + // "forvarlifetime_old.go", Disabled for golang.org/cl/603895. Fix and re-enable. "ifaceconv.go", "ifaceprom.go", "initorder.go", @@ -129,7 +129,7 @@ var testdataTests = []string{ "slice2arrayptr.go", "static.go", "width32.go", - "rangevarlifetime_old.go", + // "rangevarlifetime_old.go", Disabled for golang.org/cl/603895. Fix and re-enable. "fixedbugs/issue52342.go", "fixedbugs/issue55115.go", "fixedbugs/issue52835.go", diff --git a/gopls/doc/generate/generate.go b/gopls/doc/generate/generate.go index 33735270a9c..bb799e52331 100644 --- a/gopls/doc/generate/generate.go +++ b/gopls/doc/generate/generate.go @@ -39,7 +39,6 @@ import ( "golang.org/x/tools/gopls/internal/doc" "golang.org/x/tools/gopls/internal/golang" "golang.org/x/tools/gopls/internal/mod" - "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/protocol/command/commandmeta" "golang.org/x/tools/gopls/internal/settings" "golang.org/x/tools/gopls/internal/util/maps" @@ -136,23 +135,12 @@ func loadAPI() (*doc.API, error) { &packages.Config{ Mode: packages.NeedTypes | packages.NeedTypesInfo | packages.NeedSyntax | packages.NeedDeps, }, - "golang.org/x/tools/gopls/internal/settings", // for settings - "golang.org/x/tools/gopls/internal/protocol", // for lenses + "golang.org/x/tools/gopls/internal/settings", ) if err != nil { return nil, err } - // TODO(adonovan): document at packages.Load that the result - // order does not match the pattern order. - var protocolPkg, settingsPkg *packages.Package - for _, pkg := range pkgs { - switch pkg.Types.Name() { - case "settings": - settingsPkg = pkg - case "protocol": - protocolPkg = pkg - } - } + settingsPkg := pkgs[0] defaults := settings.DefaultOptions() api := &doc.API{ @@ -164,7 +152,7 @@ func loadAPI() (*doc.API, error) { if err != nil { return nil, err } - api.Lenses, err = loadLenses(protocolPkg, defaults.Codelenses) + api.Lenses, err = loadLenses(settingsPkg, defaults.Codelenses) if err != nil { return nil, err } @@ -185,8 +173,8 @@ func loadAPI() (*doc.API, error) { catName := strings.TrimSuffix(category.Type().Name(), "Options") api.Options[catName] = opts - // Hardcode the expected values for the analyses and code lenses - // settings, since their keys are not enums. + // Hardcode the expected values for the "analyses" and "hints" settings, + // since their map keys are strings, not enums. for _, opt := range opts { switch opt.Name { case "analyses": @@ -197,24 +185,9 @@ func loadAPI() (*doc.API, error) { Default: strconv.FormatBool(a.Default), }) } - case "codelenses": - // Hack: Lenses don't set default values, and we don't want to - // pass in the list of expected lenses to loadOptions. Instead, - // format the defaults using reflection here. The hackiest part - // is reversing lowercasing of the field name. - reflectField := category.FieldByName(upperFirst(opt.Name)) - for _, l := range api.Lenses { - def, err := formatDefaultFromEnumBoolMap(reflectField, l.Lens) - if err != nil { - return nil, err - } - opt.EnumKeys.Keys = append(opt.EnumKeys.Keys, doc.EnumKey{ - Name: fmt.Sprintf("%q", l.Lens), - Doc: l.Doc, - Default: def, - }) - } case "hints": + // TODO(adonovan): simplify InlayHints to use an enum, + // following CodeLensSource. for _, a := range api.Hints { opt.EnumKeys.Keys = append(opt.EnumKeys.Keys, doc.EnumKey{ Name: fmt.Sprintf("%q", a.Name), @@ -282,24 +255,48 @@ func loadOptions(category reflect.Value, optsType types.Object, pkg *packages.Pa return nil, err } + // Derive the doc-and-api.json type from the Go field type. + // + // In principle, we should use JSON nomenclature here + // (number, array, object, etc; see #68057), but in + // practice we use the Go type string ([]T, map[K]V, + // etc) with only one tweak: enumeration types are + // replaced by "enum", including when they appear as + // map keys. + // + // Notable edge cases: + // - any (e.g. in linksInHover) is really a sum of false | true | "internal". + // - time.Duration is really a string with a particular syntax. typ := typesField.Type().String() if _, ok := enums[typesField.Type()]; ok { typ = "enum" } name := lowerFirst(typesField.Name()) + // enum-keyed maps var enumKeys doc.EnumKeys if m, ok := typesField.Type().Underlying().(*types.Map); ok { - e, ok := enums[m.Key()] + values, ok := enums[m.Key()] if ok { - typ = strings.Replace(typ, m.Key().String(), m.Key().Underlying().String(), 1) - } - keys, err := collectEnumKeys(name, m, reflectField, e) - if err != nil { - return nil, err + // Update type name: "map[CodeLensSource]T" -> "map[enum]T" + // hack: assumes key substring is unique! + typ = strings.Replace(typ, m.Key().String(), "enum", 1) } - if keys != nil { - enumKeys = *keys + + // Edge case: "analyses" is a string (not enum) keyed map, + // but its EnumKeys.ValueType was historically populated. + // (But not "env"; not sure why.) + if ok || name == "analyses" { + enumKeys.ValueType = m.Elem().String() + + // For map[enum]T fields, gather the set of valid + // EnumKeys (from type information). If T=bool, also + // record the default value (from reflection). + keys, err := collectEnumKeys(m, reflectField, values) + if err != nil { + return nil, err + } + enumKeys.Keys = keys } } @@ -350,20 +347,13 @@ func loadEnums(pkg *packages.Package) (map[types.Type][]doc.EnumValue, error) { return enums, nil } -func collectEnumKeys(name string, m *types.Map, reflectField reflect.Value, enumValues []doc.EnumValue) (*doc.EnumKeys, error) { - // Make sure the value type gets set for analyses and codelenses - // too. - if len(enumValues) == 0 && !hardcodedEnumKeys(name) { - return nil, nil - } - keys := &doc.EnumKeys{ - ValueType: m.Elem().String(), - } +func collectEnumKeys(m *types.Map, reflectField reflect.Value, enumValues []doc.EnumValue) ([]doc.EnumKey, error) { // We can get default values for enum -> bool maps. var isEnumBoolMap bool if basic, ok := m.Elem().Underlying().(*types.Basic); ok && basic.Kind() == types.Bool { isEnumBoolMap = true } + var keys []doc.EnumKey for _, v := range enumValues { var def string if isEnumBoolMap { @@ -373,7 +363,7 @@ func collectEnumKeys(name string, m *types.Map, reflectField reflect.Value, enum return nil, err } } - keys.Keys = append(keys.Keys, doc.EnumKey{ + keys = append(keys, doc.EnumKey{ Name: v.Value, Doc: v.Doc, Default: def, @@ -529,19 +519,19 @@ func structDoc(fields []*commandmeta.Field, level int) string { return b.String() } -// loadLenses combines the syntactic comments from the protocol +// loadLenses combines the syntactic comments from the settings // package with the default values from settings.DefaultOptions(), and // returns a list of Code Lens descriptors. -func loadLenses(protocolPkg *packages.Package, defaults map[protocol.CodeLensSource]bool) ([]*doc.Lens, error) { +func loadLenses(settingsPkg *packages.Package, defaults map[settings.CodeLensSource]bool) ([]*doc.Lens, error) { // Find the CodeLensSource enums among the files of the protocol package. // Map each enum value to its doc comment. enumDoc := make(map[string]string) - for _, f := range protocolPkg.Syntax { + for _, f := range settingsPkg.Syntax { for _, decl := range f.Decls { if decl, ok := decl.(*ast.GenDecl); ok && decl.Tok == token.CONST { for _, spec := range decl.Specs { spec := spec.(*ast.ValueSpec) - posn := safetoken.StartPosition(protocolPkg.Fset, spec.Pos()) + posn := safetoken.StartPosition(settingsPkg.Fset, spec.Pos()) if id, ok := spec.Type.(*ast.Ident); ok && id.Name == "CodeLensSource" { if len(spec.Names) != 1 || len(spec.Values) != 1 { return nil, fmt.Errorf("%s: declare one CodeLensSource per line", posn) @@ -566,7 +556,7 @@ func loadLenses(protocolPkg *packages.Package, defaults map[protocol.CodeLensSou // Build list of Lens descriptors. var lenses []*doc.Lens - addAll := func(sources map[protocol.CodeLensSource]cache.CodeLensSourceFunc, fileType string) error { + addAll := func(sources map[settings.CodeLensSource]cache.CodeLensSourceFunc, fileType string) error { slice := maps.Keys(sources) sort.Slice(slice, func(i, j int) bool { return slice[i] < slice[j] }) for _, source := range slice { @@ -732,10 +722,6 @@ func rewriteSettings(prevContent []byte, api *doc.API) ([]byte, error) { buf.WriteString(opt.Doc) // enums - // - // TODO(adonovan): `CodeLensSource` should be treated as an enum, - // but loadEnums considers only the `settings` package, - // not `protocol`. write := func(name, doc string) { if doc != "" { unbroken := parBreakRE.ReplaceAllString(doc, "\\\n") @@ -745,12 +731,14 @@ func rewriteSettings(prevContent []byte, api *doc.API) ([]byte, error) { } } if len(opt.EnumValues) > 0 && opt.Type == "enum" { + // enum as top-level type constructor buf.WriteString("\nMust be one of:\n\n") for _, val := range opt.EnumValues { write(val.Value, val.Doc) } } else if len(opt.EnumKeys.Keys) > 0 && shouldShowEnumKeysInSettings(opt.Name) { - buf.WriteString("\nCan contain any of:\n\n") + // enum as map key (currently just "annotations") + buf.WriteString("\nEach enum must be one of:\n\n") for _, val := range opt.EnumKeys.Keys { write(val.Name, val.Doc) } @@ -772,7 +760,9 @@ func rewriteSettings(prevContent []byte, api *doc.API) ([]byte, error) { var parBreakRE = regexp.MustCompile("\n{2,}") func shouldShowEnumKeysInSettings(name string) bool { - // These fields have too many possible options to print. + // These fields have too many possible options, + // or too voluminous documentation, to render as enums. + // Instead they each get their own page in the manual. return !(name == "analyses" || name == "codelenses" || name == "hints") } @@ -830,10 +820,6 @@ func collectGroups(opts []*doc.Option) []optionsGroup { return groups } -func hardcodedEnumKeys(name string) bool { - return name == "analyses" || name == "codelenses" -} - func capitalize(s string) string { return string(unicode.ToUpper(rune(s[0]))) + s[1:] } diff --git a/gopls/doc/settings.md b/gopls/doc/settings.md index 1a08c56e32a..6aec26fd2a1 100644 --- a/gopls/doc/settings.md +++ b/gopls/doc/settings.md @@ -180,7 +180,7 @@ Default: `false`. ## UI -### `codelenses` *map[golang.org/x/tools/gopls/internal/protocol.CodeLensSource]bool* +### `codelenses` *map[enum]bool* codelenses overrides the enabled/disabled state of each of gopls' sources of [Code Lenses](codelenses.md). @@ -325,14 +325,14 @@ These analyses are documented on Default: `false`. -### `annotations` *map[string]bool* +### `annotations` *map[enum]bool* **This setting is experimental and may be deleted.** annotations specifies the various kinds of optimization diagnostics that should be reported by the gc_details command. -Can contain any of: +Each enum must be one of: * `"bounds"` controls bounds checking diagnostics. * `"escape"` controls diagnostics about escape choices. diff --git a/gopls/go.mod b/gopls/go.mod index a331ba6dba9..f4d2cba8af4 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -5,11 +5,11 @@ go 1.19 // => default GODEBUG has gotypesalias=0 require ( github.com/google/go-cmp v0.6.0 github.com/jba/templatecheck v0.7.0 - golang.org/x/mod v0.18.0 - golang.org/x/sync v0.7.0 - golang.org/x/telemetry v0.0.0-20240607193123-221703e18637 + golang.org/x/mod v0.20.0 + golang.org/x/sync v0.8.0 + golang.org/x/telemetry v0.0.0-20240829154258-f29ab539cc98 golang.org/x/text v0.16.0 - golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d + golang.org/x/tools v0.22.1-0.20240829175637-39126e24d653 golang.org/x/vuln v1.0.4 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.4.7 @@ -21,9 +21,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.21.0 // indirect + golang.org/x/sys v0.23.0 // indirect 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 a41ecff8031..44ff347f685 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -10,37 +10,22 @@ github.com/jba/templatecheck v0.7.0/go.mod h1:n1Etw+Rrw1mDDD8dDRsEKTwMZsJ98Ekktg 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/crypto v0.24.0/go.mod h1:Z1PMYSOR5nyMcyAVAIQSKCDwalqy85Aqn1x3Ws4L5DM= 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.17.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= -golang.org/x/mod v0.18.0 h1:5+9lSbEzPSdWkH32vYPBwEpX8KwDbM52Ud9xBUvNlb0= -golang.org/x/mod v0.18.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/net v0.26.0/go.mod h1:5YKkiSynbBIh3p6iOc/vibscux0x38BZDkn8sCUPxHE= -golang.org/x/sync v0.7.0 h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M= -golang.org/x/sync v0.7.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/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws= -golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/telemetry v0.0.0-20240521205824-bda55230c457/go.mod h1:pRgIJT+bRLFKnoM1ldnzKoxTIn14Yxz928LQRYYgIN0= -golang.org/x/telemetry v0.0.0-20240607193123-221703e18637 h1:3Wt8mZlbFwG8llny+t18kh7AXxyWePFycXMuVdHxnyM= -golang.org/x/telemetry v0.0.0-20240607193123-221703e18637/go.mod h1:n38mvGdgc4dA684EC4NwQwoPKSw4jyKw8/DgZHDA1Dk= -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/term v0.21.0/go.mod h1:ooXLefLobQVslOqselCNF4SxFAaoS6KujMbsGzSDmX0= +golang.org/x/mod v0.20.0 h1:utOm6MM3R3dnawAiJgn0y+xvuYRsm1RKM/4giyfDgV0= +golang.org/x/mod v0.20.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= +golang.org/x/sync v0.8.0 h1:3NFvSEYkUoMifnESzZl15y791HH1qU2xm6eCJU5ZPXQ= +golang.org/x/sync v0.8.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/sys v0.23.0 h1:YfKFowiIMvtgl1UERQoTPPToxltDeZfbj4H7dVUCwmM= +golang.org/x/sys v0.23.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/telemetry v0.0.0-20240829154258-f29ab539cc98 h1:Wm3cG5X6sZ0RSVRc/H1/sciC4AT6HAKgLCSH2lbpR/c= +golang.org/x/telemetry v0.0.0-20240829154258-f29ab539cc98/go.mod h1:m7R/r+o5h7UvF2JD9n2iLSGY4v8v+zNSyTJ6xynLrqs= 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/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/text v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4= golang.org/x/text v0.16.0/go.mod h1:GhwF1Be+LQoKShO3cGOHzqOgRrGaYc9AvblQOmPVHnI= +golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= +golang.org/x/tools v0.22.1-0.20240829175637-39126e24d653 h1:6bJEg2w2kUHWlfdJaESYsmNfI1LKAZQi6zCa7LUn7eI= +golang.org/x/tools v0.22.1-0.20240829175637-39126e24d653/go.mod h1:aCwcsjqvq7Yqt6TNyX7QMU2enbQ/Gt0bo6krSeEri+c= golang.org/x/vuln v1.0.4 h1:SP0mPeg2PmGCu03V+61EcQiOjmpri2XijexKdzv8Z1I= golang.org/x/vuln v1.0.4/go.mod h1:NbJdUQhX8jY++FtuhrXs2Eyx0yePo9pF7nPlIjo9aaQ= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/gopls/internal/cache/view.go b/gopls/internal/cache/view.go index 1e3448d42f4..f1a13e358da 100644 --- a/gopls/internal/cache/view.go +++ b/gopls/internal/cache/view.go @@ -67,6 +67,7 @@ type GoEnv struct { GOPRIVATE string GOFLAGS string GO111MODULE string + GOTOOLCHAIN string // Go version output. GoVersion int // The X in Go 1.X @@ -992,6 +993,7 @@ func FetchGoEnv(ctx context.Context, folder protocol.DocumentURI, opts *settings "GOMODCACHE": &env.GOMODCACHE, "GOFLAGS": &env.GOFLAGS, "GO111MODULE": &env.GO111MODULE, + "GOTOOLCHAIN": &env.GOTOOLCHAIN, } if err := loadGoEnv(ctx, dir, opts.EnvSlice(), runner, envvars); err != nil { return nil, err diff --git a/gopls/internal/cmd/codelens.go b/gopls/internal/cmd/codelens.go index b07f15429ce..452e978094f 100644 --- a/gopls/internal/cmd/codelens.go +++ b/gopls/internal/cmd/codelens.go @@ -78,9 +78,9 @@ func (r *codelens) Run(ctx context.Context, args ...string) error { origOptions(opts) } if opts.Codelenses == nil { - opts.Codelenses = make(map[protocol.CodeLensSource]bool) + opts.Codelenses = make(map[settings.CodeLensSource]bool) } - opts.Codelenses[protocol.CodeLensTest] = true + opts.Codelenses[settings.CodeLensTest] = true } conn, err := r.app.connect(ctx) diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json index 96ff0545041..ca4a9384899 100644 --- a/gopls/internal/doc/api.json +++ b/gopls/internal/doc/api.json @@ -646,7 +646,7 @@ }, { "Name": "annotations", - "Type": "map[string]bool", + "Type": "map[enum]bool", "Doc": "annotations specifies the various kinds of optimization diagnostics\nthat should be reported by the gc_details command.\n", "EnumKeys": { "ValueType": "bool", @@ -799,49 +799,49 @@ }, { "Name": "codelenses", - "Type": "map[golang.org/x/tools/gopls/internal/protocol.CodeLensSource]bool", + "Type": "map[enum]bool", "Doc": "codelenses overrides the enabled/disabled state of each of gopls'\nsources of [Code Lenses](codelenses.md).\n\nExample Usage:\n\n```json5\n\"gopls\": {\n...\n \"codelenses\": {\n \"generate\": false, // Don't show the `go generate` lens.\n \"gc_details\": true // Show a code lens toggling the display of gc's choices.\n }\n...\n}\n```\n", "EnumKeys": { "ValueType": "bool", "Keys": [ { "Name": "\"gc_details\"", - "Doc": "\nThis codelens source causes the `package` declaration of\neach file to be annotated with a command to toggle the\nstate of the per-session variable that controls whether\noptimization decisions from the Go compiler (formerly known\nas \"gc\") should be displayed as diagnostics.\n\nOptimization decisions include:\n- whether a variable escapes, and how escape is inferred;\n- whether a nil-pointer check is implied or eliminated;\n- whether a function can be inlined.\n\nTODO(adonovan): this source is off by default because the\nannotation is annoying and because VS Code has a separate\n\"Toggle gc details\" command. Replace it with a Code Action\n(\"Source action...\").\n", + "Doc": "`\"gc_details\"`: Toggle display of Go compiler optimization decisions\n\nThis codelens source causes the `package` declaration of\neach file to be annotated with a command to toggle the\nstate of the per-session variable that controls whether\noptimization decisions from the Go compiler (formerly known\nas \"gc\") should be displayed as diagnostics.\n\nOptimization decisions include:\n- whether a variable escapes, and how escape is inferred;\n- whether a nil-pointer check is implied or eliminated;\n- whether a function can be inlined.\n\nTODO(adonovan): this source is off by default because the\nannotation is annoying and because VS Code has a separate\n\"Toggle gc details\" command. Replace it with a Code Action\n(\"Source action...\").\n", "Default": "false" }, { "Name": "\"generate\"", - "Doc": "\nThis codelens source annotates any `//go:generate` comments\nwith commands to run `go generate` in this directory, on\nall directories recursively beneath this one.\n\nSee [Generating code](https://go.dev/blog/generate) for\nmore details.\n", + "Doc": "`\"generate\"`: Run `go generate`\n\nThis codelens source annotates any `//go:generate` comments\nwith commands to run `go generate` in this directory, on\nall directories recursively beneath this one.\n\nSee [Generating code](https://go.dev/blog/generate) for\nmore details.\n", "Default": "true" }, { "Name": "\"regenerate_cgo\"", - "Doc": "\nThis codelens source annotates an `import \"C\"` declaration\nwith a command to re-run the [cgo\ncommand](https://pkg.go.dev/cmd/cgo) to regenerate the\ncorresponding Go declarations.\n\nUse this after editing the C code in comments attached to\nthe import, or in C header files included by it.\n", + "Doc": "`\"regenerate_cgo\"`: Re-generate cgo declarations\n\nThis codelens source annotates an `import \"C\"` declaration\nwith a command to re-run the [cgo\ncommand](https://pkg.go.dev/cmd/cgo) to regenerate the\ncorresponding Go declarations.\n\nUse this after editing the C code in comments attached to\nthe import, or in C header files included by it.\n", "Default": "true" }, { - "Name": "\"test\"", - "Doc": "\nThis codelens source annotates each `Test` and `Benchmark`\nfunction in a `*_test.go` file with a command to run it.\n\nThis source is off by default because VS Code has\na client-side custom UI for testing, and because progress\nnotifications are not a great UX for streamed test output.\nSee:\n- golang/go#67400 for a discussion of this feature.\n- https://github.com/joaotavora/eglot/discussions/1402\n for an alternative approach.\n", + "Name": "\"run_govulncheck\"", + "Doc": "`\"run_govulncheck\"`: Run govulncheck\n\nThis codelens source annotates the `module` directive in a\ngo.mod file with a command to run Govulncheck.\n\n[Govulncheck](https://go.dev/blog/vuln) is a static\nanalysis tool that computes the set of functions reachable\nwithin your application, including dependencies;\nqueries a database of known security vulnerabilities; and\nreports any potential problems it finds.\n", "Default": "false" }, { - "Name": "\"run_govulncheck\"", - "Doc": "\nThis codelens source annotates the `module` directive in a\ngo.mod file with a command to run Govulncheck.\n\n[Govulncheck](https://go.dev/blog/vuln) is a static\nanalysis tool that computes the set of functions reachable\nwithin your application, including dependencies;\nqueries a database of known security vulnerabilities; and\nreports any potential problems it finds.\n", + "Name": "\"test\"", + "Doc": "`\"test\"`: Run tests and benchmarks\n\nThis codelens source annotates each `Test` and `Benchmark`\nfunction in a `*_test.go` file with a command to run it.\n\nThis source is off by default because VS Code has\na client-side custom UI for testing, and because progress\nnotifications are not a great UX for streamed test output.\nSee:\n- golang/go#67400 for a discussion of this feature.\n- https://github.com/joaotavora/eglot/discussions/1402\n for an alternative approach.\n", "Default": "false" }, { "Name": "\"tidy\"", - "Doc": "\nThis codelens source annotates the `module` directive in a\ngo.mod file with a command to run [`go mod\ntidy`](https://go.dev/ref/mod#go-mod-tidy), which ensures\nthat the go.mod file matches the source code in the module.\n", + "Doc": "`\"tidy\"`: Tidy go.mod file\n\nThis codelens source annotates the `module` directive in a\ngo.mod file with a command to run [`go mod\ntidy`](https://go.dev/ref/mod#go-mod-tidy), which ensures\nthat the go.mod file matches the source code in the module.\n", "Default": "true" }, { "Name": "\"upgrade_dependency\"", - "Doc": "\nThis codelens source annotates the `module` directive in a\ngo.mod file with commands to:\n\n- check for available upgrades,\n- upgrade direct dependencies, and\n- upgrade all dependencies transitively.\n", + "Doc": "`\"upgrade_dependency\"`: Update dependencies\n\nThis codelens source annotates the `module` directive in a\ngo.mod file with commands to:\n\n- check for available upgrades,\n- upgrade direct dependencies, and\n- upgrade all dependencies transitively.\n", "Default": "true" }, { "Name": "\"vendor\"", - "Doc": "\nThis codelens source annotates the `module` directive in a\ngo.mod file with a command to run [`go mod\nvendor`](https://go.dev/ref/mod#go-mod-vendor), which\ncreates or updates the directory named `vendor` in the\nmodule root so that it contains an up-to-date copy of all\nnecessary package dependencies.\n", + "Doc": "`\"vendor\"`: Update vendor directory\n\nThis codelens source annotates the `module` directive in a\ngo.mod file with a command to run [`go mod\nvendor`](https://go.dev/ref/mod#go-mod-vendor), which\ncreates or updates the directory named `vendor` in the\nmodule root so that it contains an up-to-date copy of all\nnecessary package dependencies.\n", "Default": "true" } ] diff --git a/gopls/internal/golang/code_lens.go b/gopls/internal/golang/code_lens.go index 82ff0f5bec0..a4aab3e16b0 100644 --- a/gopls/internal/golang/code_lens.go +++ b/gopls/internal/golang/code_lens.go @@ -17,15 +17,16 @@ import ( "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" ) // CodeLensSources returns the supported sources of code lenses for Go files. -func CodeLensSources() map[protocol.CodeLensSource]cache.CodeLensSourceFunc { - return map[protocol.CodeLensSource]cache.CodeLensSourceFunc{ - protocol.CodeLensGenerate: goGenerateCodeLens, // commands: Generate - protocol.CodeLensTest: runTestCodeLens, // commands: Test - protocol.CodeLensRegenerateCgo: regenerateCgoLens, // commands: RegenerateCgo - protocol.CodeLensGCDetails: toggleDetailsCodeLens, // commands: GCDetails +func CodeLensSources() map[settings.CodeLensSource]cache.CodeLensSourceFunc { + return map[settings.CodeLensSource]cache.CodeLensSourceFunc{ + settings.CodeLensGenerate: goGenerateCodeLens, // commands: Generate + settings.CodeLensTest: runTestCodeLens, // commands: Test + settings.CodeLensRegenerateCgo: regenerateCgoLens, // commands: RegenerateCgo + settings.CodeLensGCDetails: toggleDetailsCodeLens, // commands: GCDetails } } diff --git a/gopls/internal/golang/completion/package.go b/gopls/internal/golang/completion/package.go index 12d4ff0be36..a52c449dc78 100644 --- a/gopls/internal/golang/completion/package.go +++ b/gopls/internal/golang/completion/package.go @@ -106,39 +106,20 @@ func packageCompletionSurrounding(pgf *parsego.File, offset int) (*Selection, er // First, consider the possibility that we have a valid "package" keyword // with an empty package name ("package "). "package" is parsed as an - // *ast.BadDecl since it is a keyword. This logic would allow "package" to - // appear on any line of the file as long as it's the first code expression - // in the file. - lines := strings.Split(string(pgf.Src), "\n") - cursorLine := safetoken.Line(tok, cursor) - if cursorLine <= 0 || cursorLine > len(lines) { - return nil, fmt.Errorf("invalid line number") + // *ast.BadDecl since it is a keyword. + start, err := safetoken.Offset(tok, expr.Pos()) + if err != nil { + return nil, err } - if safetoken.StartPosition(fset, expr.Pos()).Line == cursorLine { - words := strings.Fields(lines[cursorLine-1]) - if len(words) > 0 && words[0] == PACKAGE { - content := PACKAGE - // Account for spaces if there are any. - if len(words) > 1 { - content += " " - } - - start := expr.Pos() - end := token.Pos(int(expr.Pos()) + len(content) + 1) - // We have verified that we have a valid 'package' keyword as our - // first expression. Ensure that cursor is in this keyword or - // otherwise fallback to the general case. - if cursor >= start && cursor <= end { - return &Selection{ - content: content, - cursor: cursor, - tokFile: tok, - start: start, - end: end, - mapper: m, - }, nil - } - } + if offset > start && string(bytes.TrimRight(pgf.Src[start:offset], " ")) == PACKAGE { + return &Selection{ + content: string(pgf.Src[start:offset]), + cursor: cursor, + tokFile: tok, + start: expr.Pos(), + end: cursor, + mapper: m, + }, nil } // If the cursor is after the start of the expression, no package diff --git a/gopls/internal/golang/hover.go b/gopls/internal/golang/hover.go index 81594a6bf39..5e912285323 100644 --- a/gopls/internal/golang/hover.go +++ b/gopls/internal/golang/hover.go @@ -70,6 +70,7 @@ type hoverJSON struct { // LinkPath is the pkg.go.dev link for the given symbol. // For example, the "go/ast" part of "pkg.go.dev/go/ast#Node". + // It may have a module version suffix "@v1.2.3". LinkPath string `json:"linkPath"` // LinkAnchor is the pkg.go.dev link anchor for the given symbol. @@ -98,6 +99,7 @@ type hoverJSON struct { } // Hover implements the "textDocument/hover" RPC for Go files. +// It may return nil even on success. // // If pkgURL is non-nil, it should be used to generate doc links. func Hover(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, position protocol.Position, pkgURL func(path PackagePath, fragment string) protocol.URI) (*protocol.Hover, error) { @@ -200,7 +202,7 @@ func hover(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, pp pro if hoverRange == nil { hoverRange = &rng } - return *hoverRange, hoverJSON, nil + return *hoverRange, hoverJSON, nil // (hoverJSON may be nil) } } // Handle hovering over (non-import-path) literals. @@ -1159,7 +1161,8 @@ func formatLink(h *hoverJSON, options *settings.Options, pkgURL func(path Packag var url protocol.URI var caption string if pkgURL != nil { // LinksInHover == "gopls" - url = pkgURL(PackagePath(h.LinkPath), h.LinkAnchor) + path, _, _ := strings.Cut(h.LinkPath, "@") // remove optional module version suffix + url = pkgURL(PackagePath(path), h.LinkAnchor) caption = "in gopls doc viewer" } else { if options.LinkTarget == "" { diff --git a/gopls/internal/mod/code_lens.go b/gopls/internal/mod/code_lens.go index 89942722e75..f23cc641365 100644 --- a/gopls/internal/mod/code_lens.go +++ b/gopls/internal/mod/code_lens.go @@ -15,15 +15,16 @@ import ( "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" ) // CodeLensSources returns the sources of code lenses for go.mod files. -func CodeLensSources() map[protocol.CodeLensSource]cache.CodeLensSourceFunc { - return map[protocol.CodeLensSource]cache.CodeLensSourceFunc{ - protocol.CodeLensUpgradeDependency: upgradeLenses, // commands: CheckUpgrades, UpgradeDependency - protocol.CodeLensTidy: tidyLens, // commands: Tidy - protocol.CodeLensVendor: vendorLens, // commands: Vendor - protocol.CodeLensRunGovulncheck: vulncheckLenses, // commands: RunGovulncheck +func CodeLensSources() map[settings.CodeLensSource]cache.CodeLensSourceFunc { + return map[settings.CodeLensSource]cache.CodeLensSourceFunc{ + settings.CodeLensUpgradeDependency: upgradeLenses, // commands: CheckUpgrades, UpgradeDependency + settings.CodeLensTidy: tidyLens, // commands: Tidy + settings.CodeLensVendor: vendorLens, // commands: Vendor + settings.CodeLensRunGovulncheck: vulncheckLenses, // commands: RunGovulncheck } } diff --git a/gopls/internal/protocol/codeactionkind.go b/gopls/internal/protocol/codeactionkind.go index d493b452f86..045067dd71e 100644 --- a/gopls/internal/protocol/codeactionkind.go +++ b/gopls/internal/protocol/codeactionkind.go @@ -4,7 +4,7 @@ package protocol -// This file defines constants for non-standard CodeActions and CodeLenses. +// This file defines constants for non-standard CodeActions. // CodeAction kinds specific to gopls // @@ -84,114 +84,3 @@ const ( // CodeAction request is unknown. A missing // CodeActionContext.TriggerKind should be treated as equivalent. const CodeActionUnknownTrigger CodeActionTriggerKind = 0 - -// A CodeLensSource identifies an (algorithmic) source of code lenses. -type CodeLensSource string - -// CodeLens sources -// -// These identifiers appear in the "codelenses" configuration setting, -// and in the user documentation thereof, which is generated by -// gopls/doc/generate/generate.go parsing this file. -// -// Doc comments should use GitHub Markdown. -// The first line becomes the title. -// -// (For historical reasons, each code lens source identifier typically -// matches the name of one of the command.Commands returned by it, -// but that isn't essential.) -const ( - // Toggle display of Go compiler optimization decisions - // - // This codelens source causes the `package` declaration of - // each file to be annotated with a command to toggle the - // state of the per-session variable that controls whether - // optimization decisions from the Go compiler (formerly known - // as "gc") should be displayed as diagnostics. - // - // Optimization decisions include: - // - whether a variable escapes, and how escape is inferred; - // - whether a nil-pointer check is implied or eliminated; - // - whether a function can be inlined. - // - // TODO(adonovan): this source is off by default because the - // annotation is annoying and because VS Code has a separate - // "Toggle gc details" command. Replace it with a Code Action - // ("Source action..."). - CodeLensGCDetails CodeLensSource = "gc_details" - - // Run `go generate` - // - // This codelens source annotates any `//go:generate` comments - // with commands to run `go generate` in this directory, on - // all directories recursively beneath this one. - // - // See [Generating code](https://go.dev/blog/generate) for - // more details. - CodeLensGenerate CodeLensSource = "generate" - - // Re-generate cgo declarations - // - // This codelens source annotates an `import "C"` declaration - // with a command to re-run the [cgo - // command](https://pkg.go.dev/cmd/cgo) to regenerate the - // corresponding Go declarations. - // - // Use this after editing the C code in comments attached to - // the import, or in C header files included by it. - CodeLensRegenerateCgo CodeLensSource = "regenerate_cgo" - - // Run govulncheck - // - // This codelens source annotates the `module` directive in a - // go.mod file with a command to run Govulncheck. - // - // [Govulncheck](https://go.dev/blog/vuln) is a static - // analysis tool that computes the set of functions reachable - // within your application, including dependencies; - // queries a database of known security vulnerabilities; and - // reports any potential problems it finds. - CodeLensRunGovulncheck CodeLensSource = "run_govulncheck" - - // Run tests and benchmarks - // - // This codelens source annotates each `Test` and `Benchmark` - // function in a `*_test.go` file with a command to run it. - // - // This source is off by default because VS Code has - // a client-side custom UI for testing, and because progress - // notifications are not a great UX for streamed test output. - // See: - // - golang/go#67400 for a discussion of this feature. - // - https://github.com/joaotavora/eglot/discussions/1402 - // for an alternative approach. - CodeLensTest CodeLensSource = "test" - - // Tidy go.mod file - // - // This codelens source annotates the `module` directive in a - // go.mod file with a command to run [`go mod - // tidy`](https://go.dev/ref/mod#go-mod-tidy), which ensures - // that the go.mod file matches the source code in the module. - CodeLensTidy CodeLensSource = "tidy" - - // Update dependencies - // - // This codelens source annotates the `module` directive in a - // go.mod file with commands to: - // - // - check for available upgrades, - // - upgrade direct dependencies, and - // - upgrade all dependencies transitively. - CodeLensUpgradeDependency CodeLensSource = "upgrade_dependency" - - // Update vendor directory - // - // This codelens source annotates the `module` directive in a - // go.mod file with a command to run [`go mod - // vendor`](https://go.dev/ref/mod#go-mod-vendor), which - // creates or updates the directory named `vendor` in the - // module root so that it contains an up-to-date copy of all - // necessary package dependencies. - CodeLensVendor CodeLensSource = "vendor" -) diff --git a/gopls/internal/server/code_lens.go b/gopls/internal/server/code_lens.go index 5a720cdc78b..67b359e866c 100644 --- a/gopls/internal/server/code_lens.go +++ b/gopls/internal/server/code_lens.go @@ -15,6 +15,7 @@ import ( "golang.org/x/tools/gopls/internal/label" "golang.org/x/tools/gopls/internal/mod" "golang.org/x/tools/gopls/internal/protocol" + "golang.org/x/tools/gopls/internal/settings" "golang.org/x/tools/internal/event" ) @@ -30,7 +31,7 @@ func (s *server) CodeLens(ctx context.Context, params *protocol.CodeLensParams) } defer release() - var lensFuncs map[protocol.CodeLensSource]cache.CodeLensSourceFunc + var lensFuncs map[settings.CodeLensSource]cache.CodeLensSourceFunc switch snapshot.FileKind(fh) { case file.Mod: lensFuncs = mod.CodeLensSources() diff --git a/gopls/internal/server/general.go b/gopls/internal/server/general.go index 340b27dc023..08b65b1bc84 100644 --- a/gopls/internal/server/general.go +++ b/gopls/internal/server/general.go @@ -468,6 +468,19 @@ func (s *server) newFolder(ctx context.Context, folder protocol.DocumentURI, nam if err != nil { return nil, err } + + // Increment folder counters. + switch { + case env.GOTOOLCHAIN == "auto" || strings.Contains(env.GOTOOLCHAIN, "+auto"): + counter.New("gopls/gotoolchain:auto").Inc() + case env.GOTOOLCHAIN == "path" || strings.Contains(env.GOTOOLCHAIN, "+path"): + counter.New("gopls/gotoolchain:path").Inc() + case env.GOTOOLCHAIN == "local": // local+auto and local+path handled above + counter.New("gopls/gotoolchain:local").Inc() + default: + counter.New("gopls/gotoolchain:other").Inc() + } + return &cache.Folder{ Dir: folder, Name: name, diff --git a/gopls/internal/server/prompt.go b/gopls/internal/server/prompt.go index a1dba8d234d..02d8e87411a 100644 --- a/gopls/internal/server/prompt.go +++ b/gopls/internal/server/prompt.go @@ -12,6 +12,7 @@ import ( "time" "golang.org/x/telemetry" + "golang.org/x/telemetry/counter" "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/internal/event" ) @@ -91,15 +92,6 @@ func (s *server) maybePromptForTelemetry(ctx context.Context, enabled bool) { defer work.End(ctx, "Done.") } - if !enabled { // check this after the work progress message for testing. - return // prompt is disabled - } - - if s.telemetryMode() == "on" || s.telemetryMode() == "off" { - // Telemetry is already on or explicitly off -- nothing to ask about. - return - } - errorf := func(format string, args ...any) { err := fmt.Errorf(format, args...) event.Error(ctx, "telemetry prompt failed", err) @@ -112,12 +104,14 @@ func (s *server) maybePromptForTelemetry(ctx context.Context, enabled bool) { return } + // Read the current prompt file. + var ( promptDir = filepath.Join(configDir, "prompt") // prompt configuration directory promptFile = filepath.Join(promptDir, "telemetry") // telemetry prompt file ) - // prompt states, to be written to the prompt file + // prompt states, stored in the prompt file const ( pYes = "yes" // user said yes pNo = "no" // user said no @@ -138,10 +132,8 @@ func (s *server) maybePromptForTelemetry(ctx context.Context, enabled bool) { ) if content, err := os.ReadFile(promptFile); err == nil { if _, err := fmt.Sscanf(string(content), "%s %d", &state, &attempts); err == nil && validStates[state] { - if state == pYes || state == pNo { - // Prompt has been answered. Nothing to do. - return - } + // successfully parsed! + // ~ v0.16: must have only two fields, state and attempts. } else { state, attempts = "", 0 errorf("malformed prompt result %q", string(content)) @@ -149,12 +141,55 @@ func (s *server) maybePromptForTelemetry(ctx context.Context, enabled bool) { } else if !os.IsNotExist(err) { errorf("reading prompt file: %v", err) // Something went wrong. Since we don't know how many times we've asked the - // prompt, err on the side of not spamming. + // prompt, err on the side of not asking. + // + // But record this in telemetry, in case some users enable telemetry by + // other means. + counter.New("gopls/telemetryprompt/corrupted").Inc() + return + } + + counter.New(fmt.Sprintf("gopls/telemetryprompt/attempts:%d", attempts)).Inc() + + // Check terminal conditions. + + if state == pYes { + // Prompt has been accepted. + // + // We record this counter for every gopls session, rather than when the + // prompt actually accepted below, because if we only recorded it in the + // counter file at the time telemetry is enabled, we'd never upload it, + // because we exclude any counter files that overlap with a time period + // that has telemetry uploading is disabled. + counter.New("gopls/telemetryprompt/accepted").Inc() + return + } + if state == pNo { + // Prompt has been declined. In most cases, this means we'll never see the + // counter below, but it's possible that the user may enable telemetry by + // other means later on. If we see a significant number of users that have + // accepted telemetry but declined the prompt, it may be an indication that + // the prompt is not working well. + counter.New("gopls/telemetryprompt/declined").Inc() + return + } + if attempts >= 5 { // pPending or pFailed + // We've tried asking enough; give up. Record that the prompt expired, in + // case the user decides to enable telemetry by other means later on. + // (see also the pNo case). + counter.New("gopls/telemetryprompt/expired").Inc() return } - if attempts >= 5 { - // We've tried asking enough; give up. + // We only check enabled after (1) the work progress is started, and (2) the + // prompt file has been read. (1) is for testing purposes, and (2) is so that + // we record the "gopls/telemetryprompt/accepted" counter for every session. + if !enabled { + return // prompt is disabled + } + + if s.telemetryMode() == "on" || s.telemetryMode() == "off" { + // Telemetry is already on or explicitly off -- nothing to ask about. return } if attempts == 0 { diff --git a/gopls/internal/settings/default.go b/gopls/internal/settings/default.go index fec64e39d2f..fe7749de1d3 100644 --- a/gopls/internal/settings/default.go +++ b/gopls/internal/settings/default.go @@ -101,14 +101,14 @@ func DefaultOptions(overrides ...func(*Options)) *Options { ExperimentalPostfixCompletions: true, CompleteFunctionCalls: true, }, - Codelenses: map[protocol.CodeLensSource]bool{ - protocol.CodeLensGenerate: true, - protocol.CodeLensRegenerateCgo: true, - protocol.CodeLensTidy: true, - protocol.CodeLensGCDetails: false, - protocol.CodeLensUpgradeDependency: true, - protocol.CodeLensVendor: true, - protocol.CodeLensRunGovulncheck: false, // TODO(hyangah): enable + Codelenses: map[CodeLensSource]bool{ + CodeLensGenerate: true, + CodeLensRegenerateCgo: true, + CodeLensTidy: true, + CodeLensGCDetails: false, + CodeLensUpgradeDependency: true, + CodeLensVendor: true, + CodeLensRunGovulncheck: false, // TODO(hyangah): enable }, }, }, diff --git a/gopls/internal/settings/settings.go b/gopls/internal/settings/settings.go index a5e9518d6a1..3281d206880 100644 --- a/gopls/internal/settings/settings.go +++ b/gopls/internal/settings/settings.go @@ -184,7 +184,7 @@ type UIOptions struct { // ... // } // ``` - Codelenses map[protocol.CodeLensSource]bool + Codelenses map[CodeLensSource]bool // SemanticTokens controls whether the LSP server will send // semantic tokens to the client. @@ -197,6 +197,117 @@ type UIOptions struct { NoSemanticNumber bool `status:"experimental"` } +// A CodeLensSource identifies an (algorithmic) source of code lenses. +type CodeLensSource string + +// CodeLens sources +// +// These identifiers appear in the "codelenses" configuration setting, +// and in the user documentation thereof, which is generated by +// gopls/doc/generate/generate.go parsing this file. +// +// Doc comments should use GitHub Markdown. +// The first line becomes the title. +// +// (For historical reasons, each code lens source identifier typically +// matches the name of one of the command.Commands returned by it, +// but that isn't essential.) +const ( + // Toggle display of Go compiler optimization decisions + // + // This codelens source causes the `package` declaration of + // each file to be annotated with a command to toggle the + // state of the per-session variable that controls whether + // optimization decisions from the Go compiler (formerly known + // as "gc") should be displayed as diagnostics. + // + // Optimization decisions include: + // - whether a variable escapes, and how escape is inferred; + // - whether a nil-pointer check is implied or eliminated; + // - whether a function can be inlined. + // + // TODO(adonovan): this source is off by default because the + // annotation is annoying and because VS Code has a separate + // "Toggle gc details" command. Replace it with a Code Action + // ("Source action..."). + CodeLensGCDetails CodeLensSource = "gc_details" + + // Run `go generate` + // + // This codelens source annotates any `//go:generate` comments + // with commands to run `go generate` in this directory, on + // all directories recursively beneath this one. + // + // See [Generating code](https://go.dev/blog/generate) for + // more details. + CodeLensGenerate CodeLensSource = "generate" + + // Re-generate cgo declarations + // + // This codelens source annotates an `import "C"` declaration + // with a command to re-run the [cgo + // command](https://pkg.go.dev/cmd/cgo) to regenerate the + // corresponding Go declarations. + // + // Use this after editing the C code in comments attached to + // the import, or in C header files included by it. + CodeLensRegenerateCgo CodeLensSource = "regenerate_cgo" + + // Run govulncheck + // + // This codelens source annotates the `module` directive in a + // go.mod file with a command to run Govulncheck. + // + // [Govulncheck](https://go.dev/blog/vuln) is a static + // analysis tool that computes the set of functions reachable + // within your application, including dependencies; + // queries a database of known security vulnerabilities; and + // reports any potential problems it finds. + CodeLensRunGovulncheck CodeLensSource = "run_govulncheck" + + // Run tests and benchmarks + // + // This codelens source annotates each `Test` and `Benchmark` + // function in a `*_test.go` file with a command to run it. + // + // This source is off by default because VS Code has + // a client-side custom UI for testing, and because progress + // notifications are not a great UX for streamed test output. + // See: + // - golang/go#67400 for a discussion of this feature. + // - https://github.com/joaotavora/eglot/discussions/1402 + // for an alternative approach. + CodeLensTest CodeLensSource = "test" + + // Tidy go.mod file + // + // This codelens source annotates the `module` directive in a + // go.mod file with a command to run [`go mod + // tidy`](https://go.dev/ref/mod#go-mod-tidy), which ensures + // that the go.mod file matches the source code in the module. + CodeLensTidy CodeLensSource = "tidy" + + // Update dependencies + // + // This codelens source annotates the `module` directive in a + // go.mod file with commands to: + // + // - check for available upgrades, + // - upgrade direct dependencies, and + // - upgrade all dependencies transitively. + CodeLensUpgradeDependency CodeLensSource = "upgrade_dependency" + + // Update vendor directory + // + // This codelens source annotates the `module` directive in a + // go.mod file with a command to run [`go mod + // vendor`](https://go.dev/ref/mod#go-mod-vendor), which + // creates or updates the directory named `vendor` in the + // module root so that it contains an up-to-date copy of all + // necessary package dependencies. + CodeLensVendor CodeLensSource = "vendor" +) + // Note: CompletionOptions must be comparable with reflect.DeepEqual. type CompletionOptions struct { // Placeholders enables placeholders for function parameters or struct @@ -845,7 +956,7 @@ func (o *Options) set(name string, value any, seen map[string]struct{}) error { ModeVulncheckImports) case "codelenses", "codelens": - lensOverrides, err := asBoolMap[protocol.CodeLensSource](value) + lensOverrides, err := asBoolMap[CodeLensSource](value) if err != nil { return err } diff --git a/gopls/internal/telemetry/telemetry_test.go b/gopls/internal/telemetry/telemetry_test.go index 6c1944b2230..7aaca41ab55 100644 --- a/gopls/internal/telemetry/telemetry_test.go +++ b/gopls/internal/telemetry/telemetry_test.go @@ -26,13 +26,13 @@ import ( ) func TestMain(m *testing.M) { - tmp, err := os.MkdirTemp("", "gopls-telemetry-test") + tmp, err := os.MkdirTemp("", "gopls-telemetry-test-counters") if err != nil { panic(err) } countertest.Open(tmp) code := Main(m) - os.RemoveAll(tmp) + os.RemoveAll(tmp) // golang/go#68243: ignore error; cleanup fails on Windows os.Exit(code) } @@ -54,6 +54,7 @@ func TestTelemetry(t *testing.T) { counter.New("gopls/client:" + editor), counter.New("gopls/goversion:1." + goversion), counter.New("fwd/vscode/linter:a"), + counter.New("gopls/gotoolchain:local"), } initialCounts := make([]uint64, len(sessionCounters)) for i, c := range sessionCounters { @@ -70,6 +71,9 @@ func TestTelemetry(t *testing.T) { Modes(Default), // must be in-process to receive the bug report below Settings{"showBugReports": true}, ClientName("Visual Studio Code"), + EnvVars{ + "GOTOOLCHAIN": "local", // so that the local counter is incremented + }, ).Run(t, "", func(_ *testing.T, env *Env) { goversion = strconv.Itoa(env.GoVersion()) addForwardedCounters(env, []string{"vscode/linter:a"}, []int64{1}) @@ -93,6 +97,7 @@ func TestTelemetry(t *testing.T) { // gopls/editor:client // gopls/goversion:1.x // fwd/vscode/linter:a + // gopls/gotoolchain:local for i, c := range sessionCounters { want := initialCounts[i] + 1 got, err := countertest.ReadCounter(c) diff --git a/gopls/internal/test/integration/codelens/codelens_test.go b/gopls/internal/test/integration/codelens/codelens_test.go index 800856d3f7a..75b9fda1fbf 100644 --- a/gopls/internal/test/integration/codelens/codelens_test.go +++ b/gopls/internal/test/integration/codelens/codelens_test.go @@ -10,6 +10,7 @@ import ( "testing" "golang.org/x/tools/gopls/internal/server" + "golang.org/x/tools/gopls/internal/settings" "golang.org/x/tools/gopls/internal/test/compare" . "golang.org/x/tools/gopls/internal/test/integration" "golang.org/x/tools/gopls/internal/util/bug" @@ -54,7 +55,7 @@ const ( }, { label: "generate disabled", - enabled: map[string]bool{string(protocol.CodeLensGenerate): false}, + enabled: map[string]bool{string(settings.CodeLensGenerate): false}, wantCodeLens: false, }, } diff --git a/gopls/internal/test/integration/completion/completion_test.go b/gopls/internal/test/integration/completion/completion_test.go index 0ca46dabcb3..c96e569f1ad 100644 --- a/gopls/internal/test/integration/completion/completion_test.go +++ b/gopls/internal/test/integration/completion/completion_test.go @@ -121,11 +121,11 @@ package want: nil, }, { - name: "package completion after keyword 'package'", + name: "package completion after package keyword", filename: "fruits/testfile2.go", triggerRegexp: "package()", want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}, - editRegexp: "package\n", + editRegexp: "package", }, { name: "package completion with 'pac' prefix", @@ -164,14 +164,14 @@ package filename: "123f_r.u~its-123/testfile.go", triggerRegexp: "package()", want: []string{"package fruits123", "package fruits123_test", "package main"}, - editRegexp: "package\n", + editRegexp: "package", }, { name: "package completion for invalid dir name", filename: ".invalid-dir@-name/testfile.go", triggerRegexp: "package()", want: []string{"package main"}, - editRegexp: "package\n", + editRegexp: "package", }, } { t.Run(tc.name, func(t *testing.T) { diff --git a/gopls/internal/test/integration/completion/fixedbugs_test.go b/gopls/internal/test/integration/completion/fixedbugs_test.go new file mode 100644 index 00000000000..52695847c4d --- /dev/null +++ b/gopls/internal/test/integration/completion/fixedbugs_test.go @@ -0,0 +1,40 @@ +// 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 completion + +import ( + "testing" + + . "golang.org/x/tools/gopls/internal/test/integration" +) + +func TestPackageCompletionCrash_Issue68169(t *testing.T) { + // This test reproduces the scenario of golang/go#68169, a crash in + // completion.Selection.Suffix. + // + // The file content here is extracted from the issue. + const files = ` +-- go.mod -- +module example.com + +go 1.18 +-- playdos/play.go -- +package +` + + Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("playdos/play.go") + // Previously, this call would crash gopls as it was incorrectly computing + // the surrounding completion suffix. + completions := env.Completion(env.RegexpSearch("playdos/play.go", "package ()")) + if len(completions.Items) == 0 { + t.Fatal("Completion() returned empty results") + } + // Sanity check: we should get package clause compeltion. + if got, want := completions.Items[0].Label, "package playdos"; got != want { + t.Errorf("Completion()[0].Label == %s, want %s", got, want) + } + }) +} diff --git a/gopls/internal/test/integration/fake/editor.go b/gopls/internal/test/integration/fake/editor.go index 487a255da3d..4e6600a8583 100644 --- a/gopls/internal/test/integration/fake/editor.go +++ b/gopls/internal/test/integration/fake/editor.go @@ -200,7 +200,8 @@ func (e *Editor) Exit(ctx context.Context) error { return nil } -// Close issues the shutdown and exit sequence an editor should. +// Close disconnects the LSP client session. +// TODO(rfindley): rename to 'Disconnect'. func (e *Editor) Close(ctx context.Context) error { if err := e.Shutdown(ctx); err != nil { return err @@ -1595,6 +1596,7 @@ func (e *Editor) EditResolveSupport() (bool, error) { } // Hover triggers a hover at the given position in an open buffer. +// It may return (nil, zero) if no symbol was selected. func (e *Editor) Hover(ctx context.Context, loc protocol.Location) (*protocol.MarkupContent, protocol.Location, error) { if err := e.checkBufferLocation(loc); err != nil { return nil, protocol.Location{}, err @@ -1608,7 +1610,7 @@ func (e *Editor) Hover(ctx context.Context, loc protocol.Location) (*protocol.Ma return nil, protocol.Location{}, fmt.Errorf("hover: %w", err) } if resp == nil { - return nil, protocol.Location{}, nil + return nil, protocol.Location{}, nil // e.g. no selected symbol } return &resp.Contents, protocol.Location{URI: loc.URI, Range: resp.Range}, nil } diff --git a/gopls/internal/test/integration/fake/workdir.go b/gopls/internal/test/integration/fake/workdir.go index 977bf5458c5..25b3cb5c557 100644 --- a/gopls/internal/test/integration/fake/workdir.go +++ b/gopls/internal/test/integration/fake/workdir.go @@ -19,6 +19,7 @@ import ( "time" "golang.org/x/tools/gopls/internal/protocol" + "golang.org/x/tools/gopls/internal/util/slices" "golang.org/x/tools/internal/robustio" ) @@ -333,8 +334,7 @@ func (w *Workdir) CheckForFileChanges(ctx context.Context) error { return nil } w.watcherMu.Lock() - watchers := make([]func(context.Context, []protocol.FileEvent), len(w.watchers)) - copy(watchers, w.watchers) + watchers := slices.Clone(w.watchers) w.watcherMu.Unlock() for _, w := range watchers { w(ctx, evts) diff --git a/gopls/internal/test/integration/misc/hover_test.go b/gopls/internal/test/integration/misc/hover_test.go index d3445ddec4a..2d5687ed847 100644 --- a/gopls/internal/test/integration/misc/hover_test.go +++ b/gopls/internal/test/integration/misc/hover_test.go @@ -553,3 +553,53 @@ func main() { }) } } + +func TestHoverInternalLinksIssue68116(t *testing.T) { + // Links for the internal viewer should not include a module version suffix: + // the package path and the view are an unambiguous key; see #68116. + + const proxy = ` +-- example.com@v1.2.3/go.mod -- +module example.com + +go 1.12 + +-- example.com@v1.2.3/a/a.go -- +package a + +// F is a function. +func F() +` + + const mod = ` +-- go.mod -- +module main + +go 1.12 + +require example.com v1.2.3 + +-- main.go -- +package main + +import "example.com/a" + +func main() { + a.F() +} +` + WithOptions( + ProxyFiles(proxy), + Settings{"linksInHover": "gopls"}, + WriteGoSum("."), + ).Run(t, mod, func(t *testing.T, env *Env) { + env.OpenFile("main.go") + got, _ := env.Hover(env.RegexpSearch("main.go", "F")) + const wantRE = "\\[`a.F` in gopls doc viewer\\]\\(http://127.0.0.1:[0-9]+/gopls/[^/]+/pkg/example.com\\?view=[0-9]+#F\\)" // no version + if m, err := regexp.MatchString(wantRE, got.Value); err != nil { + t.Fatalf("bad regexp in test: %v", err) + } else if !m { + t.Fatalf("hover output does not match %q; got:\n\n%s", wantRE, got.Value) + } + }) +} diff --git a/gopls/internal/test/integration/misc/misc_test.go b/gopls/internal/test/integration/misc/misc_test.go index 666887f9f14..ca0125894c8 100644 --- a/gopls/internal/test/integration/misc/misc_test.go +++ b/gopls/internal/test/integration/misc/misc_test.go @@ -9,15 +9,22 @@ import ( "strings" "testing" + "golang.org/x/telemetry/counter/countertest" "golang.org/x/tools/gopls/internal/protocol" - "golang.org/x/tools/gopls/internal/test/integration" . "golang.org/x/tools/gopls/internal/test/integration" "golang.org/x/tools/gopls/internal/util/bug" ) func TestMain(m *testing.M) { bug.PanicOnBugs = true - os.Exit(integration.Main(m)) + tmp, err := os.MkdirTemp("", "gopls-misc-test-counters") + if err != nil { + panic(err) + } + countertest.Open(tmp) + code := Main(m) + os.RemoveAll(tmp) // golang/go#68243: ignore error; cleanup fails on Windows + os.Exit(code) } // TestDocumentURIFix ensures that a DocumentURI supplied by the diff --git a/gopls/internal/test/integration/misc/prompt_test.go b/gopls/internal/test/integration/misc/prompt_test.go index 26c0e9322ac..e4b7277e940 100644 --- a/gopls/internal/test/integration/misc/prompt_test.go +++ b/gopls/internal/test/integration/misc/prompt_test.go @@ -11,6 +11,9 @@ import ( "regexp" "testing" + "github.com/google/go-cmp/cmp" + "golang.org/x/telemetry/counter" + "golang.org/x/telemetry/counter/countertest" "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/protocol/command" "golang.org/x/tools/gopls/internal/server" @@ -69,6 +72,10 @@ func main() { // Test that responding to the telemetry prompt results in the expected state. func TestTelemetryPrompt_Response(t *testing.T) { + if !countertest.SupportedPlatform { + t.Skip("requires counter support") + } + const src = ` -- go.mod -- module mod.com @@ -81,18 +88,63 @@ func main() { } ` + var ( + acceptanceCounter = "gopls/telemetryprompt/accepted" + declinedCounter = "gopls/telemetryprompt/declined" + attempt1Counter = "gopls/telemetryprompt/attempts:1" + allCounters = []string{acceptanceCounter, declinedCounter, attempt1Counter} + ) + + // We must increment counters in order for the initial reads below to + // succeed. + // + // TODO(rfindley): ReadCounter should simply return 0 for uninitialized + // counters. + for _, name := range allCounters { + counter.New(name).Inc() + } + + readCounts := func(t *testing.T) map[string]uint64 { + t.Helper() + counts := make(map[string]uint64) + for _, name := range allCounters { + count, err := countertest.ReadCounter(counter.New(name)) + if err != nil { + t.Fatalf("ReadCounter(%q) failed: %v", name, err) + } + counts[name] = count + } + return counts + } + tests := []struct { - name string // subtest name - response string // response to choose for the telemetry dialog - wantMode string // resulting telemetry mode - wantMsg string // substring contained in the follow-up popup (if empty, no popup is expected) + name string // subtest name + response string // response to choose for the telemetry dialog + wantMode string // resulting telemetry mode + wantMsg string // substring contained in the follow-up popup (if empty, no popup is expected) + wantInc uint64 // expected 'prompt accepted' counter increment + wantCounts map[string]uint64 }{ - {"yes", server.TelemetryYes, "on", "uploading is now enabled"}, - {"no", server.TelemetryNo, "", ""}, - {"empty", "", "", ""}, + {"yes", server.TelemetryYes, "on", "uploading is now enabled", 1, map[string]uint64{ + acceptanceCounter: 1, + declinedCounter: 0, + attempt1Counter: 1, + }}, + {"no", server.TelemetryNo, "", "", 0, map[string]uint64{ + acceptanceCounter: 0, + declinedCounter: 1, + attempt1Counter: 1, + }}, + {"empty", "", "", "", 0, map[string]uint64{ + acceptanceCounter: 0, + declinedCounter: 0, + attempt1Counter: 1, + }}, } + for _, test := range tests { t.Run(test.name, func(t *testing.T) { + initialCounts := readCounts(t) modeFile := filepath.Join(t.TempDir(), "mode") msgRE := regexp.MustCompile(".*Would you like to enable Go telemetry?") respond := func(m *protocol.ShowMessageRequestParams) (*protocol.MessageActionItem, error) { @@ -136,6 +188,23 @@ func main() { if gotMode != test.wantMode { t.Errorf("after prompt, mode=%s, want %s", gotMode, test.wantMode) } + + // We increment the acceptance counter when checking the prompt file + // before prompting, so start a second, transient gopls session and + // verify that the acceptance counter is incremented. + env2 := ConnectGoplsEnv(t, env.Ctx, env.Sandbox, env.Editor.Config(), env.Server) + env2.Await(CompletedWork(server.TelemetryPromptWorkTitle, 1, true)) + if err := env2.Editor.Close(env2.Ctx); err != nil { + t.Errorf("closing second editor: %v", err) + } + + gotCounts := readCounts(t) + for k := range gotCounts { + gotCounts[k] -= initialCounts[k] + } + if diff := cmp.Diff(test.wantCounts, gotCounts); diff != "" { + t.Errorf("counter mismatch (-want +got):\n%s", diff) + } }) }) } diff --git a/gopls/internal/test/integration/misc/shared_test.go b/gopls/internal/test/integration/misc/shared_test.go index b91dde2d282..b0bbcaa030a 100644 --- a/gopls/internal/test/integration/misc/shared_test.go +++ b/gopls/internal/test/integration/misc/shared_test.go @@ -8,7 +8,6 @@ import ( "testing" . "golang.org/x/tools/gopls/internal/test/integration" - "golang.org/x/tools/gopls/internal/test/integration/fake" ) // Smoke test that simultaneous editing sessions in the same workspace works. @@ -32,19 +31,7 @@ func main() { ).Run(t, sharedProgram, func(t *testing.T, env1 *Env) { // Create a second test session connected to the same workspace and server // as the first. - awaiter := NewAwaiter(env1.Sandbox.Workdir) - editor, err := fake.NewEditor(env1.Sandbox, env1.Editor.Config()).Connect(env1.Ctx, env1.Server, awaiter.Hooks()) - if err != nil { - t.Fatal(err) - } - env2 := &Env{ - T: t, - Ctx: env1.Ctx, - Sandbox: env1.Sandbox, - Server: env1.Server, - Editor: editor, - Awaiter: awaiter, - } + env2 := ConnectGoplsEnv(t, env1.Ctx, env1.Sandbox, env1.Editor.Config(), env1.Server) env2.Await(InitialWorkspaceLoad) // In editor #1, break fmt.Println as before. env1.OpenFile("main.go") diff --git a/gopls/internal/test/integration/runner.go b/gopls/internal/test/integration/runner.go index fff5e77300a..7b3b757536f 100644 --- a/gopls/internal/test/integration/runner.go +++ b/gopls/internal/test/integration/runner.go @@ -215,19 +215,7 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio framer = ls.framer(jsonrpc2.NewRawStream) ts := servertest.NewPipeServer(ss, framer) - awaiter := NewAwaiter(sandbox.Workdir) - editor, err := fake.NewEditor(sandbox, config.editor).Connect(ctx, ts, awaiter.Hooks()) - if err != nil { - t.Fatal(err) - } - env := &Env{ - T: t, - Ctx: ctx, - Sandbox: sandbox, - Editor: editor, - Server: ts, - Awaiter: awaiter, - } + env := ConnectGoplsEnv(t, ctx, sandbox, config.editor, ts) defer func() { if t.Failed() && r.PrintGoroutinesOnFailure { pprof.Lookup("goroutine").WriteTo(os.Stderr, 1) @@ -242,7 +230,7 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio // the editor: in general we want to clean up before proceeding to the // next test, and if there is a deadlock preventing closing it will // eventually be handled by the `go test` timeout. - if err := editor.Close(xcontext.Detach(ctx)); err != nil { + if err := env.Editor.Close(xcontext.Detach(ctx)); err != nil { t.Errorf("closing editor: %v", err) } }() @@ -253,6 +241,28 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio } } +// ConnectGoplsEnv creates a new Gopls environment for the given sandbox, +// editor config, and server connector. +// +// TODO(rfindley): significantly refactor the way testing environments are +// constructed. +func ConnectGoplsEnv(t testing.TB, ctx context.Context, sandbox *fake.Sandbox, config fake.EditorConfig, connector servertest.Connector) *Env { + awaiter := NewAwaiter(sandbox.Workdir) + editor, err := fake.NewEditor(sandbox, config).Connect(ctx, connector, awaiter.Hooks()) + if err != nil { + t.Fatal(err) + } + env := &Env{ + T: t, + Ctx: ctx, + Sandbox: sandbox, + Server: connector, + Editor: editor, + Awaiter: awaiter, + } + return env +} + // longBuilders maps builders that are skipped when -short is set to a // (possibly empty) justification. var longBuilders = map[string]string{ diff --git a/gopls/internal/test/integration/wrappers.go b/gopls/internal/test/integration/wrappers.go index 8c5e35e93c0..88145e73209 100644 --- a/gopls/internal/test/integration/wrappers.go +++ b/gopls/internal/test/integration/wrappers.go @@ -233,6 +233,7 @@ func (e *Env) GetQuickFixes(path string, diagnostics []protocol.Diagnostic) []pr } // Hover in the editor, calling t.Fatal on any error. +// It may return (nil, zero) even on success. func (e *Env) Hover(loc protocol.Location) (*protocol.MarkupContent, protocol.Location) { e.T.Helper() c, loc, err := e.Editor.Hover(e.Ctx, loc) diff --git a/gopls/internal/test/marker/testdata/hover/generics.txt b/gopls/internal/test/marker/testdata/hover/generics.txt index 86e2b2ce17b..50ce49bb33f 100644 --- a/gopls/internal/test/marker/testdata/hover/generics.txt +++ b/gopls/internal/test/marker/testdata/hover/generics.txt @@ -3,8 +3,11 @@ This file contains tests for hovering over generic Go code. Requires go1.20+ for the new go/doc/comment package, and a change in Go 1.20 that affected the formatting of constraint interfaces. +Its size expectations assume a 64-bit machine. + -- flags -- -min_go=go1.20 +-skip_goarch=386,arm -- go.mod -- // A go.mod is require for correct pkgsite links. @@ -13,10 +16,28 @@ module mod.com go 1.18 +-- issue68213.go -- +package generics + +// Hovering over an interface with empty type set must not panic. +type empty interface { //@hover("empty", "empty", empty) + int + string +} + +-- @empty -- +```go +type empty interface { // size=16 (0x10) + int + string +} +``` + +Hovering over an interface with empty type set must not panic. -- generics.go -- package generics -type value[T any] struct { //hover("lue", "value", value),hover("T", "T", valueT) +type value[T any] struct { //@hover("lue", "value", value),hover("T", "T", valueT) val T //@hover("T", "T", valuevalT) Q int64 //@hover("Q", "Q", valueQ) } @@ -30,51 +51,44 @@ func F[P interface{ ~int | string }]() { //@hover("P", "P", Ptparam) var _ P //@hover("P","P",Pvar) } --- inferred.go -- -package generics - -func app[S interface{ ~[]E }, E interface{}](s S, e E) S { - return append(s, e) -} - -func _() { - _ = app[[]int] //@hover("app", "app", appint) - _ = app[[]int, int] //@hover("app", "app", appint) - _ = app[[]int]([]int{}, 0) //@hover("app", "app", appint), diag("[[]int]", re"unnecessary") - _ = app([]int{}, 0) //@hover("app", "app", appint) -} - --- @ValueQ -- +-- @value -- ```go -field Q int64 // size=8 +type value[T any] struct { + val T //@hover("T", "T", valuevalT) + Q int64 //@hover("Q", "Q", valueQ) +} ``` - -@hover("Q", "Q", ValueQ) - - -[`(generics.Value).Q` on pkg.go.dev](https://pkg.go.dev/mod.com#Value.Q) --- @ValueT -- +-- @valueT -- ```go type parameter T any ``` --- @ValuevalT -- +-- @valuevalT -- ```go type parameter T any ``` --- @appint -- -```go -func app(s []int, e int) []int // func[S interface{~[]E}, E interface{}](s S, e E) S -``` -- @valueQ -- ```go field Q int64 // size=8 ``` @hover("Q", "Q", valueQ) --- @valuevalT -- +-- @ValueT -- +```go +type parameter T any +``` +-- @ValuevalT -- ```go type parameter T any ``` +-- @ValueQ -- +```go +field Q int64 // size=8 +``` + +@hover("Q", "Q", ValueQ) + + +[`(generics.Value).Q` on pkg.go.dev](https://pkg.go.dev/mod.com#Value.Q) -- @Ptparam -- ```go type parameter P interface{~int | string} @@ -83,3 +97,21 @@ type parameter P interface{~int | string} ```go type parameter P interface{~int | string} ``` +-- inferred.go -- +package generics + +func app[S interface{ ~[]E }, E interface{}](s S, e E) S { + return append(s, e) +} + +func _() { + _ = app[[]int] //@hover("app", "app", appint) + _ = app[[]int, int] //@hover("app", "app", appint) + _ = app[[]int]([]int{}, 0) //@hover("app", "app", appint), diag("[[]int]", re"unnecessary") + _ = app([]int{}, 0) //@hover("app", "app", appint) +} + +-- @appint -- +```go +func app(s []int, e int) []int // func[S interface{~[]E}, E interface{}](s S, e E) S +``` diff --git a/internal/testfiles/testfiles_test.go b/internal/testfiles/testfiles_test.go index d9563260e14..9c6b8f93508 100644 --- a/internal/testfiles/testfiles_test.go +++ b/internal/testfiles/testfiles_test.go @@ -17,6 +17,7 @@ import ( ) func TestTestDir(t *testing.T) { + t.Skip("Disabled for golang.org/cl/603895. Fix and re-enable.") testenv.NeedsGo1Point(t, 22) // TODO(taking): Expose a helper for this pattern? diff --git a/internal/typeparams/free.go b/internal/typeparams/free.go index de3496d10b3..a1d138226c9 100644 --- a/internal/typeparams/free.go +++ b/internal/typeparams/free.go @@ -21,7 +21,6 @@ type Free struct { // Has reports whether the specified type has a free type parameter. func (w *Free) Has(typ types.Type) (res bool) { - // detect cycles if x, ok := w.seen[typ]; ok { return x @@ -83,7 +82,7 @@ func (w *Free) Has(typ types.Type) (res bool) { } terms, err := InterfaceTermSet(t) if err != nil { - panic(err) + return false // ill typed } for _, term := range terms { if w.Has(term.Type()) { diff --git a/internal/versions/types_test.go b/internal/versions/types_test.go index 59f6d18b45f..df3705cc7a9 100644 --- a/internal/versions/types_test.go +++ b/internal/versions/types_test.go @@ -38,7 +38,7 @@ func Test(t *testing.T) { pversion string tests []fileTest }{ - {"", "", []fileTest{{"noversion.go", ""}, {"gobuild.go", ""}}}, + // {"", "", []fileTest{{"noversion.go", ""}, {"gobuild.go", ""}}}, // TODO(matloob): re-enable this test (with modifications) once CL 607955 has been submitted {"go1.22", "go1.22", []fileTest{{"noversion.go", "go1.22"}, {"gobuild.go", "go1.23"}}}, } { name := fmt.Sprintf("types.Config{GoVersion:%q}", item.goversion)