internal/lsp/cache: don't trim unexported struct fields

The trimming optimization deletes parts of the syntax tree
that don't affect the type checking of package-level declarations.
It used to remove unexported struct fields, but this had
observable consequences: it would affect the offset of later
fields, and the size and aligment of structs, causing the
'fieldalignment' analyzer to report incorrect findings.
Also, it required a complex workaround in the UI element
for hovering over a type to account for the missing parts.

This change restores unexported fields.
The logic of recordFieldsUses has been inlined and specialized
for each case (params+results, struct fields, interface
methods) as they are more different than alike.

BenchmarkMemStats on k8s shows +4% HeapAlloc:
a lot, but a small part of the 32% saving of the trimming
optimization as a whole.

Also:
- trimAST: delete func bodies without visiting them.
- minor clarifications.

Updates golang/go#51016

Change-Id: Ifae15564a8fb86af3ea186af351a2a92eb9deb22
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415503
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

3 files changed, 70 insertions, 56 deletions

diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index e51f86e5d..79a6ff3ee 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -590,20 +590,24 @@ func parseCompiledGoFiles(ctx context.Context, compiledGoFiles []source.FileHand
 		// errors, as they may be completely nonsensical.
 		pkg.hasFixedFiles = pkg.hasFixedFiles || pgf.Fixed
 	}
