gopls/internal/lsp/source/completion: avoid Snapshot.CachedPackages

This change removes the last use of CachedPackages, from unimported
completions. Instead, we find candidates in the workspace using
the metadata, then perform a quick parse of each file to extract
the names of exported package members. The quick parse first
uses the scanner to find the function bodies and then deletes
them, eliminating most of the input to the actual scanner.

Unfortunately we can't simply load export data for the relevant
package (which should be quick) and use its type information in
the usual way, because the deep completion machinery is tightly
coupled to the notion of a single realm of objects. We can only
display syntactic information (e.g. var/func/const/type).
Thus this change is a slight functional regression from the old
behavior which presented accurate type information.

Fixes golang/go#58663

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

11 files changed, 333 insertions, 122 deletions

diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index cd8e498e7..f81c54adb 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -1113,14 +1113,6 @@ func (s *snapshot) AllMetadata(ctx context.Context) ([]*source.Metadata, error)
 	return meta, nil
 }
 
-func (s *snapshot) CachedPackages(ctx context.Context) []source.Package {
-	// Cached packages do not make sense with incremental gopls.
-	//
-	// TODO(golang/go#58663): re-implement unimported completions to not depend
-	// on cached import paths.
-	return nil
-}
-
 // TODO(rfindley): clarify that this is only active modules. Or update to just
 // use findRootPattern.
 func (s *snapshot) GoModForFile(uri span.URI) span.URI {
diff --git a/gopls/internal/lsp/source/completion/completion.go b/gopls/internal/lsp/source/completion/completion.go
index 7ecd7a786..f8c7654f6 100644
--- a/gopls/internal/lsp/source/completion/completion.go
+++ b/gopls/internal/lsp/source/completion/completion.go
@@ -7,10 +7,12 @@
 package completion
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"go/ast"
 	"go/constant"
+	"go/parser"
 	"go/scanner"
 	"go/token"
 	"go/types"
@@ -19,20 +21,28 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 	"unicode"
 
+	"golang.org/x/sync/errgroup"
 	"golang.org/x/tools/go/ast/astutil"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
+	"golang.org/x/tools/gopls/internal/lsp/safetoken"
 	"golang.org/x/tools/gopls/internal/lsp/snippet"
 	"golang.org/x/tools/gopls/internal/lsp/source"
+	"golang.org/x/tools/gopls/internal/span"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/fuzzy"
 	"golang.org/x/tools/internal/imports"
 	"golang.org/x/tools/internal/typeparams"
 )
 
