gopls/internal/lsp/source: don't rely on global FileSet when stubbing

Update the method stubbing logic not to rely on a global FileSet.

For golang/go#57987

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

Author: findleyr

gopls/internal/lsp/analysis/stubmethods/stubmethods.go

@@ -60,7 +60,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			endPos = analysisinternal.TypeErrorEndPos(pass.Fset, buf.Bytes(), err.Pos)
 		}
 		path, _ := astutil.PathEnclosingInterval(file, err.Pos, endPos)
-		si := GetStubInfo(pass.TypesInfo, path, err.Pos)
+		si := GetStubInfo(pass.Fset, pass.TypesInfo, path, err.Pos)
 		if si == nil {
 			continue
 		}
@@ -84,33 +84,34 @@ type StubInfo struct {
 	// in the case where the concrete type file requires a new import that happens to be renamed
 	// in the interface file.
 	// TODO(marwan-at-work): implement interface literals.
+	Fset      *token.FileSet // the FileSet used to type-check the types below
 	Interface *types.TypeName
 	Concrete  *types.Named
 	Pointer   bool
 }
 
 // GetStubInfo determines whether the "missing method error"
 // can be used to deduced what the concrete and interface types are.
-func GetStubInfo(ti *types.Info, path []ast.Node, pos token.Pos) *StubInfo {
+func GetStubInfo(fset *token.FileSet, ti *types.Info, path []ast.Node, pos token.Pos) *StubInfo {
 	for _, n := range path {
 		switch n := n.(type) {
 		case *ast.ValueSpec:
-			return fromValueSpec(ti, n, pos)
+			return fromValueSpec(fset, ti, n, pos)
 		case *ast.ReturnStmt:
 			// An error here may not indicate a real error the user should know about, but it may.
 			// Therefore, it would be best to log it out for debugging/reporting purposes instead of ignoring
 			// it. However, event.Log takes a context which is not passed via the analysis package.
 			// TODO(marwan-at-work): properly log this error.
-			si, _ := fromReturnStmt(ti, pos, path, n)
+			si, _ := fromReturnStmt(fset, ti, pos, path, n)
 			return si
 		case *ast.AssignStmt:
-			return fromAssignStmt(ti, n, pos)
+			return fromAssignStmt(fset, ti, n, pos)
 		case *ast.CallExpr:
 			// Note that some call expressions don't carry the interface type
 			// because they don't point to a function or method declaration elsewhere.
 			// For eaxmple, "var Interface = (*Concrete)(nil)". In that case, continue
 			// this loop to encounter other possibilities such as *ast.ValueSpec or others.
-			si := fromCallExpr(ti, pos, n)
+			si := fromCallExpr(fset, ti, pos, n)
 			if si != nil {
 				return si
 			}
@@ -122,7 +123,7 @@ func GetStubInfo(ti *types.Info, path []ast.Node, pos token.Pos) *StubInfo {
 // fromCallExpr tries to find an *ast.CallExpr's function declaration and
 // analyzes a function call's signature against the passed in parameter to deduce
 // the concrete and interface types.
-func fromCallExpr(ti *types.Info, pos token.Pos, ce *ast.CallExpr) *StubInfo {
+func fromCallExpr(fset *token.FileSet, ti *types.Info, pos token.Pos, ce *ast.CallExpr) *StubInfo {
 	paramIdx := -1
 	for i, p := range ce.Args {
 		if pos >= p.Pos() && pos <= p.End() {
@@ -152,6 +153,7 @@ func fromCallExpr(ti *types.Info, pos token.Pos, ce *ast.CallExpr) *StubInfo {
 		return nil
 	}
 	return &StubInfo{
+		Fset:      fset,
 		Concrete:  concObj,
 		Pointer:   pointer,
 		Interface: iface,
@@ -163,7 +165,7 @@ func fromCallExpr(ti *types.Info, pos token.Pos, ce *ast.CallExpr) *StubInfo {
 //
 // For example, func() io.Writer { return myType{} }
 // would return StubInfo with the interface being io.Writer and the concrete type being myType{}.
-func fromReturnStmt(ti *types.Info, pos token.Pos, path []ast.Node, rs *ast.ReturnStmt) (*StubInfo, error) {
+func fromReturnStmt(fset *token.FileSet, ti *types.Info, pos token.Pos, path []ast.Node, rs *ast.ReturnStmt) (*StubInfo, error) {
 	returnIdx := -1
 	for i, r := range rs.Results {
 		if pos >= r.Pos() && pos <= r.End() {
@@ -186,6 +188,7 @@ func fromReturnStmt(ti *types.Info, pos token.Pos, path []ast.Node, rs *ast.Retu
 		return nil, nil
 	}
 	return &StubInfo{
+		Fset:      fset,
 		Concrete:  concObj,
 		Pointer:   pointer,
 		Interface: iface,
@@ -194,7 +197,7 @@ func fromReturnStmt(ti *types.Info, pos token.Pos, path []ast.Node, rs *ast.Retu
 
 // fromValueSpec returns *StubInfo from a variable declaration such as
 // var x io.Writer = &T{}
-func fromValueSpec(ti *types.Info, vs *ast.ValueSpec, pos token.Pos) *StubInfo {
+func fromValueSpec(fset *token.FileSet, ti *types.Info, vs *ast.ValueSpec, pos token.Pos) *StubInfo {
 	var idx int
 	for i, vs := range vs.Values {
 		if pos >= vs.Pos() && pos <= vs.End() {
@@ -221,6 +224,7 @@ func fromValueSpec(ti *types.Info, vs *ast.ValueSpec, pos token.Pos) *StubInfo {
 		return nil
 	}
 	return &StubInfo{
+		Fset:      fset,
 		Concrete:  concObj,
 		Interface: ifaceObj,
 		Pointer:   pointer,
@@ -230,7 +234,7 @@ func fromValueSpec(ti *types.Info, vs *ast.ValueSpec, pos token.Pos) *StubInfo {
 // fromAssignStmt returns *StubInfo from a variable re-assignment such as
 // var x io.Writer
 // x = &T{}
-func fromAssignStmt(ti *types.Info, as *ast.AssignStmt, pos token.Pos) *StubInfo {
+func fromAssignStmt(fset *token.FileSet, ti *types.Info, as *ast.AssignStmt, pos token.Pos) *StubInfo {
 	idx := -1
 	var lhs, rhs ast.Expr
 	// Given a re-assignment interface conversion error,
@@ -263,6 +267,7 @@ func fromAssignStmt(ti *types.Info, as *ast.AssignStmt, pos token.Pos) *StubInfo
 		return nil
 	}
 	return &StubInfo{
+		Fset:      fset,
 		Concrete:  concType,
 		Interface: ifaceObj,
 		Pointer:   pointer,
@@ -283,6 +288,8 @@ func fromAssignStmt(ti *types.Info, as *ast.AssignStmt, pos token.Pos) *StubInfo
 // is presented with a package that is not imported. This is useful so that as types.TypeString is
 // formatting a function signature, it is identifying packages that will need to be imported when
 // stubbing an interface.
+//
+// TODO(rfindley): investigate if this can be merged with source.Qualifier.
 func RelativeToFiles(concPkg *types.Package, concFile *ast.File, ifaceImports []*ast.ImportSpec, missingImport func(name, path string)) types.Qualifier {
 	return func(other *types.Package) string {
 		if other == concPkg {

gopls/internal/lsp/source/stub.go

@@ -22,6 +22,7 @@ import (
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	"golang.org/x/tools/gopls/internal/lsp/safetoken"
 	"golang.org/x/tools/gopls/internal/span"
+	"golang.org/x/tools/internal/bug"
 	"golang.org/x/tools/internal/typeparams"
 )
 
@@ -34,7 +35,7 @@ func stubSuggestedFixFunc(ctx context.Context, snapshot Snapshot, fh FileHandle,
 	if err != nil {
 		return nil, nil, fmt.Errorf("getNodes: %w", err)
 	}
-	si := stubmethods.GetStubInfo(pkg.GetTypesInfo(), nodes, pos)
+	si := stubmethods.GetStubInfo(pkg.FileSet(), pkg.GetTypesInfo(), nodes, pos)
 	if si == nil {
 		return nil, nil, fmt.Errorf("nil interface request")
 	}
@@ -47,14 +48,14 @@ func stubSuggestedFixFunc(ctx context.Context, snapshot Snapshot, fh FileHandle,
 	}
 
 	// Parse the file defining the concrete type.
-	concreteFilename := safetoken.StartPosition(snapshot.FileSet(), si.Concrete.Obj().Pos()).Filename
+	concreteFilename := safetoken.StartPosition(si.Fset, si.Concrete.Obj().Pos()).Filename
 	concreteFH, err := snapshot.GetFile(ctx, span.URIFromPath(concreteFilename))
 	if err != nil {
 		return nil, nil, err
 	}
 	parsedConcreteFile, err := snapshot.ParseGo(ctx, concreteFH, ParseFull)
 	if err != nil {
-		return nil, nil, fmt.Errorf("failed to parse file declaring implementation type: %w", err)
+		return nil, nil, fmt.Errorf("failed to parse file %q declaring implementation type: %w", concreteFH.URI(), err)
 	}
 	var (
 		methodsSrc  []byte
@@ -72,21 +73,28 @@ func stubSuggestedFixFunc(ctx context.Context, snapshot Snapshot, fh FileHandle,
 	// Splice the methods into the file.
 	// The insertion point is after the top-level declaration
 	// enclosing the (package-level) type object.
-	insertPos := parsedConcreteFile.File.End()
+	insertOffset, err := safetoken.Offset(parsedConcreteFile.Tok, parsedConcreteFile.File.End())
+	if err != nil {
+		return nil, nil, bug.Errorf("internal error: end position outside file bounds: %v", err)
+	}
+	concOffset, err := safetoken.Offset(pkg.FileSet().File(conc.Pos()), conc.Pos())
+	if err != nil {
+		return nil, nil, bug.Errorf("internal error: finding type decl offset: %v", err)
+	}
 	for _, decl := range parsedConcreteFile.File.Decls {
-		if decl.End() > conc.Pos() {
-			insertPos = decl.End()
+		declEndOffset, err := safetoken.Offset(parsedConcreteFile.Tok, decl.End())
+		if err != nil {
+			return nil, nil, bug.Errorf("internal error: finding decl offset: %v", err)
+		}
+		if declEndOffset > concOffset {
+			insertOffset = declEndOffset
 			break
 		}
 	}
 	concreteSrc, err := concreteFH.Read()
 	if err != nil {
 		return nil, nil, fmt.Errorf("error reading concrete file source: %w", err)
 	}
-	insertOffset, err := safetoken.Offset(parsedConcreteFile.Tok, insertPos)
-	if err != nil || insertOffset >= len(concreteSrc) {
-		return nil, nil, fmt.Errorf("insertion position is past the end of the file")
-	}
 	var buf bytes.Buffer
 	buf.Write(concreteSrc[:insertOffset])
 	buf.WriteByte('\n')
@@ -126,7 +134,7 @@ func stubSuggestedFixFunc(ctx context.Context, snapshot Snapshot, fh FileHandle,
 // that implement the given interface
 func stubMethods(ctx context.Context, concreteFile *ast.File, si *stubmethods.StubInfo, snapshot Snapshot) ([]byte, []*stubImport, error) {
 	concMS := types.NewMethodSet(types.NewPointer(si.Concrete.Obj().Type()))
-	missing, err := missingMethods(ctx, snapshot, concMS, si.Concrete.Obj().Pkg(), si.Interface, map[string]struct{}{})
+	missing, err := missingMethods(ctx, si.Fset, snapshot, concMS, si.Concrete.Obj().Pkg(), si.Interface, map[string]struct{}{})
 	if err != nil {
 		return nil, nil, fmt.Errorf("missingMethods: %w", err)
 	}
@@ -249,8 +257,10 @@ returns
 			missing: []*types.Func{Hello}
 		},
 	}
+
+The provided FileSet must be the FileSet used when type-checking concPkg.
 */
-func missingMethods(ctx context.Context, snapshot Snapshot, concMS *types.MethodSet, concPkg *types.Package, ifaceObj *types.TypeName, visited map[string]struct{}) ([]*missingInterface, error) {
+func missingMethods(ctx context.Context, fset *token.FileSet, snapshot Snapshot, concMS *types.MethodSet, concPkg *types.Package, ifaceObj *types.TypeName, visited map[string]struct{}) ([]*missingInterface, error) {
 	iface, ok := ifaceObj.Type().Underlying().(*types.Interface)
 	if !ok {
 		return nil, fmt.Errorf("expected %v to be an interface but got %T", iface, ifaceObj.Type().Underlying())
@@ -270,7 +280,7 @@ func missingMethods(ctx context.Context, snapshot Snapshot, concMS *types.Method
 	}
 
 	// Parse the imports from the file that declares the interface.
-	ifaceFilename := safetoken.StartPosition(snapshot.FileSet(), ifaceObj.Pos()).Filename
+	ifaceFilename := safetoken.StartPosition(fset, ifaceObj.Pos()).Filename
 	ifaceFH, err := snapshot.GetFile(ctx, span.URIFromPath(ifaceFilename))
 	if err != nil {
 		return nil, err
@@ -311,7 +321,7 @@ func missingMethods(ctx context.Context, snapshot Snapshot, concMS *types.Method
 	var missingInterfaces []*missingInterface
 	for i := 0; i < iface.NumEmbeddeds(); i++ {
 		eiface := iface.Embedded(i).Obj()
-		em, err := missingMethods(ctx, snapshot, concMS, concPkg, eiface, visited)
+		em, err := missingMethods(ctx, fset, snapshot, concMS, concPkg, eiface, visited)
 		if err != nil {
 			return nil, err
 		}