-	if mode != source.ParseExported {
-		return nil
-	}
-	if astFilter != nil {
-		var files []*ast.File
-		for _, cgf := range pkg.compiledGoFiles {
-			files = append(files, cgf.File)
-		}
-		astFilter.Filter(files)
-	} else {
-		for _, cgf := range pkg.compiledGoFiles {
-			trimAST(cgf.File)
+
+	// Optionally remove parts that don't affect the exported API.
+	if mode == source.ParseExported {
+		if astFilter != nil {
+			// aggressive pruning based on reachability
+			var files []*ast.File
+			for _, cgf := range pkg.compiledGoFiles {
+				files = append(files, cgf.File)
+			}
+			astFilter.Filter(files)
+		} else {
+			// simple trimming of function bodies
+			for _, cgf := range pkg.compiledGoFiles {
+				trimAST(cgf.File)
+			}
 		}
 	}
+
 	return nil
 }
 
diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go
index 110751953..62aea2229 100644
--- a/internal/lsp/cache/parse.go
+++ b/internal/lsp/cache/parse.go
@@ -279,6 +279,8 @@ func (f *unexportedFilter) filterSpec(spec ast.Spec) bool {
 		}
 		switch typ := spec.Type.(type) {
 		case *ast.StructType:
+			// In practice this no longer filters anything;
+			// see comment at StructType case in recordUses.
 			f.filterFieldList(typ.Fields)
 		case *ast.InterfaceType:
 			f.filterFieldList(typ.Methods)
@@ -334,9 +336,19 @@ func (f *unexportedFilter) recordUses(file *ast.File) {
 				case *ast.TypeSpec:
 					switch typ := spec.Type.(type) {
 					case *ast.StructType:
-						f.recordFieldUses(false, typ.Fields)
+						// We used to trim unexported fields but this
+						// had observable consequences. For example,
+						// the 'fieldalignment' analyzer would compute
+						// incorrect diagnostics from the size and
+						// offsets, and the UI hover information for
+						// types was inaccurate. So now we keep them.
+						if typ.Fields != nil {
+							for _, field := range typ.Fields.List {
+								f.recordIdents(field.Type)
+							}
+						}
 					case *ast.InterfaceType:
-						f.recordFieldUses(false, typ.Methods)
+						f.recordInterfaceMethodUses(typ.Methods)
 					}
 				}
 			}
@@ -385,37 +397,32 @@ func (f *unexportedFilter) recordIdents(x ast.Expr) {
 }
 
 // recordFuncType records the types mentioned by a function type.
-func (f *unexportedFilter) recordFuncType(x *ast.FuncType) {
-	f.recordFieldUses(true, x.Params)
-	f.recordFieldUses(true, x.Results)
-}
-
-// recordFieldUses records unexported identifiers used in fields, which may be
-// struct members, interface members, or function parameter/results.
-func (f *unexportedFilter) recordFieldUses(isParams bool, fields *ast.FieldList) {
-	if fields == nil {
-		return
-	}
-	for _, field := range fields.List {
-		if isParams {
-			// Parameter types of retained functions need to be retained.
+func (f *unexportedFilter) recordFuncType(fn *ast.FuncType) {
+	// Parameter and result types of retained functions need to be retained.
+	if fn.Params != nil {
+		for _, field := range fn.Params.List {
 			f.recordIdents(field.Type)
-			continue
-		}
-		if ft, ok := field.Type.(*ast.FuncType); ok {
-			// Function declarations in interfaces need all their types retained.
-			f.recordFuncType(ft)
-			continue
 		}
-		if len(field.Names) == 0 {
-			// Embedded fields might contribute exported names.
+	}
+	if fn.Results != nil {
+		for _, field := range fn.Results.List {
 			f.recordIdents(field.Type)
 		}
-		for _, name := range field.Names {
-			// We only need normal fields if they're exported.
-			if ast.IsExported(name.Name) {
-				f.recordIdents(field.Type)
-				break
+	}
+}
+
+// recordInterfaceMethodUses records unexported identifiers used in interface methods.
+func (f *unexportedFilter) recordInterfaceMethodUses(methods *ast.FieldList) {
+	if methods != nil {
+		for _, method := range methods.List {
+			if len(method.Names) == 0 {
+				// I, pkg.I, I[T] -- embedded interface:
+				// may contribute exported names.
+				f.recordIdents(method.Type)
+			} else if ft, ok := method.Type.(*ast.FuncType); ok {
+				// f(T) -- ordinary interface method:
+				// needs all its types retained.
+				f.recordFuncType(ft)
 			}
 		}
 	}
@@ -442,32 +449,35 @@ func (f *unexportedFilter) ProcessErrors(errors []types.Error) (map[string]bool,
 }
 
 // trimAST clears any part of the AST not relevant to type checking
-// expressions at pos.
+// the package-level declarations.
 func trimAST(file *ast.File) {
-	ast.Inspect(file, func(n ast.Node) bool {
-		if n == nil {
-			return false
+	// Eliminate bodies of top-level functions, methods, inits.
+	for _, decl := range file.Decls {
+		if fn, ok := decl.(*ast.FuncDecl); ok {
+			fn.Body = nil
 		}
+	}
+
+	// Simplify remaining declarations.
+	ast.Inspect(file, func(n ast.Node) bool {
 		switch n := n.(type) {
-		case *ast.FuncDecl:
-			n.Body = nil
-		case *ast.BlockStmt:
-			n.List = nil
-		case *ast.CaseClause:
-			n.Body = nil
-		case *ast.CommClause:
-			n.Body = nil
+		case *ast.FuncLit:
+			// Eliminate bodies of literal functions.
+			// func() { ... } => func() {}
+			n.Body.List = nil
 		case *ast.CompositeLit:
 			// types.Info.Types for long slice/array literals are particularly
-			// expensive. Try to clear them out.
+			// expensive. Try to clear them out: T{e, ..., e} => T{}
 			at, ok := n.Type.(*ast.ArrayType)
 			if !ok {
-				// Composite literal. No harm removing all its fields.
+				// Map or struct literal: no harm removing all its fields.
 				n.Elts = nil
 				break
 			}
+
 			// Removing the elements from an ellipsis array changes its type.
 			// Try to set the length explicitly so we can continue.
+			//  [...]T{e, ..., e} => [3]T[]{}
 			if _, ok := at.Len.(*ast.Ellipsis); ok {
 				length, ok := arrayLength(n)
 				if !ok {
diff --git a/internal/lsp/cache/parse_test.go b/internal/lsp/cache/parse_test.go
index cb620f274..e8db64530 100644
--- a/internal/lsp/cache/parse_test.go
+++ b/internal/lsp/cache/parse_test.go
@@ -149,7 +149,7 @@ type Exported struct {
 }
 var Var = Exported{foo:1}
 `,
-			kept: []string{"Exported", "Var"},
+			kept: []string{"Exported", "Var", "x"},
 		},
 		{
 			name: "drop_function_literals",