gopls/internal/lsp/source/typerefs: move test into _test.go

This change moves logic needed only by the test
into the test package. (I got quite confused trying
to understand the algorithm before I realized this code
is never used in the product.)

Also:
- move Decode trace logic into test.
- expose PackageSet.AddDeclaringPackage to test.
- rename Symbol.PackageID(PackageIndex) to
  PackageIndex.DeclaringPackage(Symbol) to match.
- use receiver name 'index' consistently.

Fixes golang/go#60598

Change-Id: Icb88bde030a18bcab71d4cb74e0004fe27c815f4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/502536
Run-TryBot: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

gopls/internal/lsp/cache/check.go

@@ -884,15 +884,15 @@ func (b *packageHandleBuilder) getTransitiveRefsLocked(id PackageID) map[string]
 	for name := range ph.refs {
 		if token.IsExported(name) {
 			pkgs := b.s.pkgIndex.NewSet()
-			for _, node := range ph.refs[name] {
+			for _, sym := range ph.refs[name] {
 				// TODO: opt: avoid int -> PackageID -> int conversions here.
-				id := node.PackageID(b.s.pkgIndex)
+				id := b.s.pkgIndex.DeclaringPackage(sym)
 				pkgs.Add(id)
 				otherRefs := b.getTransitiveRefsLocked(id)
 				if otherRefs == nil {
 					return nil // a predecessor failed: exit early
 				}
-				if otherSet, ok := otherRefs[node.Name]; ok {
+				if otherSet, ok := otherRefs[sym.Name]; ok {
 					pkgs.Union(otherSet)
 				}
 			}

gopls/internal/lsp/source/typerefs/packageset.go

@@ -34,25 +34,25 @@ func NewPackageIndex() *PackageIndex {
 
 // idx returns the packageIdx referencing id, creating one if id is not yet
 // tracked by the receiver.
-func (r *PackageIndex) idx(id source.PackageID) int {
-	r.mu.Lock()
-	defer r.mu.Unlock()
-	if i, ok := r.m[id]; ok {
+func (index *PackageIndex) idx(id source.PackageID) int {
+	index.mu.Lock()
+	defer index.mu.Unlock()
+	if i, ok := index.m[id]; ok {
 		return i
 	}
-	i := len(r.ids)
-	r.m[id] = i
-	r.ids = append(r.ids, id)
+	i := len(index.ids)
+	index.m[id] = i
+	index.ids = append(index.ids, id)
 	return i
 }
 
 // id returns the PackageID for idx.
 //
 // idx must have been created by this PackageIndex instance.
-func (r *PackageIndex) id(idx int) source.PackageID {
-	r.mu.Lock()
-	defer r.mu.Unlock()
-	return r.ids[idx]
+func (index *PackageIndex) id(idx int) source.PackageID {
+	index.mu.Lock()
+	defer index.mu.Unlock()
+	return index.ids[idx]
 }
 
 // A PackageSet is a set of source.PackageIDs, optimized for inuse memory
@@ -70,18 +70,27 @@ const blockSize = bits.UintSize
 //
 // PackageSets may only be combined with other PackageSets from the same
 // instance.
-func (s *PackageIndex) NewSet() *PackageSet {
+func (index *PackageIndex) NewSet() *PackageSet {
 	return &PackageSet{
-		parent: s,
+		parent: index,
 		sparse: make(map[int]blockType),
 	}
 }
 
+// DeclaringPackage returns the ID of the symbol's declaring package.
+// The package index must be the one used during decoding.
+func (index *PackageIndex) DeclaringPackage(sym Symbol) source.PackageID {
+	return index.id(sym.pkgIdx)
+}
+
 // Add records a new element in the package set.
 func (s *PackageSet) Add(id source.PackageID) {
 	s.add(s.parent.idx(id))
 }
 
+// AddDeclaringPackage adds sym's declaring package to the set.
+func (s *PackageSet) AddDeclaringPackage(sym Symbol) { s.add(sym.pkgIdx) }
+
 func (s *PackageSet) add(idx int) {
 	i := int(idx)
 	s.sparse[i/blockSize] |= 1 << (i % blockSize)

gopls/internal/lsp/source/typerefs/pkgrefs.go renamed to gopls/internal/lsp/source/typerefs/pkggraph_test.go

@@ -2,15 +2,23 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package typerefs
+package typerefs_test
+
+// This file is logically part of the test in pkgrefs_test.go: that
+// file defines the test assertion logic; this file provides a
+// reference implementation of a client of the typerefs package.
 
 import (
+	"bytes"
 	"context"
+	"fmt"
+	"os"
 	"runtime"
 	"sync"
 
 	"golang.org/x/sync/errgroup"
 	"golang.org/x/tools/gopls/internal/lsp/source"
+	"golang.org/x/tools/gopls/internal/lsp/source/typerefs"
 	"golang.org/x/tools/gopls/internal/span"
 )
 
@@ -19,9 +27,6 @@ const (
 	//
 	// Warning: produces a lot of output! Best to run with small package queries.
 	trace = false
-
-	// debug enables additional assertions.
-	debug = false
 )
 
 // A Package holds reference information for a single package.
@@ -32,18 +37,18 @@ type Package struct {
 	// transitiveRefs records, for each exported declaration in the package, the
 	// transitive set of packages within the containing graph that are
 	// transitively reachable through references, starting with the given decl.
-	transitiveRefs map[string]*PackageSet
+	transitiveRefs map[string]*typerefs.PackageSet
 
 	// ReachesViaDeps records the set of packages in the containing graph whose
 	// syntax may affect the current package's types. See the package
 	// documentation for more details of what this means.
-	ReachesByDeps *PackageSet
+	ReachesByDeps *typerefs.PackageSet
 }
 
 // A PackageGraph represents a fully analyzed graph of packages and their
 // dependencies.
 type PackageGraph struct {
-	pkgIndex *PackageIndex
+	pkgIndex *typerefs.PackageIndex
 	meta     source.MetadataSource
 	parse    func(context.Context, span.URI) (*source.ParsedGoFile, error)
 
@@ -63,7 +68,7 @@ type PackageGraph struct {
 // algorithm.
 func BuildPackageGraph(ctx context.Context, meta source.MetadataSource, ids []source.PackageID, parse func(context.Context, span.URI) (*source.ParsedGoFile, error)) (*PackageGraph, error) {
 	g := &PackageGraph{
-		pkgIndex: NewPackageIndex(),
+		pkgIndex: typerefs.NewPackageIndex(),
 		meta:     meta,
 		parse:    parse,
 		packages: make(map[source.PackageID]*futurePackage),
@@ -120,7 +125,7 @@ func (g *PackageGraph) Package(ctx context.Context, id source.PackageID) (*Packa
 func (g *PackageGraph) buildPackage(ctx context.Context, id source.PackageID) (*Package, error) {
 	p := &Package{
 		metadata:       g.meta.Metadata(id),
-		transitiveRefs: make(map[string]*PackageSet),
+		transitiveRefs: make(map[string]*typerefs.PackageSet),
 	}
 	var files []*source.ParsedGoFile
 	for _, filename := range p.metadata.CompiledGoFiles {
@@ -138,9 +143,10 @@ func (g *PackageGraph) buildPackage(ctx context.Context, id source.PackageID) (*
 	}
 
 	// Compute the symbol-level dependencies through this package.
-	// TODO(adonovan): opt: persist this in the filecache, keyed
+	data := typerefs.Encode(files, id, imports)
+
+	// data can be persisted in a filecache, keyed
 	// by hash(id, CompiledGoFiles, imports).
-	data := Encode(files, id, imports)
 
 	//      This point separates the local preprocessing
 	//  --  of a single package (above) from the global   --
@@ -150,9 +156,33 @@ func (g *PackageGraph) buildPackage(ctx context.Context, id source.PackageID) (*
 	// package and declarations in this package or another
 	// package. See the package documentation for a detailed
 	// description of what these edges do (and do not) represent.
-	classes := Decode(g.pkgIndex, id, data)
-
-	idx := g.pkgIndex.idx(id)
+	classes := typerefs.Decode(g.pkgIndex, id, data)
+
+	// Debug
+	if trace && len(classes) > 0 {
+		var buf bytes.Buffer
+		fmt.Fprintf(&buf, "%s\n", id)
+		for _, class := range classes {
+			for i, name := range class.Decls {
+				if i == 0 {
+					fmt.Fprintf(&buf, "\t")
+				}
+				fmt.Fprintf(&buf, " .%s", name)
+			}
+			// Group symbols by package.
+			var prevID PackageID
+			for _, sym := range class.Refs {
+				id := g.pkgIndex.DeclaringPackage(sym)
+				if id != prevID {
+					prevID = id
+					fmt.Fprintf(&buf, "\n\t\t-> %s:", id)
+				}
+				fmt.Fprintf(&buf, " .%s", sym.Name)
+			}
+			fmt.Fprintln(&buf)
+		}
+		os.Stderr.Write(buf.Bytes())
+	}
 
 	// Now compute the transitive closure of packages reachable
 	// from any exported symbol of this package.
@@ -164,8 +194,10 @@ func (g *PackageGraph) buildPackage(ctx context.Context, id source.PackageID) (*
 		// when the package id changes.
 		depP := p
 		for _, sym := range class.Refs {
-			assert(sym.pkgIdx != idx, "intra-package edge")
-			symPkgID := g.pkgIndex.id(sym.pkgIdx)
+			symPkgID := g.pkgIndex.DeclaringPackage(sym)
+			if symPkgID == id {
+				panic("intra-package edge")
+			}
 			if depP.metadata.ID != symPkgID {
 				// package changed
 				var err error
@@ -174,7 +206,7 @@ func (g *PackageGraph) buildPackage(ctx context.Context, id source.PackageID) (*
 					return nil, err
 				}
 			}
-			set.add(sym.pkgIdx)
+			set.AddDeclaringPackage(sym)
 			set.Union(depP.transitiveRefs[sym.Name])
 		}
 		for _, name := range class.Decls {
@@ -195,7 +227,7 @@ func (g *PackageGraph) buildPackage(ctx context.Context, id source.PackageID) (*
 
 // reachesByDeps computes the set of packages that are reachable through
 // dependencies of the package m.
-func (g *PackageGraph) reachesByDeps(ctx context.Context, m *source.Metadata) (*PackageSet, error) {
+func (g *PackageGraph) reachesByDeps(ctx context.Context, m *source.Metadata) (*typerefs.PackageSet, error) {
 	transitive := g.pkgIndex.NewSet()
 	for _, depID := range m.DepsByPkgPath {
 		dep, err := g.Package(ctx, depID)

gopls/internal/lsp/source/typerefs/pkgrefs_test.go

@@ -23,7 +23,6 @@ import (
 	"golang.org/x/tools/gopls/internal/astutil"
 	"golang.org/x/tools/gopls/internal/lsp/cache"
 	"golang.org/x/tools/gopls/internal/lsp/source"
-	"golang.org/x/tools/gopls/internal/lsp/source/typerefs"
 	"golang.org/x/tools/gopls/internal/span"
 	"golang.org/x/tools/internal/packagesinternal"
 	"golang.org/x/tools/internal/testenv"
@@ -87,7 +86,7 @@ func TestBuildPackageGraph(t *testing.T) {
 	})
 
 	t0 = time.Now()
-	g, err := typerefs.BuildPackageGraph(ctx, meta, ids, newParser().parse)
+	g, err := BuildPackageGraph(ctx, meta, ids, newParser().parse)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -263,7 +262,7 @@ func BenchmarkBuildPackageGraph(b *testing.B) {
 	b.ResetTimer()
 
 	for i := 0; i < b.N; i++ {
-		_, err := typerefs.BuildPackageGraph(ctx, meta, ids, newParser().parse)
+		_, err := BuildPackageGraph(ctx, meta, ids, newParser().parse)
 		if err != nil {
 			b.Fatal(err)
 		}

gopls/internal/lsp/source/typerefs/refs.go

@@ -11,7 +11,6 @@ import (
 	"go/ast"
 	"go/token"
 	"log"
-	"os"
 	"sort"
 	"strings"
 
@@ -65,12 +64,6 @@ type Symbol struct {
 	Name   string
 }
 
-// PackageID returns the symbol's package identifier.
-// The package index must be the one used during decoding.
-func (sym Symbol) PackageID(index *PackageIndex) source.PackageID {
-	return index.id(sym.pkgIdx)
-}
-
 // -- internals --
 
 // A symbolSet is a set of symbols used internally during index construction.
@@ -726,14 +719,16 @@ func (decl *declNode) find() *declNode {
 	return rep
 }
 
+const debugSCC = false // enable assertions in strong-component algorithm
+
 func checkCanonical(x *declNode) {
-	if debug {
+	if debugSCC {
 		assert(x == x.find(), "not canonical")
 	}
 }
 
 func assert(cond bool, msg string) {
-	if debug && !cond {
+	if debugSCC && !cond {
 		panic(msg)
 	}
 }
@@ -828,31 +823,6 @@ func decode(pkgIndex *PackageIndex, id source.PackageID, data []byte) []Class {
 		return classes[i].Decls[0] < classes[j].Decls[0]
 	})
 
-	// Debug
-	if trace && len(classes) > 0 {
-		var buf bytes.Buffer
-		fmt.Fprintf(&buf, "%s\n", id)
-		for _, class := range classes {
-			for i, name := range class.Decls {
-				if i == 0 {
-					fmt.Fprintf(&buf, "\t")
-				}
-				fmt.Fprintf(&buf, " .%s", name)
-			}
-			// Group symbols by package.
-			prevID := -1
-			for _, sym := range class.Refs {
-				if sym.pkgIdx != prevID {
-					prevID = sym.pkgIdx
-					fmt.Fprintf(&buf, "\n\t\t-> %s:", sym.PackageID(pkgIndex))
-				}
-				fmt.Fprintf(&buf, " .%s", sym.Name)
-			}
-			fmt.Fprintln(&buf)
-		}
-		os.Stderr.Write(buf.Bytes())
-	}
-
 	return classes
 }
 

gopls/internal/lsp/source/typerefs/refs_test.go

@@ -543,7 +543,7 @@ type Z map[ext.A]ext.B
 				for _, name := range class.Decls {
 					var syms []string
 					for _, sym := range class.Refs {
-						syms = append(syms, fmt.Sprintf("%s.%s", sym.PackageID(index), sym.Name))
+						syms = append(syms, fmt.Sprintf("%s.%s", index.DeclaringPackage(sym), sym.Name))
 					}
 					sort.Strings(syms)
 					got[name] = syms