+// A CompletionItem represents a possible completion suggested by the algorithm.
 type CompletionItem struct {
+
+	// Invariant: CompletionItem does not refer to syntax or types.
+
 	// Label is the primary text the user sees for this completion item.
 	Label string
 
@@ -85,9 +95,10 @@ type CompletionItem struct {
 	// Documentation is the documentation for the completion item.
 	Documentation string
 
-	// obj is the object from which this candidate was derived, if any.
-	// obj is for internal use only.
-	obj types.Object
+	// isSlice reports whether the underlying type of the object
+	// from which this candidate was derived is a slice.
+	// (Used to complete append() calls.)
+	isSlice bool
 }
 
 // completionOptions holds completion specific configuration.
@@ -1095,92 +1106,91 @@ const (
 func (c *completer) selector(ctx context.Context, sel *ast.SelectorExpr) error {
 	c.inference.objChain = objChain(c.pkg.GetTypesInfo(), sel.X)
 
-	// Is sel a qualified identifier?
-	if id, ok := sel.X.(*ast.Ident); ok {
-		if pkgName, ok := c.pkg.GetTypesInfo().Uses[id].(*types.PkgName); ok {
-			// If this package path is not a known dep, it means that it resolves to
-			// an import path that couldn't be resolved by go/packages.
-			//
-			// Try to complete from the package cache.
-			pkgPath := source.PackagePath(pkgName.Imported().Path())
-			if _, ok := c.pkg.Metadata().DepsByPkgPath[pkgPath]; !ok && c.opts.unimported {
-				if err := c.unimportedMembers(ctx, id); err != nil {
-					return err
-				}
-			}
-			c.packageMembers(pkgName.Imported(), stdScore, nil, func(cand candidate) {
-				c.deepState.enqueue(cand)
-			})
-			return nil
-		}
-	}
-
-	// Invariant: sel is a true selector.
-	tv, ok := c.pkg.GetTypesInfo().Types[sel.X]
-	if ok {
-		c.methodsAndFields(tv.Type, tv.Addressable(), nil, func(cand candidate) {
-			c.deepState.enqueue(cand)
-		})
-
+	// True selector?
+	if tv, ok := c.pkg.GetTypesInfo().Types[sel.X]; ok {
+		c.methodsAndFields(tv.Type, tv.Addressable(), nil, c.deepState.enqueue)
 		c.addPostfixSnippetCandidates(ctx, sel)
-
 		return nil
 	}
 
-	// Try unimported packages.
-	if id, ok := sel.X.(*ast.Ident); ok && c.opts.unimported {
-		if err := c.unimportedMembers(ctx, id); err != nil {
-			return err
-		}
+	id, ok := sel.X.(*ast.Ident)
+	if !ok {
+		return nil
 	}
-	return nil
-}
 
-func (c *completer) unimportedMembers(ctx context.Context, id *ast.Ident) error {
-	// Try loaded packages first. They're relevant, fast, and fully typed.
-	//
-	// Find all cached packages that are imported a nonzero amount of time.
-	//
-	// TODO(rfindley): this is pre-existing behavior, and a test fails if we
-	// don't do the importCount filter, but why do we care if a package is
-	// imported a nonzero amount of times?
-	//
-	// TODO(adonovan): avoid the need for type-checked packges entirely here.
-	pkgs := make(map[source.PackagePath]source.Package)
-	imported := make(map[source.PackagePath]bool)
-	for _, pkg := range c.snapshot.CachedPackages(ctx) {
-		m := pkg.Metadata()
-		for dep := range m.DepsByPkgPath {
-			imported[dep] = true
+	// Treat sel as a qualified identifier.
+	var filter func(*source.Metadata) bool
+	needImport := false
+	if pkgName, ok := c.pkg.GetTypesInfo().Uses[id].(*types.PkgName); ok {
+		// Qualified identifier with import declaration.
+		imp := pkgName.Imported()
+
+		// Known direct dependency? Expand using type information.
+		if _, ok := c.pkg.Metadata().DepsByPkgPath[source.PackagePath(imp.Path())]; ok {
+			c.packageMembers(imp, stdScore, nil, c.deepState.enqueue)
+			return nil
 		}
-		if m.Name == "main" {
-			continue
+
+		// Imported declaration with missing type information.
+		// Fall through to shallow completion of unimported package members.
+		// Match candidate packages by path.
+		// TODO(adonovan): simplify by merging with else case and matching on name only?
+		filter = func(m *source.Metadata) bool {
+			return strings.TrimPrefix(string(m.PkgPath), "vendor/") == imp.Path()
 		}
-		if old, ok := pkgs[m.PkgPath]; ok {
-			if len(m.CompiledGoFiles) < len(old.Metadata().CompiledGoFiles) {
-				pkgs[m.PkgPath] = pkg
-			}
-		} else {
-			pkgs[m.PkgPath] = pkg
+	} else {
+		// Qualified identifier without import declaration.
+		// Match candidate packages by name.
+		filter = func(m *source.Metadata) bool {
+			return string(m.Name) == id.Name
 		}
+		needImport = true
 	}
-	known := make(map[source.PackagePath]*types.Package)
-	for pkgPath, pkg := range pkgs {
-		if imported[pkgPath] {
-			known[pkgPath] = pkg.GetTypes()
-		}
+
+	// Search unimported packages.
+	if !c.opts.unimported {
+		return nil // feature disabled
 	}
 
+	// The deep completion algorithm is exceedingly complex and
+	// deeply coupled to the now obsolete notions that all
+	// token.Pos values can be interpreted by as a single FileSet
+	// belonging to the Snapshot and that all types.Object values
+	// are canonicalized by a single types.Importer mapping.
+	// These invariants are no longer true now that gopls uses
+	// an incremental approach, parsing and type-checking each
+	// package separately.
+	//
+	// Consequently, completion of symbols defined in packages that
+	// are not currently imported by the query file cannot use the
+	// deep completion machinery which is based on type information.
+	// Instead it must use only syntax information from a quick
+	// parse of top-level declarations (but not function bodies).
+	//
+	// TODO(adonovan): rewrite the deep completion machinery to
+	// not assume global Pos/Object realms and then use export
+	// data instead of the quick parse approach taken here.
+
+	// First, we search among packages in the workspace.
+	// We'll use a fast parse to extract package members
+	// from those that match the name/path criterion.
+	all, err := c.snapshot.AllMetadata(ctx)
+	if err != nil {
+		return err
+	}
 	var paths []string
-	for path, pkg := range known {
-		if pkg.Name() != id.Name {
+	known := make(map[source.PackagePath][]*source.Metadata) // may include test variant
+	for _, m := range all {
+		if m.IsIntermediateTestVariant() || m.Name == "main" || !filter(m) {
 			continue
 		}
-		paths = append(paths, string(path))
+		known[m.PkgPath] = append(known[m.PkgPath], m)
+		paths = append(paths, string(m.PkgPath))
 	}
 
+	// Rank import paths as goimports would.
 	var relevances map[string]float64
-	if len(paths) != 0 {
+	if len(paths) > 0 {
 		if err := c.snapshot.RunProcessEnvFunc(ctx, func(opts *imports.Options) error {
 			var err error
 			relevances, err = imports.ScoreImportPaths(ctx, opts.Env, paths)
@@ -1188,32 +1198,119 @@ func (c *completer) unimportedMembers(ctx context.Context, id *ast.Ident) error
 		}); err != nil {
 			return err
 		}
+		sort.Slice(paths, func(i, j int) bool {
+			return relevances[paths[i]] > relevances[paths[j]]
+		})
 	}
-	sort.Slice(paths, func(i, j int) bool {
-		return relevances[paths[i]] > relevances[paths[j]]
-	})
 
-	for _, path := range paths {
-		pkg := known[source.PackagePath(path)]
-		if pkg.Name() != id.Name {
-			continue
+	// quickParse does a quick parse of a single file of package m,
+	// extracts exported package members and adds candidates to c.items.
+	var itemsMu sync.Mutex // guards c.items
+	var enough int32       // atomic bool
+	quickParse := func(uri span.URI, m *source.Metadata) error {
+		if atomic.LoadInt32(&enough) != 0 {
+			return nil
 		}
-		imp := &importInfo{
-			importPath: path,
+
+		fh, err := c.snapshot.GetFile(ctx, uri)
+		if err != nil {
+			return err
 		}
-		if imports.ImportPathToAssumedName(path) != pkg.Name() {
-			imp.name = pkg.Name()
+		content, err := fh.Read()
+		if err != nil {
+			return err
 		}
-		c.packageMembers(pkg, unimportedScore(relevances[path]), imp, func(cand candidate) {
-			c.deepState.enqueue(cand)
+		path := string(m.PkgPath)
+		forEachPackageMember(content, func(tok token.Token, id *ast.Ident, fn *ast.FuncDecl) {
+			if atomic.LoadInt32(&enough) != 0 {
+				return
+			}
+
+			if !id.IsExported() ||
+				sel.Sel.Name != "_" && !strings.HasPrefix(id.Name, sel.Sel.Name) {
+				return // not a match
+			}
+
+			// The only detail is the kind and package: `var (from "example.com/foo")`
+			// TODO(adonovan): pretty-print FuncDecl.FuncType or TypeSpec.Type?
+			item := CompletionItem{
+				Label:      id.Name,
+				Detail:     fmt.Sprintf("%s (from %q)", strings.ToLower(tok.String()), m.PkgPath),
+				InsertText: id.Name,
+				Score:      unimportedScore(relevances[path]),
+			}
+			switch tok {
+			case token.FUNC:
+				item.Kind = protocol.FunctionCompletion
+			case token.VAR:
+				item.Kind = protocol.VariableCompletion
+			case token.CONST:
+				item.Kind = protocol.ConstantCompletion
+			case token.TYPE:
+				// Without types, we can't distinguish Class from Interface.
+				item.Kind = protocol.ClassCompletion
+			}
+
+			if needImport {
+				imp := &importInfo{importPath: path}
+				if imports.ImportPathToAssumedName(path) != string(m.Name) {
+					imp.name = string(m.Name)
+				}
+				item.AdditionalTextEdits, _ = c.importEdits(imp)
+			}
+
+			// For functions, add a parameter snippet.
+			if fn != nil {
+				var sn snippet.Builder
+				sn.WriteText(id.Name)
+				sn.WriteText("(")
+				var nparams int
+				for _, field := range fn.Type.Params.List {
+					if field.Names != nil {
+						nparams += len(field.Names)
+					} else {
+						nparams++
+					}
+				}
+				for i := 0; i < nparams; i++ {
+					if i > 0 {
+						sn.WriteText(", ")
+					}
+					sn.WritePlaceholder(nil)
+				}
+				sn.WriteText(")")
+				item.snippet = &sn
+			}
+
+			itemsMu.Lock()
+			c.items = append(c.items, item)
+			if len(c.items) >= unimportedMemberTarget {
+				atomic.StoreInt32(&enough, 1)
+			}
+			itemsMu.Unlock()
 		})
-		if len(c.items) >= unimportedMemberTarget {
-			return nil
+		return nil
+	}
+
+	// Extract the package-level candidates using a quick parse.
+	var g errgroup.Group
+	for _, path := range paths {
+		for _, m := range known[source.PackagePath(path)] {
+			m := m
+			for _, uri := range m.CompiledGoFiles {
+				uri := uri
+				g.Go(func() error {
+					return quickParse(uri, m)
+				})
+			}
 		}
 	}
+	if err := g.Wait(); err != nil {
+		return err
+	}
 
+	// In addition, we search in the module cache using goimports.
 	ctx, cancel := context.WithCancel(ctx)
-
 	var mu sync.Mutex
 	add := func(pkgExport imports.PackageExport) {
 		mu.Lock()
@@ -3060,3 +3157,96 @@ func (c *completer) innermostScope() *types.Scope {
 	}
 	return nil
 }
+
+// isSlice reports whether the object's underlying type is a slice.
+func isSlice(obj types.Object) bool {
+	if obj != nil && obj.Type() != nil {
+		if _, ok := obj.Type().Underlying().(*types.Slice); ok {
+			return true
+		}
+	}
+	return false
+}
+
+// forEachPackageMember calls f(tok, id, fn) for each package-level
+// TYPE/VAR/CONST/FUNC declaration in the Go source file, based on a
+// quick partial parse. fn is non-nil only for function declarations.
+// The AST position information is garbage.
+func forEachPackageMember(content []byte, f func(tok token.Token, id *ast.Ident, fn *ast.FuncDecl)) {
+	purged := purgeFuncBodies(content)
+	file, _ := parser.ParseFile(token.NewFileSet(), "", purged, 0)
+	for _, decl := range file.Decls {
+		switch decl := decl.(type) {
+		case *ast.GenDecl:
+			for _, spec := range decl.Specs {
+				switch spec := spec.(type) {
+				case *ast.ValueSpec: // var/const
+					for _, id := range spec.Names {
+						f(decl.Tok, id, nil)
+					}
+				case *ast.TypeSpec:
+					f(decl.Tok, spec.Name, nil)
+				}
+			}
+		case *ast.FuncDecl:
+			if decl.Recv == nil {
+				f(token.FUNC, decl.Name, decl)
+			}
+		}
+	}
+}
+
+// purgeFuncBodies returns a copy of src in which the contents of each
+// outermost {...} region except struct and interface types have been
+// deleted. It does not preserve newlines. This reduces the amount of
+// work required to parse the top-level declarations.
+func purgeFuncBodies(src []byte) []byte {
+	// Destroy the content of any {...}-bracketed regions that are
+	// not immediately preceded by a "struct" or "interface"
+	// token.  That includes function bodies, composite literals,
+	// switch/select bodies, and all blocks of statements.
+	// This will lead to non-void functions that don't have return
+	// statements, which of course is a type error, but that's ok.
+
+	var out bytes.Buffer
+	file := token.NewFileSet().AddFile("", -1, len(src))
+	var sc scanner.Scanner
+	sc.Init(file, src, nil, 0)
+	var prev token.Token
+	var cursor int         // last consumed src offset
+	var braces []token.Pos // stack of unclosed braces or -1 for struct/interface type
+	for {
+		pos, tok, _ := sc.Scan()
+		if tok == token.EOF {
+			break
+		}
+		switch tok {
+		case token.COMMENT:
+			// TODO(adonovan): opt: skip, to save an estimated 20% of time.
+
+		case token.LBRACE:
+			if prev == token.STRUCT || prev == token.INTERFACE {
+				pos = -1
+			}
+			braces = append(braces, pos)
+
+		case token.RBRACE:
+			if last := len(braces) - 1; last >= 0 {
+				top := braces[last]
+				braces = braces[:last]
+				if top < 0 {
+					// struct/interface type: leave alone
+				} else if len(braces) == 0 { // toplevel only
+					// Delete {...} body.
+					start, _ := safetoken.Offset(file, top)
+					end, _ := safetoken.Offset(file, pos)
+					out.Write(src[cursor : start+len("{")])
+					cursor = end
+				}
+			}
+		}
+		prev = tok
+	}
+	out.Write(src[cursor:])
+	return out.Bytes()
+}
diff --git a/gopls/internal/lsp/source/completion/definition.go b/gopls/internal/lsp/source/completion/definition.go
index a0160a1a1..d7f51f002 100644
--- a/gopls/internal/lsp/source/completion/definition.go
+++ b/gopls/internal/lsp/source/completion/definition.go
@@ -144,7 +144,7 @@ func defSnippet(prefix, suffix string, obj types.Object) CompletionItem {
 		Score:         10,
 		snippet:       &sn,
 		Documentation: prefix + " test function",
-		obj:           obj,
+		isSlice:       isSlice(obj),
 	}
 }
 func defItem(val string, obj types.Object) CompletionItem {
@@ -155,6 +155,6 @@ func defItem(val string, obj types.Object) CompletionItem {
 		Depth:         0,
 		Score:         9, // prefer the snippets when available
 		Documentation: "complete the function name",
-		obj:           obj,
+		isSlice:       isSlice(obj),
 	}
 }
diff --git a/gopls/internal/lsp/source/completion/format.go b/gopls/internal/lsp/source/completion/format.go
index 634cdc8bd..c2d2c0bc0 100644
--- a/gopls/internal/lsp/source/completion/format.go
+++ b/gopls/internal/lsp/source/completion/format.go
@@ -228,7 +228,7 @@ Suffixes:
 		Score:               cand.score,
 		Depth:               len(cand.path),
 		snippet:             &snip,
-		obj:                 obj,
+		isSlice:             isSlice(obj),
 	}
 	// If the user doesn't want documentation for completion items.
 	if !c.opts.documentation {
diff --git a/gopls/internal/lsp/source/completion/fuzz.go b/gopls/internal/lsp/source/completion/fuzz.go
index d7912ceab..08e7654c7 100644
--- a/gopls/internal/lsp/source/completion/fuzz.go
+++ b/gopls/internal/lsp/source/completion/fuzz.go
@@ -121,7 +121,7 @@ Loop:
 			Depth:         0,
 			Score:         10, // pretty confident the user should see this
 			Documentation: "argument types from f.Add",
-			obj:           nil,
+			isSlice:       false,
 		}
 		c.items = append(c.items, xx)
 		for i := 0; i < mset.Len(); i++ {
diff --git a/gopls/internal/lsp/source/completion/statements.go b/gopls/internal/lsp/source/completion/statements.go
index 809cb808a..707375fa1 100644
--- a/gopls/internal/lsp/source/completion/statements.go
+++ b/gopls/internal/lsp/source/completion/statements.go
@@ -106,11 +106,7 @@ func (c *completer) addAssignAppend() {
 		// Offer the long form assign + append candidate if our best
 		// candidate is a slice.
 		bestItem := c.topCandidate()
-		if bestItem == nil || bestItem.obj == nil || bestItem.obj.Type() == nil {
-			return
-		}
-
-		if _, isSlice := bestItem.obj.Type().Underlying().(*types.Slice); !isSlice {
+		if bestItem == nil || !bestItem.isSlice {
 			return
 		}
 
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index ebb9128c9..41bcbac4b 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -156,14 +156,6 @@ type Snapshot interface {
 	// excluding id itself.
 	ReverseDependencies(ctx context.Context, id PackageID, transitive bool) (map[PackageID]*Metadata, error)
 
-	// CachedPackages returns a new, unordered array of all
-	// packages currently cached in this snapshot, which is a
-	// poorly defined set that depends on the history of
-	// operations up to this point. Do not use it.
-	//
-	// TODO(adonovan): get rid of the last call from completions.
-	CachedPackages(ctx context.Context) []Package
-
 	// ActiveMetadata returns a new, unordered slice containing
 	// metadata for all packages considered 'active' in the workspace.
 	//
diff --git a/gopls/internal/lsp/testdata/unimported/export_test.go b/gopls/internal/lsp/testdata/unimported/export_test.go
index 964d27d3b..707768e1d 100644
--- a/gopls/internal/lsp/testdata/unimported/export_test.go
+++ b/gopls/internal/lsp/testdata/unimported/export_test.go
@@ -1,3 +1,3 @@
 package unimported
 
-var TestExport int //@item(testexport, "TestExport", "(from \"golang.org/lsptests/unimported\")", "var")
+var TestExport int //@item(testexport, "TestExport", "var (from \"golang.org/lsptests/unimported\")", "var")
diff --git a/gopls/internal/lsp/testdata/unimported/unimported.go.in b/gopls/internal/lsp/testdata/unimported/unimported.go.in
index 4d1438d1b..74d51ffe8 100644
--- a/gopls/internal/lsp/testdata/unimported/unimported.go.in
+++ b/gopls/internal/lsp/testdata/unimported/unimported.go.in
@@ -6,7 +6,7 @@ func _() {
 	ring.Ring     //@unimported("Ring", ringring)
 	signature.Foo //@unimported("Foo", signaturefoo)
 
-	context.Bac //@unimported(" //", contextBackground, contextBackgroundErr)
+	context.Bac //@unimported(" //", contextBackground)
 }
 
 // Create markers for unimported std lib packages. Only for use by this test.
@@ -14,7 +14,10 @@ func _() {
 
 /* ring.Ring */ //@item(ringring, "Ring", "(from \"container/ring\")", "var")
 
-/* signature.Foo */ //@item(signaturefoo, "Foo", "func(a string, b int) (c bool) (from \"golang.org/lsptests/signature\")", "func")
+/* signature.Foo */ //@item(signaturefoo, "Foo", "func (from \"golang.org/lsptests/signature\")", "func")
 
-/* context.Background */ //@item(contextBackground, "Background", "func() context.Context (from \"context\")", "func")
-/* context.Background().Err */ //@item(contextBackgroundErr, "Background().Err", "func() error (from \"context\")", "method")
+/* context.Background */ //@item(contextBackground, "Background", "func (from \"context\")", "func")
+
+// Now that we no longer type-check imported completions,
+// we don't expect the context.Background().Err method (see golang/go#58663).
+/* context.Background().Err */ //@item(contextBackgroundErr, "Background().Err", "func (from \"context\")", "method")
diff --git a/gopls/internal/lsp/tests/tests.go b/gopls/internal/lsp/tests/tests.go
index 00521f54c..5bf8a92d0 100644
--- a/gopls/internal/lsp/tests/tests.go
+++ b/gopls/internal/lsp/tests/tests.go
@@ -611,7 +611,6 @@ func Run(t *testing.T, tests Tests, data *Data) {
 
 	t.Run("UnimportedCompletion", func(t *testing.T) {
 		t.Helper()
-		t.Skip("golang/go#58663: currently broken with incremental gopls")
 		eachCompletion(t, data.UnimportedCompletions, tests.UnimportedCompletion)
 	})
 
diff --git a/gopls/internal/regtest/completion/completion_test.go b/gopls/internal/regtest/completion/completion_test.go
index a52fcc8fc..81addba11 100644
--- a/gopls/internal/regtest/completion/completion_test.go
+++ b/gopls/internal/regtest/completion/completion_test.go
@@ -297,7 +297,7 @@ func _() {
 		if len(completions.Items) == 0 {
 			t.Fatalf("no completion items")
 		}
-		env.AcceptCompletion(loc, completions.Items[0])
+		env.AcceptCompletion(loc, completions.Items[0]) // adds blah import to main.go
 		env.Await(env.DoneWithChange())
 
 		// Trigger completions once again for the blah.<> selector.
@@ -497,8 +497,6 @@ func doit() {
 }
 
 func TestUnimportedCompletion_VSCodeIssue1489(t *testing.T) {
-	t.Skip("golang/go#58663: currently broken with incremental gopls")
-
 	const src = `
 -- go.mod --
 module mod.com
@@ -518,7 +516,7 @@ func main() {
 	WithOptions(
 		WindowsLineEndings(),
 	).Run(t, src, func(t *testing.T, env *Env) {
-		// Trigger unimported completions for the example.com/blah package.
+		// Trigger unimported completions for the mod.com package.
 		env.OpenFile("main.go")
 		env.Await(env.DoneWithOpen())
 		loc := env.RegexpSearch("main.go", "Sqr()")
@@ -536,6 +534,47 @@ func main() {
 	})
 }
 
+func TestPackageMemberCompletionAfterSyntaxError(t *testing.T) {
+	// This test documents the current broken behavior due to golang/go#58833.
+	const src = `
+-- go.mod --
+module mod.com
+
+go 1.14
+
+-- main.go --
+package main
+
+import "math"
+
+func main() {
+	math.Sqrt(,0)
+	math.Ldex
+}
+`
+	Run(t, src, func(t *testing.T, env *Env) {
+		env.OpenFile("main.go")
+		env.Await(env.DoneWithOpen())
+		loc := env.RegexpSearch("main.go", "Ldex()")
+		completions := env.Completion(loc)
+		if len(completions.Items) == 0 {
+			t.Fatalf("no completion items")
+		}
+		env.AcceptCompletion(loc, completions.Items[0])
+		env.Await(env.DoneWithChange())
+		got := env.BufferText("main.go")
+		// The completion of math.Ldex after the syntax error on the
+		// previous line is not "math.Ldexp" but "math.Ldexmath.Abs".
+		// (In VSCode, "Abs" wrongly appears in the completion menu.)
+		// This is a consequence of poor error recovery in the parser
+		// causing "math.Ldex" to become a BadExpr.
+		want := "package main\n\nimport \"math\"\n\nfunc main() {\n\tmath.Sqrt(,0)\n\tmath.Ldexmath.Abs(${1:})\n}\n"
+		if diff := cmp.Diff(want, got); diff != "" {
+			t.Errorf("unimported completion (-want +got):\n%s", diff)
+		}
+	})
+}
+
 func TestDefinition(t *testing.T) {
 	testenv.NeedsGo1Point(t, 17) // in go1.16, The FieldList in func x is not empty
 	files := `