gopls/internal/golang: cleanups while investigating golang/go#74434

- golang.mapPosition: doc tweak
- cache: add (*Package).FileEnclosing method.
  Call it from findLinkname, renamer.update, goasm.mapPosition.
- resolveDocLink: doc tweak; use labeled break; fix bad-style loop;
  name local vars.
- goasm.Definition: return range (not start) of symbol name.

Change-Id: I23a12627acfd3580c38f5dd41027aacad9030fdb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/684976
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

gopls/internal/cache/package.go

@@ -19,6 +19,7 @@ import (
 	"golang.org/x/tools/gopls/internal/cache/testfuncs"
 	"golang.org/x/tools/gopls/internal/cache/xrefs"
 	"golang.org/x/tools/gopls/internal/protocol"
+	"golang.org/x/tools/gopls/internal/util/safetoken"
 )
 
 // Convenient aliases for very heavily used types.
@@ -132,15 +133,26 @@ func (p *Package) File(uri protocol.DocumentURI) (*parsego.File, error) {
 	return p.pkg.File(uri)
 }
 
-func (pkg *syntaxPackage) File(uri protocol.DocumentURI) (*parsego.File, error) {
-	for _, cgf := range pkg.compiledGoFiles {
-		if cgf.URI == uri {
-			return cgf, nil
+// FileEnclosing returns the file of pkg that encloses the specified position,
+// which must be mapped by p.FileSet().
+func (p *Package) FileEnclosing(pos token.Pos) (*parsego.File, error) {
+	for _, files := range [...][]*parsego.File{p.pkg.compiledGoFiles, p.pkg.goFiles} {
+		for _, pgf := range files {
+			if pgf.File.FileStart <= pos && pos <= pgf.File.FileEnd {
+				return pgf, nil
+			}
 		}
 	}
-	for _, gf := range pkg.goFiles {
-		if gf.URI == uri {
-			return gf, nil
+	return nil, fmt.Errorf("no parsed file for position %d (%s) in %v",
+		pos, safetoken.StartPosition(p.FileSet(), pos), p.pkg.id)
+}
+
+func (pkg *syntaxPackage) File(uri protocol.DocumentURI) (*parsego.File, error) {
+	for _, files := range [...][]*parsego.File{pkg.compiledGoFiles, pkg.goFiles} {
+		for _, pgf := range files {
+			if pgf.URI == uri {
+				return pgf, nil
+			}
 		}
 	}
 	return nil, fmt.Errorf("no parsed file for %s in %v", uri, pkg.id)

gopls/internal/goasm/definition.go

@@ -85,6 +85,7 @@ func Definition(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p
 			return nil, fmt.Errorf("package %q is not a dependency", pkgpath)
 		}
 
+		// Find declared symbol in syntax package.
 		pkgs, err := snapshot.TypeCheck(ctx, declaring.ID)
 		if err != nil {
 			return nil, err
@@ -94,10 +95,18 @@ func Definition(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p
 		if def == nil {
 			return nil, fmt.Errorf("no symbol %q in package %q", name, pkgpath)
 		}
-		loc, err := mapPosition(ctx, pkg.FileSet(), snapshot, def.Pos(), def.Pos())
-		if err == nil {
-			return []protocol.Location{loc}, nil
+
+		// Map position.
+		pos := def.Pos()
+		pgf, err := pkg.FileEnclosing(pos)
+		if err != nil {
+			return nil, err
 		}
+		loc, err := pgf.PosLocation(pos, pos+token.Pos(len(name)))
+		if err != nil {
+			return nil, err
+		}
+		return []protocol.Location{loc}, nil
 
 	} else {
 		// local symbols (funcs, vars, labels)
@@ -116,21 +125,3 @@ func Definition(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p
 
 	return nil, nil
 }
-
-// TODO(rfindley): avoid the duplicate column mapping here, by associating a
-// column mapper with each file handle.
-// TODO(adonovan): plundered from ../golang; factor.
-func mapPosition(ctx context.Context, fset *token.FileSet, s file.Source, start, end token.Pos) (protocol.Location, error) {
-	file := fset.File(start)
-	uri := protocol.URIFromPath(file.Name())
-	fh, err := s.ReadFile(ctx, uri)
-	if err != nil {
-		return protocol.Location{}, err
-	}
-	content, err := fh.Content()
-	if err != nil {
-		return protocol.Location{}, err
-	}
-	m := protocol.NewMapper(fh.URI(), content)
-	return m.PosLocation(file, start, end)
-}

gopls/internal/golang/comment.go

@@ -71,19 +71,17 @@ func docLinkDefinition(ctx context.Context, snapshot *cache.Snapshot, pkg *cache
 }
 
 // resolveDocLink parses a doc link in a comment such as [fmt.Println]
-// and returns the symbol at pos, along with the link's start position.
+// and returns the symbol at pos, along with the link's range.
 func resolveDocLink(pkg *cache.Package, pgf *parsego.File, pos token.Pos) (types.Object, protocol.Range, error) {
 	var comment *ast.Comment
+outer:
 	for _, cg := range pgf.File.Comments {
 		for _, c := range cg.List {
 			if c.Pos() <= pos && pos <= c.End() {
 				comment = c
-				break
+				break outer
 			}
 		}
-		if comment != nil {
-			break
-		}
 	}
 	if comment == nil {
 		return nil, protocol.Range{}, errNoCommentReference
@@ -110,32 +108,32 @@ func resolveDocLink(pkg *cache.Package, pgf *parsego.File, pos token.Pos) (types
 	lineOffset := int(pos - start)
 
 	for _, idx := range docLinkRegex.FindAllStringSubmatchIndex(text, -1) {
-		// The [idx[2], idx[3]) identifies the first submatch,
+		mstart, mend := idx[2], idx[3]
+		// [mstart, mend) identifies the first submatch,
 		// which is the reference name in the doc link (sans '*').
 		// e.g. The "[fmt.Println]" reference name is "fmt.Println".
-		if !(idx[2] <= lineOffset && lineOffset < idx[3]) {
-			continue
-		}
-		p := lineOffset - idx[2]
-		name := text[idx[2]:idx[3]]
-		i := strings.LastIndexByte(name, '.')
-		for i != -1 {
-			if p > i {
-				break
+		if mstart <= lineOffset && lineOffset < mend {
+			p := lineOffset - mstart
+			name := text[mstart:mend]
+			i := strings.LastIndexByte(name, '.')
+			for i != -1 {
+				if p > i {
+					break
+				}
+				name = name[:i]
+				i = strings.LastIndexByte(name, '.')
 			}
-			name = name[:i]
-			i = strings.LastIndexByte(name, '.')
-		}
-		obj := lookupDocLinkSymbol(pkg, pgf, name)
-		if obj == nil {
-			return nil, protocol.Range{}, errNoCommentReference
-		}
-		namePos := start + token.Pos(idx[2]+i+1)
-		rng, err := pgf.PosRange(namePos, namePos+token.Pos(len(obj.Name())))
-		if err != nil {
-			return nil, protocol.Range{}, err
+			obj := lookupDocLinkSymbol(pkg, pgf, name)
+			if obj == nil {
+				return nil, protocol.Range{}, errNoCommentReference
+			}
+			namePos := start + token.Pos(mstart+i+1)
+			rng, err := pgf.PosRange(namePos, namePos+token.Pos(len(obj.Name())))
+			if err != nil {
+				return nil, protocol.Range{}, err
+			}
+			return obj, rng, nil // success
 		}
-		return obj, rng, nil
 	}
 
 	return nil, protocol.Range{}, errNoCommentReference

gopls/internal/golang/definition.go

@@ -438,6 +438,10 @@ func importDefinition(ctx context.Context, s *cache.Snapshot, pkg *cache.Package
 	return locs, nil
 }
 
+// mapPosition returns the location of (start, end) in the file
+// that encloses that range according to fset.
+// It may need to read the file content, hence (ctx, s).
+//
 // TODO(rfindley): avoid the duplicate column mapping here, by associating a
 // column mapper with each file handle.
 func mapPosition(ctx context.Context, fset *token.FileSet, s file.Source, start, end token.Pos) (protocol.Location, error) {

gopls/internal/golang/linkname.go

@@ -15,7 +15,6 @@ import (
 	"golang.org/x/tools/gopls/internal/cache/metadata"
 	"golang.org/x/tools/gopls/internal/cache/parsego"
 	"golang.org/x/tools/gopls/internal/protocol"
-	"golang.org/x/tools/gopls/internal/util/safetoken"
 )
 
 // ErrNoLinkname is returned by LinknameDefinition when no linkname
@@ -135,11 +134,7 @@ func findLinkname(ctx context.Context, snapshot *cache.Snapshot, pkgPath Package
 		return nil, nil, token.NoPos, fmt.Errorf("package %q does not define %s", pkgPath, name)
 	}
 
-	objURI := safetoken.StartPosition(pkg.FileSet(), obj.Pos())
-	pgf, err := pkg.File(protocol.URIFromPath(objURI.Filename))
-	if err != nil {
-		return nil, nil, token.NoPos, err
-	}
-
-	return pkg, pgf, obj.Pos(), nil
+	pos := obj.Pos()
+	pgf, err := pkg.FileEnclosing(pos)
+	return pkg, pgf, pos, err
 }

gopls/internal/golang/rename.go

@@ -1294,9 +1294,9 @@ func (r *renamer) update() (map[protocol.DocumentURI][]diff.Edit, error) {
 
 	// Update each identifier, and its doc comment if it is a declaration.
 	for _, item := range items {
-		pgf, ok := enclosingFile(r.pkg, item.node.Pos())
-		if !ok {
-			bug.Reportf("edit does not belong to syntax of package %q", r.pkg)
+		pgf, err := r.pkg.FileEnclosing(item.node.Pos())
+		if err != nil {
+			bug.Reportf("edit does not belong to syntax of package %q: %v", r.pkg, err)
 			continue
 		}
 
@@ -1661,16 +1661,6 @@ func parsePackageNameDecl(ctx context.Context, snapshot *cache.Snapshot, fh file
 	return pgf, goplsastutil.NodeContains(pgf.File.Name, pos), nil
 }
 
-// enclosingFile returns the CompiledGoFile of pkg that contains the specified position.
-func enclosingFile(pkg *cache.Package, pos token.Pos) (*parsego.File, bool) {
-	for _, pgf := range pkg.CompiledGoFiles() {
-		if pgf.File.FileStart <= pos && pos <= pgf.File.FileEnd {
-			return pgf, true
-		}
-	}
-	return nil, false
-}
-
 // posEdit returns an edit to replace the (start, end) range of tf with 'new'.
 func posEdit(tf *token.File, start, end token.Pos, new string) (diff.Edit, error) {
 	startOffset, endOffset, err := safetoken.Offsets(tf, start, end)

gopls/internal/golang/rename_check.go

@@ -439,7 +439,7 @@ func (r *renamer) checkStructField(from *types.Var) {
 	// go/types offers no easy way to get from a field (or interface
 	// method) to its declaring struct (or interface), so we must
 	// ascend the AST.
-	if pgf, ok := enclosingFile(r.pkg, from.Pos()); ok {
+	if pgf, err := r.pkg.FileEnclosing(from.Pos()); err == nil {
 		path, _ := astutil.PathEnclosingInterval(pgf.File, from.Pos(), from.Pos())
 		// path matches this pattern:
 		// [Ident SelectorExpr? StarExpr? Field FieldList StructType ParenExpr* ... File]

gopls/internal/test/marker/testdata/definition/asm.txt

@@ -16,7 +16,7 @@ package a
 import _ "fmt"
 import _ "example.com/b"
 
-func ff() //@ loc(ffgo, re"()ff"), def("ff", ffasm)
+func ff() //@ loc(ffgo, "ff"), def("ff", ffasm)
 
 var _ = ff // pacify unusedfunc analyzer
 
@@ -33,4 +33,4 @@ label:					//@ loc(label,"label")
 -- b/b.go --
 package b
 
-func B() {} //@ loc(bB, re"()B")
+func B() {} //@ loc(bB, "B")