[tools] gopls,internal/lsp: Implement method stubbing via CodeAction
Marwan Sulaiman (Gerrit)
Marwan Sulaiman has uploaded this change for review.
gopls,internal/lsp: Implement method stubbing via CodeAction
This CL adds a quickfix CodeAction that detects "missing method"
compiler errors and suggests adding method stubs to the concrete
type that would implement the interface. There are many ways that
a user might indicate a concrete type is meant to be used as an interface.
This PR detects two types of those errors: variable declaration and function returns.
For variable declarations, things like the following should be detected:
1. var _ SomeInterface = SomeType{}
2. var _ = SomeInterface(SomeType{})
3. var _ SomeInterface = (*SomeType)(nil)
For function returns, the following example is the primary detection:
func newIface() SomeInterface {
return &SomeType{}
}
More detections can be added in the future of course.
Fixes golang/go#37537
Change-Id: Ibb7784622184c9885eff2ccc786767682876b4d3
---
M internal/lsp/code_action.go
M internal/lsp/source/completion/completion.go
M internal/lsp/source/completion/labels.go
M internal/lsp/source/completion/statements.go
A internal/lsp/source/copy_ast.go
A internal/lsp/source/stub.go
M internal/lsp/source/util.go
A internal/lsp/testdata/stub/add_selector.go
A internal/lsp/testdata/stub/add_selector.go.golden
A internal/lsp/testdata/stub/embedded.go
A internal/lsp/testdata/stub/embedded.go.golden
A internal/lsp/testdata/stub/function_return.go
A internal/lsp/testdata/stub/function_return.go.golden
A internal/lsp/testdata/stub/ignored_imports.go
A internal/lsp/testdata/stub/ignored_imports.go.golden
A internal/lsp/testdata/stub/renamed_import.go
A internal/lsp/testdata/stub/renamed_import.go.golden
A internal/lsp/testdata/stub/stdlib.go
A internal/lsp/testdata/stub/stdlib.go.golden
M internal/lsp/testdata/summary.txt.golden
20 files changed, 1,435 insertions(+), 39 deletions(-)
diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go
index 9987eab..5bccc8c 100644
--- a/internal/lsp/code_action.go
+++ b/internal/lsp/code_action.go
@@ -77,6 +77,11 @@
return nil, nil
}
diagnostics := params.Context.Diagnostics
+ implActions, err := source.MethodStubActions(ctx, diagnostics, snapshot, fh)
+ if err != nil {
+ return nil, fmt.Errorf("implActions: %w", err)
+ }
+ codeActions = append(codeActions, implActions...)
// First, process any missing imports and pair them with the
// diagnostics they fix.
diff --git a/internal/lsp/source/completion/completion.go b/internal/lsp/source/completion/completion.go
index 19fcb5f..f8272aa 100644
--- a/internal/lsp/source/completion/completion.go
+++ b/internal/lsp/source/completion/completion.go
@@ -194,7 +194,7 @@
// enclosingFunc contains information about the function enclosing
// the position.
- enclosingFunc *funcInfo
+ enclosingFunc *source.FuncInfo
// enclosingCompositeLiteral contains information about the composite literal
// enclosing the position.
@@ -219,15 +219,6 @@
startTime time.Time
}
-// funcInfo holds info about a function object.
-type funcInfo struct {
- // sig is the function declaration enclosing the position.
- sig *types.Signature
-
- // body is the function's body.
- body *ast.BlockStmt
-}
-
type compLitInfo struct {
// cl is the *ast.CompositeLit enclosing the position.
cl *ast.CompositeLit
@@ -502,7 +493,7 @@
path: path,
pos: pos,
seen: make(map[types.Object]bool),
- enclosingFunc: enclosingFunction(path, pkg.GetTypesInfo()),
+ enclosingFunc: source.EnclosingFunction(path, pkg.GetTypesInfo()),
enclosingCompositeLiteral: enclosingCompositeLiteral(path, rng.Start, pkg.GetTypesInfo()),
deepState: deepCompletionState{
enabled: opts.DeepCompletion,
@@ -1642,30 +1633,6 @@
return nil
}
-// enclosingFunction returns the signature and body of the function
-// enclosing the given position.
-func enclosingFunction(path []ast.Node, info *types.Info) *funcInfo {
- for _, node := range path {
- switch t := node.(type) {
- case *ast.FuncDecl:
- if obj, ok := info.Defs[t.Name]; ok {
- return &funcInfo{
- sig: obj.Type().(*types.Signature),
- body: t.Body,
- }
- }
- case *ast.FuncLit:
- if typ, ok := info.Types[t]; ok {
- return &funcInfo{
- sig: typ.Type.(*types.Signature),
- body: t.Body,
- }
- }
- }
- }
- return nil
-}
-
func (c *completer) expectedCompositeLiteralType() types.Type {
clInfo := c.enclosingCompositeLiteral
switch t := clInfo.clType.(type) {
@@ -1985,7 +1952,7 @@
}
case *ast.ReturnStmt:
if c.enclosingFunc != nil {
- sig := c.enclosingFunc.sig
+ sig := c.enclosingFunc.Sig
// Find signature result that corresponds to our return statement.
if resultIdx := exprAtPos(c.pos, node.Results); resultIdx < len(node.Results) {
if resultIdx < sig.Results().Len() {
diff --git a/internal/lsp/source/completion/labels.go b/internal/lsp/source/completion/labels.go
index e4fd961..2854925 100644
--- a/internal/lsp/source/completion/labels.go
+++ b/internal/lsp/source/completion/labels.go
@@ -87,7 +87,7 @@
// Goto accepts any label in the same function not in a nested
// block. It also doesn't take labels that would jump across
// variable definitions, but ignore that case for now.
- ast.Inspect(c.enclosingFunc.body, func(n ast.Node) bool {
+ ast.Inspect(c.enclosingFunc.Body, func(n ast.Node) bool {
if n == nil {
return false
}
diff --git a/internal/lsp/source/completion/statements.go b/internal/lsp/source/completion/statements.go
index 62d3cf0..3e7ee82 100644
--- a/internal/lsp/source/completion/statements.go
+++ b/internal/lsp/source/completion/statements.go
@@ -179,7 +179,7 @@
var (
errorType = types.Universe.Lookup("error").Type()
- result = c.enclosingFunc.sig.Results()
+ result = c.enclosingFunc.Sig.Results()
)
// Make sure our enclosing function returns an error.
if result.Len() == 0 || !types.Identical(result.At(result.Len()-1).Type(), errorType) {
diff --git a/internal/lsp/source/copy_ast.go b/internal/lsp/source/copy_ast.go
new file mode 100644
index 0000000..1b82aec
--- /dev/null
+++ b/internal/lsp/source/copy_ast.go
@@ -0,0 +1,481 @@
+package source
+
+import (
+ "fmt"
+ "go/ast"
+
+ "golang.org/x/tools/go/ast/astutil"
+)
+
+// copied from github.com/google/wire
+
+// copyAST performs a deep copy of an AST. *ast.Ident identity will be
+// preserved.
+//
+// This allows using astutil.Apply to rewrite an AST without modifying
+// the original AST.
+func copyAST(original ast.Node) ast.Node {
+ // This function is necessarily long. No utility function exists to do this
+ // clone, as most any attempt would need to have customization options, which
+ // would need to be as expressive as Apply. A possibility to shorten the code
+ // here would be to use reflection, but that trades clarity for shorter code.
+
+ m := make(map[ast.Node]ast.Node)
+ astutil.Apply(original, nil, func(c *astutil.Cursor) bool {
+ switch node := c.Node().(type) {
+ case nil:
+ // No-op.
+ case *ast.ArrayType:
+ m[node] = &ast.ArrayType{
+ Lbrack: node.Lbrack,
+ Len: exprFromMap(m, node.Len),
+ Elt: exprFromMap(m, node.Elt),
+ }
+ case *ast.AssignStmt:
+ m[node] = &ast.AssignStmt{
+ Lhs: copyExprList(m, node.Lhs),
+ TokPos: node.TokPos,
+ Tok: node.Tok,
+ Rhs: copyExprList(m, node.Rhs),
+ }
+ case *ast.BadDecl:
+ m[node] = &ast.BadDecl{
+ From: node.From,
+ To: node.To,
+ }
+ case *ast.BadExpr:
+ m[node] = &ast.BadExpr{
+ From: node.From,
+ To: node.To,
+ }
+ case *ast.BadStmt:
+ m[node] = &ast.BadStmt{
+ From: node.From,
+ To: node.To,
+ }
+ case *ast.BasicLit:
+ m[node] = &ast.BasicLit{
+ ValuePos: node.ValuePos,
+ Kind: node.Kind,
+ Value: node.Value,
+ }
+ case *ast.BinaryExpr:
+ m[node] = &ast.BinaryExpr{
+ X: exprFromMap(m, node.X),
+ OpPos: node.OpPos,
+ Op: node.Op,
+ Y: exprFromMap(m, node.Y),
+ }
+ case *ast.BlockStmt:
+ m[node] = &ast.BlockStmt{
+ Lbrace: node.Lbrace,
+ List: copyStmtList(m, node.List),
+ Rbrace: node.Rbrace,
+ }
+ case *ast.BranchStmt:
+ m[node] = &ast.BranchStmt{
+ TokPos: node.TokPos,
+ Tok: node.Tok,
+ Label: identFromMap(m, node.Label),
+ }
+ case *ast.CallExpr:
+ m[node] = &ast.CallExpr{
+ Fun: exprFromMap(m, node.Fun),
+ Lparen: node.Lparen,
+ Args: copyExprList(m, node.Args),
+ Ellipsis: node.Ellipsis,
+ Rparen: node.Rparen,
+ }
+ case *ast.CaseClause:
+ m[node] = &ast.CaseClause{
+ Case: node.Case,
+ List: copyExprList(m, node.List),
+ Colon: node.Colon,
+ Body: copyStmtList(m, node.Body),
+ }
+ case *ast.ChanType:
+ m[node] = &ast.ChanType{
+ Begin: node.Begin,
+ Arrow: node.Arrow,
+ Dir: node.Dir,
+ Value: exprFromMap(m, node.Value),
+ }
+ case *ast.CommClause:
+ m[node] = &ast.CommClause{
+ Case: node.Case,
+ Comm: stmtFromMap(m, node.Comm),
+ Colon: node.Colon,
+ Body: copyStmtList(m, node.Body),
+ }
+ case *ast.Comment:
+ m[node] = &ast.Comment{
+ Slash: node.Slash,
+ Text: node.Text,
+ }
+ case *ast.CommentGroup:
+ cg := new(ast.CommentGroup)
+ if node.List != nil {
+ cg.List = make([]*ast.Comment, len(node.List))
+ for i := range node.List {
+ cg.List[i] = m[node.List[i]].(*ast.Comment)
+ }
+ }
+ m[node] = cg
+ case *ast.CompositeLit:
+ m[node] = &ast.CompositeLit{
+ Type: exprFromMap(m, node.Type),
+ Lbrace: node.Lbrace,
+ Elts: copyExprList(m, node.Elts),
+ Rbrace: node.Rbrace,
+ }
+ case *ast.DeclStmt:
+ m[node] = &ast.DeclStmt{
+ Decl: m[node.Decl].(ast.Decl),
+ }
+ case *ast.DeferStmt:
+ m[node] = &ast.DeferStmt{
+ Defer: node.Defer,
+ Call: callExprFromMap(m, node.Call),
+ }
+ case *ast.Ellipsis:
+ m[node] = &ast.Ellipsis{
+ Ellipsis: node.Ellipsis,
+ Elt: exprFromMap(m, node.Elt),
+ }
+ case *ast.EmptyStmt:
+ m[node] = &ast.EmptyStmt{
+ Semicolon: node.Semicolon,
+ Implicit: node.Implicit,
+ }
+ case *ast.ExprStmt:
+ m[node] = &ast.ExprStmt{
+ X: exprFromMap(m, node.X),
+ }
+ case *ast.Field:
+ m[node] = &ast.Field{
+ Doc: commentGroupFromMap(m, node.Doc),
+ Names: copyIdentList(m, node.Names),
+ Type: exprFromMap(m, node.Type),
+ Tag: basicLitFromMap(m, node.Tag),
+ Comment: commentGroupFromMap(m, node.Comment),
+ }
+ case *ast.FieldList:
+ fl := &ast.FieldList{
+ Opening: node.Opening,
+ Closing: node.Closing,
+ }
+ if node.List != nil {
+ fl.List = make([]*ast.Field, len(node.List))
+ for i := range node.List {
+ fl.List[i] = m[node.List[i]].(*ast.Field)
+ }
+ }
+ m[node] = fl
+ case *ast.ForStmt:
+ m[node] = &ast.ForStmt{
+ For: node.For,
+ Init: stmtFromMap(m, node.Init),
+ Cond: exprFromMap(m, node.Cond),
+ Post: stmtFromMap(m, node.Post),
+ Body: blockStmtFromMap(m, node.Body),
+ }
+ case *ast.FuncDecl:
+ m[node] = &ast.FuncDecl{
+ Doc: commentGroupFromMap(m, node.Doc),
+ Recv: fieldListFromMap(m, node.Recv),
+ Name: identFromMap(m, node.Name),
+ Type: funcTypeFromMap(m, node.Type),
+ Body: blockStmtFromMap(m, node.Body),
+ }
+ case *ast.FuncLit:
+ m[node] = &ast.FuncLit{
+ Type: funcTypeFromMap(m, node.Type),
+ Body: blockStmtFromMap(m, node.Body),
+ }
+ case *ast.FuncType:
+ m[node] = &ast.FuncType{
+ Func: node.Func,
+ Params: fieldListFromMap(m, node.Params),
+ Results: fieldListFromMap(m, node.Results),
+ }
+ case *ast.GenDecl:
+ decl := &ast.GenDecl{
+ Doc: commentGroupFromMap(m, node.Doc),
+ TokPos: node.TokPos,
+ Tok: node.Tok,
+ Lparen: node.Lparen,
+ Rparen: node.Rparen,
+ }
+ if node.Specs != nil {
+ decl.Specs = make([]ast.Spec, len(node.Specs))
+ for i := range node.Specs {
+ decl.Specs[i] = m[node.Specs[i]].(ast.Spec)
+ }
+ }
+ m[node] = decl
+ case *ast.GoStmt:
+ m[node] = &ast.GoStmt{
+ Go: node.Go,
+ Call: callExprFromMap(m, node.Call),
+ }
+ case *ast.Ident:
+ // Keep identifiers the same identity so they can be conveniently
+ // used with the original *types.Info.
+ m[node] = node
+ case *ast.IfStmt:
+ m[node] = &ast.IfStmt{
+ If: node.If,
+ Init: stmtFromMap(m, node.Init),
+ Cond: exprFromMap(m, node.Cond),
+ Body: blockStmtFromMap(m, node.Body),
+ Else: stmtFromMap(m, node.Else),
+ }
+ case *ast.ImportSpec:
+ m[node] = &ast.ImportSpec{
+ Doc: commentGroupFromMap(m, node.Doc),
+ Name: identFromMap(m, node.Name),
+ Path: basicLitFromMap(m, node.Path),
+ Comment: commentGroupFromMap(m, node.Comment),
+ EndPos: node.EndPos,
+ }
+ case *ast.IncDecStmt:
+ m[node] = &ast.IncDecStmt{
+ X: exprFromMap(m, node.X),
+ TokPos: node.TokPos,
+ Tok: node.Tok,
+ }
+ case *ast.IndexExpr:
+ m[node] = &ast.IndexExpr{
+ X: exprFromMap(m, node.X),
+ Lbrack: node.Lbrack,
+ Index: exprFromMap(m, node.Index),
+ Rbrack: node.Rbrack,
+ }
+ case *ast.InterfaceType:
+ m[node] = &ast.InterfaceType{
+ Interface: node.Interface,
+ Methods: fieldListFromMap(m, node.Methods),
+ Incomplete: node.Incomplete,
+ }
+ case *ast.KeyValueExpr:
+ m[node] = &ast.KeyValueExpr{
+ Key: exprFromMap(m, node.Key),
+ Colon: node.Colon,
+ Value: exprFromMap(m, node.Value),
+ }
+ case *ast.LabeledStmt:
+ m[node] = &ast.LabeledStmt{
+ Label: identFromMap(m, node.Label),
+ Colon: node.Colon,
+ Stmt: stmtFromMap(m, node.Stmt),
+ }
+ case *ast.MapType:
+ m[node] = &ast.MapType{
+ Map: node.Map,
+ Key: exprFromMap(m, node.Key),
+ Value: exprFromMap(m, node.Value),
+ }
+ case *ast.ParenExpr:
+ m[node] = &ast.ParenExpr{
+ Lparen: node.Lparen,
+ X: exprFromMap(m, node.X),
+ Rparen: node.Rparen,
+ }
+ case *ast.RangeStmt:
+ m[node] = &ast.RangeStmt{
+ For: node.For,
+ Key: exprFromMap(m, node.Key),
+ Value: exprFromMap(m, node.Value),
+ TokPos: node.TokPos,
+ Tok: node.Tok,
+ X: exprFromMap(m, node.X),
+ Body: blockStmtFromMap(m, node.Body),
+ }
+ case *ast.ReturnStmt:
+ m[node] = &ast.ReturnStmt{
+ Return: node.Return,
+ Results: copyExprList(m, node.Results),
+ }
+ case *ast.SelectStmt:
+ m[node] = &ast.SelectStmt{
+ Select: node.Select,
+ Body: blockStmtFromMap(m, node.Body),
+ }
+ case *ast.SelectorExpr:
+ m[node] = &ast.SelectorExpr{
+ X: exprFromMap(m, node.X),
+ Sel: identFromMap(m, node.Sel),
+ }
+ case *ast.SendStmt:
+ m[node] = &ast.SendStmt{
+ Chan: exprFromMap(m, node.Chan),
+ Arrow: node.Arrow,
+ Value: exprFromMap(m, node.Value),
+ }
+ case *ast.SliceExpr:
+ m[node] = &ast.SliceExpr{
+ X: exprFromMap(m, node.X),
+ Lbrack: node.Lbrack,
+ Low: exprFromMap(m, node.Low),
+ High: exprFromMap(m, node.High),
+ Max: exprFromMap(m, node.Max),
+ Slice3: node.Slice3,
+ Rbrack: node.Rbrack,
+ }
+ case *ast.StarExpr:
+ m[node] = &ast.StarExpr{
+ Star: node.Star,
+ X: exprFromMap(m, node.X),
+ }
+ case *ast.StructType:
+ m[node] = &ast.StructType{
+ Struct: node.Struct,
+ Fields: fieldListFromMap(m, node.Fields),
+ Incomplete: node.Incomplete,
+ }
+ case *ast.SwitchStmt:
+ m[node] = &ast.SwitchStmt{
+ Switch: node.Switch,
+ Init: stmtFromMap(m, node.Init),
+ Tag: exprFromMap(m, node.Tag),
+ Body: blockStmtFromMap(m, node.Body),
+ }
+ case *ast.TypeAssertExpr:
+ m[node] = &ast.TypeAssertExpr{
+ X: exprFromMap(m, node.X),
+ Lparen: node.Lparen,
+ Type: exprFromMap(m, node.Type),
+ Rparen: node.Rparen,
+ }
+ case *ast.TypeSpec:
+ m[node] = &ast.TypeSpec{
+ Doc: commentGroupFromMap(m, node.Doc),
+ Name: identFromMap(m, node.Name),
+ Assign: node.Assign,
+ Type: exprFromMap(m, node.Type),
+ Comment: commentGroupFromMap(m, node.Comment),
+ }
+ case *ast.TypeSwitchStmt:
+ m[node] = &ast.TypeSwitchStmt{
+ Switch: node.Switch,
+ Init: stmtFromMap(m, node.Init),
+ Assign: stmtFromMap(m, node.Assign),
+ Body: blockStmtFromMap(m, node.Body),
+ }
+ case *ast.UnaryExpr:
+ m[node] = &ast.UnaryExpr{
+ OpPos: node.OpPos,
+ Op: node.Op,
+ X: exprFromMap(m, node.X),
+ }
+ case *ast.ValueSpec:
+ m[node] = &ast.ValueSpec{
+ Doc: commentGroupFromMap(m, node.Doc),
+ Names: copyIdentList(m, node.Names),
+ Type: exprFromMap(m, node.Type),
+ Values: copyExprList(m, node.Values),
+ Comment: commentGroupFromMap(m, node.Comment),
+ }
+ default:
+ panic(fmt.Sprintf("unhandled AST node: %T", node))
+ }
+ return true
+ })
+ return m[original]
+}
+
+func commentGroupFromMap(m map[ast.Node]ast.Node, key *ast.CommentGroup) *ast.CommentGroup {
+ if key == nil {
+ return nil
+ }
+ return m[key].(*ast.CommentGroup)
+}
+
+func exprFromMap(m map[ast.Node]ast.Node, key ast.Expr) ast.Expr {
+ if key == nil {
+ return nil
+ }
+ return m[key].(ast.Expr)
+}
+
+func stmtFromMap(m map[ast.Node]ast.Node, key ast.Stmt) ast.Stmt {
+ if key == nil {
+ return nil
+ }
+ return m[key].(ast.Stmt)
+}
+
+func identFromMap(m map[ast.Node]ast.Node, key *ast.Ident) *ast.Ident {
+ if key == nil {
+ return nil
+ }
+ return m[key].(*ast.Ident)
+}
+
+func blockStmtFromMap(m map[ast.Node]ast.Node, key *ast.BlockStmt) *ast.BlockStmt {
+ if key == nil {
+ return nil
+ }
+ return m[key].(*ast.BlockStmt)
+}
+
+func fieldListFromMap(m map[ast.Node]ast.Node, key *ast.FieldList) *ast.FieldList {
+ if key == nil {
+ return nil
+ }
+ return m[key].(*ast.FieldList)
+}
+
+func callExprFromMap(m map[ast.Node]ast.Node, key *ast.CallExpr) *ast.CallExpr {
+ if key == nil {
+ return nil
+ }
+ return m[key].(*ast.CallExpr)
+}
+
+func basicLitFromMap(m map[ast.Node]ast.Node, key *ast.BasicLit) *ast.BasicLit {
+ if key == nil {
+ return nil
+ }
+ return m[key].(*ast.BasicLit)
+}
+
+func funcTypeFromMap(m map[ast.Node]ast.Node, key *ast.FuncType) *ast.FuncType {
+ if key == nil {
+ return nil
+ }
+ return m[key].(*ast.FuncType)
+}
+
+func copyExprList(m map[ast.Node]ast.Node, exprs []ast.Expr) []ast.Expr {
+ if exprs == nil {
+ return nil
+ }
+ newExprs := make([]ast.Expr, len(exprs))
+ for i := range exprs {
+ newExprs[i] = m[exprs[i]].(ast.Expr)
+ }
+ return newExprs
+}
+
+func copyStmtList(m map[ast.Node]ast.Node, stmts []ast.Stmt) []ast.Stmt {
+ if stmts == nil {
+ return nil
+ }
+ newStmts := make([]ast.Stmt, len(stmts))
+ for i := range stmts {
+ newStmts[i] = m[stmts[i]].(ast.Stmt)
+ }
+ return newStmts
+}
+
+func copyIdentList(m map[ast.Node]ast.Node, idents []*ast.Ident) []*ast.Ident {
+ if idents == nil {
+ return nil
+ }
+ newIdents := make([]*ast.Ident, len(idents))
+ for i := range idents {
+ newIdents[i] = m[idents[i]].(*ast.Ident)
+ }
+ return newIdents
+}
diff --git a/internal/lsp/source/stub.go b/internal/lsp/source/stub.go
new file mode 100644
index 0000000..99837c3
--- /dev/null
+++ b/internal/lsp/source/stub.go
@@ -0,0 +1,664 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package source
+
+import (
+ "bytes"
+ "context"
+ "fmt"
+ "go/ast"
+ "go/format"
+ "go/parser"
+ "go/token"
+ "go/types"
+ "strconv"
+ "strings"
+ "text/template"
+
+ "golang.org/x/tools/go/ast/astutil"
+ "golang.org/x/tools/internal/lsp/protocol"
+ "golang.org/x/tools/internal/span"
+)
+
+type methodData struct {
+ Method string
+ Interface string
+ Concrete string
+ Signature string
+}
+
+const tmpl = `// {{ .Method }} implements {{ .Interface }}
+func ({{ .Concrete }}) {{ .Method }}{{ .Signature }} {
+ panic("unimplemented")
+}
+`
+
+// MethodStubActions returns code actions that can generate interface
+// stubs to fix "missing method" actions. The CodeAction fix will contain the entire
+// source file as it might add new imports along with the interface stubs
+func MethodStubActions(ctx context.Context, diagnostics []protocol.Diagnostic, snapshot Snapshot, fh VersionedFileHandle) ([]protocol.CodeAction, error) {
+ actions := []protocol.CodeAction{}
+ for _, d := range diagnostics {
+ if !isMissingMethodErr(d) && !isConversionErr(d) {
+ continue
+ }
+ pkg, pgf, err := GetParsedFile(ctx, snapshot, fh, NarrowestPackage)
+ if err != nil {
+ return nil, fmt.Errorf("GetParsedFile: %w", err)
+ }
+ nodes, pos, err := getNodes(pgf, d)
+ if err != nil {
+ return nil, fmt.Errorf("getNodes: %w", err)
+ }
+ ir := getImplementRequest(nodes, pkg, pos)
+ if ir == nil {
+ continue
+ }
+ concreteFile, concreteFH, err := getFile(ctx, ir.concreteObj, snapshot)
+ if err != nil {
+ return nil, fmt.Errorf("getFile(concrete): %w", err)
+ }
+ ct := &concreteType{
+ pkg: ir.concreteObj.Pkg(),
+ fset: snapshot.FileSet(),
+ file: concreteFile,
+ tms: types.NewMethodSet(ir.concreteObj.Type()),
+ pms: types.NewMethodSet(types.NewPointer(ir.concreteObj.Type())),
+ }
+ missing, err := missingMethods(ctx, snapshot, ct, ir.ifaceObj, ir.ifacePkg, map[string]struct{}{})
+ if err != nil {
+ return nil, fmt.Errorf("missingMethods: %w", err)
+ }
+ if len(missing) == 0 {
+ return nil, nil
+ }
+ t := template.Must(template.New("").Parse(tmpl))
+ var methodsBuffer bytes.Buffer
+ for _, mi := range missing {
+ for _, m := range mi.missing {
+ var sig bytes.Buffer
+ nn, _ := astutil.PathEnclosingInterval(mi.file, m.Pos(), m.Pos())
+ var n ast.Node = nn[1].(*ast.Field).Type
+ n = copyAST(n)
+ n = astutil.Apply(n, func(c *astutil.Cursor) bool {
+ sel, ok := c.Node().(*ast.SelectorExpr)
+ if ok {
+ renamed := mightRenameSelector(ctx, c, sel, mi.pkg, ct)
+ removed := mightRemoveSelector(ctx, c, sel, mi.pkg, ct.pkg.Path())
+ return removed || renamed
+ }
+ ident, ok := c.Node().(*ast.Ident)
+ if ok {
+ return mightAddSelector(c, ident, ir.ifacePkg, ct)
+ }
+ return true
+ }, nil)
+ err = format.Node(&sig, snapshot.FileSet(), n)
+ if err != nil {
+ return nil, fmt.Errorf("could not format function signature: %w", err)
+ }
+ concrete := ir.concreteObj.Name()
+ if ir.pointer {
+ concrete = "*" + concrete
+ }
+ md := methodData{
+ Method: m.Name(),
+ Concrete: concrete,
+ Interface: getIfaceName(pkg, ir.ifacePkg, ir.ifaceObj),
+ Signature: strings.TrimPrefix(sig.String(), "func"),
+ }
+ err = t.Execute(&methodsBuffer, md)
+ if err != nil {
+ return nil, fmt.Errorf("error executing method template: %w", err)
+ }
+ methodsBuffer.WriteRune('\n')
+ }
+ }
+ nodes, _ = astutil.PathEnclosingInterval(concreteFile, ir.concreteObj.Pos(), ir.concreteObj.Pos())
+ var concBuf bytes.Buffer
+ err = format.Node(&concBuf, snapshot.FileSet(), concreteFile)
+ if err != nil {
+ return nil, fmt.Errorf("error formatting concrete file: %w", err)
+ }
+ concreteSrc := concBuf.Bytes()
+ insertPos := snapshot.FileSet().Position(nodes[1].End()).Offset
+ var buf bytes.Buffer
+ buf.Write(concreteSrc[:insertPos])
+ buf.WriteByte('\n')
+ buf.Write(methodsBuffer.Bytes())
+ buf.Write(concreteSrc[insertPos:])
+ fset := token.NewFileSet()
+ newF, err := parser.ParseFile(fset, concreteFile.Name.Name, buf.Bytes(), parser.ParseComments)
+ if err != nil {
+ return nil, fmt.Errorf("could not reparse file: %w", err)
+ }
+ for _, imp := range ct.addedImports {
+ astutil.AddNamedImport(fset, newF, imp.Name, imp.Path)
+ }
+ var source bytes.Buffer
+ err = format.Node(&source, fset, newF)
+ if err != nil {
+ return nil, fmt.Errorf("format.Node: %w", err)
+ }
+ _, pgf, err = GetParsedFile(ctx, snapshot, concreteFH, NarrowestPackage)
+ if err != nil {
+ return nil, fmt.Errorf("GetParsedFile(concrete): %w", err)
+ }
+ edits, err := computeTextEdits(ctx, snapshot, pgf, source.String())
+ if err != nil {
+ return nil, fmt.Errorf("computeTextEdit: %w", err)
+ }
+ actions = append(actions, protocol.CodeAction{
+ Title: fmt.Sprintf("Implement %s", getIfaceName(pkg, ir.ifacePkg, ir.ifaceObj)),
+ Diagnostics: []protocol.Diagnostic{d},
+ Kind: protocol.QuickFix,
+ Edit: protocol.WorkspaceEdit{
+ DocumentChanges: []protocol.TextDocumentEdit{
+ {
+ TextDocument: protocol.VersionedTextDocumentIdentifier{
+ Version: concreteFH.Version(),
+ TextDocumentIdentifier: protocol.TextDocumentIdentifier{
+ URI: protocol.URIFromSpanURI(concreteFH.URI()),
+ },
+ },
+ Edits: edits,
+ },
+ },
+ },
+ })
+ }
+ return actions, nil
+}
+
+func getIfaceName(pkg, ifacePkg Package, ifaceObj types.Object) string {
+ if pkg.PkgPath() == ifacePkg.PkgPath() {
+ return ifaceObj.Name()
+ }
+ return fmt.Sprintf("%s.%s", ifacePkg.Name(), ifaceObj.Name())
+}
+
+func getNodes(pgf *ParsedGoFile, d protocol.Diagnostic) ([]ast.Node, token.Pos, error) {
+ spn, err := pgf.Mapper.RangeSpan(d.Range)
+ if err != nil {
+ return nil, 0, err
+ }
+ rng, err := spn.Range(pgf.Mapper.Converter)
+ if err != nil {
+ return nil, 0, err
+ }
+ nodes, _ := astutil.PathEnclosingInterval(pgf.File, rng.Start, rng.End)
+ return nodes, rng.Start, nil
+}
+
+// mightRemoveSelector will replace a selector such as *models.User to just be *User.
+// This is needed if the interface method imports the same package where the concrete type
+// is going to implement that method
+func mightRemoveSelector(ctx context.Context, c *astutil.Cursor, sel *ast.SelectorExpr, ifacePkg Package, implPath string) bool {
+ obj := ifacePkg.GetTypesInfo().Uses[sel.Sel]
+ if obj == nil || obj.Pkg() == nil {
+ // shouldn't really happen because the selector
+ // is always coming from the given given package
+ return false
+ }
+ if obj.Pkg().Path() == implPath {
+ c.Replace(sel.Sel)
+ return false
+ }
+ return false
+}
+
+// mightRenameSelector will take a selector such as *models.User and rename it to *somethingelse.User
+// if the target conrete type file already imports the "models" package but has renamed it.
+// If the concrete type does not have the import file, then the import file will be added along with its
+// rename if the interface file has defined one.
+func mightRenameSelector(ctx context.Context, c *astutil.Cursor, sel *ast.SelectorExpr, ifacePkg Package, ct *concreteType) bool {
+ ident, ok := sel.X.(*ast.Ident)
+ if !ok {
+ return false
+ }
+ obj := ifacePkg.GetTypesInfo().Uses[ident]
+ if obj == nil {
+ return false
+ }
+ pn, ok := obj.(*types.PkgName)
+ if !ok {
+ return false
+ }
+ pkg := pn.Imported()
+ var hasImport bool
+ var importName string
+ for _, imp := range ct.file.Imports {
+ impPath, _ := strconv.Unquote(imp.Path.Value)
+ if impPath == pkg.Path() && !isIgnoredImport(imp) {
+ hasImport = true
+ importName = pkg.Name()
+ if imp.Name != nil && imp.Name.Name != pkg.Name() {
+ importName = imp.Name.Name
+ }
+ break
+ }
+ }
+ if hasImport {
+ sel.X = &ast.Ident{
+ Name: importName,
+ NamePos: ident.NamePos,
+ Obj: ident.Obj,
+ }
+ c.Replace(sel)
+ return false
+ }
+ // there is no import, let's add a new one
+ if pkg.Path() == ct.pkg.Path() {
+ // but not if we're importing the path to the concrete type
+ // in which case we're dropping this in mightRemoveSelector
+ return false
+ }
+ // if we're adding a new import to the concrete type file, and
+ // it has been renamed in the interface file, honor the rename.
+ if pn.Name() != pkg.Name() {
+ importName = pn.Name()
+ }
+ ct.addImport(importName, pkg.Path())
+ return false
+}
+
+// mightAddSelector takes an identifier such as "User" and might turn into a selector
+// such as "models.User". This is needed when an interface method references
+// a type declaration in its own package while the concrete type is in a different package.
+// If an import already exists, it will use that import's name. If it does not exist,
+// it will add it to the ct's *ast.File.
+func mightAddSelector(
+ c *astutil.Cursor,
+ ident *ast.Ident,
+ ifacePkg Package,
+ ct *concreteType,
+) bool {
+ obj := ifacePkg.GetTypesInfo().Uses[ident]
+ if obj == nil {
+ return false
+ }
+ n, ok := obj.Type().(*types.Named)
+ if !ok {
+ return false
+ }
+ pkg := n.Obj().Pkg()
+ if pkg == nil {
+ return false
+ }
+ pkgName := pkg.Name()
+ missingImport := true
+ for _, imp := range ct.file.Imports {
+ impPath, _ := strconv.Unquote(imp.Path.Value)
+ if pkg.Path() == impPath && !isIgnoredImport(imp) {
+ missingImport = false
+ if imp.Name != nil {
+ pkgName = imp.Name.Name
+ }
+ break
+ }
+ }
+ isNotImportingDestination := pkg.Path() != ct.pkg.Path()
+ if missingImport && isNotImportingDestination {
+ ct.addImport("", pkg.Path())
+ }
+ isLocalDeclaration := pkg.Path() == ifacePkg.PkgPath() && pkg.Path() != ct.pkg.Path()
+ isDotImport := pkg.Path() != ifacePkg.PkgPath() && pkg.Path() != ct.pkg.Path()
+ // the only reason we know it's a dotImport is because we never visit an Identifier
+ // that was part of a SelectorExpr.
+ if isLocalDeclaration || isDotImport {
+ c.Replace(&ast.SelectorExpr{
+ X: &ast.Ident{Name: pkgName},
+ Sel: ident,
+ })
+ return false
+ }
+ return false
+}
+
+/*
+missingMethods takes a concrete type and returns any missing methods for the given interface as well as
+any missing interface that might have been embedded to its parent. For example:
+
+type I interface {
+ io.Writer
+ Hello()
+}
+returns []*missingInterface{
+ {
+ iface: *types.Interface (io.Writer),
+ file: *ast.File: io.go,
+ missing []*types.Func{Write},
+ },
+ {
+ iface: *types.Interface (I),
+ file: *ast.File: myfile.go,
+ missing: []*types.Func{Hello}
+ },
+}
+*/
+func missingMethods(ctx context.Context, snapshot Snapshot, ct *concreteType, ifaceObj types.Object, ifacePkg Package, 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())
+ }
+ missing := []*missingInterface{}
+ for i := 0; i < iface.NumEmbeddeds(); i++ {
+ eiface := iface.Embedded(i).Obj()
+ depPkg := ifacePkg
+ if eiface.Pkg().Path() != ifacePkg.PkgPath() {
+ var err error
+ depPkg, err = ifacePkg.GetImport(eiface.Pkg().Path())
+ if err != nil {
+ return nil, err
+ }
+ }
+ em, err := missingMethods(ctx, snapshot, ct, eiface, depPkg, visited)
+ if err != nil {
+ return nil, err
+ }
+ missing = append(missing, em...)
+ }
+ astFile, _, err := getFile(ctx, ifaceObj, snapshot)
+ if err != nil {
+ return nil, fmt.Errorf("error getting iface file: %w", err)
+ }
+ mi := &missingInterface{
+ pkg: ifacePkg,
+ iface: iface,
+ file: astFile,
+ }
+ if mi.file == nil {
+ return nil, fmt.Errorf("could not find ast.File for %v", ifaceObj.Name())
+ }
+ for i := 0; i < iface.NumExplicitMethods(); i++ {
+ method := iface.ExplicitMethod(i)
+ if ct.doesNotHaveMethod(method.Name()) {
+ if _, ok := visited[method.Name()]; !ok {
+ mi.missing = append(mi.missing, method)
+ visited[method.Name()] = struct{}{}
+ }
+ }
+ if sel := ct.getMethodSelection(method.Name()); sel != nil {
+ implSig := sel.Type().(*types.Signature)
+ ifaceSig := method.Type().(*types.Signature)
+ if !types.Identical(ifaceSig, implSig) {
+ return nil, &mismatchError{
+ name: method.Name(),
+ have: implSig,
+ want: ifaceSig,
+ }
+ }
+ }
+ }
+ if len(mi.missing) > 0 {
+ missing = append(missing, mi)
+ }
+ return missing, nil
+}
+
+func getFile(ctx context.Context, obj types.Object, snapshot Snapshot) (*ast.File, VersionedFileHandle, error) {
+ objPos := snapshot.FileSet().Position(obj.Pos())
+ objFile := span.URIFromPath(objPos.Filename)
+ objectFH := snapshot.FindFile(objFile)
+ _, goFile, err := GetParsedFile(ctx, snapshot, objectFH, WidestPackage)
+ if err != nil {
+ return nil, nil, fmt.Errorf("GetParsedFile: %w", err)
+ }
+ return goFile.File, objectFH, nil
+}
+
+// missingInterface represents an interface
+// that has all or some of its methods missing
+// from the destination concrete type
+type missingInterface struct {
+ iface *types.Interface
+ file *ast.File
+ pkg Package
+ missing []*types.Func
+}
+
+func isMissingMethodErr(d protocol.Diagnostic) bool {
+ return d.Source == "compiler" && strings.Contains(d.Message, "missing method")
+}
+
+func isConversionErr(d protocol.Diagnostic) bool {
+ return d.Source == "compiler" && strings.HasPrefix(d.Message, "cannot convert")
+}
+
+type stubRequest struct {
+ ifacePkg Package
+ ifaceObj types.Object
+
+ concretePkg Package
+ concreteObj types.Object
+
+ pointer bool
+}
+
+// getImplementRequest determines whether the "missing method error"
+// can be used to deduced what the concrete and interface types are
+func getImplementRequest(nodes []ast.Node, pkg Package, pos token.Pos) *stubRequest {
+ if vs := isVariableDeclaration(nodes); vs != nil {
+ return fromValueSpec(vs, pkg, pos)
+ }
+ if ret, ok := isReturnStatement(nodes); ok {
+ ir, _ := getRequestFromReturn(pos, nodes, ret, pkg)
+ return ir
+ }
+ return nil
+}
+
+func isReturnStatement(path []ast.Node) (*ast.ReturnStmt, bool) {
+ for _, n := range path {
+ rs, ok := n.(*ast.ReturnStmt)
+ if ok {
+ return rs, true
+ }
+ }
+ return nil, false
+}
+
+func getRequestFromReturn(pos token.Pos, path []ast.Node, rs *ast.ReturnStmt, pkg Package) (*stubRequest, error) {
+ idx, err := getReturnIndex(rs, pos)
+ if err != nil {
+ return nil, err
+ }
+ n := rs.Results[idx]
+ // TODO: make(), and other builtins
+ if _, ok := n.(*ast.MapType); ok {
+ return nil, nil
+ }
+ if _, ok := n.(*ast.ArrayType); ok {
+ return nil, nil
+ }
+ var ir *stubRequest
+ ast.Inspect(n, func(n ast.Node) bool {
+ ident, ok := n.(*ast.Ident)
+ if !ok {
+ return true
+ }
+ obj, ok := pkg.GetTypesInfo().Uses[ident]
+ if !ok {
+ return true
+ }
+ concretePkg := pkg
+ if obj.Pkg().Path() != pkg.PkgPath() {
+ var err error
+ concretePkg, err = pkg.GetImport(obj.Pkg().Path())
+ if err != nil {
+ return true
+ }
+ }
+ _, ok = obj.(*types.TypeName)
+ if !ok {
+ return true
+ }
+ ir = &stubRequest{
+ concreteObj: obj,
+ concretePkg: concretePkg,
+ }
+ return false
+ })
+ if ir == nil {
+ return nil, nil
+ }
+ fi := EnclosingFunction(path, pkg.GetTypesInfo())
+ if fi == nil {
+ return nil, fmt.Errorf("could not find function in a return statement")
+ }
+ result, ok := fi.Sig.Results().At(idx).Type().(*types.Named)
+ if !ok {
+ return nil, nil
+ }
+ ir.ifaceObj = result.Obj()
+ ifacePkg := pkg
+ if ifacePkg.PkgPath() != ir.ifaceObj.Pkg().Path() {
+ ifacePkg, _ = pkg.GetImport(ir.ifaceObj.Pkg().Path())
+ }
+ ir.ifacePkg = ifacePkg
+ return ir, nil
+}
+
+func getReturnIndex(rs *ast.ReturnStmt, pos token.Pos) (int, error) {
+ for idx, r := range rs.Results {
+ if pos >= r.Pos() && pos <= r.End() {
+ return idx, nil
+ }
+ }
+ return -1, fmt.Errorf("pos %d not within return statement bounds: [%d-%d]", pos, rs.Pos(), rs.End())
+}
+
+func fromValueSpec(vs *ast.ValueSpec, pkg Package, pos token.Pos) *stubRequest {
+ var idx int
+ for i, vs := range vs.Values {
+ if pos >= vs.Pos() && pos <= vs.End() {
+ idx = i
+ break
+ }
+ }
+ valueNode := vs.Values[idx]
+ inspectNode := func(n ast.Node) (nodeObj types.Object, nodePkg Package) {
+ ast.Inspect(n, func(n ast.Node) bool {
+ ident, ok := n.(*ast.Ident)
+ if !ok {
+ return true
+ }
+ obj, ok := pkg.GetTypesInfo().Uses[ident]
+ if !ok {
+ return true
+ }
+ _, ok = obj.(*types.TypeName)
+ if !ok {
+ return true
+ }
+ nodePkg = pkg
+ if obj.Pkg().Path() != pkg.PkgPath() {
+ var err error
+ nodePkg, err = pkg.GetImport(obj.Pkg().Path())
+ if err != nil {
+ return true
+ }
+ }
+ nodeObj = obj
+ return false
+ })
+ return nodeObj, nodePkg
+ }
+ ifaceNode := vs.Type
+ callExp, ok := valueNode.(*ast.CallExpr)
+ // if the ValueSpec is `var _ = myInterface(...)`
+ // as opposed to `var _ myInterface = ...`
+ if ifaceNode == nil && ok {
+ ifaceNode = callExp.Fun
+ }
+ ifaceObj, ifacePkg := inspectNode(ifaceNode)
+ if ifaceObj == nil || ifacePkg == nil {
+ return nil
+ }
+ concreteObj, concretePkg := inspectNode(valueNode)
+ if concreteObj == nil || concretePkg == nil {
+ return nil
+ }
+ var pointer bool
+ ast.Inspect(valueNode, func(n ast.Node) bool {
+ if ue, ok := n.(*ast.UnaryExpr); ok && ue.Op == token.AND {
+ pointer = true
+ return false
+ }
+ if _, ok := n.(*ast.StarExpr); ok {
+ pointer = true
+ return false
+ }
+ return true
+ })
+ return &stubRequest{
+ concreteObj: concreteObj,
+ concretePkg: concretePkg,
+ ifaceObj: ifaceObj,
+ ifacePkg: ifacePkg,
+ pointer: pointer,
+ }
+}
+
+func isVariableDeclaration(nodes []ast.Node) *ast.ValueSpec {
+ for _, n := range nodes {
+ vs, ok := n.(*ast.ValueSpec)
+ if !ok {
+ continue
+ }
+ return vs
+ }
+ return nil
+}
+
+// concreteType is the destination type
+// that will implement the interface methods
+type concreteType struct {
+ pkg *types.Package
+ fset *token.FileSet
+ file *ast.File
+ tms, pms *types.MethodSet
+ addedImports []*addedImport
+}
+
+func (ct *concreteType) doesNotHaveMethod(name string) bool {
+ return ct.tms.Lookup(ct.pkg, name) == nil && ct.pms.Lookup(ct.pkg, name) == nil
+}
+
+func (ct *concreteType) getMethodSelection(name string) *types.Selection {
+ if sel := ct.tms.Lookup(ct.pkg, name); sel != nil {
+ return sel
+ }
+ return ct.pms.Lookup(ct.pkg, name)
+}
+
+func (ct *concreteType) addImport(name, path string) {
+ for _, imp := range ct.addedImports {
+ if imp.Name == name && imp.Path == path {
+ return
+ }
+ }
+ ct.addedImports = append(ct.addedImports, &addedImport{name, path})
+}
+
+// addedImport represents a newly added import
+// statement to the concrete type. If name is not
+// empty, then that import is required to have that name.
+type addedImport struct {
+ Name, Path string
+}
+
+func isIgnoredImport(imp *ast.ImportSpec) bool {
+ return imp.Name != nil && (imp.Name.Name == "." || imp.Name.Name == "_")
+}
+
+type mismatchError struct {
+ name string
+ have, want *types.Signature
+}
+
+func (me *mismatchError) Error() string {
+ return fmt.Sprintf("mimsatched %q function singatures:\nhave: %s\nwant: %s", me.name, me.have, me.want)
+}
diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go
index 1fffda5..1b3e9d2 100644
--- a/internal/lsp/source/util.go
+++ b/internal/lsp/source/util.go
@@ -510,3 +510,36 @@
return false
}
}
+
+// FuncInfo holds info about a function object.
+type FuncInfo struct {
+ // Sig is the function declaration enclosing the position.
+ Sig *types.Signature
+
+ // Body is the function's Body.
+ Body *ast.BlockStmt
+}
+
+// EnclosingFunction returns the signature and body of the function
+// enclosing the given position.
+func EnclosingFunction(path []ast.Node, info *types.Info) *FuncInfo {
+ for _, node := range path {
+ switch t := node.(type) {
+ case *ast.FuncDecl:
+ if obj, ok := info.Defs[t.Name]; ok {
+ return &FuncInfo{
+ Sig: obj.Type().(*types.Signature),
+ Body: t.Body,
+ }
+ }
+ case *ast.FuncLit:
+ if typ, ok := info.Types[t]; ok {
+ return &FuncInfo{
+ Sig: typ.Type.(*types.Signature),
+ Body: t.Body,
+ }
+ }
+ }
+ }
+ return nil
+}
diff --git a/internal/lsp/testdata/stub/add_selector.go b/internal/lsp/testdata/stub/add_selector.go
new file mode 100644
index 0000000..74c4064
--- /dev/null
+++ b/internal/lsp/testdata/stub/add_selector.go
@@ -0,0 +1,12 @@
+package stub
+
+import "io"
+
+// This file tests that if an interface
+// method references a type from its own package
+// then our implementation must add the import/package selector
+// in the concrete method if the concrete type is outside of the interface
+// package
+var _ io.ReaderFrom = &readerFrom{} //@suggestedfix("&", "quickfix")
+
+type readerFrom struct{}
diff --git a/internal/lsp/testdata/stub/add_selector.go.golden b/internal/lsp/testdata/stub/add_selector.go.golden
new file mode 100644
index 0000000..64c0fbe
--- /dev/null
+++ b/internal/lsp/testdata/stub/add_selector.go.golden
@@ -0,0 +1,19 @@
+-- suggestedfix_add_selector_10_23 --
+package stub
+
+import "io"
+
+// This file tests that if an interface
+// method references a type from its own package
+// then our implementation must add the import/package selector
+// in the concrete method if the concrete type is outside of the interface
+// package
+var _ io.ReaderFrom = &readerFrom{} //@suggestedfix("&", "quickfix")
+
+type readerFrom struct{}
+
+// ReadFrom implements io.ReaderFrom
+func (*readerFrom) ReadFrom(r io.Reader) (n int64, err error) {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/embedded.go b/internal/lsp/testdata/stub/embedded.go
new file mode 100644
index 0000000..9904df4
--- /dev/null
+++ b/internal/lsp/testdata/stub/embedded.go
@@ -0,0 +1,15 @@
+package stub
+
+import (
+ "context"
+ "io"
+)
+
+var _ embeddedInterface = (*embeddedConcrete)(nil) //@suggestedfix("(", "quickfix")
+
+type embeddedConcrete struct{}
+
+type embeddedInterface interface {
+ io.Reader
+ context.Context
+}
diff --git a/internal/lsp/testdata/stub/embedded.go.golden b/internal/lsp/testdata/stub/embedded.go.golden
new file mode 100644
index 0000000..fb92da2
--- /dev/null
+++ b/internal/lsp/testdata/stub/embedded.go.golden
@@ -0,0 +1,43 @@
+-- suggestedfix_embedded_8_27 --
+package stub
+
+import (
+ "context"
+ "io"
+ "time"
+)
+
+var _ embeddedInterface = (*embeddedConcrete)(nil) //@suggestedfix("(", "quickfix")
+
+type embeddedConcrete struct{}
+
+// Deadline implements embeddedInterface
+func (*embeddedConcrete) Deadline() (deadline time.Time, ok bool) {
+ panic("unimplemented")
+}
+
+// Done implements embeddedInterface
+func (*embeddedConcrete) Done() <-chan struct{} {
+ panic("unimplemented")
+}
+
+// Err implements embeddedInterface
+func (*embeddedConcrete) Err() error {
+ panic("unimplemented")
+}
+
+// Value implements embeddedInterface
+func (*embeddedConcrete) Value(key interface{}) interface{} {
+ panic("unimplemented")
+}
+
+// Read implements embeddedInterface
+func (*embeddedConcrete) Read(p []byte) (n int, err error) {
+ panic("unimplemented")
+}
+
+type embeddedInterface interface {
+ io.Reader
+ context.Context
+}
+
diff --git a/internal/lsp/testdata/stub/function_return.go b/internal/lsp/testdata/stub/function_return.go
new file mode 100644
index 0000000..e1268ae
--- /dev/null
+++ b/internal/lsp/testdata/stub/function_return.go
@@ -0,0 +1,11 @@
+package stub
+
+import (
+ "io"
+)
+
+func newCloser() io.Closer {
+ return &closer{} //@suggestedfix("&", "quickfix")
+}
+
+type closer struct{}
diff --git a/internal/lsp/testdata/stub/function_return.go.golden b/internal/lsp/testdata/stub/function_return.go.golden
new file mode 100644
index 0000000..f294d20
--- /dev/null
+++ b/internal/lsp/testdata/stub/function_return.go.golden
@@ -0,0 +1,18 @@
+-- suggestedfix_function_return_8_9 --
+package stub
+
+import (
+ "io"
+)
+
+func newCloser() io.Closer {
+ return &closer{} //@suggestedfix("&", "quickfix")
+}
+
+type closer struct{}
+
+// Close implements io.Closer
+func (closer) Close() error {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/ignored_imports.go b/internal/lsp/testdata/stub/ignored_imports.go
new file mode 100644
index 0000000..901c429
--- /dev/null
+++ b/internal/lsp/testdata/stub/ignored_imports.go
@@ -0,0 +1,18 @@
+package stub
+
+import (
+ "context"
+ . "time"
+ _ "time"
+)
+
+// This file tests that dot-imports and underscore imports
+// are properly ignored and that a new import is added to
+// refernece method types
+
+var (
+ _ = Time{}
+ _ context.Context = (*ignoredContext)(nil) //@suggestedfix("(", "quickfix")
+)
+
+type ignoredContext struct{}
diff --git a/internal/lsp/testdata/stub/ignored_imports.go.golden b/internal/lsp/testdata/stub/ignored_imports.go.golden
new file mode 100644
index 0000000..027d860
--- /dev/null
+++ b/internal/lsp/testdata/stub/ignored_imports.go.golden
@@ -0,0 +1,41 @@
+-- suggestedfix_ignored_imports_15_22 --
+package stub
+
+import (
+ "context"
+ "time"
+ . "time"
+ _ "time"
+)
+
+// This file tests that dot-imports and underscore imports
+// are properly ignored and that a new import is added to
+// refernece method types
+
+var (
+ _ = Time{}
+ _ context.Context = (*ignoredContext)(nil) //@suggestedfix("(", "quickfix")
+)
+
+type ignoredContext struct{}
+
+// Deadline implements context.Context
+func (*ignoredContext) Deadline() (deadline time.Time, ok bool) {
+ panic("unimplemented")
+}
+
+// Done implements context.Context
+func (*ignoredContext) Done() <-chan struct{} {
+ panic("unimplemented")
+}
+
+// Err implements context.Context
+func (*ignoredContext) Err() error {
+ panic("unimplemented")
+}
+
+// Value implements context.Context
+func (*ignoredContext) Value(key interface{}) interface{} {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/renamed_import.go b/internal/lsp/testdata/stub/renamed_import.go
new file mode 100644
index 0000000..543f404
--- /dev/null
+++ b/internal/lsp/testdata/stub/renamed_import.go
@@ -0,0 +1,11 @@
+package stub
+
+import (
+ "context"
+ mytime "time"
+)
+
+var _ context.Context = &myContext{} //@suggestedfix("&", "quickfix")
+var _ = mytime.Time{}
+
+type myContext struct{}
diff --git a/internal/lsp/testdata/stub/renamed_import.go.golden b/internal/lsp/testdata/stub/renamed_import.go.golden
new file mode 100644
index 0000000..d8db064
--- /dev/null
+++ b/internal/lsp/testdata/stub/renamed_import.go.golden
@@ -0,0 +1,33 @@
+-- suggestedfix_renamed_import_8_25 --
+package stub
+
+import (
+ "context"
+ mytime "time"
+)
+
+var _ context.Context = &myContext{} //@suggestedfix("&", "quickfix")
+var _ = mytime.Time{}
+
+type myContext struct{}
+
+// Deadline implements context.Context
+func (*myContext) Deadline() (deadline mytime.Time, ok bool) {
+ panic("unimplemented")
+}
+
+// Done implements context.Context
+func (*myContext) Done() <-chan struct{} {
+ panic("unimplemented")
+}
+
+// Err implements context.Context
+func (*myContext) Err() error {
+ panic("unimplemented")
+}
+
+// Value implements context.Context
+func (*myContext) Value(key interface{}) interface{} {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/stdlib.go b/internal/lsp/testdata/stub/stdlib.go
new file mode 100644
index 0000000..63815d4
--- /dev/null
+++ b/internal/lsp/testdata/stub/stdlib.go
@@ -0,0 +1,9 @@
+package stub
+
+import (
+ "io"
+)
+
+var _ io.Writer = writer{} //@suggestedfix("w", "quickfix")
+
+type writer struct{}
diff --git a/internal/lsp/testdata/stub/stdlib.go.golden b/internal/lsp/testdata/stub/stdlib.go.golden
new file mode 100644
index 0000000..79ae730
--- /dev/null
+++ b/internal/lsp/testdata/stub/stdlib.go.golden
@@ -0,0 +1,16 @@
+-- suggestedfix_stdlib_7_19 --
+package stub
+
+import (
+ "io"
+)
+
+var _ io.Writer = writer{} //@suggestedfix("w", "quickfix")
+
+type writer struct{}
+
+// Write implements io.Writer
+func (writer) Write(p []byte) (n int, err error) {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden
index 0f327f6..63a3131 100644
--- a/internal/lsp/testdata/summary.txt.golden
+++ b/internal/lsp/testdata/summary.txt.golden
@@ -13,7 +13,7 @@
FormatCount = 6
ImportCount = 8
SemanticTokenCount = 3
-SuggestedFixCount = 38
+SuggestedFixCount = 44
FunctionExtractionCount = 12
DefinitionsCount = 64
TypeDefinitionsCount = 2
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Marwan Sulaiman uploaded patch set #2 to this change.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #3 to this change.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #4 to this change.
A internal/lsp/testdata/stub/multi_var.go
A internal/lsp/testdata/stub/multi_var.go.golden
A internal/lsp/testdata/stub/renamed_import.go
A internal/lsp/testdata/stub/renamed_import.go.golden
A internal/lsp/testdata/stub/stdlib.go
A internal/lsp/testdata/stub/stdlib.go.golden
M internal/lsp/testdata/summary.txt.golden
22 files changed, 1,464 insertions(+), 39 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Rebecca Stambler.
Rebecca Stambler removed Ian Cottrell from this change.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman.
1 comment:
Patchset:
Thanks for working on this! Two larger questions:
(1) Would it be possible to fit this under the category of "convenience" code actions? These are supposed to be ones like fillstruct that we want to show whenever possible, but we don't want to show the user diagnostics for them. Then, instead of returning suggested fixes directly, you would return an "implement" command which will only apply fixes when the user selects it (otherwise, we will always compute suggested fixes, even when the user hasn't requested them).
(2) I might consider implementing this via the go/analysis package, using what we've called a "type error analyzer" (for example, https://github.com/golang/tools/blob/master/internal/lsp/analysis/undeclaredname/undeclared.go). This analyzer matches on error messages and then provides diagnostics/suggested fixes. The benefit of using go/analysis is that it automatically caches results, so they only have to be recomputed if the package changed.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
1 comment:
Patchset:
Thanks for working on this! Two larger questions: […]
1. That definitely makes sense.
2. I started giving this a shot after finishing 1 locally and I'm stuck with the following: the SuggestedFix function signature doesn't have access to the snapshot or source.Package. And part of the code above (specifically in the getFile function) is that I need to go from a "types.Object" to the "*ast.File" it is defined in. @Rebecca, is there a way to do that or will I need to update the signature to pass the snapshot into the SuggestedFix?
Thanks!
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman.
1 comment:
Patchset:
1. That definitely makes sense. […]
Ah I see, yeah thanks for pointing that out. I think we can change the signature of the command.SuggestedFix function probably, but then suggested fix logic for this analyzer would have to be defined in the internal/lsp/source package to avoid a circular dependency. That should be fine though. If you like, I can mail a CL to do this refactoring, or would you prefer to do it?
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
1 comment:
Patchset:
If you like, I can mail a CL to do this refactoring
Please do! I'm not sure how I can refactor the signature to accept source.Snapshot without moving _all_ of the lsp/analysis sub directories (and not just the one im writing) into `source` to avoid cyclic dependencies...since they will have to update their function signature to import source.Snapshot.
The only thing I can think of is to move the Snapshot interface to its own package which feels a little scary to me, so will hold on to see what you have in mind!
Thanks 🙏
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman.
1 comment:
Patchset:
> If you like, I can mail a CL to do this refactoring […]
I was thinking of changing the SuggestedFixFunc type to func(snapshot source.Snapshot, pkg source.Package, rng span.Range) and then writing a helper to pass in the correct arguments to the functions in other packages. I'm not sure how soon I'll be able to do this, so if what I said sounds reasonable and you want to make progress sooner, you could try it out yourself. Happy to explain further if I wasn't clear--this is definitely a convoluted part of the codebase.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
1 comment:
Patchset:
I was thinking of changing the SuggestedFixFunc type to func(snapshot source.Snapshot, pkg source. […]
Hi Rebecca,
Yeah I would love a further explanation on the following:
The SuggestedFixFunc is defined in `package source`, so the signature will now look like this:
```
package sourcetype SuggestedFixFunc func(snapshot Snapshot, pkg Package, rng span.Range) (*analysis.SuggestedFix, error)
type
```But if you take `lsp/analysis/fillstruct` as an example, the problem is that `source/command.go` imports `fillstruct` to reference its suggestedFixFunc...and package fillstruct itself defines the suggestedFixFunc.
Therefore, if I update the signature above, the compiler will complain about cyclic dependencies: `fillstruct` will now have to import `source.Snapshot` for its suggestedFixFunc signature. And `source` already imports fillstruct.
So the only two solutions I can think of are to either:1. Move those analysis functions _outside_ of `package analysis/*` and into `package source`
2. Move source.Snapshot, source.Package (and all of their dependencies) into a new package like `lsp/snapshot` or something.
Both options will end up moving a significant number of code around so I'd love to know which one you prefer and if there's a third option in case I'm kind of missing something (which is super likely)Appreciate your time and help!
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman.
1 comment:
Patchset:
Hi Rebecca, […]
Sorry--my explanation wasn't clear enough--this is what I meant by "then writing a helper to pass in the correct arguments to the functions in other packages", but I should've explained more clearly.
What I mean is that, rather than use fillstruct.SuggestedFix directly here (https://github.com/golang/tools/blob/fa10ef0b874390a66d58cbcdc94ddedf859b5408/internal/lsp/source/command.go#L151), we could change that to:
suggestedFixFn: func(snapshot Snapshot, pkg Package, rng span.Range) {
f, err := pkg.File(rng.URI())
...
// get the other arguments to pass in from the pkg and snapshot
return fillstruct.SuggestedFix(...)
}Does this make more sense? There might be something that I'm missing here, but I believe that should work.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
1 comment:
Patchset:
Sorry--my explanation wasn't clear enough--this is what I meant by "then writing a helper to pass in […]
Hi Rebecca,
Yes that makes a lot of sense. Thank you for the clarification!One thing I'm noticing is that `func (c *Command) SuggestedFix` is assuming that the fixes are always applied towards the file that had the diagnostic error message. But that may not be case for this feature, for example the error can happen in one file but the concrete type might be defined in another file or another package completely.
Should I also update `func (c *Command) SuggestedFix` to not make this assumption?
Thanks!
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Patchset:
Hi Rebecca, […]
Hi again 👋🏼
I created a smaller CL which implements your suggestion above as a prerequisite to this change to keep things more small and contained: https://go-review.googlesource.com/c/tools/+/279412
Once that CL is approved/merged, I will rebase from master and continue with all of the other suggestions you made.
Thanks and happy holidays!
Marwan
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #5 to this change.
gopls,internal/lsp: Implement method stubbing via CodeAction
This CL adds a quickfix CodeAction that detects "missing method"
compiler errors and suggests adding method stubs to the concrete
type that would implement the interface. There are many ways that
a user might indicate a concrete type is meant to be used as an interface.
This PR detects two types of those errors: variable declaration and function returns.
For variable declarations, things like the following should be detected:
1. var _ SomeInterface = SomeType{}
2. var _ = SomeInterface(SomeType{})
3. var _ SomeInterface = (*SomeType)(nil)
For function returns, the following example is the primary detection:
func newIface() SomeInterface {
return &SomeType{}
}
More detections can be added in the future of course.
Fixes golang/go#37537
Change-Id: Ibb7784622184c9885eff2ccc786767682876b4d3
---
M gopls/doc/commands.md
A internal/lsp/analysis/stubmethods/stubmethods.go
M internal/lsp/analysis/unusedparams/unusedparams.go
A internal/lsp/lsputil/lsputil.go
M internal/lsp/source/api_json.go
M internal/lsp/source/command.go
M internal/lsp/source/completion/completion.go
M internal/lsp/source/completion/labels.go
M internal/lsp/source/completion/statements.go
A internal/lsp/source/copy_ast.go
M internal/lsp/source/extract.go
M internal/lsp/source/options.go
A internal/lsp/source/stub.go
A internal/lsp/testdata/stub/add_selector.go
A internal/lsp/testdata/stub/add_selector.go.golden
A internal/lsp/testdata/stub/embedded.go
A internal/lsp/testdata/stub/embedded.go.golden
A internal/lsp/testdata/stub/function_return.go
A internal/lsp/testdata/stub/function_return.go.golden
A internal/lsp/testdata/stub/ignored_imports.go
A internal/lsp/testdata/stub/ignored_imports.go.golden
A internal/lsp/testdata/stub/multi_var.go
A internal/lsp/testdata/stub/multi_var.go.golden
A internal/lsp/testdata/stub/renamed_import.go
A internal/lsp/testdata/stub/renamed_import.go.golden
A internal/lsp/testdata/stub/stdlib.go
A internal/lsp/testdata/stub/stdlib.go.golden
M internal/lsp/testdata/summary.txt.golden
28 files changed, 1,703 insertions(+), 87 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
1 comment:
Patchset:
Hi Rebecca,
I merged the SuggestedFixFunc change into this CL and also applied your initial suggestions as well.
So it's ready for review on my side! Definitely no rush given the holidays and that nothing will be merged till February anyways.
Thank you!
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman.
Marwan Sulaiman uploaded patch set #6 to this change.
gopls,internal/lsp: Implement method stubbing via CodeAction
This CL adds a quickfix CodeAction that detects "missing method"
compiler errors and suggests adding method stubs to the concrete
type that would implement the interface. There are many ways that
a user might indicate a concrete type is meant to be used as an interface.
This PR detects two types of those errors: variable declaration and function returns.
For variable declarations, things like the following should be detected:
1. var _ SomeInterface = SomeType{}
2. var _ = SomeInterface(SomeType{})
3. var _ SomeInterface = (*SomeType)(nil)
For function returns, the following example is the primary detection:
func newIface() SomeInterface {
return &SomeType{}
}
More detections can be added in the future of course.
Fixes golang/go#37537
Change-Id: Ibb7784622184c9885eff2ccc786767682876b4d3
---
A internal/lsp/analysis/stubmethods/stubmethods.go
A internal/lsp/lsputil/lsputil.go
M internal/lsp/source/completion/completion.go
M internal/lsp/source/completion/labels.go
M internal/lsp/source/completion/statements.go
A internal/lsp/source/copy_ast.go
M internal/lsp/source/fix.go
M internal/lsp/source/options.go
A internal/lsp/source/stub.go
A internal/lsp/testdata/stub/add_selector.go
A internal/lsp/testdata/stub/add_selector.go.golden
A internal/lsp/testdata/stub/embedded.go
A internal/lsp/testdata/stub/embedded.go.golden
A internal/lsp/testdata/stub/function_return.go
A internal/lsp/testdata/stub/function_return.go.golden
A internal/lsp/testdata/stub/ignored_imports.go
A internal/lsp/testdata/stub/ignored_imports.go.golden
A internal/lsp/testdata/stub/multi_var.go
A internal/lsp/testdata/stub/multi_var.go.golden
A internal/lsp/testdata/stub/renamed_import.go
A internal/lsp/testdata/stub/renamed_import.go.golden
A internal/lsp/testdata/stub/stdlib.go
A internal/lsp/testdata/stub/stdlib.go.golden
M internal/lsp/testdata/summary.txt.golden
24 files changed, 1,585 insertions(+), 42 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
14 comments:
Patchset:
Hi Rebecca,
Appreciate the first round of interviews from last time! Now that the first small part was merged in a different CL, I think this one is ready to go.I updated the code to take into account the refactors that went in since this CL was first opened. So it should hopefully be ready for another round of reviews.
Thanks,
Marwan
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #5, Line 1:
package stubmethodsthis file and all other new files need copyright notices
Ack
Patch Set #5, Line 53:
nodessuper nit: we usually call this return "path"
Ack
Patch Set #5, Line 66:
getIfaceNameyou should be able to use types.ObjectString with a types. […]
TIL. I think it'll have to be types.TypeString though because types.ObjectString seems to return the entire interface as opposed to just its name.
if ir == nil {
return false
}
hasIface := ir.IfacePkg != nil && ir.IfaceObj != nil
hasConc := ir.ConcretePkg != nil && ir.ConcreteObj != nil
return hasIface && hasConci would just inline this since it's only called once
Ack
Patch Set #5, Line 120:
getRequestFromReturnLooks like the only call ignores the error return. Remove?
I'd actually love to not ignore it but instead log out the error. But since I don't have access to a context.Context, I can't call `event.Log`, unless it's okay to pass a background context? Thanks!
And will change the name and add comments for sure.
Patch Set #5, Line 126:
// TODO: make(), and other builtinsnot sure what this means here?
I added a more comprehensive comment. Definitely let me know if it needs to be reworded or if it's still unclear. Thanks!
if _, ok := n.(*ast.MapType); ok {
return nil, nil
}
if _, ok := n.(*ast.ArrayType); ok {
return nil, nil
}switch n.(type) { […]
Ack
Patch Set #5, Line 134:
InspectMaybe i'm not understanding the intention of the ast. […]
The reason this is needed is to deconstruct the "return" statement to be able to extract the concrete type that is meant to be implementing the interface defined in the function signature.
For example, given the following:
```
type T struct{}
func GetWriter() io.Writer { return T{} }
```What will happen is the analyzer would catch the "missing method" error and we will then try to get the concrete type in the return statement so we can know which concrete type needs to have those stub methods generated.
That said, this function doesn't account for all use cases such as
```
type I int
func GetWriter(a, b I) io.Writer { return a + b }
```But I imagine there are many more edge cases such as make(T) and others that we can add one at a time in follow up CLs.
That said, definitely let me know if there's anything I'm missing here. Thanks!
Patch Set #5, Line 168:
Results().At(idx).Type()can any of these be nil?
I really hope not. Because by the time we get to this error, we already figured out that there was a "missing method" error that was inside a return statement at the given index.
But definitely let me know if I should guard against nil here in case I missed an edge case.
ast.Inspect(n, func(n ast.Node) bool {
ident, ok := n.(*ast.Ident)
if !ok {
return true
}
obj, ok := ti.Uses[ident]
if !ok {
return true
}
_, ok = obj.(*types.TypeName)
if !ok {
return true
}
nodePkg = pkg
if obj.Pkg().Path() != pkg.Path() {
nodePkg = getImport(obj.Pkg().Path())
if nodePkg == nil {
return true
}
}
nodeObj = obj
return false
})this looks like the same code as in the ast.Inspect above. […]
There are a couple of subtle differences because they deal with closures that have different contexts. For example, a variable declaration has the interface type right within it, but a return statement has the interface declaration defined all the way up in its function signature.
I'd definitely be happy to try and extract the common statements, let me know!
var pointer bool
ast.Inspect(valueNode, func(n ast.Node) bool {
if ue, ok := n.(*ast.UnaryExpr); ok && ue.Op == token.AND {
pointer = true
return false
}
if _, ok := n.(*ast.StarExpr); ok {
pointer = true
return false
}
return true
})can you use the type information to look for a https://pkg.go. […]
Not sure how tbh, but would love to see an example and test if I can replace the above. Thanks
func isReturnStatement(path []ast.Node) (*ast.ReturnStmt, bool) {
for _, n := range path {
rs, ok := n.(*ast.ReturnStmt)
if ok {
return rs, true
}
}
return nil, false
}
func getReturnIndex(rs *ast.ReturnStmt, pos token.Pos) (int, error) {
for idx, r := range rs.Results {
if pos >= r.Pos() && pos <= r.End() {
return idx, nil
}
}
return -1, fmt.Errorf("pos %d not within return statement bounds: [%d-%d]", pos, rs.Pos(), rs.End())
}
these functions are small enough that i would inline them
Ack
// FixesError checks whether an error message
// is an interface implementation error
func FixesError(msg string) bool {
return strings.Contains(msg, "missing method") || strings.HasPrefix(msg, "cannot convert")
}This will be unnecessary after https://go-review.googlesource.com/c/tools/+/297879.
I see! But I _think_ I still need it in the "run" function above? If so, I'll just inline it.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #7 to this change.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #8 to this change.
A internal/lsp/testdata/stub/stub_add_selector.go
A internal/lsp/testdata/stub/stub_add_selector.go.golden
A internal/lsp/testdata/stub/stub_embedded.go
A internal/lsp/testdata/stub/stub_embedded.go.golden
A internal/lsp/testdata/stub/stub_function_return.go
A internal/lsp/testdata/stub/stub_function_return.go.golden
A internal/lsp/testdata/stub/stub_ignored_imports.go
A internal/lsp/testdata/stub/stub_ignored_imports.go.golden
A internal/lsp/testdata/stub/stub_multi_var.go
A internal/lsp/testdata/stub/stub_multi_var.go.golden
A internal/lsp/testdata/stub/stub_pointer.go
A internal/lsp/testdata/stub/stub_pointer.go.golden
A internal/lsp/testdata/stub/stub_renamed_import.go
A internal/lsp/testdata/stub/stub_renamed_import.go.golden
A internal/lsp/testdata/stub/stub_stdlib.go
A internal/lsp/testdata/stub/stub_stdlib.go.golden
M internal/lsp/testdata/summary.txt.golden
26 files changed, 1,595 insertions(+), 42 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
1 comment:
File internal/lsp/analysis/stubmethods/stubmethods.go:
ast.Inspect(n, func(n ast.Node) bool {
ident, ok := n.(*ast.Ident)
if !ok {
return true
}
obj, ok := ti.Uses[ident]
if !ok {
return true
}
_, ok = obj.(*types.TypeName)
if !ok {
return true
}
nodePkg = pkg
if obj.Pkg().Path() != pkg.Path() {
nodePkg = getImport(obj.Pkg().Path())
if nodePkg == nil {
return true
}
}
nodeObj = obj
return false
})
There are a couple of subtle differences because they deal with closures that have different context […]
Actually, found a way to put them all in a helper function and so far so good. Thanks for the suggestion!
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #9 to this change.
26 files changed, 1,576 insertions(+), 42 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #10 to this change.
A internal/lsp/testdata/stub/stub_assign.go
A internal/lsp/testdata/stub/stub_assign.go.golden
A internal/lsp/testdata/stub/stub_assign_multivars.go
A internal/lsp/testdata/stub/stub_assign_multivars.go.golden
A internal/lsp/testdata/stub/stub_embedded.go
A internal/lsp/testdata/stub/stub_embedded.go.golden
A internal/lsp/testdata/stub/stub_err.go
A internal/lsp/testdata/stub/stub_err.go.golden
A internal/lsp/testdata/stub/stub_function_return.go
A internal/lsp/testdata/stub/stub_function_return.go.golden
A internal/lsp/testdata/stub/stub_ignored_imports.go
A internal/lsp/testdata/stub/stub_ignored_imports.go.golden
A internal/lsp/testdata/stub/stub_multi_var.go
A internal/lsp/testdata/stub/stub_multi_var.go.golden
A internal/lsp/testdata/stub/stub_pointer.go
A internal/lsp/testdata/stub/stub_pointer.go.golden
A internal/lsp/testdata/stub/stub_renamed_import.go
A internal/lsp/testdata/stub/stub_renamed_import.go.golden
A internal/lsp/testdata/stub/stub_stdlib.go
A internal/lsp/testdata/stub/stub_stdlib.go.golden
M internal/lsp/testdata/summary.txt.golden
32 files changed, 1,725 insertions(+), 42 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #11 to this change.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
3 comments:
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #5, Line 126:
// TODO: make(), and other builtinsI added a more comprehensive comment. […]
Looks like if I used `*types.Info.Types` instead of `*types.Info.Uses`, I actually get intended type for free without having to manually analyze the ast. So this whole block is unneeded, hopefully!
Patch Set #5, Line 134:
Inspect
The reason this is needed is to deconstruct the "return" statement to be able to extract the concret […]
Likewise here, the above is still true but by using *types.Info.Types, we get make(T) and other expressions such as "return a + b" for free.
var pointer bool
ast.Inspect(valueNode, func(n ast.Node) bool {
if ue, ok := n.(*ast.UnaryExpr); ok && ue.Op == token.AND {
pointer = true
return false
}
if _, ok := n.(*ast.StarExpr); ok {
pointer = true
return false
}
return true
})
Not sure how tbh, but would love to see an example and test if I can replace the above. […]
I think I figured out how to do this. Would love to double check if it looks correct to you. Thanks!
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #12 to this change.
32 files changed, 1,718 insertions(+), 42 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #13 to this change.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #14 to this change.
32 files changed, 1,722 insertions(+), 42 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #15 to this change.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #16 to this change.
32 files changed, 1,725 insertions(+), 42 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman.
11 comments:
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #5, Line 66:
getIfaceNameTIL. I think it'll have to be types.TypeString though because types. […]
Ack
Patch Set #5, Line 120:
getRequestFromReturn> Looks like the only call ignores the error return. Remove? […]
I think it might be too tricky to get a context in there, so let's leave it as-is for now. Or you can use log.Printf.
Patch Set #5, Line 126:
// TODO: make(), and other builtins
Looks like if I used `*types.Info.Types` instead of `*types.Info. […]
Ack
Patch Set #5, Line 134:
Inspect
Likewise here, the above is still true but by using *types.Info. […]
Ack
ast.Inspect(n, func(n ast.Node) bool {
ident, ok := n.(*ast.Ident)
if !ok {
return true
}
obj, ok := ti.Uses[ident]
if !ok {
return true
}
_, ok = obj.(*types.TypeName)
if !ok {
return true
}
nodePkg = pkg
if obj.Pkg().Path() != pkg.Path() {
nodePkg = getImport(obj.Pkg().Path())
if nodePkg == nil {
return true
}
}
nodeObj = obj
return false
})
Actually, found a way to put them all in a helper function and so far so good. […]
Ack
var pointer bool
ast.Inspect(valueNode, func(n ast.Node) bool {
if ue, ok := n.(*ast.UnaryExpr); ok && ue.Op == token.AND {
pointer = true
return false
}
if _, ok := n.(*ast.StarExpr); ok {
pointer = true
return false
}
return true
})
I think I figured out how to do this. Would love to double check if it looks correct to you. […]
Ack
File internal/lsp/analysis/stubmethods/stubmethods.go:
// Get the end position of the error.
_, _, endPos, ok := typesinternal.ReadGo116ErrorData(err)
if !ok {
var buf bytes.Buffer
if err := format.Node(&buf, pass.Fset, file); err != nil {
continue
}
endPos = analysisinternal.TypeErrorEndPos(pass.Fset, buf.Bytes(), err.Pos)
}
path, _ := astutil.PathEnclosingInterval(file, err.Pos, endPos)Do you need to get the actual end position? Usually you can just use the start position twice
Patch Set #16, Line 87:
StubInfodoes this need to be exported?
nit: period here
File internal/lsp/lsputil/lsputil.go:
Patch Set #16, Line 22:
EnclosingFunctionNot sure if we need this, and if we do, let's have it in a separate change.
We usually prefer a little copying to a little dependency, and I'm not sure if this code is complicated enough to justify a helper.
File internal/lsp/source/fix.go:
Patch Set #16, Line 49:
stubSuggestedFixfuncThe logic to do the suggested fix should live in the same file/directory as the analyzer itself--just to make things easier to understand. Can you add a function with the signature SuggestedFix(fset *token.FileSet, rng span.Range, content []byte, file *ast.File, pkg *types.Package, info *types.Info) (*analysis.SuggestedFix, error) to stubmethods?
(Sorry if this is not possible and we discussed it earlier--I may be forgetting some things)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
7 comments:
Patchset:
Thank you for continuing to review this change 🎉
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #5, Line 120:
getRequestFromReturnI think it might be too tricky to get a context in there, so let's leave it as-is for now. […]
Done
File internal/lsp/analysis/stubmethods/stubmethods.go:
// Get the end position of the error.
_, _, endPos, ok := typesinternal.ReadGo116ErrorData(err)
if !ok {
var buf bytes.Buffer
if err := format.Node(&buf, pass.Fset, file); err != nil {
continue
}
endPos = analysisinternal.TypeErrorEndPos(pass.Fset, buf.Bytes(), err.Pos)
}
path, _ := astutil.PathEnclosingInterval(file, err.Pos, endPos)Do you need to get the actual end position? Usually you can just use the start position twice
I believe we need it for "pass.Report" on line 67, which is needed so that wherever the suer puts their cursor on the range of the error, it will show you the quick fix icon in the editor's UI. Otherwise, only the first character of the error will show you the quick fix icon. As for, astutil.PathEnclosingInterval, I am not totally sure, but let me know if I should change it. Thanks!
Patch Set #16, Line 87:
StubInfodoes this need to be exported?
Yep, GetStubInfo which returns this struct is used by the lsp/source/stub.go file. Definitely let me know if I should restructure things.
nit: period here
Ack
File internal/lsp/lsputil/lsputil.go:
Patch Set #16, Line 22:
EnclosingFunction
Not sure if we need this, and if we do, let's have it in a separate change. […]
Sounds good! This was being used by both the snippet-completion logic and now also being used for this stub feature. I'll revert this and just copy the function over.
File internal/lsp/source/fix.go:
Patch Set #16, Line 49:
stubSuggestedFixfunc
The logic to do the suggested fix should live in the same file/directory as the analyzer itself--jus […]
Yes unfortunately it's not currently possible for a couple of reasons:
1. The function signature you suggested above takes only _one_ *ast.File, but the SuggestFix for this feature requires us to possibly analyze multiple files that we do not actually know right away so we need to use source.Snapshot to traverse the code and find out where the concrete type and the interface type live (edge cases include an interface that embeds another interface which lives in other files and so on). Therefore, if you look at the stubSuggestedFixfunc, it makes heavy use of source.Snapshot. This was why we needed the small CL here: https://go-review.googlesource.com/c/tools/+/279412
2. Because we need source.Snapshot, source.Package and other source-scoped types, moving this function signature to the analyzer will introduce a cyclical dependency so we'd need to do a larger refactor to either move a lot of what is needed here into a third package, or basically doing half of the work here and then half of the work in the analyzer which I'd be happy to do if needed.
Please let me know if I'm missing anything, and I'm happy to make any further changes in this CL or a following one!
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #17 to this change.
gopls,internal/lsp: Implement method stubbing via CodeAction
This CL adds a quickfix CodeAction that detects "missing method"
compiler errors and suggests adding method stubs to the concrete
type that would implement the interface. There are many ways that
a user might indicate a concrete type is meant to be used as an interface.
This PR detects two types of those errors: variable declaration and function returns.
For variable declarations, things like the following should be detected:
1. var _ SomeInterface = SomeType{}
2. var _ = SomeInterface(SomeType{})
3. var _ SomeInterface = (*SomeType)(nil)
For function returns, the following example is the primary detection:
func newIface() SomeInterface {
return &SomeType{}
}
More detections can be added in the future of course.
Fixes golang/go#37537
Change-Id: Ibb7784622184c9885eff2ccc786767682876b4d3
---
A internal/lsp/testdata/stub/stub_pointer.go.golden
A internal/lsp/testdata/stub/stub_multi_var.go
A internal/lsp/analysis/stubmethods/stubmethods.go
A internal/lsp/testdata/stub/stub_embedded.go.golden
A internal/lsp/testdata/stub/stub_pointer.go
A internal/lsp/testdata/stub/stub_err.go.golden
A internal/lsp/testdata/stub/stub_renamed_import.go
A internal/lsp/testdata/stub/stub_ignored_imports.go
M internal/lsp/source/options.go
A internal/lsp/testdata/stub/stub_stdlib.go.golden
A internal/lsp/source/stub.go
M internal/lsp/testdata/summary.txt.golden
A internal/lsp/lsputil/lsputil.go
A internal/lsp/testdata/stub/stub_add_selector.go
A internal/lsp/testdata/stub/stub_err.go
A internal/lsp/source/copy_ast.go
A internal/lsp/testdata/stub/stub_assign.go
A internal/lsp/testdata/stub/stub_ignored_imports.go.golden
M internal/lsp/source/fix.go
A internal/lsp/testdata/stub/stub_function_return.go.golden
A internal/lsp/testdata/stub/stub_assign_multivars.go
A internal/lsp/testdata/stub/stub_add_selector.go.golden
A internal/lsp/testdata/stub/stub_multi_var.go.golden
A internal/lsp/testdata/stub/stub_embedded.go
A internal/lsp/testdata/stub/stub_assign.go.golden
A internal/lsp/testdata/stub/stub_assign_multivars.go.golden
A internal/lsp/testdata/stub/stub_stdlib.go
A internal/lsp/testdata/stub/stub_renamed_import.go.golden
A internal/lsp/testdata/stub/stub_function_return.go
29 files changed, 1,758 insertions(+), 1 deletion(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #18 to this change.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #19 to this change.
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman.
1 comment:
Patchset:
Apologies for the continued delay on this. The 1.18beta push took ~all of our resources.
Just a heads up that I hope to get to this during the Go quiet weeks.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Robert Findley.
1 comment:
Patchset:
Apologies for the continued delay on this. The 1.18beta push took ~all of our resources. […]
Sounds good! I will be around on Gophers slack to help in any way. Thanks for the check in 🙏
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Heschi Kreinick (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley.
9 comments:
Patchset:
Hi again 👋🏼 […]
Seems to be closed out.
Patchset:
Hi Rebecca, […]
oof.
Patchset:
I'll be working on this CL. For starters, addressing existing comments, and then I'll review from scratch, hopefully without too much duplication. (Review bankruptcy!)
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #5, Line 168:
Results().At(idx).Type()I really hope not. […]
This code seems to have moved/changed, so I'm going to close this thread.
// FixesError checks whether an error message
// is an interface implementation error
func FixesError(msg string) bool {
return strings.Contains(msg, "missing method") || strings.HasPrefix(msg, "cannot convert")
}
I see! But I _think_ I still need it in the "run" function above? If so, I'll just inline it.
Ack
File internal/lsp/analysis/stubmethods/stubmethods.go:
// Get the end position of the error.
_, _, endPos, ok := typesinternal.ReadGo116ErrorData(err)
if !ok {
var buf bytes.Buffer
if err := format.Node(&buf, pass.Fset, file); err != nil {
continue
}
endPos = analysisinternal.TypeErrorEndPos(pass.Fset, buf.Bytes(), err.Pos)
}
path, _ := astutil.PathEnclosingInterval(file, err.Pos, endPos)
I believe we need it for "pass. […]
More than that, it has to exactly match the Range that gopls generated for its version of the diagnostic, otherwise https://cs.opensource.google/go/x/tools/+/master:internal/lsp/cache/analysis.go;l=417;drc=72cd390b556613764c310ea7f4bccd00a75c3261 will not connect them and we'll get duplicate diagnostics. Which this would appear to do.
I'm not fond of the duplicated logic, but this is how the other type error analyzers (fillreturns, nonewvars, etc) work and this is probably not the time to fight the battle.
Patch Set #16, Line 87:
StubInfo
Yep, GetStubInfo which returns this struct is used by the lsp/source/stub.go file. […]
Ack
File internal/lsp/lsputil/lsputil.go:
Patch Set #16, Line 22:
EnclosingFunction
Sounds good! This was being used by both the snippet-completion logic and now also being used for th […]
Ack
File internal/lsp/source/fix.go:
Patch Set #16, Line 49:
stubSuggestedFixfunc
Yes unfortunately it's not currently possible for a couple of reasons: […]
What you're saying makes sense to me.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Heschi Kreinick (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley.
33 comments:
Patchset:
First pass. Most of this is relatively trivial. I'm still getting back up to speed and I was never so expert with ast/types so I might be making iffy suggestions.
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #5, Line 1:
package stubmethodsAck
2022 :'(
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #19, Line 38:
ifaceErr := strings.Contains(err.Msg, "missing method") || strings.HasPrefix(err.Msg, "cannot convert")Can this be made more specific? "missing method" appears unambiguous, but there are lots of "cannot converts" in the compiler. Moving the GetStubInfo call up would also be okay; mostly I just worry about calling format.Node for situations where GetStubInfo is going to give up. (But then we should make sure GetStubInfo returns quickly.)
Patch Set #19, Line 80:
return other.Name()Is this confusing if someone has imported the package under a different name? Would showing the full path with types.RelativeTo be better, or at least less ambiguous?
Patch Set #19, Line 94:
func GetStubInfo(ti *types.Info, path []ast.Node, pkg *types.Package, pos token.Pos) *StubInfo {Ugh. I was going to suggest we parse the compiler error to get this stuff, but it doesn't print full package paths so it's not very feasible.
Patch Set #19, Line 102:
case *ast.AssignStmt:I suspect function calls are another common case but that can be a followup CL as mentioned in your description.
Patch Set #19, Line 124:
n := rs.Results[returnIdx]nit: inline?
Patch Set #19, Line 129:
si := StubInfo{nit: you can do this all at once in the return statement.
Patch Set #19, Line 137:
ef.Results.List[returnIdx].TypeChecking my understanding: this is safe because if the return statement was returning the wrong number of things we wouldn't have gotten the iface conversion error?
Patch Set #19, Line 182:
converstiontypo
stray newline?
Patch Set #19, Line 236:
if named.Obj().Pkg() == nil && named.Obj().Name() != "error" {What's this accomplishing? Excluding any? Comment might be helpful.
Patch Set #19, Line 264:
signature and body oftype of?
File internal/lsp/lsputil/lsputil.go:
Patch Set #19, Line 1:
package lsputilDelete?
File internal/lsp/source/copy_ast.go:
Patch Set #19, Line 14:
// copied from github.com/google/wireSince that's a Google project, the copying should be okay.
Patch Set #19, Line 21:
func copyAST(original ast.Node) ast.Node {We have something like this as source.cloneExpr; can you use that?
File internal/lsp/source/stub.go:
Patch Set #19, Line 34:
const tmpl = `// {{ .Method }} implements {{ .Interface }}This is too generic a name for a package-level variable. I'd also move it closer to its use.
(I'm not 100% sold that we need a template for something this simple, honestly. A simple function that does string concatenation seems like it'd be fine to me too. And it'd sidestep concerns about when to parse it.)
Patch Set #19, Line 40:
VersionedFileHandleDo you need a versioned handle?
FixFunc?
hasIface := si.Interface != nil && (si.Interface.Pkg() != nil || si.Interface.Name() == "error")
hasConc := si.Concrete != nil && si.Concrete.Pkg() != nil
isValid := hasIface && hasConcI would expect GetStubInfo to take care of all of this validation, and as far as I can tell it does?
Patch Set #19, Line 64:
concreteFile = parsedConcreteFile.Fileinline? I thought this was going to get mutated and it was confusing.
Patch Set #19, Line 69:
methodsBytes, err = stubErr(ctx, concreteFile, si, snapshot)Why does error need special handling?
Patch Set #19, Line 78:
err = format.Node(&concBuf, snapshot.FileSet(), concreteFile)How is this different than concreteFH.Read?
Patch Set #19, Line 85:
return nil, fmt.Errorf("insertPos(%d) is greater than length of source: %d", insertPos, len(concreteSrc))This should never happen, but a more user-centric error like "insertion position is past the end of the file" would be a little nicer.
Patch Set #19, Line 98:
astutil.AddNamedImport(fset, newF, imp.Name, imp.Path)FWIW, this will leave things misgrouped for goimports to fix on next save. Fixing it is almost certainly more trouble than it's worth.
Patch Set #19, Line 105:
_, _, err = GetParsedFile(ctx, snapshot, concreteFH, NarrowestPackage)What is this accomplishing? I'm not convinced it's possible for this to return an error.
Patch Set #19, Line 133:
ctx context.Context,Merge this onto one line, unless there's a reason? Here and below.
Patch Set #19, Line 162:
var n ast.Node = nn[1].(*ast.Field).TypeDid you consider using the type information as input here rather than the AST? I'm not as familiar as I'd like with the APIs, but it seems like it'd be easier: no AST cloning, no need to treat embedded interfaces specially, and you can use stuff like types.TypeString to handle qualification for you.
Patch Set #19, Line 248:
func getNodes(pgf *ParsedGoFile, pRng protocol.Range) ([]ast.Node, token.Pos, error) {A lot of the functions in this package are too generic; the source package is unfortunately huge and nobody is going to agree that this is what "get nodes" means. Similar for getFile and maybe others.
Patch Set #19, Line 264:
func mightRemoveSelector(ctx context.Context, c *astutil.Cursor, sel *ast.SelectorExpr, ifacePkg Package, implPath string) bool {I think it makes sense to combine all three of these functions, is there a good reason to keep them separate? It's really just "fix the selector after we move the code" and I think you can get rid of some amount of redundancy.
Patch Set #19, Line 268:
// is always coming from the given given packagetypo
Patch Set #19, Line 511:
if imp.Name == name && imp.Path == path {Do you intend to add duplicate imports if the name isn't the default?
Patch Set #19, Line 535:
singaturestypo
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Robert Findley, Heschi Kreinick.
30 comments:
Patchset:
Thanks for the round of review! I'm about to update to take into account a lot of your suggestions.
I think the most significant part is using types info instead of AST to format the method signature. It definitely checks out, but I just wanted you to take a look before removing all of the AST code .
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #5, Line 1:
package stubmethods
2022 :'(
Ack
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #19, Line 38:
ifaceErr := strings.Contains(err.Msg, "missing method") || strings.HasPrefix(err.Msg, "cannot convert")
Can this be made more specific? "missing method" appears unambiguous, but there are lots of "cannot […]
I'd definitely love it if it can be more specific but I'm not sure. Ideally the Go compiler gives more concrete error messages or codes. But definitely let me know if there's a way around it.
I'm happy to move GetStubInfo up but that means we prioritize calling "PathEnclosingInterval" over "format.Node", if that's okay.
Patch Set #19, Line 80:
return other.Name()
Is this confusing if someone has imported the package under a different name? Would showing the full […]
Definitely a good point if there's a renamed package. But I think the context that the user is in at that moment should make it obvious. Showing the fully qualified path is a little too noisy for some UIs like VSCode and would still not be correct for renamed packages (tho less ambiguous). That said, I'll change the logic up to do the correct thing by going over the concrete file's import statements to get the right name of the package out (we already have logic to figure that out). Let me know if it'll look correct to you.
Patch Set #19, Line 124:
n := rs.Results[returnIdx]nit: inline?
Ack
Patch Set #19, Line 129:
si := StubInfo{nit: you can do this all at once in the return statement.
Ack
Patch Set #19, Line 137:
ef.Results.List[returnIdx].Type
Checking my understanding: this is safe because if the return statement was returning the wrong numb […]
Yes. The first for-loop of this function (followed by the -1 check) guarantees that the index won't be out of range.
Patch Set #19, Line 182:
converstiontypo
Ack
stray newline?
Ack
Patch Set #19, Line 236:
if named.Obj().Pkg() == nil && named.Obj().Name() != "error" {What's this accomplishing? Excluding any? Comment might be helpful.
From my testing, interfaces defined in the "builtin" package return nil when you call `.Pkg()` on their object. But they are still real interfaces that users might want to implement.
Therefore, we have to make a special case for them (for example in the "stubErr" function). This check protects gopls from panicking in the future if a new interface was added to the builtin package.
I'll add a comment for all of this but do let me know if I'm missing something or there's another way around my findings.
Thanks!
Patch Set #19, Line 264:
signature and body oftype of?
Ack
File internal/lsp/lsputil/lsputil.go:
Patch Set #19, Line 1:
package lsputilDelete?
Ack
File internal/lsp/source/copy_ast.go:
Patch Set #19, Line 21:
func copyAST(original ast.Node) ast.Node {We have something like this as source. […]
Right off the bat no since I need to use astutil.PathEnclosingInterval which returns ast.Node, but let me know if I can safely cast its results to ast.Expr.
This is hopefully irrelevant if using the go/types package is sufficient.
File internal/lsp/source/stub.go:
Patch Set #19, Line 34:
const tmpl = `// {{ .Method }} implements {{ .Interface }}
This is too generic a name for a package-level variable. I'd also move it closer to its use. […]
Good point, will change this to a function that just prints the string.
FixFunc?
Ack
Patch Set #19, Line 40:
VersionedFileHandleDo you need a versioned handle?
I believe so as this function must satisfy the "SuggestedFixFunc" signature.
hasIface := si.Interface != nil && (si.Interface.Pkg() != nil || si.Interface.Name() == "error")
hasConc := si.Concrete != nil && si.Concrete.Pkg() != nil
isValid := hasIface && hasConcI would expect GetStubInfo to take care of all of this validation, and as far as I can tell it does?
Not fully but I will make sure that they do the full check and remove the validation from here 👍
Patch Set #19, Line 64:
concreteFile = parsedConcreteFile.Fileinline? I thought this was going to get mutated and it was confusing.
Ack
Patch Set #19, Line 69:
methodsBytes, err = stubErr(ctx, concreteFile, si, snapshot)Why does error need special handling?
Same reason for having to check Pkg() == nil: the error interface has a nil-package and therefore needs special handling when stubbing out its methods. That said, I'm happy to refactor this or take any suggestions 👍
Patch Set #19, Line 78:
err = format.Node(&concBuf, snapshot.FileSet(), concreteFile)
How is this different than concreteFH. […]
Probably because I didn't know of concreteFH.Read :) -- changed.
Patch Set #19, Line 85:
return nil, fmt.Errorf("insertPos(%d) is greater than length of source: %d", insertPos, len(concreteSrc))
This should never happen, but a more user-centric error like "insertion position is past the end of […]
Ack
Patch Set #19, Line 98:
astutil.AddNamedImport(fset, newF, imp.Name, imp.Path)
FWIW, this will leave things misgrouped for goimports to fix on next save. […]
Do you mean that we shouldn't automatically add imports for the user and just let goimports do the work on file-save? The problem is that goimports can be really slow and sometimes imports the wrong thing. But let me know and I'm happy to make any changes.
Patch Set #19, Line 105:
_, _, err = GetParsedFile(ctx, snapshot, concreteFH, NarrowestPackage)What is this accomplishing? I'm not convinced it's possible for this to return an error.
Good catch. Will remove this.
Patch Set #19, Line 133:
ctx context.Context,Merge this onto one line, unless there's a reason? Here and below.
Ack
Patch Set #19, Line 162:
var n ast.Node = nn[1].(*ast.Field).Type
To be honest, I don't remember exactly why I ended up not using the types package when I first implemented this a while ago. But I did some experimenting and the tests do pass so I'd be a big fan of using that instead of AST cloning. Does the go/types package guarantee to print valid Go code if we wanted to print an entire function signature? Particularly types.TypeString.
In any case, I'm going to push an update that does your suggestion, but I won't remove all of the AST code just yet, just for comparison. Let me know if it looks correct to you, and I'll delete all of the other AST-related code.
no need to treat embedded interfaces specially
Hm, not totally sure what you mean. The part of this feature where embedded interfaces are concerned already uses the types package instead of AST.
Patch Set #19, Line 248:
func getNodes(pgf *ParsedGoFile, pRng protocol.Range) ([]ast.Node, token.Pos, error) {
A lot of the functions in this package are too generic; the source package is unfortunately huge and […]
Changing to "getStubFile" and "getStubNodes". But happy to take other suggestions. Can also move a lot of this functionality into a sub-package if need be (hopefully there aren't private symbols that are needed but will need to scan).
Patch Set #19, Line 264:
func mightRemoveSelector(ctx context.Context, c *astutil.Cursor, sel *ast.SelectorExpr, ifacePkg Package, implPath string) bool {
I think it makes sense to combine all three of these functions, is there a good reason to keep them […]
I thought keeping them separate would make the code a little bit more clear, even though there's some duplication. But happy to try and make them into one function if you'd like 👍🏼
Patch Set #19, Line 268:
// is always coming from the given given packagetypo
Ack
Patch Set #19, Line 511:
if imp.Name == name && imp.Path == path {Do you intend to add duplicate imports if the name isn't the default?
I don't believe so. If the name is different, that will get re-used vi mightRenameSelector.
Patch Set #19, Line 535:
singaturestypo
Ack
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Robert Findley, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #20 to this change.
gopls,internal/lsp: Implement method stubbing via CodeAction
This CL adds a quickfix CodeAction that detects "missing method"
compiler errors and suggests adding method stubs to the concrete
type that would implement the interface. There are many ways that
a user might indicate a concrete type is meant to be used as an interface.
This PR detects two types of those errors: variable declaration and function returns.
For variable declarations, things like the following should be detected:
1. var _ SomeInterface = SomeType{}
2. var _ = SomeInterface(SomeType{})
3. var _ SomeInterface = (*SomeType)(nil)
For function returns, the following example is the primary detection:
func newIface() SomeInterface {
return &SomeType{}
}
More detections can be added in the future of course.
Fixes golang/go#37537
Change-Id: Ibb7784622184c9885eff2ccc786767682876b4d3
---
A internal/lsp/analysis/stubmethods/stubmethods.go
M internal/lsp/testdata/summary_go1.18.txt.golden
29 files changed, 1,759 insertions(+), 2 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Robert Findley, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #21 to this change.
gopls,internal/lsp: Implement method stubbing via CodeAction
This CL adds a quickfix CodeAction that detects "missing method"
compiler errors and suggests adding method stubs to the concrete
type that would implement the interface. There are many ways that
a user might indicate a concrete type is meant to be used as an interface.
This PR detects two types of those errors: variable declaration and function returns.
For variable declarations, things like the following should be detected:
1. var _ SomeInterface = SomeType{}
2. var _ = SomeInterface(SomeType{})
3. var _ SomeInterface = (*SomeType)(nil)
For function returns, the following example is the primary detection:
func newIface() SomeInterface {
return &SomeType{}
}
More detections can be added in the future of course.
Fixes golang/go#37537
Change-Id: Ibb7784622184c9885eff2ccc786767682876b4d3
---
A internal/lsp/analysis/stubmethods/stubmethods.go
28 files changed, 1,124 insertions(+), 2 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Heschi Kreinick (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley.
14 comments:
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #19, Line 38:
ifaceErr := strings.Contains(err.Msg, "missing method") || strings.HasPrefix(err.Msg, "cannot convert")
I'd definitely love it if it can be more specific but I'm not sure. […]
PEI is definitely cheaper than reformatting the entire file.
Patch Set #19, Line 80:
return other.Name()
Definitely a good point if there's a renamed package. […]
(you didn't end up using the renamed name, but that's okay by me.)
Patch Set #19, Line 236:
if named.Obj().Pkg() == nil && named.Obj().Name() != "error" {
From my testing, interfaces defined in the "builtin" package return nil when you call `. […]
Future-proofing makes sense.
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #21, Line 129:
&& imp.Name.Name != other.Name()this doesn't actually change behavior, remove?
Patch Set #21, Line 135:
if hasImport {Style preference, I suppose, but you could do this return on 132 and save predeclaring the vars.
File internal/lsp/source/copy_ast.go:
Patch Set #19, Line 21:
func copyAST(original ast.Node) ast.Node {
Right off the bat no since I need to use astutil.PathEnclosingInterval which returns ast. […]
Obsolete.
File internal/lsp/source/stub.go:
Patch Set #19, Line 40:
VersionedFileHandle
I believe so as this function must satisfy the "SuggestedFixFunc" signature.
Ah, indeed. Though there's no reason that signature takes a versioned file. Not this CL.
hasIface := si.Interface != nil && (si.Interface.Pkg() != nil || si.Interface.Name() == "error")
hasConc := si.Concrete != nil && si.Concrete.Pkg() != nil
isValid := hasIface && hasConc
Not fully but I will make sure that they do the full check and remove the validation from here 👍
Ack
Patch Set #19, Line 69:
methodsBytes, err = stubErr(ctx, concreteFile, si, snapshot)
Same reason for having to check Pkg() == nil: the error interface has a nil-package and therefore ne […]
Meh. I was going to suggest adding nil checks so that the stubMethods function could do error too, but stubErr is so trivial now that it's not worth fussing about.
Patch Set #19, Line 98:
astutil.AddNamedImport(fset, newF, imp.Name, imp.Path)
Do you mean that we shouldn't automatically add imports for the user and just let goimports do the w […]
Hopefully this is clearer after our discussion. Whether to fix it is entirely up to you.
Patch Set #19, Line 162:
var n ast.Node = nn[1].(*ast.Field).Type
To be honest, I don't remember exactly why I ended up not using the types package when I first imple […]
Mmm, okay. I thought you'd be able to return a flat list of missing methods, and "mi" is tantalizingly close to dead now. But you are still using the file.
Patch Set #19, Line 248:
func getNodes(pgf *ParsedGoFile, pRng protocol.Range) ([]ast.Node, token.Pos, error) {
Changing to "getStubFile" and "getStubNodes". But happy to take other suggestions. […]
Nah, this is good enough.
Patch Set #19, Line 264:
func mightRemoveSelector(ctx context.Context, c *astutil.Cursor, sel *ast.SelectorExpr, ifacePkg Package, implPath string) bool {
I thought keeping them separate would make the code a little bit more clear, even though there's som […]
Obsolete.
Patch Set #19, Line 511:
if imp.Name == name && imp.Path == path {
I don't believe so. If the name is different, that will get re-used vi mightRenameSelector.
I think the code now only calls this when it knows there isn't a pre-existing import, so this check is unnecessary and you could probably inline the whole function?
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley.
Marwan Sulaiman uploaded patch set #22 to this change.
28 files changed, 1,120 insertions(+), 2 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Robert Findley, Heschi Kreinick.
6 comments:
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #19, Line 38:
ifaceErr := strings.Contains(err.Msg, "missing method") || strings.HasPrefix(err.Msg, "cannot convert")
PEI is definitely cheaper than reformatting the entire file.
Sounds good, happy to do that in this CL or as a follow up.
Patch Set #19, Line 80:
return other.Name()
(you didn't end up using the renamed name, but that's okay by me. […]
I _think_ it's using renamed names in the message because it's now re-using `RelativeToFiles` instead.
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #21, Line 129:
&& imp.Name.Name != other.Name()this doesn't actually change behavior, remove?
From what I tested, "imp.Name.Name" returns the potentially renamed import path while "other.Name()" returns the default package name and not the potentially renamed one. Let me know if I missed something, thanks!
Patch Set #21, Line 135:
if hasImport {Style preference, I suppose, but you could do this return on 132 and save predeclaring the vars.
Ack
File internal/lsp/source/stub.go:
Patch Set #19, Line 98:
astutil.AddNamedImport(fset, newF, imp.Name, imp.Path)Hopefully this is clearer after our discussion. Whether to fix it is entirely up to you.
I added a note to look into this after this CL is merged, thanks for pointing it out!
Patch Set #19, Line 511:
if imp.Name == name && imp.Path == path {
I think the code now only calls this when it knows there isn't a pre-existing import, so this check […]
Sounds good on inlining 👍
Note this loop is checking "previously added imports to the ct.addImports slice" as opposed to "the concrete file's already existing import statements". It is just a de-duping loop in the scenario where an import path is referenced multiple times within an interface. I believe "missingMethod" will be called for each one because we lazily update the AST after all of this is done.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Heschi Kreinick (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley.
5 comments:
Patchset:
This looks pretty good. I want to take a fresh look tomorrow but in my mind it should be basically ready to merge. Rob, do you want to do a pass, since you're more knowledgeable on go/types and more recent on gopls?
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #21, Line 129:
&& imp.Name.Name != other.Name()
From what I tested, "imp.Name.Name" returns the potentially renamed import path while "other. […]
Yeah, that's right, I just meant that if imp.Name.Name == other.Name(), then doing importName = imp.Name.Name is harmless. So this clause of the if is essentially irrelevant.
File internal/lsp/source/stub.go:
Patch Set #19, Line 511:
if imp.Name == name && imp.Path == path {Sounds good on inlining 👍 […]
Ah, true. I think subsequent AddNamedImport calls will do nothing if the import already exists, but fair enough.
File internal/lsp/testdata/stub/stub_add_selector.go.golden:
Um, just checking: You're aware these can be more than one character, right? These are all kind of unspecific. I'd do "&readerFrom" here.
File internal/lsp/testdata/stub/stub_stdlib.go:
Patch Set #22, Line 1:
package stubWhat does this case cover that's new?
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman, Heschi Kreinick.
2 comments:
Patchset:
This looks pretty good. […]
Thank you Heschi for helping review, and thank you Marwan for your responsiveness (especially after all this time!).
I will review tomorrow. Let's get this merged!
Thank you
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #23 to this change.
28 files changed, 1,114 insertions(+), 2 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #24 to this change.
gopls,internal/lsp: Implement method stubbing via CodeAction
This CL adds a quickfix CodeAction that detects "missing method"
compiler errors and suggests adding method stubs to the concrete
type that would implement the interface. There are many ways that
a user might indicate a concrete type is meant to be used as an interface.
This PR detects two types of those errors: variable declaration and function returns.
For variable declarations, things like the following should be detected:
1. var _ SomeInterface = SomeType{}
2. var _ = SomeInterface(SomeType{})
3. var _ SomeInterface = (*SomeType)(nil)
For function returns, the following example is the primary detection:
func newIface() SomeInterface {
return &SomeType{}
}
More detections can be added in the future of course.
Fixes golang/go#37537
Change-Id: Ibb7784622184c9885eff2ccc786767682876b4d3
---
A internal/lsp/analysis/stubmethods/stubmethods.go
M internal/lsp/source/fix.go
M internal/lsp/source/options.go
A internal/lsp/source/stub.go
A internal/lsp/testdata/stub/other/other.go
A internal/lsp/testdata/stub/stub_add_selector.go
A internal/lsp/testdata/stub/stub_add_selector.go.golden
A internal/lsp/testdata/stub/stub_assign.go
A internal/lsp/testdata/stub/stub_assign.go.golden
A internal/lsp/testdata/stub/stub_assign_multivars.go
A internal/lsp/testdata/stub/stub_assign_multivars.go.golden
A internal/lsp/testdata/stub/stub_embedded.go
A internal/lsp/testdata/stub/stub_embedded.go.golden
A internal/lsp/testdata/stub/stub_err.go
A internal/lsp/testdata/stub/stub_err.go.golden
A internal/lsp/testdata/stub/stub_function_return.go
A internal/lsp/testdata/stub/stub_function_return.go.golden
A internal/lsp/testdata/stub/stub_ignored_imports.go
A internal/lsp/testdata/stub/stub_ignored_imports.go.golden
A internal/lsp/testdata/stub/stub_multi_var.go
A internal/lsp/testdata/stub/stub_multi_var.go.golden
A internal/lsp/testdata/stub/stub_pointer.go
A internal/lsp/testdata/stub/stub_pointer.go.golden
A internal/lsp/testdata/stub/stub_renamed_import.go
A internal/lsp/testdata/stub/stub_renamed_import.go.golden
A internal/lsp/testdata/stub/stub_renamed_import_iface.go
A internal/lsp/testdata/stub/stub_renamed_import_iface.go.golden
A internal/lsp/testdata/stub/stub_stdlib.go
A internal/lsp/testdata/stub/stub_stdlib.go.golden
M internal/lsp/testdata/summary.txt.golden
M internal/lsp/testdata/summary_go1.18.txt.golden
31 files changed, 1,159 insertions(+), 2 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #25 to this change.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Heschi Kreinick.
4 comments:
Patchset:
Just FYI, I renamed "addedImports" to "stubImports" in the same spirit of making things less ambiguous within the larger source package. Happy to make more of these changes.
Also, one last things I noticed is that the tests are failing in Go1.17 because the golden file reads "any" while Go1.17 prints "interface{}".
I wonder if I should:
1. Duplicate all tests with build tags.
2. Or, is there a way to write the Go test file (such as "testdata/stub/stub_add_selector.go") only once but write two different golden files one for each version of those two versions of Go as they output different results? This would save us from having to duplicate the test input.
3. Or maybe just remove any tests that deal with the empty interface.Thank you both!
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #21, Line 129:
&& imp.Name.Name != other.Name()
Yeah, that's right, I just meant that if imp.Name.Name == other.Name(), then doing importName = imp. […]
Got it, will remove 👌
File internal/lsp/testdata/stub/stub_add_selector.go.golden:
Um, just checking: You're aware these can be more than one character, right? These are all kind of u […]
Sounds good, will change this to "&readerFrom". Let me know if you'd like me to update the other tests as well.
File internal/lsp/testdata/stub/stub_stdlib.go:
Patch Set #22, Line 1:
package stubWhat does this case cover that's new?
It covers implementing a standard library interface as opposed to a workspace-defined one. But it might be overkill, so let me know if I should remove it.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Heschi Kreinick.
1 comment:
Patchset:
*By remove, I mean change the test so that no empty interfaces are present.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Heschi Kreinick.
Marwan Sulaiman uploaded patch set #26 to this change.
31 files changed, 1,123 insertions(+), 2 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Heschi Kreinick.
1 comment:
Patchset:
Ended up going with option 3, but happy to make further changes.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman, Heschi Kreinick.
29 comments:
Patchset:
Looks good overall.
A lot of comments but mostly minor in nature regarding the usage of go/types. (clarifying object vs type and method sets).
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #22, Line 34:
RunDespiteErrors: true,FWIW, I don't believe gopls honors this. Should it?
Patch Set #22, Line 71:
types.TypeString(si.Interface.Type(), qf)This may be rather verbose, but this is OK for now.
Patch Set #22, Line 80:
Interface types.ObjectComment what this is. For an interface literal, this is a parameter? For a defined interface, this is the TypeName?
Patch Set #22, Line 81:
types.ObjectComment what this is as well. Presumably it's the defined type that we want to stub out.
I think it would be better if this were a *types.Named (or a *types.TypeName, but that is actually less accurate than *types.Named).
Patch Set #22, Line 91:
fromValueSpecnit: can we move these from* helpers to be directly below GetStubInfo? It would make this analyzer easier to read from top-to-bottom.
If we're never going to use the error returned by fromReturnStmt, why have it return an error at all?
Patch Set #22, Line 305:
getConcreteTypeJust 'concreteType' works. No need for 'get' (also compare enclosingFunction below).
Also, this returns an Object, not a Type. How about having this return a *types.Named?
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #26, Line 114:
RelativeToFilesCurious: can we share logic with internal/lsp/source.Qualifier?
Patch Set #26, Line 179:
concObj.Pkg() == nilHere and elsewhere, what's the reason for this Pkg() == nil check?
Patch Set #26, Line 277:
getIfaceTypeHow about 'ifaceObject'. No need for 'get', and this returns an Object, not a Type.
EDIT: but see comment below, let's return a *types.Named, and call this 'ifaceType'.
Patch Set #26, Line 297:
return named.Obj()Oh wait, this only handles *Named types? In that case, let's have it return *types.Named for the additional safety and readability.
I thought it would return a non-named *Interface as well in the case of an interface literal parameter
e.g.
func Do(doer interface{ do() error })
Patch Set #26, Line 300:
will trys/will try/tries
ptr, isPtr := typ.(*types.Pointer)
if isPtr {
typ = ptr.Elem()
}nit: inline
if ptr, isPtr := typ.(*types.Pointer); isPtr {
...
}
File internal/lsp/source/stub.go:
optional nit: just 'rng' works. I hope that most functions only have one type of range in scope.
Patch Set #26, Line 43:
methodsBytesnit: 'methodsSrc' might be a little clearer?
methodsSrc // source code of method stubs
Patch Set #26, Line 44:
stubImports []*stubImportsAdd inline comment: "additional imports needed for method stubs"
(or whatever this means, I'm reading top-to-bottom :))
Patch Set #26, Line 46:
si.Interface.Pkg() == nilThis is how we're checking for 'error'? Can we check if its name is "error" and it is in the universe scope?
If I weren't very experienced with go/types, I would not understand this clause :)
Aside: I wish we had a good way to know if the file were using CRLF line endings, but we don't (and the editor should fix this up).
Patch Set #26, Line 107:
fset: snapshot.FileSet(),It looks like we always have the snapshot, so don't need the fset here.
if si.Pointer {
concrete = "*" + concrete
}
_, err = methodsBuffer.Write(printStubMethod(methodData{
Method: m.Name(),
Concrete: concrete,
Interface: deduceIfaceName(si.Concrete.Pkg(), si.Interface.Pkg(), si.Concrete, si.Interface),
Signature: strings.TrimPrefix(sig, "func"),
}))
if errThis is duplicated in stubErr below. I think stubErr isn't necessary, but if it is perhaps we can refactor a bit to share more code.
Patch Set #26, Line 158:
stubErrWhy is this specialization necessary? It seems like stubMethods should just work?
if si.Pointer {
concrete = "*" + concrete
}I don't actually understand this logic: why should the receiver be a pointer if the type was a pointer? Could use some discussion.
/*
// {{ .Method }} implements {{ .Interface }}
func ({{ .Concrete }}) {{ .Method }}{{ .Signature }} {
panic("unimplemented")
}
*/Let's just use '//', rather than /* */.
Among other things, gerrit gets confused...
for i := 0; i < iface.NumEmbeddeds(); i++ {
eiface := iface.Embedded(i).Obj()
depPkg := ifacePkg
if eiface.Pkg().Path() != ifacePkg.PkgPath() {
var err error
depPkg, err = ifacePkg.GetImport(eiface.Pkg().Path())
if err != nil {
return nil, err
}
}
em, err := missingMethods(ctx, snapshot, ct, eiface, depPkg, visited)
if err != nil {
return nil, err
}
missing = append(missing, em...)
}Why not use types.MethodSet here? It looks like it would simplify this function significantly.
&mismatchError{
name: method.Name(),
have: implSig,
want: ifaceSig,
}From what I can tell mismatchError probably doesn't carry its weight. it looks like it's just an indirection versus fmt.Errorf.
Patch Set #26, Line 331:
concreteTypeLooking at usage, it appears this type mostly exists to manage the lookup in the pair of method sets tms, pms, but as commented below we don't actually need both: pms will suffice. So I think concreteType is probably unnecessary.
Patch Set #26, Line 335:
tms, pmsGive these longer names, or a comment.
if sel := ct.tms.Lookup(ct.pkg, name); sel != nil {
return sel
}
return ct.pms.Lookup(ct.pkg, name)Why not just look up in pointer method set? We don't need both
https://tip.golang.org/ref/spec#Method_sets
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Robert Findley, Heschi Kreinick.
26 comments:
Patchset:
Thanks for the look over 👍 next patch should address some/most of the comments so far.
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #22, Line 34:
RunDespiteErrors: true,
FWIW, I don't believe gopls honors this. […]
I think this is just me copying from other analyzers. It's no biggie either way, but reading the docs of RunDespiteErrors it makes sense to be true.
Patch Set #22, Line 80:
Interface types.Object
Comment what this is. […]
Sounds good. I'll also leave a comment about interface literals since those are not supported yet but I'd love to add that as a follow up.
Patch Set #22, Line 81:
types.Object
Comment what this is as well. Presumably it's the defined type that we want to stub out. […]
Sounds good. Will change that to *types.Named.
Patch Set #22, Line 91:
fromValueSpec
nit: can we move these from* helpers to be directly below GetStubInfo? It would make this analyzer e […]
Ack
If we're never going to use the error returned by fromReturnStmt, why have it return an error at all […]
I believe the reason was that I would have liked to log out the error (for debugging/reporting purposes) instead of returning/showing the user errors that might not be actual errors. However, the proper gopls logger _I think_ is not available since we don't have a context to pass. But I could be totally wrong. Let me know if:
1. I should add a comment describing the above as a future improvement.
2. Maybe there's a way to log this out? Would love to know how.
3. Just silently fail and remove all error handling.
Patch Set #22, Line 305:
getConcreteType
Just 'concreteType' works. No need for 'get' (also compare enclosingFunction below). […]
Ack
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #26, Line 114:
RelativeToFiles
Curious: can we share logic with internal/lsp/source. […]
For now, I'd say it's a bit of an effort for two reasons:
1. It would introduce a cyclical import because this package would need to import source.
2. Looking at both functions, they're a little different and it would take some effort to generalize in a readable way. But happy to hear ideas or make changes if need be.
Patch Set #26, Line 179:
concObj.Pkg() == nilHere and elsewhere, what's the reason for this Pkg() == nil check?
"builtin" types have a nil package from what I've seen. Someone could technically try to implement an interface on a builtin concrete type and I wanted to make sure we avoid any potential panics. Let me know if I should add a comment or do anything differently.
Patch Set #26, Line 277:
getIfaceType
How about 'ifaceObject'. No need for 'get', and this returns an Object, not a Type. […]
Ack
Patch Set #26, Line 297:
return named.Obj()
Oh wait, this only handles *Named types? In that case, let's have it return *types. […]
So while experimenting with my follow-up-generics-support, I needed to make the Interface part *types.Named so it def makes sense in that context as well.
However, I'd also to support literal interfaces because they're quite common in my experience. I'm not yet sure how that would change things, but let me know if I should keep this as a types.Object with a comment like mentioned above. Thanks!
Patch Set #26, Line 300:
will trys/will try/tries
Ack
ptr, isPtr := typ.(*types.Pointer)
if isPtr {
typ = ptr.Elem()
}
nit: inline […]
I don't believe that would work as the variables would come out of the function's scope and I need to return "isPtr" back to the caller.
File internal/lsp/source/stub.go:
Patch Set #26, Line 43:
methodsBytesnit: 'methodsSrc' might be a little clearer? […]
Ack
Patch Set #26, Line 44:
stubImports []*stubImports
Add inline comment: "additional imports needed for method stubs" […]
Ack
Patch Set #26, Line 46:
si.Interface.Pkg() == nil
This is how we're checking for 'error'? Can we check if its name is "error" and it is in the univers […]
That's a good point. Will add the "error" name check. From other code, it looks like I can do "si.Interface.Parent() == types.Universe" to check it's in the universe scope. Let me know if that's incorrect. Thanks!
Patch Set #26, Line 107:
fset: snapshot.FileSet(),It looks like we always have the snapshot, so don't need the fset here.
Ack
if si.Pointer {
concrete = "*" + concrete
}
_, err = methodsBuffer.Write(printStubMethod(methodData{
Method: m.Name(),
Concrete: concrete,
Interface: deduceIfaceName(si.Concrete.Pkg(), si.Interface.Pkg(), si.Concrete, si.Interface),
Signature: strings.TrimPrefix(sig, "func"),
}))
if errThis is duplicated in stubErr below. […]
I extracted this to "getStubReceiver" which looks potentially silly, but I added a todo that will make sure it's definitely not silly once I follow up with generics support as it will need to format the receiver with a little more configuration.
As for removing stubErr, I do agree that it would be nice to remove it. But I'd love to do that as a follow up because of two things:
1. Pkg() == nil, like I mentioned above, adds a lot more edge cases to the rest of the logic that always assumes there's a package to an interface.
2. "getStubFile" just panics because there is no file associated to where the error is defined.So would love to hear if there's an easier way around this. Otherwise it might be better to leave builtin interfaces as a special case and keep thinking of a clear/readable way to merge the two.
Definitely happy to hear your thoughts.
Patch Set #26, Line 158:
stubErrWhy is this specialization necessary? It seems like stubMethods should just work?
Same as above comment.
if si.Pointer {
concrete = "*" + concrete
}
I don't actually understand this logic: why should the receiver be a pointer if the type was a point […]
I agree that it may not be necessary but I thought it would be a better UX that if the user has used a pointer that they most likely mean to implement the method on the pointer receiver and not value receiver.
For example:
```
var _ io.Reader = (*myType)(nil) // indicates they want to implement io.Reader on the pointer receiver
var _ io.Reader = myType{} // indicates they want to implement io.Reader on the value receiver
```Definitely let me know otherwise, happy to change it up.
/*
// {{ .Method }} implements {{ .Interface }}
func ({{ .Concrete }}) {{ .Method }}{{ .Signature }} {
panic("unimplemented")
}
*/
Let's just use '//', rather than /* */. […]
Ack
for i := 0; i < iface.NumEmbeddeds(); i++ {
eiface := iface.Embedded(i).Obj()
depPkg := ifacePkg
if eiface.Pkg().Path() != ifacePkg.PkgPath() {
var err error
depPkg, err = ifacePkg.GetImport(eiface.Pkg().Path())
if err != nil {
return nil, err
}
}
em, err := missingMethods(ctx, snapshot, ct, eiface, depPkg, visited)
if err != nil {
return nil, err
}
missing = append(missing, em...)
}Why not use types.MethodSet here? It looks like it would simplify this function significantly.
Not totally sure what you mean, as in replace []*missingInterface with "types.MethodSet"? Mainly, I'm looking to find the *ast.File for each interface which I'm not sure if types.MethodSet here carries.
But always happy to learn/simplify more, so def let me know.
&mismatchError{
name: method.Name(),
have: implSig,
want: ifaceSig,
}
From what I can tell mismatchError probably doesn't carry its weight. […]
Ack
Patch Set #26, Line 331:
concreteType
Looking at usage, it appears this type mostly exists to manage the lookup in the pair of method sets […]
Yep! Will remove this type.
Patch Set #26, Line 335:
tms, pmsGive these longer names, or a comment.
I think given that I don't "tms" anymore, I can just remove it and rename "pms" to "ms" which shouldn't require a comment? Happy to add one otherwise.
if sel := ct.tms.Lookup(ct.pkg, name); sel != nil {
return sel
}
return ct.pms.Lookup(ct.pkg, name)
Why not just look up in pointer method set? We don't need both […]
Ah that sounds great, can the same assumption be made for the "doesNotHaveMethod" method?
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Robert Findley, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #27 to this change.
31 files changed, 1,094 insertions(+), 2 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Hyang-Ah Hana Kim (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
1 comment:
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #27, Line 105:
getStubInfoFromReturnsfromReturnStmt ...
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #28 to this change.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Robert Findley, Hyang-Ah Hana Kim, Heschi Kreinick.
1 comment:
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #27, Line 105:
getStubInfoFromReturnsfromReturnStmt ...
Done
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim, Heschi Kreinick.
11 comments:
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #22, Line 80:
Interface types.ObjectSounds good. […]
Aha, per discussion below let's make this a types.Type. Having it as an object means we would capture the var declaration on a parameter with interface literal type. That's a bit odd, when what we really want is a type.
If we need the object for named types, we have types.Named.Obj.
I believe the reason was that I would have liked to log out the error (for debugging/reporting purpo […]
I think a comment would suffice for now.
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #26, Line 114:
RelativeToFiles
For now, I'd say it's a bit of an effort for two reasons: […]
Ack, thanks for investigating.
Patch Set #26, Line 179:
concObj.Pkg() == nil
"builtin" types have a nil package from what I've seen. […]
Ack (though a better condition is probably to only implement interfaces for packages in the current workspace, which would automatically exclude the case of builtin).
Patch Set #26, Line 297:
return named.Obj()
So while experimenting with my follow-up-generics-support, I needed to make the Interface part *type […]
Per my previous comment, probably we should just capture a types.Type.
ptr, isPtr := typ.(*types.Pointer)
if isPtr {
typ = ptr.Elem()
}
I don't believe that would work as the variables would come out of the function's scope and I need t […]
Whoops! Missed that.
File internal/lsp/source/stub.go:
Patch Set #26, Line 46:
si.Interface.Pkg() == nilThat's a good point. Will add the "error" name check. From other code, it looks like I can do "si. […]
LG.
Patch Set #26, Line 107:
fset: snapshot.FileSet(),
Ack
(BTW, no problem at all but I usually think of 'Ack' as meaning 'acknowledged, but not done', whereas 'Done' means 'acknowledged and done').
if si.Pointer {
concrete = "*" + concrete
}
_, err = methodsBuffer.Write(printStubMethod(methodData{
Method: m.Name(),
Concrete: concrete,
Interface: deduceIfaceName(si.Concrete.Pkg(), si.Interface.Pkg(), si.Concrete, si.Interface),
Signature: strings.TrimPrefix(sig, "func"),
}))
if err
I extracted this to "getStubReceiver" which looks potentially silly, but I added a todo that will ma […]
I think there's something wrong here. We shouldn't ever need the stubFile for the package in which the _interface_ is defined, since we always want to stub methods in the package in which the _concrete type_ is defined.
I think it should fall out of this that we don't need special handling for types in the universe scope.
Am I missing something?
if si.Pointer {
concrete = "*" + concrete
}
I agree that it may not be necessary but I thought it would be a better UX that if the user has used […]
Ok, I think you may be right. Let's go with this for now.
if sel := ct.tms.Lookup(ct.pkg, name); sel != nil {
return sel
}
return ct.pms.Lookup(ct.pkg, name)
Ah that sounds great, can the same assumption be made for the "doesNotHaveMethod" method?
Yes: the method set of the pointer is a super set of the method set of the base type.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #29 to this change.
31 files changed, 1,102 insertions(+), 2 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Robert Findley, Hyang-Ah Hana Kim, Heschi Kreinick.
5 comments:
Patchset:
Added some more comments, happy to change anything if needed. Thanks!
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #22, Line 80:
Interface types.Object
Aha, per discussion below let's make this a types.Type. […]
After sync'ing on this; it makes sense to keep this as a types.Object because we still need a reference to the declaring object's package and the ast file in case we need to keep track of renamed imports. Also, the case for for interface literals is that the types.Type is immediately a *types.Interface which loses such reference if we don't keep track of the types.Object which maybe for example a variable such as "p interface{m()}".
I'll leave a comment on top of the declaration explaining the above. Thanks!
I think a comment would suffice for now.
Done
File internal/lsp/source/stub.go:
if si.Pointer {
concrete = "*" + concrete
}
_, err = methodsBuffer.Write(printStubMethod(methodData{
Method: m.Name(),
Concrete: concrete,
Interface: deduceIfaceName(si.Concrete.Pkg(), si.Interface.Pkg(), si.Concrete, si.Interface),
Signature: strings.TrimPrefix(sig, "func"),
}))
if err
We shouldn't ever need the stubFile for the package in which the _interface_ is defined.
The use case for the above is when we are adding an import to the _concrete file_ and the import happens to be renamed in the _interface file_. The UX here is that we want to keep renaming consistent. This is quite common with things like google APIs and protobufs where packages are conventionally renamed such as:
```
import secretmanagerpb "google.golang.org/genproto/googleapis/cloud/secretmanager/v1"
```in this example https://cloud.google.com/secret-manager/docs/create-secret to give a quick point of reference.
if sel := ct.tms.Lookup(ct.pkg, name); sel != nil {
return sel
}
return ct.pms.Lookup(ct.pkg, name)
Yes: the method set of the pointer is a super set of the method set of the base type.
Done
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim, Heschi Kreinick.
Patch set 29:Code-Review +2
3 comments:
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #22, Line 80:
Interface types.Object
After sync'ing on this; it makes sense to keep this as a types. […]
Done
File internal/lsp/source/stub.go:
Patch Set #26, Line 158:
stubErrSame as above comment.
Ack
File internal/lsp/testdata/stub/stub_stdlib.go:
Patch Set #22, Line 1:
package stub
It covers implementing a standard library interface as opposed to a workspace-defined one. […]
Ack
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim, Heschi Kreinick.
6 comments:
File internal/lsp/analysis/stubmethods/stubmethods.go:
Patch Set #21, Line 129:
&& imp.Name.Name != other.Name()Got it, will remove 👌
Done
File internal/lsp/source/stub.go:
if si.Pointer {
concrete = "*" + concrete
}
_, err = methodsBuffer.Write(printStubMethod(methodData{
Method: m.Name(),
Concrete: concrete,
Interface: deduceIfaceName(si.Concrete.Pkg(), si.Interface.Pkg(), si.Concrete, si.Interface),
Signature: strings.TrimPrefix(sig, "func"),
}))
if err
> We shouldn't ever need the stubFile for the package in which the _interface_ is defined. […]
Ack
for i := 0; i < iface.NumEmbeddeds(); i++ {
eiface := iface.Embedded(i).Obj()
depPkg := ifacePkg
if eiface.Pkg().Path() != ifacePkg.PkgPath() {
var err error
depPkg, err = ifacePkg.GetImport(eiface.Pkg().Path())
if err != nil {
return nil, err
}
}
em, err := missingMethods(ctx, snapshot, ct, eiface, depPkg, visited)
if err != nil {
return nil, err
}
missing = append(missing, em...)
}
Not totally sure what you mean, as in replace []*missingInterface with "types. […]
Ack
Patch Set #26, Line 331:
concreteType
Yep! Will remove this type.
Done
Patch Set #26, Line 335:
tms, pms
I think given that I don't "tms" anymore, I can just remove it and rename "pms" to "ms" which should […]
Done
File internal/lsp/testdata/stub/stub_add_selector.go.golden:
Sounds good, will change this to "&readerFrom". […]
Done
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim, Heschi Kreinick.
1 comment:
Patchset:
Marwan, if you're around would you mind rebasing? Anything that touches the summary.*.golden files is liable to break upon merging.
Otherwise I will just merge and fix any test breakages.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Hyang-Ah Hana Kim, Heschi Kreinick.
1 comment:
Patchset:
Marwan, if you're around would you mind rebasing? Anything that touches the summary.*. […]
Just rebased from master 👍
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Heschi Kreinick (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim.
Patch set 30:Code-Review +2
1 comment:
Patchset:
Hooray :)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim.
Patch set 30:Run-TryBot +1Code-Review +2
kokoro (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/011fa4d8-a2c1-49da-b1f3-97f3b4efecee
Patch set 30:gopls-CI +1
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim.
Marwan Sulaiman uploaded patch set #31 to this change.
gopls,internal/lsp: Implement method stubbing via CodeAction
This CL adds a quickfix CodeAction that detects "missing method"
compiler errors and suggests adding method stubs to the concrete
type that would implement the interface. There are many ways that
a user might indicate a concrete type is meant to be used as an interface.
This PR detects two types of those errors: variable declaration and function returns.
For variable declarations, things like the following should be detected:
1. var _ SomeInterface = SomeType{}
2. var _ = SomeInterface(SomeType{})
3. var _ SomeInterface = (*SomeType)(nil)
For function returns, the following example is the primary detection:
func newIface() SomeInterface {
return &SomeType{}
}
More detections can be added in the future of course.
Fixes golang/go#37537
Change-Id: Ibb7784622184c9885eff2ccc786767682876b4d3
---
M gopls/doc/analyzers.md
32 files changed, 1,111 insertions(+), 2 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim.
Patch set 31:Run-TryBot +1Code-Review +2
1 comment:
Patchset:
Marwan, if you get a chance could you please do the following?
generate_test.go:24: documentation needs updating. run: `go run doc/generate.go` from the gopls module.
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim.
1 comment:
Patchset:
Marwan, if you get a chance could you please do the following? […]
You're too fast!
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/1a2dabd1-90d4-4a5e-a3a3-6964a06adc54
Patch set 31:gopls-CI +1
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim.
1 comment:
Patchset:
Build is still in progress... Status page: https://farmer.golang.org/try?commit=1a09d720 […]
Looks like it's still failing. Did you run the command in the test output?
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Robert Findley, Hyang-Ah Hana Kim.
1 comment:
Patchset:
13 of 14 TryBots failed. […]
I believe I did. Not sure if there's another command I'm missing to run?
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Robert Findley, Hyang-Ah Hana Kim.
Marwan Sulaiman uploaded patch set #32 to this change.
gopls,internal/lsp: Implement method stubbing via CodeAction
This CL adds a quickfix CodeAction that detects "missing method"
compiler errors and suggests adding method stubs to the concrete
type that would implement the interface. There are many ways that
a user might indicate a concrete type is meant to be used as an interface.
This PR detects two types of those errors: variable declaration and function returns.
For variable declarations, things like the following should be detected:
1. var _ SomeInterface = SomeType{}
2. var _ = SomeInterface(SomeType{})
3. var _ SomeInterface = (*SomeType)(nil)
For function returns, the following example is the primary detection:
func newIface() SomeInterface {
return &SomeType{}
}
More detections can be added in the future of course.
Fixes golang/go#37537
Change-Id: Ibb7784622184c9885eff2ccc786767682876b4d3
---
M gopls/doc/analyzers.md
A internal/lsp/analysis/stubmethods/stubmethods.go
M internal/lsp/source/api_json.go
33 files changed, 1,121 insertions(+), 2 deletions(-)
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Robert Findley, Hyang-Ah Hana Kim.
1 comment:
Patchset:
Ran it again and saw a new thing got added. I must have missed something haha
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim.
Patch set 32:Run-TryBot +1Code-Review +2
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/4fd72fca-3dd5-4921-9cc4-10816bb97405
Patch set 32:gopls-CI +1
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman, Hyang-Ah Hana Kim.
1 comment:
Patchset:
I believe I did. […]
Done
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Robert Findley submitted this change.
gopls,internal/lsp: Implement method stubbing via CodeAction
This CL adds a quickfix CodeAction that detects "missing method"
compiler errors and suggests adding method stubs to the concrete
type that would implement the interface. There are many ways that
a user might indicate a concrete type is meant to be used as an interface.
This PR detects two types of those errors: variable declaration and function returns.
For variable declarations, things like the following should be detected:
1. var _ SomeInterface = SomeType{}
2. var _ = SomeInterface(SomeType{})
3. var _ SomeInterface = (*SomeType)(nil)
For function returns, the following example is the primary detection:
func newIface() SomeInterface {
return &SomeType{}
}
More detections can be added in the future of course.
Fixes golang/go#37537
Change-Id: Ibb7784622184c9885eff2ccc786767682876b4d3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/274372
Reviewed-by: Heschi Kreinick <hes...@google.com>
Reviewed-by: Robert Findley <rfin...@google.com>
Run-TryBot: Robert Findley <rfin...@google.com>
gopls-CI: kokoro <noreply...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
33 files changed, 1,127 insertions(+), 2 deletions(-)
diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md
index e79f01a..121fa49 100644
--- a/gopls/doc/analyzers.md
+++ b/gopls/doc/analyzers.md
@@ -655,4 +655,13 @@
**Enabled by default.**
+## **stubmethods**
+
+stub methods analyzer
+
+This analyzer generates method stubs for concrete types
+in order to implement a target interface
+
+**Enabled by default.**
+
<!-- END Analyzers: DO NOT MANUALLY EDIT THIS SECTION -->
diff --git a/internal/lsp/analysis/stubmethods/stubmethods.go b/internal/lsp/analysis/stubmethods/stubmethods.go
new file mode 100644
index 0000000..ba0a343
--- /dev/null
+++ b/internal/lsp/analysis/stubmethods/stubmethods.go
@@ -0,0 +1,351 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package stubmethods
+
+import (
+ "bytes"
+ "fmt"
+ "go/ast"
+ "go/format"
+ "go/token"
+ "go/types"
+ "strconv"
+ "strings"
+
+ "golang.org/x/tools/go/analysis"
+ "golang.org/x/tools/go/analysis/passes/inspect"
+ "golang.org/x/tools/go/ast/astutil"
+ "golang.org/x/tools/internal/analysisinternal"
+ "golang.org/x/tools/internal/typesinternal"
+)
+
+const Doc = `stub methods analyzer
+
+This analyzer generates method stubs for concrete types
+in order to implement a target interface`
+
+var Analyzer = &analysis.Analyzer{
+ Name: "stubmethods",
+ Doc: Doc,
+ Requires: []*analysis.Analyzer{inspect.Analyzer},
+ Run: run,
+ RunDespiteErrors: true,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+ for _, err := range analysisinternal.GetTypeErrors(pass) {
+ ifaceErr := strings.Contains(err.Msg, "missing method") || strings.HasPrefix(err.Msg, "cannot convert")
+ if !ifaceErr {
+ continue
+ }
+ var file *ast.File
+ for _, f := range pass.Files {
+ if f.Pos() <= err.Pos && err.Pos < f.End() {
+ file = f
+ break
+ }
+ }
+ if file == nil {
+ continue
+ }
+ // Get the end position of the error.
+ _, _, endPos, ok := typesinternal.ReadGo116ErrorData(err)
+ if !ok {
+ var buf bytes.Buffer
+ if err := format.Node(&buf, pass.Fset, file); err != nil {
+ continue
+ }
+ endPos = analysisinternal.TypeErrorEndPos(pass.Fset, buf.Bytes(), err.Pos)
+ }
+ path, _ := astutil.PathEnclosingInterval(file, err.Pos, endPos)
+ si := GetStubInfo(pass.TypesInfo, path, pass.Pkg, err.Pos)
+ if si == nil {
+ continue
+ }
+ qf := RelativeToFiles(si.Concrete.Obj().Pkg(), file, nil, nil)
+ pass.Report(analysis.Diagnostic{
+ Pos: err.Pos,
+ End: endPos,
+ Message: fmt.Sprintf("Implement %s", types.TypeString(si.Interface.Type(), qf)),
+ })
+ }
+ return nil, nil
+}
+
+// StubInfo represents a concrete type
+// that wants to stub out an interface type
+type StubInfo struct {
+ // Interface is the interface that the client wants to implement.
+ // When the interface is defined, the underlying object will be a TypeName.
+ // Note that we keep track of types.Object instead of types.Type in order
+ // to keep a reference to the declaring object's package and the ast file
+ // 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.
+ Interface types.Object
+ 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, pkg *types.Package, pos token.Pos) *StubInfo {
+ for _, n := range path {
+ switch n := n.(type) {
+ case *ast.ValueSpec:
+ return fromValueSpec(ti, n, pkg, 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, pkg)
+ return si
+ case *ast.AssignStmt:
+ return fromAssignStmt(ti, n, pkg, pos)
+ }
+ }
+ return nil
+}
+
+// fromReturnStmt analyzes a "return" statement to extract
+// a concrete type that is trying to be returned as an interface type.
+//
+// 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, pkg *types.Package) (*StubInfo, error) {
+ returnIdx := -1
+ for i, r := range rs.Results {
+ if pos >= r.Pos() && pos <= r.End() {
+ returnIdx = i
+ }
+ }
+ if returnIdx == -1 {
+ return nil, fmt.Errorf("pos %d not within return statement bounds: [%d-%d]", pos, rs.Pos(), rs.End())
+ }
+ concObj, pointer := concreteType(rs.Results[returnIdx], ti)
+ if concObj == nil || concObj.Obj().Pkg() == nil {
+ return nil, nil
+ }
+ ef := enclosingFunction(path, ti)
+ if ef == nil {
+ return nil, fmt.Errorf("could not find the enclosing function of the return statement")
+ }
+ iface := ifaceType(ef.Results.List[returnIdx].Type, ti)
+ if iface == nil {
+ return nil, nil
+ }
+ return &StubInfo{
+ Concrete: concObj,
+ Pointer: pointer,
+ Interface: iface,
+ }, nil
+}
+
+// fromValueSpec returns *StubInfo from a variable declaration such as
+// var x io.Writer = &T{}
+func fromValueSpec(ti *types.Info, vs *ast.ValueSpec, pkg *types.Package, pos token.Pos) *StubInfo {
+ var idx int
+ for i, vs := range vs.Values {
+ if pos >= vs.Pos() && pos <= vs.End() {
+ idx = i
+ break
+ }
+ }
+
+ valueNode := vs.Values[idx]
+ ifaceNode := vs.Type
+ callExp, ok := valueNode.(*ast.CallExpr)
+ // if the ValueSpec is `var _ = myInterface(...)`
+ // as opposed to `var _ myInterface = ...`
+ if ifaceNode == nil && ok && len(callExp.Args) == 1 {
+ ifaceNode = callExp.Fun
+ valueNode = callExp.Args[0]
+ }
+ concObj, pointer := concreteType(valueNode, ti)
+ if concObj == nil || concObj.Obj().Pkg() == nil {
+ return nil
+ }
+ ifaceObj := ifaceType(ifaceNode, ti)
+ if ifaceObj == nil {
+ return nil
+ }
+ return &StubInfo{
+ Concrete: concObj,
+ Interface: ifaceObj,
+ Pointer: pointer,
+ }
+}
+
+// fromAssignStmt returns *StubInfo from a variable re-assignment such as
+// var x io.Writer
+// x = &T{}
+func fromAssignStmt(ti *types.Info, as *ast.AssignStmt, pkg *types.Package, pos token.Pos) *StubInfo {
+ idx := -1
+ var lhs, rhs ast.Expr
+ // Given a re-assignment interface conversion error,
+ // the compiler error shows up on the right hand side of the expression.
+ // For example, x = &T{} where x is io.Writer highlights the error
+ // under "&T{}" and not "x".
+ for i, hs := range as.Rhs {
+ if pos >= hs.Pos() && pos <= hs.End() {
+ idx = i
+ break
+ }
+ }
+ if idx == -1 {
+ return nil
+ }
+ // Technically, this should never happen as
+ // we would get a "cannot assign N values to M variables"
+ // before we get an interface conversion error. Nonetheless,
+ // guard against out of range index errors.
+ if idx >= len(as.Lhs) {
+ return nil
+ }
+ lhs, rhs = as.Lhs[idx], as.Rhs[idx]
+ ifaceObj := ifaceType(lhs, ti)
+ if ifaceObj == nil {
+ return nil
+ }
+ concType, pointer := concreteType(rhs, ti)
+ if concType == nil || concType.Obj().Pkg() == nil {
+ return nil
+ }
+ return &StubInfo{
+ Concrete: concType,
+ Interface: ifaceObj,
+ Pointer: pointer,
+ }
+}
+
+// RelativeToFiles returns a types.Qualifier that formats package names
+// according to the files where the concrete and interface types are defined.
+//
+// This is similar to types.RelativeTo except if a file imports the package with a different name,
+// then it will use it. And if the file does import the package but it is ignored,
+// then it will return the original name. It also prefers package names in ifaceFile in case
+// an import is missing from concFile but is present in ifaceFile.
+//
+// Additionally, if missingImport is not nil, the function will be called whenever the concFile
+// 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.
+func RelativeToFiles(concPkg *types.Package, concFile, ifaceFile *ast.File, missingImport func(name, path string)) types.Qualifier {
+ return func(other *types.Package) string {
+ if other == concPkg {
+ return ""
+ }
+
+ // Check if the concrete file already has the given import,
+ // if so return the default package name or the renamed import statement.
+ for _, imp := range concFile.Imports {
+ impPath, _ := strconv.Unquote(imp.Path.Value)
+ isIgnored := imp.Name != nil && (imp.Name.Name == "." || imp.Name.Name == "_")
+ if impPath == other.Path() && !isIgnored {
+ importName := other.Name()
+ if imp.Name != nil {
+ importName = imp.Name.Name
+ }
+ return importName
+ }
+ }
+
+ // If the concrete file does not have the import, check if the package
+ // is renamed in the interface file and prefer that.
+ var importName string
+ if ifaceFile != nil {
+ for _, imp := range ifaceFile.Imports {
+ impPath, _ := strconv.Unquote(imp.Path.Value)
+ isIgnored := imp.Name != nil && (imp.Name.Name == "." || imp.Name.Name == "_")
+ if impPath == other.Path() && !isIgnored {
+ if imp.Name != nil && imp.Name.Name != concPkg.Name() {
+ importName = imp.Name.Name
+ }
+ break
+ }
+ }
+ }
+
+ if missingImport != nil {
+ missingImport(importName, other.Path())
+ }
+
+ // Up until this point, importName must stay empty when calling missingImport,
+ // otherwise we'd end up with `import time "time"` which doesn't look idiomatic.
+ if importName == "" {
+ importName = other.Name()
+ }
+ return importName
+ }
+}
+
+// ifaceType will try to extract the types.Object that defines
+// the interface given the ast.Expr where the "missing method"
+// or "conversion" errors happen.
+func ifaceType(n ast.Expr, ti *types.Info) types.Object {
+ tv, ok := ti.Types[n]
+ if !ok {
+ return nil
+ }
+ typ := tv.Type
+ named, ok := typ.(*types.Named)
+ if !ok {
+ return nil
+ }
+ _, ok = named.Underlying().(*types.Interface)
+ if !ok {
+ return nil
+ }
+ // Interfaces defined in the "builtin" package return nil a Pkg().
+ // But they are still real interfaces that we need to make a special case for.
+ // Therefore, protect gopls from panicking if a new interface type was added in the future.
+ if named.Obj().Pkg() == nil && named.Obj().Name() != "error" {
+ return nil
+ }
+ return named.Obj()
+}
+
+// concreteType tries to extract the *types.Named that defines
+// the concrete type given the ast.Expr where the "missing method"
+// or "conversion" errors happened. If the concrete type is something
+// that cannot have methods defined on it (such as basic types), this
+// method will return a nil *types.Named. The second return parameter
+// is a boolean that indicates whether the concreteType was defined as a
+// pointer or value.
+func concreteType(n ast.Expr, ti *types.Info) (*types.Named, bool) {
+ tv, ok := ti.Types[n]
+ if !ok {
+ return nil, false
+ }
+ typ := tv.Type
+ ptr, isPtr := typ.(*types.Pointer)
+ if isPtr {
+ typ = ptr.Elem()
+ }
+ named, ok := typ.(*types.Named)
+ if !ok {
+ return nil, false
+ }
+ return named, isPtr
+}
+
+// enclosingFunction returns the signature and type of the function
+// enclosing the given position.
+func enclosingFunction(path []ast.Node, info *types.Info) *ast.FuncType {
+ for _, node := range path {
+ switch t := node.(type) {
+ case *ast.FuncDecl:
+ if _, ok := info.Defs[t.Name]; ok {
+ return t.Type
+ }
+ case *ast.FuncLit:
+ if _, ok := info.Types[t]; ok {
+ return t.Type
+ }
+ }
+ }
+ return nil
+}
diff --git a/internal/lsp/source/api_json.go b/internal/lsp/source/api_json.go
index 4e8e9df..4742fb1 100755
--- a/internal/lsp/source/api_json.go
+++ b/internal/lsp/source/api_json.go
@@ -433,6 +433,11 @@
Doc: "note incomplete struct initializations\n\nThis analyzer provides diagnostics for any struct literals that do not have\nany fields initialized. Because the suggested fix for this analysis is\nexpensive to compute, callers should compute it separately, using the\nSuggestedFix function below.\n",
Default: "true",
},
+ {
+ Name: "\"stubmethods\"",
+ Doc: "stub methods analyzer\n\nThis analyzer generates method stubs for concrete types\nin order to implement a target interface",
+ Default: "true",
+ },
},
},
Default: "{}",
@@ -946,5 +951,10 @@
Doc: "note incomplete struct initializations\n\nThis analyzer provides diagnostics for any struct literals that do not have\nany fields initialized. Because the suggested fix for this analysis is\nexpensive to compute, callers should compute it separately, using the\nSuggestedFix function below.\n",
Default: true,
},
+ {
+ Name: "stubmethods",
+ Doc: "stub methods analyzer\n\nThis analyzer generates method stubs for concrete types\nin order to implement a target interface",
+ Default: true,
+ },
},
}
diff --git a/internal/lsp/source/fix.go b/internal/lsp/source/fix.go
index e0046ee..2f921ad 100644
--- a/internal/lsp/source/fix.go
+++ b/internal/lsp/source/fix.go
@@ -32,6 +32,7 @@
const (
FillStruct = "fill_struct"
+ StubMethods = "stub_methods"
UndeclaredName = "undeclared_name"
ExtractVariable = "extract_variable"
ExtractFunction = "extract_function"
@@ -45,6 +46,7 @@
ExtractVariable: singleFile(extractVariable),
ExtractFunction: singleFile(extractFunction),
ExtractMethod: singleFile(extractMethod),
+ StubMethods: stubSuggestedFixFunc,
}
// singleFile calls analyzers that expect inputs for a single file
diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go
index 5c34656..139ae70 100644
--- a/internal/lsp/source/options.go
+++ b/internal/lsp/source/options.go
@@ -55,6 +55,7 @@
"golang.org/x/tools/internal/lsp/analysis/simplifycompositelit"
"golang.org/x/tools/internal/lsp/analysis/simplifyrange"
"golang.org/x/tools/internal/lsp/analysis/simplifyslice"
+ "golang.org/x/tools/internal/lsp/analysis/stubmethods"
"golang.org/x/tools/internal/lsp/analysis/undeclaredname"
"golang.org/x/tools/internal/lsp/analysis/unusedparams"
"golang.org/x/tools/internal/lsp/analysis/useany"
@@ -1217,6 +1218,12 @@
Enabled: true,
ActionKind: []protocol.CodeActionKind{protocol.RefactorRewrite},
},
+ stubmethods.Analyzer.Name: {
+ Analyzer: stubmethods.Analyzer,
+ ActionKind: []protocol.CodeActionKind{protocol.RefactorRewrite},
+ Fix: StubMethods,
+ Enabled: true,
+ },
}
}
diff --git a/internal/lsp/source/stub.go b/internal/lsp/source/stub.go
new file mode 100644
index 0000000..276bec1
--- /dev/null
+++ b/internal/lsp/source/stub.go
@@ -0,0 +1,328 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package source
+
+import (
+ "bytes"
+ "context"
+ "fmt"
+ "go/ast"
+ "go/format"
+ "go/parser"
+ "go/token"
+ "go/types"
+ "strings"
+
+ "golang.org/x/tools/go/analysis"
+ "golang.org/x/tools/go/ast/astutil"
+ "golang.org/x/tools/internal/lsp/analysis/stubmethods"
+ "golang.org/x/tools/internal/lsp/protocol"
+ "golang.org/x/tools/internal/span"
+)
+
+func stubSuggestedFixFunc(ctx context.Context, snapshot Snapshot, fh VersionedFileHandle, rng protocol.Range) (*analysis.SuggestedFix, error) {
+ pkg, pgf, err := GetParsedFile(ctx, snapshot, fh, NarrowestPackage)
+ if err != nil {
+ return nil, fmt.Errorf("GetParsedFile: %w", err)
+ }
+ nodes, pos, err := getStubNodes(pgf, rng)
+ if err != nil {
+ return nil, fmt.Errorf("getNodes: %w", err)
+ }
+ si := stubmethods.GetStubInfo(pkg.GetTypesInfo(), nodes, pkg.GetTypes(), pos)
+ if si == nil {
+ return nil, fmt.Errorf("nil interface request")
+ }
+ parsedConcreteFile, concreteFH, err := getStubFile(ctx, si.Concrete.Obj(), snapshot)
+ if err != nil {
+ return nil, fmt.Errorf("getFile(concrete): %w", err)
+ }
+ var (
+ methodsSrc []byte
+ stubImports []*stubImport // additional imports needed for method stubs
+ )
+ if si.Interface.Pkg() == nil && si.Interface.Name() == "error" && si.Interface.Parent() == types.Universe {
+ methodsSrc = stubErr(ctx, parsedConcreteFile.File, si, snapshot)
+ } else {
+ methodsSrc, stubImports, err = stubMethods(ctx, parsedConcreteFile.File, si, snapshot)
+ }
+ if err != nil {
+ return nil, fmt.Errorf("stubMethods: %w", err)
+ }
+ nodes, _ = astutil.PathEnclosingInterval(parsedConcreteFile.File, si.Concrete.Obj().Pos(), si.Concrete.Obj().Pos())
+ concreteSrc, err := concreteFH.Read()
+ if err != nil {
+ return nil, fmt.Errorf("error reading concrete file source: %w", err)
+ }
+ insertPos := snapshot.FileSet().Position(nodes[1].End()).Offset
+ if insertPos >= len(concreteSrc) {
+ return nil, fmt.Errorf("insertion position is past the end of the file")
+ }
+ var buf bytes.Buffer
+ buf.Write(concreteSrc[:insertPos])
+ buf.WriteByte('\n')
+ buf.Write(methodsSrc)
+ buf.Write(concreteSrc[insertPos:])
+ fset := token.NewFileSet()
+ newF, err := parser.ParseFile(fset, parsedConcreteFile.File.Name.Name, buf.Bytes(), parser.ParseComments)
+ if err != nil {
+ return nil, fmt.Errorf("could not reparse file: %w", err)
+ }
+ for _, imp := range stubImports {
+ astutil.AddNamedImport(fset, newF, imp.Name, imp.Path)
+ }
+ var source bytes.Buffer
+ err = format.Node(&source, fset, newF)
+ if err != nil {
+ return nil, fmt.Errorf("format.Node: %w", err)
+ }
+ diffEdits, err := snapshot.View().Options().ComputeEdits(parsedConcreteFile.URI, string(parsedConcreteFile.Src), source.String())
+ if err != nil {
+ return nil, err
+ }
+ var edits []analysis.TextEdit
+ for _, edit := range diffEdits {
+ rng, err := edit.Span.Range(parsedConcreteFile.Mapper.Converter)
+ if err != nil {
+ return nil, err
+ }
+ edits = append(edits, analysis.TextEdit{
+ Pos: rng.Start,
+ End: rng.End,
+ NewText: []byte(edit.NewText),
+ })
+ }
+ return &analysis.SuggestedFix{
+ TextEdits: edits,
+ }, nil
+}
+
+// stubMethods returns the Go code of all methods
+// that implement the given interface
+func stubMethods(ctx context.Context, concreteFile *ast.File, si *stubmethods.StubInfo, snapshot Snapshot) ([]byte, []*stubImport, error) {
+ ifacePkg, err := deducePkgFromTypes(ctx, snapshot, si.Interface)
+ if err != nil {
+ return nil, nil, err
+ }
+ si.Concrete.Obj().Type()
+ concMS := types.NewMethodSet(types.NewPointer(si.Concrete.Obj().Type()))
+ missing, err := missingMethods(ctx, snapshot, concMS, si.Concrete.Obj().Pkg(), si.Interface, ifacePkg, map[string]struct{}{})
+ if err != nil {
+ return nil, nil, fmt.Errorf("missingMethods: %w", err)
+ }
+ if len(missing) == 0 {
+ return nil, nil, fmt.Errorf("no missing methods found")
+ }
+ var (
+ stubImports []*stubImport
+ methodsBuffer bytes.Buffer
+ )
+ for _, mi := range missing {
+ for _, m := range mi.missing {
+ // TODO(marwan-at-work): this should share the same logic with source.FormatVarType
+ // as it also accounts for type aliases.
+ sig := types.TypeString(m.Type(), stubmethods.RelativeToFiles(si.Concrete.Obj().Pkg(), concreteFile, mi.file, func(name, path string) {
+ for _, imp := range stubImports {
+ if imp.Name == name && imp.Path == path {
+ return
+ }
+ }
+ stubImports = append(stubImports, &stubImport{name, path})
+ }))
+ _, err = methodsBuffer.Write(printStubMethod(methodData{
+ Method: m.Name(),
+ Concrete: getStubReceiver(si),
+ Interface: deduceIfaceName(si.Concrete.Obj().Pkg(), si.Interface.Pkg(), si.Concrete.Obj(), si.Interface),
+ Signature: strings.TrimPrefix(sig, "func"),
+ }))
+ if err != nil {
+ return nil, nil, fmt.Errorf("error printing method: %w", err)
+ }
+ methodsBuffer.WriteRune('\n')
+ }
+ }
+ return methodsBuffer.Bytes(), stubImports, nil
+}
+
+// stubErr reurns the Go code implementation
+// of an error interface relevant to the
+// concrete type
+func stubErr(ctx context.Context, concreteFile *ast.File, si *stubmethods.StubInfo, snapshot Snapshot) []byte {
+ return printStubMethod(methodData{
+ Method: "Error",
+ Interface: "error",
+ Concrete: getStubReceiver(si),
+ Signature: "() string",
+ })
+}
+
+// getStubReceiver returns the concrete type's name as a method receiver.
+// TODO(marwan-at-work): add type parameters to the receiver when the concrete type
+// is a generic one.
+func getStubReceiver(si *stubmethods.StubInfo) string {
+ concrete := si.Concrete.Obj().Name()
+ if si.Pointer {
+ concrete = "*" + concrete
+ }
+ return concrete
+}
+
+type methodData struct {
+ Method string
+ Interface string
+ Concrete string
+ Signature string
+}
+
+// printStubMethod takes methodData and returns Go code that represents the given method such as:
+// // {{ .Method }} implements {{ .Interface }}
+// func ({{ .Concrete }}) {{ .Method }}{{ .Signature }} {
+// panic("unimplemented")
+// }
+func printStubMethod(md methodData) []byte {
+ var b bytes.Buffer
+ fmt.Fprintf(&b, "// %s implements %s\n", md.Method, md.Interface)
+ fmt.Fprintf(&b, "func (%s) %s%s {\n\t", md.Concrete, md.Method, md.Signature)
+ fmt.Fprintln(&b, `panic("unimplemented")`)
+ fmt.Fprintln(&b, "}")
+ return b.Bytes()
+}
+
+func deducePkgFromTypes(ctx context.Context, snapshot Snapshot, ifaceObj types.Object) (Package, error) {
+ pkgs, err := snapshot.KnownPackages(ctx)
+ if err != nil {
+ return nil, err
+ }
+ for _, p := range pkgs {
+ if p.PkgPath() == ifaceObj.Pkg().Path() {
+ return p, nil
+ }
+ }
+ return nil, fmt.Errorf("pkg %q not found", ifaceObj.Pkg().Path())
+}
+
+func deduceIfaceName(concretePkg, ifacePkg *types.Package, concreteObj, ifaceObj types.Object) string {
+ if concretePkg.Path() == ifacePkg.Path() {
+ return ifaceObj.Name()
+ }
+ return fmt.Sprintf("%s.%s", ifacePkg.Name(), ifaceObj.Name())
+}
+
+func getStubNodes(pgf *ParsedGoFile, pRng protocol.Range) ([]ast.Node, token.Pos, error) {
+ spn, err := pgf.Mapper.RangeSpan(pRng)
+ if err != nil {
+ return nil, 0, err
+ }
+ rng, err := spn.Range(pgf.Mapper.Converter)
+ if err != nil {
+ return nil, 0, err
+ }
+ nodes, _ := astutil.PathEnclosingInterval(pgf.File, rng.Start, rng.End)
+ return nodes, rng.Start, nil
+}
+
+/*
+missingMethods takes a concrete type and returns any missing methods for the given interface as well as
+any missing interface that might have been embedded to its parent. For example:
+
+type I interface {
+ io.Writer
+ Hello()
+}
+returns []*missingInterface{
+ {
+ iface: *types.Interface (io.Writer),
+ file: *ast.File: io.go,
+ missing []*types.Func{Write},
+ },
+ {
+ iface: *types.Interface (I),
+ file: *ast.File: myfile.go,
+ missing: []*types.Func{Hello}
+ },
+}
+*/
+func missingMethods(ctx context.Context, snapshot Snapshot, concMS *types.MethodSet, concPkg *types.Package, ifaceObj types.Object, ifacePkg Package, 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())
+ }
+ missing := []*missingInterface{}
+ for i := 0; i < iface.NumEmbeddeds(); i++ {
+ eiface := iface.Embedded(i).Obj()
+ depPkg := ifacePkg
+ if eiface.Pkg().Path() != ifacePkg.PkgPath() {
+ var err error
+ depPkg, err = ifacePkg.GetImport(eiface.Pkg().Path())
+ if err != nil {
+ return nil, err
+ }
+ }
+ em, err := missingMethods(ctx, snapshot, concMS, concPkg, eiface, depPkg, visited)
+ if err != nil {
+ return nil, err
+ }
+ missing = append(missing, em...)
+ }
+ parsedFile, _, err := getStubFile(ctx, ifaceObj, snapshot)
+ if err != nil {
+ return nil, fmt.Errorf("error getting iface file: %w", err)
+ }
+ mi := &missingInterface{
+ pkg: ifacePkg,
+ iface: iface,
+ file: parsedFile.File,
+ }
+ if mi.file == nil {
+ return nil, fmt.Errorf("could not find ast.File for %v", ifaceObj.Name())
+ }
+ for i := 0; i < iface.NumExplicitMethods(); i++ {
+ method := iface.ExplicitMethod(i)
+ // if the concrete type does not have the interface method
+ if concMS.Lookup(concPkg, method.Name()) == nil {
+ if _, ok := visited[method.Name()]; !ok {
+ mi.missing = append(mi.missing, method)
+ visited[method.Name()] = struct{}{}
+ }
+ }
+ if sel := concMS.Lookup(concPkg, method.Name()); sel != nil {
+ implSig := sel.Type().(*types.Signature)
+ ifaceSig := method.Type().(*types.Signature)
+ if !types.Identical(ifaceSig, implSig) {
+ return nil, fmt.Errorf("mimsatched %q function signatures:\nhave: %s\nwant: %s", method.Name(), implSig, ifaceSig)
+ }
+ }
+ }
+ if len(mi.missing) > 0 {
+ missing = append(missing, mi)
+ }
+ return missing, nil
+}
+
+func getStubFile(ctx context.Context, obj types.Object, snapshot Snapshot) (*ParsedGoFile, VersionedFileHandle, error) {
+ objPos := snapshot.FileSet().Position(obj.Pos())
+ objFile := span.URIFromPath(objPos.Filename)
+ objectFH := snapshot.FindFile(objFile)
+ _, goFile, err := GetParsedFile(ctx, snapshot, objectFH, WidestPackage)
+ if err != nil {
+ return nil, nil, fmt.Errorf("GetParsedFile: %w", err)
+ }
+ return goFile, objectFH, nil
+}
+
+// missingInterface represents an interface
+// that has all or some of its methods missing
+// from the destination concrete type
+type missingInterface struct {
+ iface *types.Interface
+ file *ast.File
+ pkg Package
+ missing []*types.Func
+}
+
+// stubImport represents a newly added import
+// statement to the concrete type. If name is not
+// empty, then that import is required to have that name.
+type stubImport struct{ Name, Path string }
diff --git a/internal/lsp/testdata/stub/other/other.go b/internal/lsp/testdata/stub/other/other.go
new file mode 100644
index 0000000..ba3c174
--- /dev/null
+++ b/internal/lsp/testdata/stub/other/other.go
@@ -0,0 +1,10 @@
+package other
+
+import (
+ "bytes"
+ renamed_context "context"
+)
+
+type Interface interface {
+ Get(renamed_context.Context) *bytes.Buffer
+}
diff --git a/internal/lsp/testdata/stub/stub_add_selector.go b/internal/lsp/testdata/stub/stub_add_selector.go
new file mode 100644
index 0000000..a15afd7
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_add_selector.go
@@ -0,0 +1,12 @@
+package stub
+
+import "io"
+
+// This file tests that if an interface
+// method references a type from its own package
+// then our implementation must add the import/package selector
+// in the concrete method if the concrete type is outside of the interface
+// package
+var _ io.ReaderFrom = &readerFrom{} //@suggestedfix("&readerFrom", "refactor.rewrite")
+
+type readerFrom struct{}
diff --git a/internal/lsp/testdata/stub/stub_add_selector.go.golden b/internal/lsp/testdata/stub/stub_add_selector.go.golden
new file mode 100644
index 0000000..e885483
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_add_selector.go.golden
@@ -0,0 +1,19 @@
+-- suggestedfix_stub_add_selector_10_23 --
+package stub
+
+import "io"
+
+// This file tests that if an interface
+// method references a type from its own package
+// then our implementation must add the import/package selector
+// in the concrete method if the concrete type is outside of the interface
+// package
+var _ io.ReaderFrom = &readerFrom{} //@suggestedfix("&readerFrom", "refactor.rewrite")
+
+type readerFrom struct{}
+
+// ReadFrom implements io.ReaderFrom
+func (*readerFrom) ReadFrom(r io.Reader) (n int64, err error) {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/stub_assign.go b/internal/lsp/testdata/stub/stub_assign.go
new file mode 100644
index 0000000..9336361
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_assign.go
@@ -0,0 +1,10 @@
+package stub
+
+import "io"
+
+func main() {
+ var br io.ByteWriter
+ br = &byteWriter{} //@suggestedfix("&", "refactor.rewrite")
+}
+
+type byteWriter struct{}
diff --git a/internal/lsp/testdata/stub/stub_assign.go.golden b/internal/lsp/testdata/stub/stub_assign.go.golden
new file mode 100644
index 0000000..a52a823
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_assign.go.golden
@@ -0,0 +1,17 @@
+-- suggestedfix_stub_assign_7_7 --
+package stub
+
+import "io"
+
+func main() {
+ var br io.ByteWriter
+ br = &byteWriter{} //@suggestedfix("&", "refactor.rewrite")
+}
+
+type byteWriter struct{}
+
+// WriteByte implements io.ByteWriter
+func (*byteWriter) WriteByte(c byte) error {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/stub_assign_multivars.go b/internal/lsp/testdata/stub/stub_assign_multivars.go
new file mode 100644
index 0000000..01b330f
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_assign_multivars.go
@@ -0,0 +1,11 @@
+package stub
+
+import "io"
+
+func main() {
+ var br io.ByteWriter
+ var i int
+ i, br = 1, &multiByteWriter{} //@suggestedfix("&", "refactor.rewrite")
+}
+
+type multiByteWriter struct{}
diff --git a/internal/lsp/testdata/stub/stub_assign_multivars.go.golden b/internal/lsp/testdata/stub/stub_assign_multivars.go.golden
new file mode 100644
index 0000000..e1e71ad
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_assign_multivars.go.golden
@@ -0,0 +1,18 @@
+-- suggestedfix_stub_assign_multivars_8_13 --
+package stub
+
+import "io"
+
+func main() {
+ var br io.ByteWriter
+ var i int
+ i, br = 1, &multiByteWriter{} //@suggestedfix("&", "refactor.rewrite")
+}
+
+type multiByteWriter struct{}
+
+// WriteByte implements io.ByteWriter
+func (*multiByteWriter) WriteByte(c byte) error {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/stub_embedded.go b/internal/lsp/testdata/stub/stub_embedded.go
new file mode 100644
index 0000000..6d6a986
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_embedded.go
@@ -0,0 +1,15 @@
+package stub
+
+import (
+ "io"
+ "sort"
+)
+
+var _ embeddedInterface = (*embeddedConcrete)(nil) //@suggestedfix("(", "refactor.rewrite")
+
+type embeddedConcrete struct{}
+
+type embeddedInterface interface {
+ sort.Interface
+ io.Reader
+}
diff --git a/internal/lsp/testdata/stub/stub_embedded.go.golden b/internal/lsp/testdata/stub/stub_embedded.go.golden
new file mode 100644
index 0000000..c258eba
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_embedded.go.golden
@@ -0,0 +1,37 @@
+-- suggestedfix_stub_embedded_8_27 --
+package stub
+
+import (
+ "io"
+ "sort"
+)
+
+var _ embeddedInterface = (*embeddedConcrete)(nil) //@suggestedfix("(", "refactor.rewrite")
+
+type embeddedConcrete struct{}
+
+// Len implements embeddedInterface
+func (*embeddedConcrete) Len() int {
+ panic("unimplemented")
+}
+
+// Less implements embeddedInterface
+func (*embeddedConcrete) Less(i int, j int) bool {
+ panic("unimplemented")
+}
+
+// Swap implements embeddedInterface
+func (*embeddedConcrete) Swap(i int, j int) {
+ panic("unimplemented")
+}
+
+// Read implements embeddedInterface
+func (*embeddedConcrete) Read(p []byte) (n int, err error) {
+ panic("unimplemented")
+}
+
+type embeddedInterface interface {
+ sort.Interface
+ io.Reader
+}
+
diff --git a/internal/lsp/testdata/stub/stub_err.go b/internal/lsp/testdata/stub/stub_err.go
new file mode 100644
index 0000000..908c7d3
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_err.go
@@ -0,0 +1,7 @@
+package stub
+
+func main() {
+ var br error = &customErr{} //@suggestedfix("&", "refactor.rewrite")
+}
+
+type customErr struct{}
diff --git a/internal/lsp/testdata/stub/stub_err.go.golden b/internal/lsp/testdata/stub/stub_err.go.golden
new file mode 100644
index 0000000..717aed8
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_err.go.golden
@@ -0,0 +1,14 @@
+-- suggestedfix_stub_err_4_17 --
+package stub
+
+func main() {
+ var br error = &customErr{} //@suggestedfix("&", "refactor.rewrite")
+}
+
+type customErr struct{}
+
+// Error implements error
+func (*customErr) Error() string {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/stub_function_return.go b/internal/lsp/testdata/stub/stub_function_return.go
new file mode 100644
index 0000000..bbf0588
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_function_return.go
@@ -0,0 +1,11 @@
+package stub
+
+import (
+ "io"
+)
+
+func newCloser() io.Closer {
+ return closer{} //@suggestedfix("c", "refactor.rewrite")
+}
+
+type closer struct{}
diff --git a/internal/lsp/testdata/stub/stub_function_return.go.golden b/internal/lsp/testdata/stub/stub_function_return.go.golden
new file mode 100644
index 0000000..f80874d
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_function_return.go.golden
@@ -0,0 +1,18 @@
+-- suggestedfix_stub_function_return_8_9 --
+package stub
+
+import (
+ "io"
+)
+
+func newCloser() io.Closer {
+ return closer{} //@suggestedfix("c", "refactor.rewrite")
+}
+
+type closer struct{}
+
+// Close implements io.Closer
+func (closer) Close() error {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/stub_ignored_imports.go b/internal/lsp/testdata/stub/stub_ignored_imports.go
new file mode 100644
index 0000000..f0abcc5
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_ignored_imports.go
@@ -0,0 +1,18 @@
+package stub
+
+import (
+ "compress/zlib"
+ . "io"
+ _ "io"
+)
+
+// This file tests that dot-imports and underscore imports
+// are properly ignored and that a new import is added to
+// refernece method types
+
+var (
+ _ Reader
+ _ zlib.Resetter = (*ignoredResetter)(nil) //@suggestedfix("(", "refactor.rewrite")
+)
+
+type ignoredResetter struct{}
diff --git a/internal/lsp/testdata/stub/stub_ignored_imports.go.golden b/internal/lsp/testdata/stub/stub_ignored_imports.go.golden
new file mode 100644
index 0000000..4f34d79
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_ignored_imports.go.golden
@@ -0,0 +1,26 @@
+-- suggestedfix_stub_ignored_imports_15_20 --
+package stub
+
+import (
+ "compress/zlib"
+ "io"
+ . "io"
+ _ "io"
+)
+
+// This file tests that dot-imports and underscore imports
+// are properly ignored and that a new import is added to
+// refernece method types
+
+var (
+ _ Reader
+ _ zlib.Resetter = (*ignoredResetter)(nil) //@suggestedfix("(", "refactor.rewrite")
+)
+
+type ignoredResetter struct{}
+
+// Reset implements zlib.Resetter
+func (*ignoredResetter) Reset(r io.Reader, dict []byte) error {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/stub_multi_var.go b/internal/lsp/testdata/stub/stub_multi_var.go
new file mode 100644
index 0000000..4276b79
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_multi_var.go
@@ -0,0 +1,11 @@
+package stub
+
+import "io"
+
+// This test ensures that a variable declaration that
+// has multiple values on the same line can still be
+// analyzed correctly to target the interface implementation
+// diagnostic.
+var one, two, three io.Reader = nil, &multiVar{}, nil //@suggestedfix("&", "refactor.rewrite")
+
+type multiVar struct{}
diff --git a/internal/lsp/testdata/stub/stub_multi_var.go.golden b/internal/lsp/testdata/stub/stub_multi_var.go.golden
new file mode 100644
index 0000000..b9ac423
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_multi_var.go.golden
@@ -0,0 +1,18 @@
+-- suggestedfix_stub_multi_var_9_38 --
+package stub
+
+import "io"
+
+// This test ensures that a variable declaration that
+// has multiple values on the same line can still be
+// analyzed correctly to target the interface implementation
+// diagnostic.
+var one, two, three io.Reader = nil, &multiVar{}, nil //@suggestedfix("&", "refactor.rewrite")
+
+type multiVar struct{}
+
+// Read implements io.Reader
+func (*multiVar) Read(p []byte) (n int, err error) {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/stub_pointer.go b/internal/lsp/testdata/stub/stub_pointer.go
new file mode 100644
index 0000000..2b3681b
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_pointer.go
@@ -0,0 +1,9 @@
+package stub
+
+import "io"
+
+func getReaderFrom() io.ReaderFrom {
+ return &pointerImpl{} //@suggestedfix("&", "refactor.rewrite")
+}
+
+type pointerImpl struct{}
diff --git a/internal/lsp/testdata/stub/stub_pointer.go.golden b/internal/lsp/testdata/stub/stub_pointer.go.golden
new file mode 100644
index 0000000..c4133d7
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_pointer.go.golden
@@ -0,0 +1,16 @@
+-- suggestedfix_stub_pointer_6_9 --
+package stub
+
+import "io"
+
+func getReaderFrom() io.ReaderFrom {
+ return &pointerImpl{} //@suggestedfix("&", "refactor.rewrite")
+}
+
+type pointerImpl struct{}
+
+// ReadFrom implements io.ReaderFrom
+func (*pointerImpl) ReadFrom(r io.Reader) (n int64, err error) {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/stub_renamed_import.go b/internal/lsp/testdata/stub/stub_renamed_import.go
new file mode 100644
index 0000000..eaebe25
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_renamed_import.go
@@ -0,0 +1,11 @@
+package stub
+
+import (
+ "compress/zlib"
+ myio "io"
+)
+
+var _ zlib.Resetter = &myIO{} //@suggestedfix("&", "refactor.rewrite")
+var _ myio.Reader
+
+type myIO struct{}
diff --git a/internal/lsp/testdata/stub/stub_renamed_import.go.golden b/internal/lsp/testdata/stub/stub_renamed_import.go.golden
new file mode 100644
index 0000000..48ff4f1
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_renamed_import.go.golden
@@ -0,0 +1,18 @@
+-- suggestedfix_stub_renamed_import_8_23 --
+package stub
+
+import (
+ "compress/zlib"
+ myio "io"
+)
+
+var _ zlib.Resetter = &myIO{} //@suggestedfix("&", "refactor.rewrite")
+var _ myio.Reader
+
+type myIO struct{}
+
+// Reset implements zlib.Resetter
+func (*myIO) Reset(r myio.Reader, dict []byte) error {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/stub_renamed_import_iface.go b/internal/lsp/testdata/stub/stub_renamed_import_iface.go
new file mode 100644
index 0000000..96caf54
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_renamed_import_iface.go
@@ -0,0 +1,13 @@
+package stub
+
+import (
+ "golang.org/x/tools/internal/lsp/stub/other"
+)
+
+// This file tests that if an interface
+// method references an import from its own package
+// that the concrete type does not yet import, and that import happens
+// to be renamed, then we prefer the renaming of the interface.
+var _ other.Interface = &otherInterfaceImpl{} //@suggestedfix("&otherInterfaceImpl", "refactor.rewrite")
+
+type otherInterfaceImpl struct{}
diff --git a/internal/lsp/testdata/stub/stub_renamed_import_iface.go.golden b/internal/lsp/testdata/stub/stub_renamed_import_iface.go.golden
new file mode 100644
index 0000000..9ba2cb4
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_renamed_import_iface.go.golden
@@ -0,0 +1,22 @@
+-- suggestedfix_stub_renamed_import_iface_11_25 --
+package stub
+
+import (
+ "bytes"
+ renamed_context "context"
+ "golang.org/x/tools/internal/lsp/stub/other"
+)
+
+// This file tests that if an interface
+// method references an import from its own package
+// that the concrete type does not yet import, and that import happens
+// to be renamed, then we prefer the renaming of the interface.
+var _ other.Interface = &otherInterfaceImpl{} //@suggestedfix("&otherInterfaceImpl", "refactor.rewrite")
+
+type otherInterfaceImpl struct{}
+
+// Get implements other.Interface
+func (*otherInterfaceImpl) Get(renamed_context.Context) *bytes.Buffer {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/stub/stub_stdlib.go b/internal/lsp/testdata/stub/stub_stdlib.go
new file mode 100644
index 0000000..0d54a6d
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_stdlib.go
@@ -0,0 +1,9 @@
+package stub
+
+import (
+ "io"
+)
+
+var _ io.Writer = writer{} //@suggestedfix("w", "refactor.rewrite")
+
+type writer struct{}
diff --git a/internal/lsp/testdata/stub/stub_stdlib.go.golden b/internal/lsp/testdata/stub/stub_stdlib.go.golden
new file mode 100644
index 0000000..8636cea
--- /dev/null
+++ b/internal/lsp/testdata/stub/stub_stdlib.go.golden
@@ -0,0 +1,16 @@
+-- suggestedfix_stub_stdlib_7_19 --
+package stub
+
+import (
+ "io"
+)
+
+var _ io.Writer = writer{} //@suggestedfix("w", "refactor.rewrite")
+
+type writer struct{}
+
+// Write implements io.Writer
+func (writer) Write(p []byte) (n int, err error) {
+ panic("unimplemented")
+}
+
diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden
index bfe40b3..2949392 100644
--- a/internal/lsp/testdata/summary.txt.golden
+++ b/internal/lsp/testdata/summary.txt.golden
@@ -13,7 +13,7 @@
FormatCount = 6
ImportCount = 8
SemanticTokenCount = 3
-SuggestedFixCount = 49
+SuggestedFixCount = 61
FunctionExtractionCount = 25
MethodExtractionCount = 6
DefinitionsCount = 95
diff --git a/internal/lsp/testdata/summary_go1.18.txt.golden b/internal/lsp/testdata/summary_go1.18.txt.golden
index 02c6742..86b7b1e 100644
--- a/internal/lsp/testdata/summary_go1.18.txt.golden
+++ b/internal/lsp/testdata/summary_go1.18.txt.golden
@@ -13,7 +13,7 @@
FormatCount = 6
ImportCount = 8
SemanticTokenCount = 3
-SuggestedFixCount = 49
+SuggestedFixCount = 61
FunctionExtractionCount = 25
MethodExtractionCount = 6
DefinitionsCount = 99
To view, visit change 274372. To unsubscribe, or for help writing mail filters, visit settings.