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 <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>

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

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)
 	}
 }

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)
 
-func (r *runner) WorkspaceSymbols(t *testing.T, uri span.URI, query string, typ tests.WorkspaceSymbolsTestType) {
-	r.callWorkspaceSymbols(t, uri, query, typ)
+	// 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) callWorkspaceSymbols(t *testing.T, uri span.URI, query string, typ tests.WorkspaceSymbolsTestType) {
-	t.Helper()
-
+func (r *runner) WorkspaceSymbols(t *testing.T, uri span.URI, query string, typ tests.WorkspaceSymbolsTestType) {
 	matcher := tests.WorkspaceSymbolsTestTypeToMatcher(typ)
 
 	original := r.server.session.Options()

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())
+}

gopls/internal/lsp/protocol/span.go

@@ -148,13 +148,20 @@ 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
 	}
 	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

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) {