From 1bd98f437a3c7dd409dfb799cb20c0de2f939c9d Mon Sep 17 00:00:00 2001 From: Denis Date: Sun, 17 Nov 2024 21:40:57 +0100 Subject: [PATCH] x/tools/gopls: implement struct field generation quickfix --- gopls/doc/release/v0.17.0.md | 16 +- .../analysis/yield/testdata/src/a/a.go | 3 - gopls/internal/analysis/yield/yield.go | 12 - gopls/internal/analysis/yield/yield_test.go | 1 - gopls/internal/doc/api.json | 3 - gopls/internal/golang/assembly.go | 2 +- gopls/internal/golang/codeaction.go | 2 +- .../golang/stubmethods/stubcalledfunc.go | 329 ------------------ gopls/internal/golang/type_definition.go | 1 - gopls/internal/util/typesutil/typesutil.go | 167 +++++++++ 10 files changed, 177 insertions(+), 359 deletions(-) diff --git a/gopls/doc/release/v0.17.0.md b/gopls/doc/release/v0.17.0.md index 603c2726395..4e5b3fa46b0 100644 --- a/gopls/doc/release/v0.17.0.md +++ b/gopls/doc/release/v0.17.0.md @@ -88,14 +88,6 @@ where T is the concrete type and f is the undefined method. The stub method's signature is inferred from the context of the call. -## Generate missing struct field from access -When you attempt to access a field on a type that does not have the field, -the compiler will report an error like “type X has no field or method Y”. -Gopls now offers a new code action, “Declare missing field of T.f”, -where T is the concrete type and f is the undefined field. -The stub field's signature is inferred -from the context of the access. - ## `yield` analyzer The new `yield` analyzer detects mistakes using the `yield` function @@ -122,3 +114,11 @@ Since this feature is implemented by the server (gopls), it is compatible with all LSP-compliant editors. VS Code users may continue to use the client-side `Go: Generate Unit Tests For file/function/package` command which utilizes the [gotests](https://github.com/cweill/gotests) tool. + +## Generate missing struct field from access +When you attempt to access a field on a type that does not have the field, +the compiler will report an error like “type X has no field or method Y”. +Gopls now offers a new code action, “Declare missing field of T.f”, +where T is the concrete type and f is the undefined field. +The stub field's signature is inferred +from the context of the access. diff --git a/gopls/internal/analysis/yield/testdata/src/a/a.go b/gopls/internal/analysis/yield/testdata/src/a/a.go index 5960cddb220..9eb88b5ae69 100644 --- a/gopls/internal/analysis/yield/testdata/src/a/a.go +++ b/gopls/internal/analysis/yield/testdata/src/a/a.go @@ -83,7 +83,6 @@ func tricky(in io.ReadCloser) func(yield func(string, error) bool) { } } } -<<<<<<< HEAD // Regression test for issue #70598. func shortCircuitAND(yield func(int) bool) { @@ -119,5 +118,3 @@ func tricky3(yield func(int) bool) { yield(3) } } -======= ->>>>>>> 9b6e4f21d (gopls/internal/analysis/yield: analyzer for iterator (yield) mistakes) diff --git a/gopls/internal/analysis/yield/yield.go b/gopls/internal/analysis/yield/yield.go index 9e580d6872a..9eaea3f0407 100644 --- a/gopls/internal/analysis/yield/yield.go +++ b/gopls/internal/analysis/yield/yield.go @@ -20,10 +20,7 @@ import ( _ "embed" "fmt" "go/ast" -<<<<<<< HEAD "go/constant" -======= ->>>>>>> 9b6e4f21d (gopls/internal/analysis/yield: analyzer for iterator (yield) mistakes) "go/token" "go/types" @@ -123,7 +120,6 @@ func run(pass *analysis.Pass) (interface{}, error) { // In that case visit only the "if !yield()" block. cond := instr.Cond t, f := b.Succs[0], b.Succs[1] -<<<<<<< HEAD // Strip off any NOT operator. cond, t, f = unnegate(cond, t, f) @@ -152,11 +148,6 @@ func run(pass *analysis.Pass) (interface{}, error) { } } -======= - if unop, ok := cond.(*ssa.UnOp); ok && unop.Op == token.NOT { - cond, t, f = unop.X, f, t - } ->>>>>>> 9b6e4f21d (gopls/internal/analysis/yield: analyzer for iterator (yield) mistakes) if cond, ok := cond.(*ssa.Call); ok && ssaYieldCalls[cond] != nil { // Skip the successor reached by "if yield() { ... }". } else { @@ -178,7 +169,6 @@ func run(pass *analysis.Pass) (interface{}, error) { return nil, nil } -<<<<<<< HEAD func unnegate(cond ssa.Value, t, f *ssa.BasicBlock) (_ ssa.Value, _, _ *ssa.BasicBlock) { if unop, ok := cond.(*ssa.UnOp); ok && unop.Op == token.NOT { @@ -186,5 +176,3 @@ func unnegate(cond ssa.Value, t, f *ssa.BasicBlock) (_ ssa.Value, _, _ *ssa.Basi } return cond, t, f } -======= ->>>>>>> 9b6e4f21d (gopls/internal/analysis/yield: analyzer for iterator (yield) mistakes) diff --git a/gopls/internal/analysis/yield/yield_test.go b/gopls/internal/analysis/yield/yield_test.go index 78944997ea5..af6784374e2 100644 --- a/gopls/internal/analysis/yield/yield_test.go +++ b/gopls/internal/analysis/yield/yield_test.go @@ -15,4 +15,3 @@ func Test(t *testing.T) { testdata := analysistest.TestData() analysistest.Run(t, testdata, yield.Analyzer, "a") } -e \ No newline at end of file diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json index e47b65c4044..b64965ab863 100644 --- a/gopls/internal/doc/api.json +++ b/gopls/internal/doc/api.json @@ -1304,15 +1304,12 @@ "Default": false }, { -<<<<<<< HEAD "Name": "waitgroup", "Doc": "check for misuses of sync.WaitGroup\n\nThis analyzer detects mistaken calls to the (*sync.WaitGroup).Add\nmethod from inside a new goroutine, causing Add to race with Wait:\n\n\t// WRONG\n\tvar wg sync.WaitGroup\n\tgo func() {\n\t wg.Add(1) // \"WaitGroup.Add called from inside new goroutine\"\n\t defer wg.Done()\n\t ...\n\t}()\n\twg.Wait() // (may return prematurely before new goroutine starts)\n\nThe correct code calls Add before starting the goroutine:\n\n\t// RIGHT\n\tvar wg sync.WaitGroup\n\twg.Add(1)\n\tgo func() {\n\t\tdefer wg.Done()\n\t\t...\n\t}()\n\twg.Wait()", "URL": "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/waitgroup", "Default": true }, { -======= ->>>>>>> 9b6e4f21d (gopls/internal/analysis/yield: analyzer for iterator (yield) mistakes) "Name": "yield", "Doc": "report calls to yield where the result is ignored\n\nAfter a yield function returns false, the caller should not call\nthe yield function again; generally the iterator should return\npromptly.\n\nThis example fails to check the result of the call to yield,\ncausing this analyzer to report a diagnostic:\n\n\tyield(1) // yield may be called again (on L2) after returning false\n\tyield(2)\n\nThe corrected code is either this:\n\n\tif yield(1) { yield(2) }\n\nor simply:\n\n\t_ = yield(1) \u0026\u0026 yield(2)\n\nIt is not always a mistake to ignore the result of yield.\nFor example, this is a valid single-element iterator:\n\n\tyield(1) // ok to ignore result\n\treturn\n\nIt is only a mistake when the yield call that returned false may be\nfollowed by another call.", "URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/yield", diff --git a/gopls/internal/golang/assembly.go b/gopls/internal/golang/assembly.go index e22bd503d7f..7f0ace4daf6 100644 --- a/gopls/internal/golang/assembly.go +++ b/gopls/internal/golang/assembly.go @@ -1,4 +1,4 @@ -ew// Copyright 2024 The Go Authors. All rights reserved. +// Copyright 2024 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. diff --git a/gopls/internal/golang/codeaction.go b/gopls/internal/golang/codeaction.go index e1cbb034c65..7196e83effc 100644 --- a/gopls/internal/golang/codeaction.go +++ b/gopls/internal/golang/codeaction.go @@ -408,7 +408,7 @@ func (si *StructFieldInfo) Emit(out *bytes.Buffer, qual types.Qualifier) error { } // Get types from context at the selector expression position - typesFromContext := stubmethods.TypesFromContext(si.Info, si.Path, si.Expr.Pos()) + typesFromContext := typesutil.TypesFromContext(si.Info, si.Path, si.Expr.Pos()) // Default to interface{} if we couldn't determine the type from context var fieldType types.Type diff --git a/gopls/internal/golang/stubmethods/stubcalledfunc.go b/gopls/internal/golang/stubmethods/stubcalledfunc.go index 869b3939202..1b1b6aba7de 100644 --- a/gopls/internal/golang/stubmethods/stubcalledfunc.go +++ b/gopls/internal/golang/stubmethods/stubcalledfunc.go @@ -10,7 +10,6 @@ import ( "go/ast" "go/token" "go/types" - "strconv" "strings" "unicode" @@ -194,234 +193,6 @@ func (si *CallStubInfo) collectParams() []param { return params } -// typesFromContext returns the type (or perhaps zero or multiple types) -// of the "hole" into which the expression identified by path must fit. -// -// For example, given -// -// s, i := "", 0 -// s, i = EXPR -// -// the hole that must be filled by EXPR has type (string, int). -// -// It returns nil on failure. -func TypesFromContext(info *types.Info, path []ast.Node, pos token.Pos) []types.Type { - var typs []types.Type - parent := parentNode(path) - if parent == nil { - return nil - } - switch parent := parent.(type) { - case *ast.AssignStmt: - // Append all lhs's type - if len(parent.Rhs) == 1 { - for _, lhs := range parent.Lhs { - t := info.TypeOf(lhs) - if t != nil && !containsInvalid(t) { - t = types.Default(t) - } else { - t = anyType - } - typs = append(typs, t) - } - break - } - - // Lhs and Rhs counts do not match, give up - if len(parent.Lhs) != len(parent.Rhs) { - break - } - - // Append corresponding index of lhs's type - for i, rhs := range parent.Rhs { - if rhs.Pos() <= pos && pos <= rhs.End() { - t := info.TypeOf(parent.Lhs[i]) - if t != nil && !containsInvalid(t) { - t = types.Default(t) - } else { - t = anyType - } - typs = append(typs, t) - break - } - } - case *ast.CallExpr: - // Find argument containing pos. - argIdx := -1 - for i, callArg := range parent.Args { - if callArg.Pos() <= pos && pos <= callArg.End() { - argIdx = i - break - } - } - if argIdx == -1 { - break - } - - t := info.TypeOf(parent.Fun) - if t == nil { - break - } - - if sig, ok := t.Underlying().(*types.Signature); ok { - var paramType types.Type - if sig.Variadic() && argIdx >= sig.Params().Len()-1 { - v := sig.Params().At(sig.Params().Len() - 1) - if s, _ := v.Type().(*types.Slice); s != nil { - paramType = s.Elem() - } - } else if argIdx < sig.Params().Len() { - paramType = sig.Params().At(argIdx).Type() - } else { - break - } - if paramType == nil || containsInvalid(paramType) { - paramType = anyType - } - typs = append(typs, paramType) - } - case *ast.SelectorExpr: - for _, n := range path { - assignExpr, ok := n.(*ast.AssignStmt) - if ok { - for _, rh := range assignExpr.Rhs { - // basic types - basicLit, ok := rh.(*ast.BasicLit) - if ok { - switch basicLit.Kind { - case token.INT: - typs = append(typs, types.Typ[types.Int]) - case token.FLOAT: - typs = append(typs, types.Typ[types.Float64]) - case token.IMAG: - typs = append(typs, types.Typ[types.Complex128]) - case token.STRING: - typs = append(typs, types.Typ[types.String]) - case token.CHAR: - typs = append(typs, types.Typ[types.Rune]) - } - break - } - callExpr, ok := rh.(*ast.CallExpr) - if ok { - if ident, ok := callExpr.Fun.(*ast.Ident); ok && ident.Name == "make" && len(callExpr.Args) > 0 { - arg := callExpr.Args[0] - composite, ok := arg.(*ast.CompositeLit) - if ok { - t := typeFromCompositeLit(info, path, composite) - typs = append(typs, t) - break - } - if t := info.TypeOf(arg); t != nil { - if !containsInvalid(t) { - t = types.Default(t) - } else { - t = anyType - } - typs = append(typs, t) - } - } - if ident, ok := callExpr.Fun.(*ast.Ident); ok && ident.Name == "new" && len(callExpr.Args) > 0 { - arg := callExpr.Args[0] - composite, ok := arg.(*ast.CompositeLit) - if ok { - t := typeFromCompositeLit(info, path, composite) - t = types.NewPointer(t) - typs = append(typs, t) - break - } - if t := info.TypeOf(arg); t != nil { - if !containsInvalid(t) { - t = types.Default(t) - t = types.NewPointer(t) - } else { - t = anyType - } - typs = append(typs, t) - } - } - break - } - // a variable - ident, ok := rh.(*ast.Ident) - if ok { - if t := typeFromIdent(info, path, ident); t != nil { - typs = append(typs, t) - } - break - } - - selectorExpr, ok := rh.(*ast.SelectorExpr) - if ok { - if t := typeFromIdent(info, path, selectorExpr.Sel); t != nil { - typs = append(typs, t) - } - break - } - // composite - composite, ok := rh.(*ast.CompositeLit) - if ok { - t := typeFromCompositeLit(info, path, composite) - typs = append(typs, t) - break - } - // a pointer - un, ok := rh.(*ast.UnaryExpr) - if ok && un.Op == token.AND { - composite, ok := un.X.(*ast.CompositeLit) - if !ok { - break - } - if t := info.TypeOf(composite); t != nil { - if !containsInvalid(t) { - t = types.Default(t) - t = types.NewPointer(t) - } else { - t = anyType - } - typs = append(typs, t) - } - } - starExpr, ok := rh.(*ast.StarExpr) - if ok { - ident, ok := starExpr.X.(*ast.Ident) - if ok { - if t := typeFromIdent(info, path, ident); t != nil { - if pointer, ok := t.(*types.Pointer); ok { - t = pointer.Elem() - } - typs = append(typs, t) - } - break - } - } - } - } - } - default: - // TODO: support other common kinds of "holes", e.g. - // x + EXPR => typeof(x) - // !EXPR => bool - // var x int = EXPR => int - // etc. - } - return typs -} - -// parentNode returns the nodes immediately enclosing path[0], -// ignoring parens. -func parentNode(path []ast.Node) ast.Node { - if len(path) <= 1 { - return nil - } - for _, n := range path[1:] { - if _, ok := n.(*ast.ParenExpr); !ok { - return n - } - } - return nil -} - // containsInvalid checks if the type name contains "invalid type", // which is not a valid syntax to generate. func containsInvalid(t types.Type) bool { @@ -486,103 +257,3 @@ func lastSection(identName string) string { return identName } } - -func typeFromCompositeLit(info *types.Info, path []ast.Node, composite *ast.CompositeLit) types.Type { - if t := info.TypeOf(composite); t != nil { - if !containsInvalid(t) { - t = types.Default(t) - if named, ok := t.(*types.Named); ok { - if pkg := named.Obj().Pkg(); pkg != nil { - // Find the file in the path that contains this assignment - var file *ast.File - for _, n := range path { - if f, ok := n.(*ast.File); ok { - file = f - break - } - } - - if file != nil { - // Look for any import spec that imports this package - var pkgName string - for _, imp := range file.Imports { - if path, _ := strconv.Unquote(imp.Path.Value); path == pkg.Path() { - // Use the alias if specified, otherwise use package name - if imp.Name != nil { - pkgName = imp.Name.Name - } else { - pkgName = pkg.Name() - } - break - } - } - if pkgName == "" { - pkgName = pkg.Name() // fallback to package name if no import found - } - - // Create new package with the correct name (either alias or original) - newPkg := types.NewPackage(pkgName, pkgName) - newName := types.NewTypeName(named.Obj().Pos(), newPkg, named.Obj().Name(), nil) - t = types.NewNamed(newName, named.Underlying(), nil) - } - } - return t - } - } else { - t = anyType - } - return t - } - return nil -} - -func typeFromIdent(info *types.Info, path []ast.Node, ident *ast.Ident) types.Type { - if t := info.TypeOf(ident); t != nil { - if !containsInvalid(t) { - t = types.Default(t) - if named, ok := t.(*types.Named); ok { - if pkg := named.Obj().Pkg(); pkg != nil { - // find the file in the path that contains this assignment - var file *ast.File - for _, n := range path { - if f, ok := n.(*ast.File); ok { - file = f - break - } - } - - if file != nil { - // look for any import spec that imports this package - var pkgName string - for _, imp := range file.Imports { - if path, _ := strconv.Unquote(imp.Path.Value); path == pkg.Path() { - // use the alias if specified, otherwise use package name - if imp.Name != nil { - pkgName = imp.Name.Name - } else { - pkgName = pkg.Name() - } - break - } - } - // fallback to package name if no import found - if pkgName == "" { - pkgName = pkg.Name() - } - - // create new package with the correct name (either alias or original) - newPkg := types.NewPackage(pkgName, pkgName) - newName := types.NewTypeName(named.Obj().Pos(), newPkg, named.Obj().Name(), nil) - t = types.NewNamed(newName, named.Underlying(), nil) - } - } - return t - } - } else { - t = anyType - } - return t - } - - return nil -} diff --git a/gopls/internal/golang/type_definition.go b/gopls/internal/golang/type_definition.go index cf452c5ca64..a396793e48a 100644 --- a/gopls/internal/golang/type_definition.go +++ b/gopls/internal/golang/type_definition.go @@ -51,4 +51,3 @@ func TypeDefinition(ctx context.Context, snapshot *cache.Snapshot, fh file.Handl } return []protocol.Location{loc}, nil } - diff --git a/gopls/internal/util/typesutil/typesutil.go b/gopls/internal/util/typesutil/typesutil.go index 98f5605200e..059778058d6 100644 --- a/gopls/internal/util/typesutil/typesutil.go +++ b/gopls/internal/util/typesutil/typesutil.go @@ -9,6 +9,7 @@ import ( "go/ast" "go/token" "go/types" + "strconv" "strings" ) @@ -209,6 +210,120 @@ func TypesFromContext(info *types.Info, path []ast.Node, pos token.Pos) []types. t := info.TypeOf(parent.X) typs = append(typs, validType(t)) } + case *ast.SelectorExpr: + for _, n := range path { + assignExpr, ok := n.(*ast.AssignStmt) + if ok { + for _, rh := range assignExpr.Rhs { + // basic types + basicLit, ok := rh.(*ast.BasicLit) + if ok { + switch basicLit.Kind { + case token.INT: + typs = append(typs, types.Typ[types.Int]) + case token.FLOAT: + typs = append(typs, types.Typ[types.Float64]) + case token.IMAG: + typs = append(typs, types.Typ[types.Complex128]) + case token.STRING: + typs = append(typs, types.Typ[types.String]) + case token.CHAR: + typs = append(typs, types.Typ[types.Rune]) + } + break + } + callExpr, ok := rh.(*ast.CallExpr) + if ok { + if ident, ok := callExpr.Fun.(*ast.Ident); ok && ident.Name == "make" && len(callExpr.Args) > 0 { + arg := callExpr.Args[0] + composite, ok := arg.(*ast.CompositeLit) + if ok { + t := typeFromExpr(info, path, composite) + typs = append(typs, t) + break + } + if t := info.TypeOf(arg); t != nil { + typs = append(typs, validType(t)) + } + } + if ident, ok := callExpr.Fun.(*ast.Ident); ok && ident.Name == "new" && len(callExpr.Args) > 0 { + arg := callExpr.Args[0] + composite, ok := arg.(*ast.CompositeLit) + if ok { + t := typeFromExpr(info, path, composite) + t = types.NewPointer(t) + typs = append(typs, t) + break + } + if t := info.TypeOf(arg); t != nil { + if !containsInvalid(t) { + t = types.Default(t) + t = types.NewPointer(t) + } else { + t = anyType + } + typs = append(typs, t) + } + } + break + } + // a variable + ident, ok := rh.(*ast.Ident) + if ok { + if t := typeFromExpr(info, path, ident); t != nil { + typs = append(typs, t) + } + break + } + + selectorExpr, ok := rh.(*ast.SelectorExpr) + if ok { + if t := typeFromExpr(info, path, selectorExpr.Sel); t != nil { + typs = append(typs, t) + } + break + } + // composite + composite, ok := rh.(*ast.CompositeLit) + if ok { + t := typeFromExpr(info, path, composite) + typs = append(typs, t) + break + } + // a pointer + un, ok := rh.(*ast.UnaryExpr) + if ok && un.Op == token.AND { + composite, ok := un.X.(*ast.CompositeLit) + if !ok { + break + } + if t := info.TypeOf(composite); t != nil { + if !containsInvalid(t) { + t = types.Default(t) + t = types.NewPointer(t) + } else { + t = anyType + } + typs = append(typs, t) + } + } + starExpr, ok := rh.(*ast.StarExpr) + if ok { + ident, ok := starExpr.X.(*ast.Ident) + if ok { + if t := typeFromExpr(info, path, ident); t != nil { + if pointer, ok := t.(*types.Pointer); ok { + t = pointer.Elem() + } + typs = append(typs, t) + } + break + } + } + } + } + } + default: // TODO: support other kinds of "holes" as the need arises. } @@ -257,3 +372,55 @@ func EnclosingSignature(path []ast.Node, info *types.Info) *types.Signature { } return nil } + +func typeFromExpr(info *types.Info, path []ast.Node, expr ast.Expr) types.Type { + t := info.TypeOf(expr) + if t == nil { + return nil + } + + if !containsInvalid(t) { + t = types.Default(t) + if named, ok := t.(*types.Named); ok { + if pkg := named.Obj().Pkg(); pkg != nil { + // find the file in the path that contains this assignment + var file *ast.File + for _, n := range path { + if f, ok := n.(*ast.File); ok { + file = f + break + } + } + + if file != nil { + // look for any import spec that imports this package + var pkgName string + for _, imp := range file.Imports { + if path, _ := strconv.Unquote(imp.Path.Value); path == pkg.Path() { + // use the alias if specified, otherwise use package name + if imp.Name != nil { + pkgName = imp.Name.Name + } else { + pkgName = pkg.Name() + } + break + } + } + // fallback to package name if no import found + if pkgName == "" { + pkgName = pkg.Name() + } + + // create new package with the correct name (either alias or original) + newPkg := types.NewPackage(pkgName, pkgName) + newName := types.NewTypeName(named.Obj().Pos(), newPkg, named.Obj().Name(), nil) + t = types.NewNamed(newName, named.Underlying(), nil) + } + } + return t + } + } else { + t = types.Universe.Lookup("any").Type() + } + return t +}