From b9adce94b7f1960056e7c2866b1a5b70a237adfe Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 9 May 2022 12:14:36 -0400 Subject: [PATCH] internal/lsp/source: derive document symbols from syntax alone The documentSymbols handler joined syntax information with type information, meaning that it was only able to satisfy requests for files in valid workspace packages. However, the value added by type information was limited, and in many cases could be derived from syntax alone. For example, while generating symbols for a const declaration, we don't need the type checker to tell us that the symbol kind is const. Refactor the documentSymbols handler to derive symbols from syntax alone. This leads to some simplifications from the code, in addition to eliminating the dependency on package data. Also, simplify symbol details to just use types.ExprString, which includes some missing information such as function return values. Also, update handling to support Go 1.18 type embedding in interfaces. Notably, this reverts decisions like golang/go#31202, in which we went to effort to make the symbol kind more accurate. In my opinion (and the opinion expressed in golang/go#52797), the cost of requiring type information is not worth the minor improvement in accuracy of the symbol kind, which (as far as I know) is only used for UI elements. To facilitate testing (and start to clean up the test framework), make several simplifications / improvements to the marker tests: - simplify the collection of symbol data - eliminate unused marks - just use cmp.Diff for comparing results - allow for arbitrary nesting of symbols. - remove unnecessary @symbol annotations from workspace_symbol tests -- their data is not used by workspace_symbol handlers - remove Symbol and WorkspaceSymbol handlers from source_test.go. On inspection, these handlers were redundant with lsp_test.go. Notably, the collection and assembly of @symbol annotations is still way too complicated. It would be much simpler to just have a single golden file summarizing the entire output, rather than weaving it together from annotations. However, I realized this too late, and so it will have to wait for a separate CL. Fixes golang/go#52797 Fixes golang/vscode-go#2242 Updates golang/go#54845 Change-Id: I3a2c2d39f59f9d045a6cedf8023ff0c80a69d974 Reviewed-on: https://go-review.googlesource.com/c/tools/+/405254 gopls-CI: kokoro TryBot-Result: Gopher Robot Reviewed-by: Hyang-Ah Hana Kim Run-TryBot: Robert Findley Reviewed-by: Alan Donovan --- gopls/internal/lsp/cache/parse.go | 27 +- gopls/internal/lsp/cmd/test/symbols.go | 5 +- gopls/internal/lsp/lsp_test.go | 30 +- gopls/internal/lsp/lsppos/token.go | 7 + gopls/internal/lsp/protocol/span.go | 7 + gopls/internal/lsp/source/source_test.go | 39 +-- gopls/internal/lsp/source/symbols.go | 294 ++++++++---------- gopls/internal/lsp/source/util.go | 61 +++- .../internal/lsp/testdata/summary.txt.golden | 2 +- .../lsp/testdata/summary_go1.18.txt.golden | 2 +- gopls/internal/lsp/testdata/symbols/go1.18.go | 16 + .../lsp/testdata/symbols/go1.18.go.golden | 7 + gopls/internal/lsp/testdata/symbols/main.go | 85 +++-- .../lsp/testdata/symbols/main.go.golden | 63 ++-- .../lsp/testdata/workspacesymbol/a/a.go | 6 +- .../testdata/workspacesymbol/a/a.go.golden | 5 - .../lsp/testdata/workspacesymbol/a/a_test.go | 2 +- .../workspacesymbol/a/a_test.go.golden | 3 - .../testdata/workspacesymbol/a/a_x_test.go | 2 +- .../workspacesymbol/a/a_x_test.go.golden | 3 - .../lsp/testdata/workspacesymbol/b/b.go | 6 +- .../testdata/workspacesymbol/b/b.go.golden | 5 - gopls/internal/lsp/tests/tests.go | 104 ++++--- gopls/internal/lsp/tests/util.go | 49 +-- 24 files changed, 415 insertions(+), 415 deletions(-) create mode 100644 gopls/internal/lsp/testdata/symbols/go1.18.go create mode 100644 gopls/internal/lsp/testdata/symbols/go1.18.go.golden delete mode 100644 gopls/internal/lsp/testdata/workspacesymbol/a/a.go.golden delete mode 100644 gopls/internal/lsp/testdata/workspacesymbol/a/a_test.go.golden delete mode 100644 gopls/internal/lsp/testdata/workspacesymbol/a/a_x_test.go.golden delete mode 100644 gopls/internal/lsp/testdata/workspacesymbol/b/b.go.golden diff --git a/gopls/internal/lsp/cache/parse.go b/gopls/internal/lsp/cache/parse.go index 5cb5ef7203..981e698fb6 100644 --- a/gopls/internal/lsp/cache/parse.go +++ b/gopls/internal/lsp/cache/parse.go @@ -18,13 +18,13 @@ import ( "strconv" "strings" - "golang.org/x/tools/internal/event" - "golang.org/x/tools/internal/event/tag" - "golang.org/x/tools/internal/diff" - "golang.org/x/tools/internal/diff/myers" "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/lsp/safetoken" "golang.org/x/tools/gopls/internal/lsp/source" + "golang.org/x/tools/internal/diff" + "golang.org/x/tools/internal/diff/myers" + "golang.org/x/tools/internal/event" + "golang.org/x/tools/internal/event/tag" "golang.org/x/tools/internal/memoize" ) @@ -237,7 +237,7 @@ func (f *unexportedFilter) keep(ident *ast.Ident) bool { func (f *unexportedFilter) filterDecl(decl ast.Decl) bool { switch decl := decl.(type) { case *ast.FuncDecl: - if ident := recvIdent(decl); ident != nil && !f.keep(ident) { + if ident := source.RecvIdent(decl.Recv); ident != nil && !f.keep(ident) { return false } return f.keep(decl.Name) @@ -312,7 +312,7 @@ func (f *unexportedFilter) recordUses(file *ast.File) { switch decl := decl.(type) { case *ast.FuncDecl: // Ignore methods on dropped types. - if ident := recvIdent(decl); ident != nil && !f.keep(ident) { + if ident := source.RecvIdent(decl.Recv); ident != nil && !f.keep(ident) { break } // Ignore functions with dropped names. @@ -356,21 +356,6 @@ func (f *unexportedFilter) recordUses(file *ast.File) { } } -// recvIdent returns the identifier of a method receiver, e.g. *int. -func recvIdent(decl *ast.FuncDecl) *ast.Ident { - if decl.Recv == nil || len(decl.Recv.List) == 0 { - return nil - } - x := decl.Recv.List[0].Type - if star, ok := x.(*ast.StarExpr); ok { - x = star.X - } - if ident, ok := x.(*ast.Ident); ok { - return ident - } - return nil -} - // recordIdents records unexported identifiers in an Expr in uses. // These may be types, e.g. in map[key]value, function names, e.g. in foo(), // or simple variable references. References that will be discarded, such diff --git a/gopls/internal/lsp/cmd/test/symbols.go b/gopls/internal/lsp/cmd/test/symbols.go index fbaf679336..3bd2fc0224 100644 --- a/gopls/internal/lsp/cmd/test/symbols.go +++ b/gopls/internal/lsp/cmd/test/symbols.go @@ -8,6 +8,7 @@ import ( "testing" "golang.org/x/tools/gopls/internal/lsp/protocol" + "golang.org/x/tools/gopls/internal/lsp/tests/compare" "golang.org/x/tools/internal/span" ) @@ -17,7 +18,7 @@ func (r *runner) Symbols(t *testing.T, uri span.URI, expectedSymbols []protocol. expect := string(r.data.Golden(t, "symbols", filename, func() ([]byte, error) { return []byte(got), nil })) - if expect != got { - t.Errorf("symbols failed for %s expected:\n%s\ngot:\n%s", filename, expect, got) + if diff := compare.Text(expect, got); diff != "" { + t.Errorf("symbols differ from expected:\n%s", diff) } } diff --git a/gopls/internal/lsp/lsp_test.go b/gopls/internal/lsp/lsp_test.go index 1cb5b3abb4..02b5727336 100644 --- a/gopls/internal/lsp/lsp_test.go +++ b/gopls/internal/lsp/lsp_test.go @@ -15,6 +15,8 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "golang.org/x/tools/gopls/internal/lsp/cache" "golang.org/x/tools/gopls/internal/lsp/command" "golang.org/x/tools/gopls/internal/lsp/protocol" @@ -1147,10 +1149,7 @@ func (r *runner) Symbols(t *testing.T, uri span.URI, expectedSymbols []protocol. if err != nil { t.Fatal(err) } - if len(got) != len(expectedSymbols) { - t.Errorf("want %d top-level symbols in %v, got %d", len(expectedSymbols), uri, len(got)) - return - } + symbols := make([]protocol.DocumentSymbol, len(got)) for i, s := range got { s, ok := s.(protocol.DocumentSymbol) @@ -1159,18 +1158,25 @@ func (r *runner) Symbols(t *testing.T, uri span.URI, expectedSymbols []protocol. } symbols[i] = s } - if diff := tests.DiffSymbols(t, uri, expectedSymbols, symbols); diff != "" { - t.Error(diff) + + // Sort by position to make it easier to find errors. + sortSymbols := func(s []protocol.DocumentSymbol) { + sort.Slice(s, func(i, j int) bool { + return protocol.CompareRange(s[i].SelectionRange, s[j].SelectionRange) < 0 + }) + } + sortSymbols(expectedSymbols) + sortSymbols(symbols) + + // Ignore 'Range' here as it is difficult (impossible?) to express + // multi-line ranges in the packagestest framework. + ignoreRange := cmpopts.IgnoreFields(protocol.DocumentSymbol{}, "Range") + if diff := cmp.Diff(expectedSymbols, symbols, ignoreRange); diff != "" { + t.Errorf("mismatching symbols (-want +got)\n%s", diff) } } func (r *runner) WorkspaceSymbols(t *testing.T, uri span.URI, query string, typ tests.WorkspaceSymbolsTestType) { - r.callWorkspaceSymbols(t, uri, query, typ) -} - -func (r *runner) callWorkspaceSymbols(t *testing.T, uri span.URI, query string, typ tests.WorkspaceSymbolsTestType) { - t.Helper() - matcher := tests.WorkspaceSymbolsTestTypeToMatcher(typ) original := r.server.session.Options() diff --git a/gopls/internal/lsp/lsppos/token.go b/gopls/internal/lsp/lsppos/token.go index 2a16ba283e..c1b726fe24 100644 --- a/gopls/internal/lsp/lsppos/token.go +++ b/gopls/internal/lsp/lsppos/token.go @@ -6,6 +6,7 @@ package lsppos import ( "errors" + "go/ast" "go/token" "golang.org/x/tools/gopls/internal/lsp/protocol" @@ -58,3 +59,9 @@ func (m *TokenMapper) Range(start, end token.Pos) (protocol.Range, error) { return protocol.Range{Start: startPos, End: endPos}, nil } + +// NodeRange returns the protocol range corresponding to the span of the given +// node. +func (m *TokenMapper) NodeRange(n ast.Node) (protocol.Range, error) { + return m.Range(n.Pos(), n.End()) +} diff --git a/gopls/internal/lsp/protocol/span.go b/gopls/internal/lsp/protocol/span.go index 744746d353..23721ee703 100644 --- a/gopls/internal/lsp/protocol/span.go +++ b/gopls/internal/lsp/protocol/span.go @@ -148,6 +148,11 @@ func IsPoint(r Range) bool { return r.Start.Line == r.End.Line && r.Start.Character == r.End.Character } +// CompareRange returns -1 if a is before b, 0 if a == b, and 1 if a is after +// b. +// +// A range a is defined to be 'before' b if a.Start is before b.Start, or +// a.Start == b.Start and a.End is before b.End. func CompareRange(a, b Range) int { if r := ComparePosition(a.Start, b.Start); r != 0 { return r @@ -155,6 +160,8 @@ func CompareRange(a, b Range) int { return ComparePosition(a.End, b.End) } +// ComparePosition returns -1 if a is before b, 0 if a == b, and 1 if a is +// after b. func ComparePosition(a, b Position) int { if a.Line < b.Line { return -1 diff --git a/gopls/internal/lsp/source/source_test.go b/gopls/internal/lsp/source/source_test.go index ce2cc0aa36..ad2ca6b72a 100644 --- a/gopls/internal/lsp/source/source_test.go +++ b/gopls/internal/lsp/source/source_test.go @@ -879,46 +879,11 @@ func (r *runner) PrepareRename(t *testing.T, src span.Span, want *source.Prepare } func (r *runner) Symbols(t *testing.T, uri span.URI, expectedSymbols []protocol.DocumentSymbol) { - fh, err := r.snapshot.GetFile(r.ctx, uri) - if err != nil { - t.Fatal(err) - } - symbols, err := source.DocumentSymbols(r.ctx, r.snapshot, fh) - if err != nil { - t.Errorf("symbols failed for %s: %v", uri, err) - } - if len(symbols) != len(expectedSymbols) { - t.Errorf("want %d top-level symbols in %v, got %d", len(expectedSymbols), uri, len(symbols)) - return - } - if diff := tests.DiffSymbols(t, uri, expectedSymbols, symbols); diff != "" { - t.Error(diff) - } + // Removed in favor of just using the lsp_test implementation. See ../lsp_test.go } func (r *runner) WorkspaceSymbols(t *testing.T, uri span.URI, query string, typ tests.WorkspaceSymbolsTestType) { - r.callWorkspaceSymbols(t, uri, query, typ) -} - -func (r *runner) callWorkspaceSymbols(t *testing.T, uri span.URI, query string, typ tests.WorkspaceSymbolsTestType) { - t.Helper() - - matcher := tests.WorkspaceSymbolsTestTypeToMatcher(typ) - gotSymbols, err := source.WorkspaceSymbols(r.ctx, matcher, r.view.Options().SymbolStyle, []source.View{r.view}, query) - if err != nil { - t.Fatal(err) - } - got, err := tests.WorkspaceSymbolsString(r.ctx, r.data, uri, gotSymbols) - if err != nil { - t.Fatal(err) - } - got = filepath.ToSlash(tests.Normalize(got, r.normalizers)) - want := string(r.data.Golden(t, fmt.Sprintf("workspace_symbol-%s-%s", strings.ToLower(string(matcher)), query), uri.Filename(), func() ([]byte, error) { - return []byte(got), nil - })) - if d := compare.Text(want, got); d != "" { - t.Error(d) - } + // Removed in favor of just using the lsp_test implementation. See ../lsp_test.go } func (r *runner) SignatureHelp(t *testing.T, spn span.Span, want *protocol.SignatureHelp) { diff --git a/gopls/internal/lsp/source/symbols.go b/gopls/internal/lsp/source/symbols.go index 802de37c34..2bab241e1d 100644 --- a/gopls/internal/lsp/source/symbols.go +++ b/gopls/internal/lsp/source/symbols.go @@ -8,25 +8,34 @@ import ( "context" "fmt" "go/ast" + "go/token" "go/types" - "golang.org/x/tools/internal/event" + "golang.org/x/tools/gopls/internal/lsp/lsppos" "golang.org/x/tools/gopls/internal/lsp/protocol" + "golang.org/x/tools/internal/event" ) func DocumentSymbols(ctx context.Context, snapshot Snapshot, fh FileHandle) ([]protocol.DocumentSymbol, error) { ctx, done := event.Start(ctx, "source.DocumentSymbols") defer done() - pkg, pgf, err := GetParsedFile(ctx, snapshot, fh, NarrowestPackage) + content, err := fh.Read() + if err != nil { + return nil, err + } + + pgf, err := snapshot.ParseGo(ctx, fh, ParseFull) if err != nil { return nil, fmt.Errorf("getting file for DocumentSymbols: %w", err) } - info := pkg.GetTypesInfo() - q := Qualifier(pgf.File, pkg.GetTypes(), info) + m := lsppos.NewTokenMapper(content, pgf.Tok) - symbolsToReceiver := make(map[types.Type]int) + // Build symbols for file declarations. When encountering a declaration with + // errors (typically because positions are invalid), we skip the declaration + // entirely. VS Code fails to show any symbols if one of the top-level + // symbols is missing position information. var symbols []protocol.DocumentSymbol for _, decl := range pgf.File.Decls { switch decl := decl.(type) { @@ -34,15 +43,11 @@ func DocumentSymbols(ctx context.Context, snapshot Snapshot, fh FileHandle) ([]p if decl.Name.Name == "_" { continue } - if obj := info.ObjectOf(decl.Name); obj != nil { - fs, err := funcSymbol(snapshot, pkg, decl, obj, q) - if err != nil { - return nil, err - } + fs, err := funcSymbol(m, decl) + if err == nil { // If function is a method, prepend the type of the method. - if fs.Kind == protocol.Method { - rtype := obj.Type().(*types.Signature).Recv().Type() - fs.Name = fmt.Sprintf("(%s).%s", types.TypeString(rtype, q), fs.Name) + if decl.Recv != nil && len(decl.Recv.List) > 0 { + fs.Name = fmt.Sprintf("(%s).%s", types.ExprString(decl.Recv.List[0].Type), fs.Name) } symbols = append(symbols, fs) } @@ -53,24 +58,17 @@ func DocumentSymbols(ctx context.Context, snapshot Snapshot, fh FileHandle) ([]p if spec.Name.Name == "_" { continue } - if obj := info.ObjectOf(spec.Name); obj != nil { - ts, err := typeSymbol(snapshot, pkg, info, spec, obj, q) - if err != nil { - return nil, err - } + ts, err := typeSymbol(m, spec) + if err == nil { symbols = append(symbols, ts) - symbolsToReceiver[obj.Type()] = len(symbols) - 1 } case *ast.ValueSpec: for _, name := range spec.Names { if name.Name == "_" { continue } - if obj := info.ObjectOf(name); obj != nil { - vs, err := varSymbol(snapshot, pkg, decl, name, obj, q) - if err != nil { - return nil, err - } + vs, err := varSymbol(m, spec, name, decl.Tok == token.CONST) + if err == nil { symbols = append(symbols, vs) } } @@ -81,185 +79,157 @@ func DocumentSymbols(ctx context.Context, snapshot Snapshot, fh FileHandle) ([]p return symbols, nil } -func funcSymbol(snapshot Snapshot, pkg Package, decl *ast.FuncDecl, obj types.Object, q types.Qualifier) (protocol.DocumentSymbol, error) { +func funcSymbol(m *lsppos.TokenMapper, decl *ast.FuncDecl) (protocol.DocumentSymbol, error) { s := protocol.DocumentSymbol{ - Name: obj.Name(), + Name: decl.Name.Name, Kind: protocol.Function, } + if decl.Recv != nil { + s.Kind = protocol.Method + } var err error - s.Range, err = nodeToProtocolRange(snapshot, pkg, decl) + s.Range, err = m.Range(decl.Pos(), decl.End()) if err != nil { return protocol.DocumentSymbol{}, err } - s.SelectionRange, err = nodeToProtocolRange(snapshot, pkg, decl.Name) + s.SelectionRange, err = m.Range(decl.Name.Pos(), decl.Name.End()) if err != nil { return protocol.DocumentSymbol{}, err } - sig, _ := obj.Type().(*types.Signature) - if sig != nil { - if sig.Recv() != nil { - s.Kind = protocol.Method - } - s.Detail += "(" - for i := 0; i < sig.Params().Len(); i++ { - if i > 0 { - s.Detail += ", " - } - param := sig.Params().At(i) - label := types.TypeString(param.Type(), q) - if param.Name() != "" { - label = fmt.Sprintf("%s %s", param.Name(), label) - } - s.Detail += label - } - s.Detail += ")" - } + s.Detail = types.ExprString(decl.Type) return s, nil } -func typeSymbol(snapshot Snapshot, pkg Package, info *types.Info, spec *ast.TypeSpec, obj types.Object, qf types.Qualifier) (protocol.DocumentSymbol, error) { +func typeSymbol(m *lsppos.TokenMapper, spec *ast.TypeSpec) (protocol.DocumentSymbol, error) { s := protocol.DocumentSymbol{ - Name: obj.Name(), + Name: spec.Name.Name, } - s.Detail, _ = FormatType(obj.Type(), qf) - s.Kind = typeToKind(obj.Type()) - var err error - s.Range, err = nodeToProtocolRange(snapshot, pkg, spec) + s.Range, err = m.NodeRange(spec) if err != nil { return protocol.DocumentSymbol{}, err } - s.SelectionRange, err = nodeToProtocolRange(snapshot, pkg, spec.Name) + s.SelectionRange, err = m.NodeRange(spec.Name) if err != nil { return protocol.DocumentSymbol{}, err } - t, objIsStruct := obj.Type().Underlying().(*types.Struct) - st, specIsStruct := spec.Type.(*ast.StructType) - if objIsStruct && specIsStruct { - for i := 0; i < t.NumFields(); i++ { - f := t.Field(i) - child := protocol.DocumentSymbol{ - Name: f.Name(), - Kind: protocol.Field, - } - child.Detail, _ = FormatType(f.Type(), qf) - - spanNode, selectionNode := nodesForStructField(i, st) - if span, err := nodeToProtocolRange(snapshot, pkg, spanNode); err == nil { - child.Range = span - } - if span, err := nodeToProtocolRange(snapshot, pkg, selectionNode); err == nil { - child.SelectionRange = span - } - s.Children = append(s.Children, child) - } - } - - ti, objIsInterface := obj.Type().Underlying().(*types.Interface) - ai, specIsInterface := spec.Type.(*ast.InterfaceType) - if objIsInterface && specIsInterface { - for i := 0; i < ti.NumExplicitMethods(); i++ { - method := ti.ExplicitMethod(i) - child := protocol.DocumentSymbol{ - Name: method.Name(), - Kind: protocol.Method, - } - - var spanNode, selectionNode ast.Node - Methods: - for _, f := range ai.Methods.List { - for _, id := range f.Names { - if id.Name == method.Name() { - spanNode, selectionNode = f, id - break Methods - } - } - } - child.Range, err = nodeToProtocolRange(snapshot, pkg, spanNode) - if err != nil { - return protocol.DocumentSymbol{}, err - } - child.SelectionRange, err = nodeToProtocolRange(snapshot, pkg, selectionNode) - if err != nil { - return protocol.DocumentSymbol{}, err - } - s.Children = append(s.Children, child) - } - - for i := 0; i < ti.NumEmbeddeds(); i++ { - embedded := ti.EmbeddedType(i) - nt, isNamed := embedded.(*types.Named) - if !isNamed { - continue - } - - child := protocol.DocumentSymbol{ - Name: types.TypeString(embedded, qf), - } - child.Kind = typeToKind(embedded) - var spanNode, selectionNode ast.Node - Embeddeds: - for _, f := range ai.Methods.List { - if len(f.Names) > 0 { - continue - } - - if t := info.TypeOf(f.Type); types.Identical(nt, t) { - spanNode, selectionNode = f, f.Type - break Embeddeds - } - } - child.Range, err = nodeToProtocolRange(snapshot, pkg, spanNode) - if err != nil { - return protocol.DocumentSymbol{}, err - } - child.SelectionRange, err = nodeToProtocolRange(snapshot, pkg, selectionNode) - if err != nil { - return protocol.DocumentSymbol{}, err - } - s.Children = append(s.Children, child) - } - } + s.Kind, s.Detail, s.Children = typeDetails(m, spec.Type) return s, nil } -func nodesForStructField(i int, st *ast.StructType) (span, selection ast.Node) { - j := 0 - for _, field := range st.Fields.List { - if len(field.Names) == 0 { - if i == j { - return field, field.Type - } - j++ - continue +func typeDetails(m *lsppos.TokenMapper, typExpr ast.Expr) (kind protocol.SymbolKind, detail string, children []protocol.DocumentSymbol) { + switch typExpr := typExpr.(type) { + case *ast.StructType: + kind = protocol.Struct + children = fieldListSymbols(m, typExpr.Fields, protocol.Field) + if len(children) > 0 { + detail = "struct{...}" + } else { + detail = "struct{}" } - for _, name := range field.Names { - if i == j { - return field, name - } - j++ + + // Find interface methods and embedded types. + case *ast.InterfaceType: + kind = protocol.Interface + children = fieldListSymbols(m, typExpr.Methods, protocol.Method) + if len(children) > 0 { + detail = "interface{...}" + } else { + detail = "interface{}" } + + case *ast.FuncType: + kind = protocol.Function + detail = types.ExprString(typExpr) + + default: + kind = protocol.Class // catch-all, for cases where we don't know the kind syntactically + detail = types.ExprString(typExpr) } - return nil, nil + return } -func varSymbol(snapshot Snapshot, pkg Package, decl ast.Node, name *ast.Ident, obj types.Object, q types.Qualifier) (protocol.DocumentSymbol, error) { +func fieldListSymbols(m *lsppos.TokenMapper, fields *ast.FieldList, fieldKind protocol.SymbolKind) []protocol.DocumentSymbol { + if fields == nil { + return nil + } + + var symbols []protocol.DocumentSymbol + for _, field := range fields.List { + detail, children := "", []protocol.DocumentSymbol(nil) + if field.Type != nil { + _, detail, children = typeDetails(m, field.Type) + } + if len(field.Names) == 0 { // embedded interface or struct field + // By default, use the formatted type details as the name of this field. + // This handles potentially invalid syntax, as well as type embeddings in + // interfaces. + child := protocol.DocumentSymbol{ + Name: detail, + Kind: protocol.Field, // consider all embeddings to be fields + Children: children, + } + + // If the field is a valid embedding, promote the type name to field + // name. + selection := field.Type + if id := embeddedIdent(field.Type); id != nil { + child.Name = id.Name + child.Detail = detail + selection = id + } + + if rng, err := m.NodeRange(field.Type); err == nil { + child.Range = rng + } + if rng, err := m.NodeRange(selection); err == nil { + child.SelectionRange = rng + } + + symbols = append(symbols, child) + } else { + for _, name := range field.Names { + child := protocol.DocumentSymbol{ + Name: name.Name, + Kind: fieldKind, + Detail: detail, + Children: children, + } + + if rng, err := m.NodeRange(field); err == nil { + child.Range = rng + } + if rng, err := m.NodeRange(name); err == nil { + child.SelectionRange = rng + } + + symbols = append(symbols, child) + } + } + + } + return symbols +} + +func varSymbol(m *lsppos.TokenMapper, spec *ast.ValueSpec, name *ast.Ident, isConst bool) (protocol.DocumentSymbol, error) { s := protocol.DocumentSymbol{ - Name: obj.Name(), + Name: name.Name, Kind: protocol.Variable, } - if _, ok := obj.(*types.Const); ok { + if isConst { s.Kind = protocol.Constant } var err error - s.Range, err = nodeToProtocolRange(snapshot, pkg, decl) + s.Range, err = m.NodeRange(spec) if err != nil { return protocol.DocumentSymbol{}, err } - s.SelectionRange, err = nodeToProtocolRange(snapshot, pkg, name) + s.SelectionRange, err = m.NodeRange(name) if err != nil { return protocol.DocumentSymbol{}, err } - s.Detail = types.TypeString(obj.Type(), q) + if spec.Type != nil { // type may be missing from the syntax + _, s.Detail, s.Children = typeDetails(m, spec.Type) + } return s, nil } diff --git a/gopls/internal/lsp/source/util.go b/gopls/internal/lsp/source/util.go index da9c3b5d43..d1c90b65d5 100644 --- a/gopls/internal/lsp/source/util.go +++ b/gopls/internal/lsp/source/util.go @@ -18,9 +18,10 @@ import ( "strings" "golang.org/x/mod/modfile" - "golang.org/x/tools/internal/bug" "golang.org/x/tools/gopls/internal/lsp/protocol" + "golang.org/x/tools/internal/bug" "golang.org/x/tools/internal/span" + "golang.org/x/tools/internal/typeparams" ) // MappedRange provides mapped protocol.Range for a span.Range, accounting for @@ -122,16 +123,6 @@ func IsGenerated(ctx context.Context, snapshot Snapshot, uri span.URI) bool { return false } -func nodeToProtocolRange(snapshot Snapshot, pkg Package, n ast.Node) (protocol.Range, error) { - mrng, err := posToMappedRange(snapshot, pkg, n.Pos(), n.End()) - if err != nil { - return protocol.Range{}, err - } - return mrng.Range() -} - -// objToMappedRange returns the MappedRange for the object's declaring -// identifier (or string literal, for an import). func objToMappedRange(snapshot Snapshot, pkg Package, obj types.Object) (MappedRange, error) { nameLen := len(obj.Name()) if pkgName, ok := obj.(*types.PkgName); ok { @@ -589,3 +580,51 @@ func ByteOffsetsToRange(m *protocol.ColumnMapper, uri span.URI, start, end int) e := span.NewPoint(line, col, end) return m.Range(span.New(uri, s, e)) } + +// RecvIdent returns the type identifier of a method receiver. +// e.g. A for all of A, *A, A[T], *A[T], etc. +func RecvIdent(recv *ast.FieldList) *ast.Ident { + if recv == nil || len(recv.List) == 0 { + return nil + } + x := recv.List[0].Type + if star, ok := x.(*ast.StarExpr); ok { + x = star.X + } + switch ix := x.(type) { // check for instantiated receivers + case *ast.IndexExpr: + x = ix.X + case *typeparams.IndexListExpr: + x = ix.X + } + if ident, ok := x.(*ast.Ident); ok { + return ident + } + return nil +} + +// embeddedIdent returns the type name identifier for an embedding x, if x in a +// valid embedding. Otherwise, it returns nil. +// +// Spec: An embedded field must be specified as a type name T or as a pointer +// to a non-interface type name *T +func embeddedIdent(x ast.Expr) *ast.Ident { + if star, ok := x.(*ast.StarExpr); ok { + x = star.X + } + switch ix := x.(type) { // check for instantiated receivers + case *ast.IndexExpr: + x = ix.X + case *typeparams.IndexListExpr: + x = ix.X + } + switch x := x.(type) { + case *ast.Ident: + return x + case *ast.SelectorExpr: + if _, ok := x.X.(*ast.Ident); ok { + return x.Sel + } + } + return nil +} diff --git a/gopls/internal/lsp/testdata/summary.txt.golden b/gopls/internal/lsp/testdata/summary.txt.golden index c8e10d15d6..107e900a73 100644 --- a/gopls/internal/lsp/testdata/summary.txt.golden +++ b/gopls/internal/lsp/testdata/summary.txt.golden @@ -23,7 +23,7 @@ InlayHintsCount = 4 ReferencesCount = 27 RenamesCount = 41 PrepareRenamesCount = 7 -SymbolsCount = 5 +SymbolsCount = 1 WorkspaceSymbolsCount = 20 SignaturesCount = 33 LinksCount = 7 diff --git a/gopls/internal/lsp/testdata/summary_go1.18.txt.golden b/gopls/internal/lsp/testdata/summary_go1.18.txt.golden index c3ac0089f3..93d9c63819 100644 --- a/gopls/internal/lsp/testdata/summary_go1.18.txt.golden +++ b/gopls/internal/lsp/testdata/summary_go1.18.txt.golden @@ -23,7 +23,7 @@ InlayHintsCount = 5 ReferencesCount = 27 RenamesCount = 48 PrepareRenamesCount = 7 -SymbolsCount = 5 +SymbolsCount = 2 WorkspaceSymbolsCount = 20 SignaturesCount = 33 LinksCount = 7 diff --git a/gopls/internal/lsp/testdata/symbols/go1.18.go b/gopls/internal/lsp/testdata/symbols/go1.18.go new file mode 100644 index 0000000000..cdf99dc20f --- /dev/null +++ b/gopls/internal/lsp/testdata/symbols/go1.18.go @@ -0,0 +1,16 @@ +//go:build go1.18 +// +build go1.18 + +package main + +type T[P any] struct { //@symbol("T", "T", "Struct", "struct{...}", "T", "") + F P //@symbol("F", "F", "Field", "P", "", "T") +} + +type Constraint interface { //@symbol("Constraint", "Constraint", "Interface", "interface{...}", "Constraint", "") + ~int | struct{ int } //@symbol("~int | struct{int}", "~int | struct{ int }", "Field", "", "", "Constraint") + + // TODO(rfindley): the selection range below is the entire interface field. + // Can we reduce it? + interface{ M() } //@symbol("interface{...}", "interface{ M() }", "Field", "", "iFaceField", "Constraint"), symbol("M", "M", "Method", "func()", "", "iFaceField") +} diff --git a/gopls/internal/lsp/testdata/symbols/go1.18.go.golden b/gopls/internal/lsp/testdata/symbols/go1.18.go.golden new file mode 100644 index 0000000000..5a0c1a94d7 --- /dev/null +++ b/gopls/internal/lsp/testdata/symbols/go1.18.go.golden @@ -0,0 +1,7 @@ +-- symbols -- +T Struct 6:6-6:7 + F Field 7:2-7:3 +Constraint Interface 10:6-10:16 + interface{...} Field 15:2-15:18 + ~int | struct{int} Field 11:2-11:22 + diff --git a/gopls/internal/lsp/testdata/symbols/main.go b/gopls/internal/lsp/testdata/symbols/main.go index 8111250f34..65e0869fd5 100644 --- a/gopls/internal/lsp/testdata/symbols/main.go +++ b/gopls/internal/lsp/testdata/symbols/main.go @@ -4,61 +4,88 @@ import ( "io" ) +// Each symbol marker in this file defines the following information: +// symbol(name, selectionSpan, kind, detail, id, parentID) +// - name: DocumentSymbol.Name +// - selectionSpan: DocumentSymbol.SelectionRange +// - kind: DocumentSymbol.Kind +// - detail: DocumentSymbol.Detail +// - id: if non-empty, a unique identifier for this symbol +// - parentID: if non-empty, the id of the parent of this symbol +// +// This data in aggregate defines a set of document symbols and their +// parent-child relationships, which is compared against the DocummentSymbols +// response from gopls for the current file. +// +// TODO(rfindley): the symbol annotations here are complicated and difficult to +// maintain. It would be simpler to just write out the full expected response +// in the golden file, perhaps as raw JSON. + var _ = 1 -var x = 42 //@mark(symbolsx, "x"), symbol("x", "x", "Variable", "", "main.x") +var x = 42 //@symbol("x", "x", "Variable", "", "", "") -const y = 43 //@symbol("y", "y", "Constant", "", "main.y") +var nested struct { //@symbol("nested", "nested", "Variable", "struct{...}", "nested", "") + nestedField struct { //@symbol("nestedField", "nestedField", "Field", "struct{...}", "nestedField", "nested") + f int //@symbol("f", "f", "Field", "int", "", "nestedField") + } +} -type Number int //@symbol("Number", "Number", "Number", "", "main.Number") +const y = 43 //@symbol("y", "y", "Constant", "", "", "") -type Alias = string //@symbol("Alias", "Alias", "String", "", "main.Alias") +type Number int //@symbol("Number", "Number", "Class", "int", "", "") -type NumberAlias = Number //@symbol("NumberAlias", "NumberAlias", "Number", "", "main.NumberAlias") +type Alias = string //@symbol("Alias", "Alias", "Class", "string", "", "") + +type NumberAlias = Number //@symbol("NumberAlias", "NumberAlias", "Class", "Number", "", "") type ( - Boolean bool //@symbol("Boolean", "Boolean", "Boolean", "", "main.Boolean") - BoolAlias = bool //@symbol("BoolAlias", "BoolAlias", "Boolean", "", "main.BoolAlias") + Boolean bool //@symbol("Boolean", "Boolean", "Class", "bool", "", "") + BoolAlias = bool //@symbol("BoolAlias", "BoolAlias", "Class", "bool", "", "") ) -type Foo struct { //@mark(symbolsFoo, "Foo"), symbol("Foo", "Foo", "Struct", "", "main.Foo") - Quux //@mark(fQuux, "Quux"), symbol("Quux", "Quux", "Field", "Foo", "main.Foo.Quux") - W io.Writer //@symbol("W" , "W", "Field", "Foo", "main.Foo.W") - Bar int //@mark(fBar, "Bar"), symbol("Bar", "Bar", "Field", "Foo", "main.Foo.Bar") - baz string //@symbol("baz", "baz", "Field", "Foo", "main.Foo.baz") +type Foo struct { //@symbol("Foo", "Foo", "Struct", "struct{...}", "Foo", "") + Quux //@symbol("Quux", "Quux", "Field", "Quux", "", "Foo") + W io.Writer //@symbol("W", "W", "Field", "io.Writer", "", "Foo") + Bar int //@symbol("Bar", "Bar", "Field", "int", "", "Foo") + baz string //@symbol("baz", "baz", "Field", "string", "", "Foo") + funcField func(int) int //@symbol("funcField", "funcField", "Field", "func(int) int", "", "Foo") } -type Quux struct { //@symbol("Quux", "Quux", "Struct", "", "main.Quux") - X, Y float64 //@mark(qX, "X"), symbol("X", "X", "Field", "Quux", "main.X"), symbol("Y", "Y", "Field", "Quux", "main.Y") +type Quux struct { //@symbol("Quux", "Quux", "Struct", "struct{...}", "Quux", "") + X, Y float64 //@symbol("X", "X", "Field", "float64", "", "Quux"), symbol("Y", "Y", "Field", "float64", "", "Quux") } -func (f Foo) Baz() string { //@symbol("(Foo).Baz", "Baz", "Method", "", "main.Foo.Baz") +type EmptyStruct struct{} //@symbol("EmptyStruct", "EmptyStruct", "Struct", "struct{}", "", "") + +func (f Foo) Baz() string { //@symbol("(Foo).Baz", "Baz", "Method", "func() string", "", "") return f.baz } func _() {} -func (q *Quux) Do() {} //@mark(qDo, "Do"), symbol("(*Quux).Do", "Do", "Method", "", "main.Quux.Do") - -func main() { //@symbol("main", "main", "Function", "", "main.main") +func (q *Quux) Do() {} //@symbol("(*Quux).Do", "Do", "Method", "func()", "", "") +func main() { //@symbol("main", "main", "Function", "func()", "", "") } -type Stringer interface { //@symbol("Stringer", "Stringer", "Interface", "", "main.Stringer") - String() string //@symbol("String", "String", "Method", "Stringer", "main.Stringer.String") +type Stringer interface { //@symbol("Stringer", "Stringer", "Interface", "interface{...}", "Stringer", "") + String() string //@symbol("String", "String", "Method", "func() string", "", "Stringer") } -type ABer interface { //@mark(ABerInterface, "ABer"), symbol("ABer", "ABer", "Interface", "", "main.ABer") - B() //@symbol("B", "B", "Method", "ABer", "main.ABer.B") - A() string //@mark(ABerA, "A"), symbol("A", "A", "Method", "ABer", "main.ABer.A") +type ABer interface { //@symbol("ABer", "ABer", "Interface", "interface{...}", "ABer", "") + B() //@symbol("B", "B", "Method", "func()", "", "ABer") + A() string //@symbol("A", "A", "Method", "func() string", "", "ABer") } -type WithEmbeddeds interface { //@symbol("WithEmbeddeds", "WithEmbeddeds", "Interface", "", "main.WithEmbeddeds") - Do() //@symbol("Do", "Do", "Method", "WithEmbeddeds", "main.WithEmbeddeds.Do") - ABer //@symbol("ABer", "ABer", "Interface", "WithEmbeddeds", "main.WithEmbeddeds.ABer") - io.Writer //@mark(ioWriter, "io.Writer"), symbol("io.Writer", "io.Writer", "Interface", "WithEmbeddeds", "main.WithEmbeddeds.Writer") +type WithEmbeddeds interface { //@symbol("WithEmbeddeds", "WithEmbeddeds", "Interface", "interface{...}", "WithEmbeddeds", "") + Do() //@symbol("Do", "Do", "Method", "func()", "", "WithEmbeddeds") + ABer //@symbol("ABer", "ABer", "Field", "ABer", "", "WithEmbeddeds") + io.Writer //@symbol("Writer", "Writer", "Field", "io.Writer", "", "WithEmbeddeds") } -func Dunk() int { return 0 } //@symbol("Dunk", "Dunk", "Function", "", "main.Dunk") +type EmptyInterface interface{} //@symbol("EmptyInterface", "EmptyInterface", "Interface", "interface{}", "", "") -func dunk() {} //@symbol("dunk", "dunk", "Function", "", "main.dunk") +func Dunk() int { return 0 } //@symbol("Dunk", "Dunk", "Function", "func() int", "", "") + +func dunk() {} //@symbol("dunk", "dunk", "Function", "func()", "", "") diff --git a/gopls/internal/lsp/testdata/symbols/main.go.golden b/gopls/internal/lsp/testdata/symbols/main.go.golden index ebb6a8a5dd..98009b02d6 100644 --- a/gopls/internal/lsp/testdata/symbols/main.go.golden +++ b/gopls/internal/lsp/testdata/symbols/main.go.golden @@ -1,31 +1,36 @@ -- symbols -- -x Variable 9:5-9:6 -y Constant 11:7-11:8 -Number Number 13:6-13:12 -Alias String 15:6-15:11 -NumberAlias Number 17:6-17:17 -Boolean Boolean 20:2-20:9 -BoolAlias Boolean 21:2-21:11 -Foo Struct 24:6-24:9 - Bar Field 27:2-27:5 - Quux Field 25:2-25:6 - W Field 26:2-26:3 - baz Field 28:2-28:5 -Quux Struct 31:6-31:10 - X Field 32:2-32:3 - Y Field 32:5-32:6 -(Foo).Baz Method 35:14-35:17 -(*Quux).Do Method 41:16-41:18 -main Function 43:6-43:10 -Stringer Interface 47:6-47:14 - String Method 48:2-48:8 -ABer Interface 51:6-51:10 - A Method 53:2-53:3 - B Method 52:2-52:3 -WithEmbeddeds Interface 56:6-56:19 - ABer Interface 58:2-58:6 - Do Method 57:2-57:4 - io.Writer Interface 59:2-59:11 -Dunk Function 62:6-62:10 -dunk Function 64:6-64:10 +x Variable 26:5-26:6 +nested Variable 28:5-28:11 + nestedField Field 29:2-29:13 +y Constant 34:7-34:8 +Number Class 36:6-36:12 +Alias Class 38:6-38:11 +NumberAlias Class 40:6-40:17 +Boolean Class 43:2-43:9 +BoolAlias Class 44:2-44:11 +Foo Struct 47:6-47:9 + Bar Field 50:2-50:5 + Quux Field 48:2-48:6 + W Field 49:2-49:3 + baz Field 51:2-51:5 + funcField Field 52:2-52:11 +Quux Struct 55:6-55:10 + X Field 56:2-56:3 + Y Field 56:5-56:6 +EmptyStruct Struct 59:6-59:17 +(Foo).Baz Method 61:14-61:17 +(*Quux).Do Method 67:16-67:18 +main Function 69:6-69:10 +Stringer Interface 72:6-72:14 + String Method 73:2-73:8 +ABer Interface 76:6-76:10 + A Method 78:2-78:3 + B Method 77:2-77:3 +WithEmbeddeds Interface 81:6-81:19 + ABer Field 83:2-83:6 + Do Method 82:2-82:4 + Writer Field 84:5-84:11 +EmptyInterface Interface 87:6-87:20 +Dunk Function 89:6-89:10 +dunk Function 91:6-91:10 diff --git a/gopls/internal/lsp/testdata/workspacesymbol/a/a.go b/gopls/internal/lsp/testdata/workspacesymbol/a/a.go index 6e5a68b16f..4ae9997a03 100644 --- a/gopls/internal/lsp/testdata/workspacesymbol/a/a.go +++ b/gopls/internal/lsp/testdata/workspacesymbol/a/a.go @@ -1,9 +1,9 @@ package a -var RandomGopherVariableA = "a" //@symbol("RandomGopherVariableA", "RandomGopherVariableA", "Variable", "", "a.RandomGopherVariableA") +var RandomGopherVariableA = "a" -const RandomGopherConstantA = "a" //@symbol("RandomGopherConstantA", "RandomGopherConstantA", "Constant", "", "a.RandomGopherConstantA") +const RandomGopherConstantA = "a" const ( - randomgopherinvariable = iota //@symbol("randomgopherinvariable", "randomgopherinvariable", "Constant", "", "a.randomgopherinvariable") + randomgopherinvariable = iota ) diff --git a/gopls/internal/lsp/testdata/workspacesymbol/a/a.go.golden b/gopls/internal/lsp/testdata/workspacesymbol/a/a.go.golden deleted file mode 100644 index c3f088577b..0000000000 --- a/gopls/internal/lsp/testdata/workspacesymbol/a/a.go.golden +++ /dev/null @@ -1,5 +0,0 @@ --- symbols -- -RandomGopherVariableA Variable 3:5-3:26 -RandomGopherConstantA Constant 5:7-5:28 -randomgopherinvariable Constant 8:2-8:24 - diff --git a/gopls/internal/lsp/testdata/workspacesymbol/a/a_test.go b/gopls/internal/lsp/testdata/workspacesymbol/a/a_test.go index 30d5340970..0d97c50d62 100644 --- a/gopls/internal/lsp/testdata/workspacesymbol/a/a_test.go +++ b/gopls/internal/lsp/testdata/workspacesymbol/a/a_test.go @@ -1,3 +1,3 @@ package a -var RandomGopherTestVariableA = "a" //@symbol("RandomGopherTestVariableA", "RandomGopherTestVariableA", "Variable", "", "a.RandomGopherTestVariableA") +var RandomGopherTestVariableA = "a" diff --git a/gopls/internal/lsp/testdata/workspacesymbol/a/a_test.go.golden b/gopls/internal/lsp/testdata/workspacesymbol/a/a_test.go.golden deleted file mode 100644 index af74619439..0000000000 --- a/gopls/internal/lsp/testdata/workspacesymbol/a/a_test.go.golden +++ /dev/null @@ -1,3 +0,0 @@ --- symbols -- -RandomGopherTestVariableA Variable 3:5-3:30 - diff --git a/gopls/internal/lsp/testdata/workspacesymbol/a/a_x_test.go b/gopls/internal/lsp/testdata/workspacesymbol/a/a_x_test.go index 76eb8487d8..747cd17ecc 100644 --- a/gopls/internal/lsp/testdata/workspacesymbol/a/a_x_test.go +++ b/gopls/internal/lsp/testdata/workspacesymbol/a/a_x_test.go @@ -1,3 +1,3 @@ package a_test -var RandomGopherXTestVariableA = "a" //@symbol("RandomGopherXTestVariableA", "RandomGopherXTestVariableA", "Variable", "", "a_test.RandomGopherXTestVariableA") +var RandomGopherXTestVariableA = "a" diff --git a/gopls/internal/lsp/testdata/workspacesymbol/a/a_x_test.go.golden b/gopls/internal/lsp/testdata/workspacesymbol/a/a_x_test.go.golden deleted file mode 100644 index dfd02a5c44..0000000000 --- a/gopls/internal/lsp/testdata/workspacesymbol/a/a_x_test.go.golden +++ /dev/null @@ -1,3 +0,0 @@ --- symbols -- -RandomGopherXTestVariableA Variable 3:5-3:31 - diff --git a/gopls/internal/lsp/testdata/workspacesymbol/b/b.go b/gopls/internal/lsp/testdata/workspacesymbol/b/b.go index 89ce0d92e0..b2e2092eed 100644 --- a/gopls/internal/lsp/testdata/workspacesymbol/b/b.go +++ b/gopls/internal/lsp/testdata/workspacesymbol/b/b.go @@ -1,7 +1,7 @@ package b -var RandomGopherVariableB = "b" //@symbol("RandomGopherVariableB", "RandomGopherVariableB", "Variable", "", "b.RandomGopherVariableB") +var RandomGopherVariableB = "b" -type RandomGopherStructB struct { //@symbol("RandomGopherStructB", "RandomGopherStructB", "Struct", "", "b.RandomGopherStructB") - Bar int //@mark(bBar, "Bar"), symbol("Bar", "Bar", "Field", "RandomGopherStructB", "b.RandomGopherStructB.Bar") +type RandomGopherStructB struct { + Bar int } diff --git a/gopls/internal/lsp/testdata/workspacesymbol/b/b.go.golden b/gopls/internal/lsp/testdata/workspacesymbol/b/b.go.golden deleted file mode 100644 index 4711c9d91a..0000000000 --- a/gopls/internal/lsp/testdata/workspacesymbol/b/b.go.golden +++ /dev/null @@ -1,5 +0,0 @@ --- symbols -- -RandomGopherVariableB Variable 3:5-3:26 -RandomGopherStructB Struct 5:6-5:25 - Bar Field 6:2-6:5 - diff --git a/gopls/internal/lsp/tests/tests.go b/gopls/internal/lsp/tests/tests.go index cfce843a97..f953a8faf0 100644 --- a/gopls/internal/lsp/tests/tests.go +++ b/gopls/internal/lsp/tests/tests.go @@ -86,8 +86,7 @@ type Highlights = map[span.Span][]span.Span type References = map[span.Span][]span.Span type Renames = map[span.Span]string type PrepareRenames = map[span.Span]*source.PrepareItem -type Symbols = map[span.URI][]protocol.DocumentSymbol -type SymbolsChildren = map[string][]protocol.DocumentSymbol +type Symbols = map[span.URI][]*symbol type SymbolInformation = map[span.Span]protocol.SymbolInformation type InlayHints = []span.Span type WorkspaceSymbols = map[WorkspaceSymbolsTestType]map[span.URI][]string @@ -125,8 +124,6 @@ type Data struct { InlayHints InlayHints PrepareRenames PrepareRenames Symbols Symbols - symbolsChildren SymbolsChildren - symbolInformation SymbolInformation WorkspaceSymbols WorkspaceSymbols Signatures Signatures Links Links @@ -250,6 +247,12 @@ type SuggestedFix struct { ActionKind, Title string } +// A symbol holds a DocumentSymbol along with its parent-child edge. +type symbol struct { + pSymbol protocol.DocumentSymbol + id, parentID string +} + type Golden struct { Filename string Archive *txtar.Archive @@ -328,8 +331,6 @@ func load(t testing.TB, mode string, dir string) *Data { FunctionExtractions: make(FunctionExtractions), MethodExtractions: make(MethodExtractions), Symbols: make(Symbols), - symbolsChildren: make(SymbolsChildren), - symbolInformation: make(SymbolInformation), WorkspaceSymbols: make(WorkspaceSymbols), Signatures: make(Signatures), Links: make(Links), @@ -504,12 +505,7 @@ func load(t testing.TB, mode string, dir string) *Data { }); err != nil { t.Fatal(err) } - for _, symbols := range datum.Symbols { - for i := range symbols { - children := datum.symbolsChildren[symbols[i].Name] - symbols[i].Children = children - } - } + // Collect names for the entries that require golden files. if err := datum.Exported.Expect(map[string]interface{}{ "godef": datum.collectDefinitionNames, @@ -847,10 +843,44 @@ func Run(t *testing.T, tests Tests, data *Data) { t.Run("Symbols", func(t *testing.T) { t.Helper() - for uri, expectedSymbols := range data.Symbols { + for uri, allSymbols := range data.Symbols { + byParent := make(map[string][]*symbol) + for _, sym := range allSymbols { + if sym.parentID != "" { + byParent[sym.parentID] = append(byParent[sym.parentID], sym) + } + } + + // collectChildren does a depth-first traversal of the symbol tree, + // computing children of child nodes before returning to their parent. + // This is necessary as the Children field is slice of non-pointer types, + // and therefore we need to be careful to mutate children first before + // assigning them to their parent. + var collectChildren func(id string) []protocol.DocumentSymbol + collectChildren = func(id string) []protocol.DocumentSymbol { + children := byParent[id] + // delete from byParent before recursing, to ensure that + // collectChildren terminates even in the presence of cycles. + delete(byParent, id) + var result []protocol.DocumentSymbol + for _, child := range children { + child.pSymbol.Children = collectChildren(child.id) + result = append(result, child.pSymbol) + } + return result + } + + var topLevel []protocol.DocumentSymbol + for _, sym := range allSymbols { + if sym.parentID == "" { + sym.pSymbol.Children = collectChildren(sym.id) + topLevel = append(topLevel, sym.pSymbol) + } + } + t.Run(uriName(uri), func(t *testing.T) { t.Helper() - tests.Symbols(t, uri, expectedSymbols) + tests.Symbols(t, uri, topLevel) }) } }) @@ -1356,36 +1386,32 @@ func (data *Data) collectPrepareRenames(src span.Span, rng span.Range, placehold } // collectSymbols is responsible for collecting @symbol annotations. -func (data *Data) collectSymbols(name string, spn span.Span, kind string, parentName string, siName string) { +func (data *Data) collectSymbols(name string, selectionRng span.Span, kind, detail, id, parentID string) { + // We don't set 'Range' here as it is difficult (impossible?) to express + // multi-line ranges in the packagestest framework. + uri := selectionRng.URI() + data.Symbols[uri] = append(data.Symbols[uri], &symbol{ + pSymbol: protocol.DocumentSymbol{ + Name: name, + Kind: protocol.ParseSymbolKind(kind), + SelectionRange: data.mustRange(selectionRng), + Detail: detail, + }, + id: id, + parentID: parentID, + }) +} + +// mustRange converts spn into a protocol.Range, calling t.Fatal on any error. +func (data *Data) mustRange(spn span.Span) protocol.Range { m, err := data.Mapper(spn.URI()) - if err != nil { - data.t.Fatal(err) - } rng, err := m.Range(spn) if err != nil { + // TODO(rfindley): this can probably just be a panic, at which point we + // don't need to close over t. data.t.Fatal(err) } - sym := protocol.DocumentSymbol{ - Name: name, - Kind: protocol.ParseSymbolKind(kind), - SelectionRange: rng, - } - if parentName == "" { - data.Symbols[spn.URI()] = append(data.Symbols[spn.URI()], sym) - } else { - data.symbolsChildren[parentName] = append(data.symbolsChildren[parentName], sym) - } - - // Reuse @symbol in the workspace symbols tests. - si := protocol.SymbolInformation{ - Name: siName, - Kind: sym.Kind, - Location: protocol.Location{ - URI: protocol.URIFromSpanURI(spn.URI()), - Range: sym.SelectionRange, - }, - } - data.symbolInformation[spn] = si + return rng } func (data *Data) collectWorkspaceSymbols(typ WorkspaceSymbolsTestType) func(*expect.Note, string) { diff --git a/gopls/internal/lsp/tests/util.go b/gopls/internal/lsp/tests/util.go index d697f18423..8a08b044b5 100644 --- a/gopls/internal/lsp/tests/util.go +++ b/gopls/internal/lsp/tests/util.go @@ -14,13 +14,12 @@ import ( "sort" "strconv" "strings" - "testing" - "golang.org/x/tools/internal/diff" - "golang.org/x/tools/internal/diff/myers" "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/lsp/source" "golang.org/x/tools/gopls/internal/lsp/source/completion" + "golang.org/x/tools/internal/diff" + "golang.org/x/tools/internal/diff/myers" "golang.org/x/tools/internal/span" ) @@ -66,50 +65,6 @@ func DiffLinks(mapper *protocol.ColumnMapper, wantLinks []Link, gotLinks []proto return "" } -// DiffSymbols prints the diff between expected and actual symbols test results. -func DiffSymbols(t *testing.T, uri span.URI, want, got []protocol.DocumentSymbol) string { - sort.Slice(want, func(i, j int) bool { return want[i].Name < want[j].Name }) - sort.Slice(got, func(i, j int) bool { return got[i].Name < got[j].Name }) - if len(got) != len(want) { - return summarizeSymbols(-1, want, got, "different lengths got %v want %v", len(got), len(want)) - } - for i, w := range want { - g := got[i] - if w.Name != g.Name { - return summarizeSymbols(i, want, got, "incorrect name got %v want %v", g.Name, w.Name) - } - if w.Kind != g.Kind { - return summarizeSymbols(i, want, got, "incorrect kind got %v want %v", g.Kind, w.Kind) - } - if protocol.CompareRange(w.SelectionRange, g.SelectionRange) != 0 { - return summarizeSymbols(i, want, got, "incorrect span got %v want %v", g.SelectionRange, w.SelectionRange) - } - if msg := DiffSymbols(t, uri, w.Children, g.Children); msg != "" { - return fmt.Sprintf("children of %s: %s", w.Name, msg) - } - } - return "" -} - -func summarizeSymbols(i int, want, got []protocol.DocumentSymbol, reason string, args ...interface{}) string { - msg := &bytes.Buffer{} - fmt.Fprint(msg, "document symbols failed") - if i >= 0 { - fmt.Fprintf(msg, " at %d", i) - } - fmt.Fprint(msg, " because of ") - fmt.Fprintf(msg, reason, args...) - fmt.Fprint(msg, ":\nexpected:\n") - for _, s := range want { - fmt.Fprintf(msg, " %v %v %v\n", s.Name, s.Kind, s.SelectionRange) - } - fmt.Fprintf(msg, "got:\n") - for _, s := range got { - fmt.Fprintf(msg, " %v %v %v\n", s.Name, s.Kind, s.SelectionRange) - } - return msg.String() -} - // DiffDiagnostics prints the diff between expected and actual diagnostics test // results. If the sole expectation is "no_diagnostics", the check is suppressed. // The Message field of each want element must be a regular expression.