gopls/internal/lsp/source: document {All,Workspace}Metadata

This change is the residue of a failed attempt to reduce the scope
of the Implementations query (and others) based on a misunderstanding.
It merely improves the documentation around these two concepts.
What is the German word for a lengthy effort that ends in
nothing more than a small amount of wistful documentation?

Also, return errors correctly in InlayHint, which I touched along
the way.

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

Author: adonovan

gopls/internal/lsp/mod/inlayhint.go

@@ -18,46 +18,46 @@ func InlayHint(ctx context.Context, snapshot source.Snapshot, fh source.FileHand
 	if err != nil {
 		return nil, err
 	}
-	return unexpectedVersion(ctx, snapshot, pm), nil
-}
 
-// Compare the version of the module used in the snapshot's metadata with the
-// version requested by the module, in both cases, taking replaces into account.
-// Produce an InlayHint when the version is the module is not the one usedd.
-func unexpectedVersion(ctx context.Context, snapshot source.Snapshot, pm *source.ParsedModule) []protocol.InlayHint {
-	var ans []protocol.InlayHint
-	if pm.File == nil {
-		return nil
-	}
+	// Compare the version of the module used in the snapshot's metadata with the
+	// version requested by the module, in both cases, taking replaces into account.
+	// Produce an InlayHint when the version is the module is not the one used.
+
 	replaces := make(map[string]*modfile.Replace)
-	requires := make(map[string]*modfile.Require)
 	for _, x := range pm.File.Replace {
 		replaces[x.Old.Path] = x
 	}
+
+	requires := make(map[string]*modfile.Require)
 	for _, x := range pm.File.Require {
 		requires[x.Mod.Path] = x
 	}
-	am, _ := snapshot.AllMetadata(ctx)
+
+	am, err := snapshot.AllMetadata(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	var ans []protocol.InlayHint
 	seen := make(map[string]bool)
 	for _, meta := range am {
-		if meta == nil || meta.Module == nil || seen[meta.Module.Path] {
+		if meta.Module == nil || seen[meta.Module.Path] {
 			continue
 		}
 		seen[meta.Module.Path] = true
-		metaMod := meta.Module
-		metaVersion := metaMod.Version
-		if metaMod.Replace != nil {
-			metaVersion = metaMod.Replace.Version
+		metaVersion := meta.Module.Version
+		if meta.Module.Replace != nil {
+			metaVersion = meta.Module.Replace.Version
 		}
 		// These versions can be blank, as in gopls/go.mod's local replace
-		if oldrepl, ok := replaces[metaMod.Path]; ok && oldrepl.New.Version != metaVersion {
+		if oldrepl, ok := replaces[meta.Module.Path]; ok && oldrepl.New.Version != metaVersion {
 			ih := genHint(oldrepl.Syntax, oldrepl.New.Version, metaVersion, pm.Mapper)
 			if ih != nil {
 				ans = append(ans, *ih)
 			}
-		} else if oldreq, ok := requires[metaMod.Path]; ok && oldreq.Mod.Version != metaVersion {
+		} else if oldreq, ok := requires[meta.Module.Path]; ok && oldreq.Mod.Version != metaVersion {
 			// maybe it was replaced:
-			if _, ok := replaces[metaMod.Path]; ok {
+			if _, ok := replaces[meta.Module.Path]; ok {
 				continue
 			}
 			ih := genHint(oldreq.Syntax, oldreq.Mod.Version, metaVersion, pm.Mapper)
@@ -66,7 +66,7 @@ func unexpectedVersion(ctx context.Context, snapshot source.Snapshot, pm *source
 			}
 		}
 	}
-	return ans
+	return ans, nil
 }
 
 func genHint(mline *modfile.Line, oldVersion, newVersion string, m *protocol.Mapper) *protocol.InlayHint {

gopls/internal/lsp/source/completion/completion.go

@@ -1174,7 +1174,8 @@ func (c *completer) selector(ctx context.Context, sel *ast.SelectorExpr) error {
 	// 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.
+	// First, we search among packages in the forward transitive
+	// closure of 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)
@@ -1610,7 +1611,7 @@ func (c *completer) unimportedPackages(ctx context.Context, seen map[string]stru
 
 	count := 0
 
-	// Search packages across the entire workspace.
+	// Search the forward transitive closure of the workspace.
 	all, err := c.snapshot.AllMetadata(ctx)
 	if err != nil {
 		return err

gopls/internal/lsp/source/implementation.go

@@ -152,8 +152,9 @@ func implementations(ctx context.Context, snapshot Snapshot, fh FileHandle, pp p
 		return nil, nil
 	}
 
-	// The global search needs to look at every package in the workspace;
-	// see package ./methodsets.
+	// The global search needs to look at every package in the
+	// forward transitive closure of the workspace; see package
+	// ./methodsets.
 	//
 	// For now we do all the type checking before beginning the search.
 	// TODO(adonovan): opt: search in parallel topological order

gopls/internal/lsp/source/known_packages.go

@@ -51,7 +51,7 @@ func KnownPackagePaths(ctx context.Context, snapshot Snapshot, fh FileHandle) ([
 		}
 	}
 
-	// Now find candidates among known packages.
+	// Now find candidates among all known packages.
 	knownPkgs, err := snapshot.AllMetadata(ctx)
 	if err != nil {
 		return nil, err

gopls/internal/lsp/source/linkname.go

@@ -117,7 +117,7 @@ func findLinknameAtOffset(pgf *ParsedGoFile, offset int) (string, int) {
 func findLinkname(ctx context.Context, snapshot Snapshot, pkgPath PackagePath, name string) (Package, *ParsedGoFile, token.Pos, error) {
 	// Typically the linkname refers to a forward dependency
 	// or a reverse dependency, but in general it may refer
-	// to any package in the workspace.
+	// to any package that is linked with this one.
 	var pkgMeta *Metadata
 	metas, err := snapshot.AllMetadata(ctx)
 	if err != nil {

gopls/internal/lsp/source/rename.go

@@ -718,8 +718,8 @@ func renamePackageName(ctx context.Context, s Snapshot, f FileHandle, newName Pa
 // directory.
 //
 // It updates package clauses and import paths for the renamed package as well
-// as any other packages affected by the directory renaming among packages
-// described by allMetadata.
+// as any other packages affected by the directory renaming among all packages
+// known to the snapshot.
 func renamePackage(ctx context.Context, s Snapshot, f FileHandle, newName PackageName) (map[span.URI][]diff.Edit, error) {
 	if strings.HasSuffix(string(newName), "_test") {
 		return nil, fmt.Errorf("cannot rename to _test package")

gopls/internal/lsp/source/view.go

@@ -175,9 +175,23 @@ type Snapshot interface {
 	// WorkspaceMetadata returns a new, unordered slice containing
 	// metadata for all ordinary and test packages (but not
 	// intermediate test variants) in the workspace.
+	//
+	// The workspace is the set of modules typically defined by a
+	// go.work file. It is not transitively closed: for example,
+	// the standard library is not usually part of the workspace
+	// even though every module in the workspace depends on it.
+	//
+	// Operations that must inspect all the dependencies of the
+	// workspace packages should instead use AllMetadata.
 	WorkspaceMetadata(ctx context.Context) ([]*Metadata, error)
 
-	// AllMetadata returns a new unordered array of metadata for all packages in the workspace.
+	// AllMetadata returns a new unordered array of metadata for
+	// all packages known to this snapshot, which includes the
+	// packages of all workspace modules plus their transitive
+	// import dependencies.
+	//
+	// It may also contain ad-hoc packages for standalone files.
+	// It includes all test variants.
 	AllMetadata(ctx context.Context) ([]*Metadata, error)
 
 	// Metadata returns the metadata for the specified package,