internal/analysisinternal: unify zero value function to typesinternal

Refactors the ZeroExpr and ZeroString functions to provide more consistent and correct handling of zero values for input types. - Refactor: Unify similar switch case statements in both functions with exception of types.Tuple. ZeroExpr panic due to the lack of a valid ast.Expr representation. - Fixing an issue where ZeroExpr returned nil for types.Array instead of a composite literal. - Adding support for type parameters in ZeroExpr, similar to ZeroString. - Consolidating tests for both functions into TestZeroValue. Change-Id: Ic77ae17ea091cf51bd4d4642186fe13093e0d461 Reviewed-on: https://go-review.googlesource.com/c/tools/+/627604 Reviewed-by: Tim King <taking@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com> Auto-Submit: Hongxiang Jiang <hxjiang@golang.org> Reviewed-by: Robert Findley <rfindley@google.com>
golang · Nov 20, 2024 · e751756 · e751756
1 parent a287481
commit e751756
Show file tree

Hide file tree

Showing 8 changed files with 402 additions and 211 deletions.
diff --git a/gopls/internal/analysis/fillreturns/fillreturns.go b/gopls/internal/analysis/fillreturns/fillreturns.go
@@ -18,6 +18,7 @@ import (
 	"golang.org/x/tools/go/ast/astutil"
 	"golang.org/x/tools/gopls/internal/fuzzy"
 	"golang.org/x/tools/internal/analysisinternal"
+	"golang.org/x/tools/internal/typesinternal"
 )
 
 //go:embed doc.go
@@ -161,7 +162,7 @@ outer:
 				if t := info.TypeOf(val); t == nil || !matchingTypes(t, retTyp) {
 					continue
 				}
-				if !analysisinternal.IsZeroValue(val) {
+				if !typesinternal.IsZeroExpr(val) {
 					match, idx = val, j
 					break
 				}
@@ -183,7 +184,7 @@ outer:
 				// If no identifier matches the pattern, generate a zero value.
 				if best := fuzzy.BestMatch(retTyp.String(), names); best != "" {
 					fixed[i] = ast.NewIdent(best)
-				} else if zero := analysisinternal.ZeroValue(file, pass.Pkg, retTyp); zero != nil {
+				} else if zero := typesinternal.ZeroExpr(file, pass.Pkg, retTyp); zero != nil {
 					fixed[i] = zero
 				} else {
 					return nil, nil
@@ -194,7 +195,7 @@ outer:
 		// Remove any non-matching "zero values" from the leftover values.
 		var nonZeroRemaining []ast.Expr
 		for _, expr := range remaining {
-			if !analysisinternal.IsZeroValue(expr) {
+			if !typesinternal.IsZeroExpr(expr) {
 				nonZeroRemaining = append(nonZeroRemaining, expr)
 			}
 		}

diff --git a/gopls/internal/analysis/fillreturns/testdata/src/a/a.go.golden b/gopls/internal/analysis/fillreturns/testdata/src/a/a.go.golden
@@ -67,7 +67,7 @@ func basic() (uint8, uint16, uint32, uint64, int8, int16, int32, int64, float32,
 }
 
 func complex() (*int, []int, [2]int, map[int]int) {
-	return nil, nil, nil, nil // want "return values"
+	return nil, nil, [2]int{}, nil // want "return values"
 }
 
 func structsAndInterfaces() (T, url.URL, T1, I, I1, io.Reader, Client, ast2.Stmt) {

diff --git a/gopls/internal/analysis/fillstruct/fillstruct.go b/gopls/internal/analysis/fillstruct/fillstruct.go
@@ -349,8 +349,8 @@ func populateValue(f *ast.File, pkg *types.Package, typ types.Type) ast.Expr {
 		}
 
 	case *types.Map:
-		k := analysisinternal.TypeExpr(f, pkg, u.Key())
-		v := analysisinternal.TypeExpr(f, pkg, u.Elem())
+		k := typesinternal.TypeExpr(f, pkg, u.Key())
+		v := typesinternal.TypeExpr(f, pkg, u.Elem())
 		if k == nil || v == nil {
 			return nil
 		}
@@ -361,7 +361,7 @@ func populateValue(f *ast.File, pkg *types.Package, typ types.Type) ast.Expr {
 			},
 		}
 	case *types.Slice:
-		s := analysisinternal.TypeExpr(f, pkg, u.Elem())
+		s := typesinternal.TypeExpr(f, pkg, u.Elem())
 		if s == nil {
 			return nil
 		}
@@ -372,7 +372,7 @@ func populateValue(f *ast.File, pkg *types.Package, typ types.Type) ast.Expr {
 		}
 
 	case *types.Array:
-		a := analysisinternal.TypeExpr(f, pkg, u.Elem())
+		a := typesinternal.TypeExpr(f, pkg, u.Elem())
 		if a == nil {
 			return nil
 		}
@@ -386,7 +386,7 @@ func populateValue(f *ast.File, pkg *types.Package, typ types.Type) ast.Expr {
 		}
 
 	case *types.Chan:
-		v := analysisinternal.TypeExpr(f, pkg, u.Elem())
+		v := typesinternal.TypeExpr(f, pkg, u.Elem())
 		if v == nil {
 			return nil
 		}
@@ -405,7 +405,7 @@ func populateValue(f *ast.File, pkg *types.Package, typ types.Type) ast.Expr {
 		}
 
 	case *types.Struct:
-		s := analysisinternal.TypeExpr(f, pkg, typ)
+		s := typesinternal.TypeExpr(f, pkg, typ)
 		if s == nil {
 			return nil
 		}
@@ -416,7 +416,7 @@ func populateValue(f *ast.File, pkg *types.Package, typ types.Type) ast.Expr {
 	case *types.Signature:
 		var params []*ast.Field
 		for i := 0; i < u.Params().Len(); i++ {
-			p := analysisinternal.TypeExpr(f, pkg, u.Params().At(i).Type())
+			p := typesinternal.TypeExpr(f, pkg, u.Params().At(i).Type())
 			if p == nil {
 				return nil
 			}
@@ -431,7 +431,7 @@ func populateValue(f *ast.File, pkg *types.Package, typ types.Type) ast.Expr {
 		}
 		var returns []*ast.Field
 		for i := 0; i < u.Results().Len(); i++ {
-			r := analysisinternal.TypeExpr(f, pkg, u.Results().At(i).Type())
+			r := typesinternal.TypeExpr(f, pkg, u.Results().At(i).Type())
 			if r == nil {
 				return nil
 			}

diff --git a/gopls/internal/golang/extract.go b/gopls/internal/golang/extract.go
@@ -22,6 +22,7 @@ import (
 	"golang.org/x/tools/gopls/internal/util/bug"
 	"golang.org/x/tools/gopls/internal/util/safetoken"
 	"golang.org/x/tools/internal/analysisinternal"
+	"golang.org/x/tools/internal/typesinternal"
 )
 
 func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
@@ -360,7 +361,7 @@ func extractFunctionMethod(fset *token.FileSet, start, end token.Pos, src []byte
 			// The blank identifier is always a local variable
 			continue
 		}
-		typ := analysisinternal.TypeExpr(file, pkg, v.obj.Type())
+		typ := typesinternal.TypeExpr(file, pkg, v.obj.Type())
 		if typ == nil {
 			return nil, nil, fmt.Errorf("nil AST expression for type: %v", v.obj.Name())
 		}
@@ -1233,7 +1234,7 @@ func generateReturnInfo(enclosing *ast.FuncType, pkg *types.Package, path []ast.
 				return nil, nil, fmt.Errorf(
 					"failed type conversion, AST expression: %T", field.Type)
 			}
-			expr := analysisinternal.TypeExpr(file, pkg, typ)
+			expr := typesinternal.TypeExpr(file, pkg, typ)
 			if expr == nil {
 				return nil, nil, fmt.Errorf("nil AST expression")
 			}
@@ -1253,7 +1254,7 @@ func generateReturnInfo(enclosing *ast.FuncType, pkg *types.Package, path []ast.
 				}
 				retName, idx := generateNameOutsideOfRange(start, end, path, pkg, info, bestName, nameIdx[bestName])
 				nameIdx[bestName] = idx
-				z := analysisinternal.ZeroValue(file, pkg, typ)
+				z := typesinternal.ZeroExpr(file, pkg, typ)
 				if z == nil {
 					return nil, nil, fmt.Errorf("can't generate zero value for %T", typ)
 				}
@@ -1332,7 +1333,7 @@ func adjustReturnStatements(returnTypes []*ast.Field, seenVars map[types.Object]
 			if typ != returnType.Type {
 				continue
 			}
-			val = analysisinternal.ZeroValue(file, pkg, obj.Type())
+			val = typesinternal.ZeroExpr(file, pkg, obj.Type())
 			break
 		}
 		if val == nil {

diff --git a/gopls/internal/golang/undeclared.go b/gopls/internal/golang/undeclared.go
@@ -19,6 +19,7 @@ import (
 	"golang.org/x/tools/gopls/internal/util/safetoken"
 	"golang.org/x/tools/gopls/internal/util/typesutil"
 	"golang.org/x/tools/internal/analysisinternal"
+	"golang.org/x/tools/internal/typesinternal"
 )
 
 // The prefix for this error message changed in Go 1.20.
@@ -221,15 +222,15 @@ func newFunctionDeclaration(path []ast.Node, file *ast.File, pkg *types.Package,
 			Names: []*ast.Ident{
 				ast.NewIdent(name),
 			},
-			Type: analysisinternal.TypeExpr(file, pkg, paramTypes[i]),
+			Type: typesinternal.TypeExpr(file, pkg, paramTypes[i]),
 		})
 	}
 
 	rets := &ast.FieldList{}
 	retTypes := typesutil.TypesFromContext(info, path[1:], path[1].Pos())
 	for _, rt := range retTypes {
 		rets.List = append(rets.List, &ast.Field{
-			Type: analysisinternal.TypeExpr(file, pkg, rt),
+			Type: typesinternal.TypeExpr(file, pkg, rt),
 		})
 	}
 

diff --git a/internal/analysisinternal/analysis.go b/internal/analysisinternal/analysis.go
@@ -15,7 +15,6 @@ import (
 	"go/types"
 	"os"
 	pathpkg "path"
-	"strconv"
 
 	"golang.org/x/tools/go/analysis"
 )
@@ -66,192 +65,6 @@ func TypeErrorEndPos(fset *token.FileSet, src []byte, start token.Pos) token.Pos
 	return end
 }
 
-// ZeroValue returns the ast.Expr representation of the "zero" value of the type t.
-// See [typesinternal.ZeroString] for a variant that returns a string.
-func ZeroValue(f *ast.File, pkg *types.Package, typ types.Type) ast.Expr {
-	// TODO(adonovan): think about generics, and also generic aliases.
-	under := types.Unalias(typ)
-	// Don't call Underlying unconditionally: although it removes
-	// Named and Alias, it also removes TypeParam.
-	if n, ok := under.(*types.Named); ok {
-		under = n.Underlying()
-	}
-	switch under := under.(type) {
-	case *types.Basic:
-		switch {
-		case under.Info()&types.IsNumeric != 0:
-			return &ast.BasicLit{Kind: token.INT, Value: "0"}
-		case under.Info()&types.IsBoolean != 0:
-			return ast.NewIdent("false")
-		case under.Info()&types.IsString != 0:
-			return &ast.BasicLit{Kind: token.STRING, Value: `""`}
-		case under == types.Typ[types.Invalid]:
-			return nil
-		default:
-			panic(fmt.Sprintf("unknown basic type %v", under))
-		}
-	case *types.Chan, *types.Interface, *types.Map, *types.Pointer, *types.Signature, *types.Slice, *types.Array:
-		return ast.NewIdent("nil")
-	case *types.Struct:
-		texpr := TypeExpr(f, pkg, typ) // typ because we want the name here.
-		if texpr == nil {
-			return nil
-		}
-		return &ast.CompositeLit{
-			Type: texpr,
-		}
-	}
-	return nil
-}
-
-// IsZeroValue checks whether the given expression is a 'zero value' (as determined by output of
-// analysisinternal.ZeroValue)
-func IsZeroValue(expr ast.Expr) bool {
-	switch e := expr.(type) {
-	case *ast.BasicLit:
-		return e.Value == "0" || e.Value == `""`
-	case *ast.Ident:
-		return e.Name == "nil" || e.Name == "false"
-	default:
-		return false
-	}
-}
-
-// TypeExpr returns syntax for the specified type. References to
-// named types from packages other than pkg are qualified by an appropriate
-// package name, as defined by the import environment of file.
-func TypeExpr(f *ast.File, pkg *types.Package, typ types.Type) ast.Expr {
-	switch t := typ.(type) {
-	case *types.Basic:
-		switch t.Kind() {
-		case types.UnsafePointer:
-			return &ast.SelectorExpr{X: ast.NewIdent("unsafe"), Sel: ast.NewIdent("Pointer")}
-		default:
-			return ast.NewIdent(t.Name())
-		}
-	case *types.Pointer:
-		x := TypeExpr(f, pkg, t.Elem())
-		if x == nil {
-			return nil
-		}
-		return &ast.UnaryExpr{
-			Op: token.MUL,
-			X:  x,
-		}
-	case *types.Array:
-		elt := TypeExpr(f, pkg, t.Elem())
-		if elt == nil {
-			return nil
-		}
-		return &ast.ArrayType{
-			Len: &ast.BasicLit{
-				Kind:  token.INT,
-				Value: fmt.Sprintf("%d", t.Len()),
-			},
-			Elt: elt,
-		}
-	case *types.Slice:
-		elt := TypeExpr(f, pkg, t.Elem())
-		if elt == nil {
-			return nil
-		}
-		return &ast.ArrayType{
-			Elt: elt,
-		}
-	case *types.Map:
-		key := TypeExpr(f, pkg, t.Key())
-		value := TypeExpr(f, pkg, t.Elem())
-		if key == nil || value == nil {
-			return nil
-		}
-		return &ast.MapType{
-			Key:   key,
-			Value: value,
-		}
-	case *types.Chan:
-		dir := ast.ChanDir(t.Dir())
-		if t.Dir() == types.SendRecv {
-			dir = ast.SEND | ast.RECV
-		}
-		value := TypeExpr(f, pkg, t.Elem())
-		if value == nil {
-			return nil
-		}
-		return &ast.ChanType{
-			Dir:   dir,
-			Value: value,
-		}
-	case *types.Signature:
-		var params []*ast.Field
-		for i := 0; i < t.Params().Len(); i++ {
-			p := TypeExpr(f, pkg, t.Params().At(i).Type())
-			if p == nil {
-				return nil
-			}
-			params = append(params, &ast.Field{
-				Type: p,
-				Names: []*ast.Ident{
-					{
-						Name: t.Params().At(i).Name(),
-					},
-				},
-			})
-		}
-		if t.Variadic() {
-			last := params[len(params)-1]
-			last.Type = &ast.Ellipsis{Elt: last.Type.(*ast.ArrayType).Elt}
-		}
-		var returns []*ast.Field
-		for i := 0; i < t.Results().Len(); i++ {
-			r := TypeExpr(f, pkg, t.Results().At(i).Type())
-			if r == nil {
-				return nil
-			}
-			returns = append(returns, &ast.Field{
-				Type: r,
-			})
-		}
-		return &ast.FuncType{
-			Params: &ast.FieldList{
-				List: params,
-			},
-			Results: &ast.FieldList{
-				List: returns,
-			},
-		}
-	case interface{ Obj() *types.TypeName }: // *types.{Alias,Named,TypeParam}
-		if t.Obj().Pkg() == nil {
-			return ast.NewIdent(t.Obj().Name())
-		}
-		if t.Obj().Pkg() == pkg {
-			return ast.NewIdent(t.Obj().Name())
-		}
-		pkgName := t.Obj().Pkg().Name()
-
-		// If the file already imports the package under another name, use that.
-		for _, cand := range f.Imports {
-			if path, _ := strconv.Unquote(cand.Path.Value); path == t.Obj().Pkg().Path() {
-				if cand.Name != nil && cand.Name.Name != "" {
-					pkgName = cand.Name.Name
-				}
-			}
-		}
-		if pkgName == "." {
-			return ast.NewIdent(t.Obj().Name())
-		}
-		return &ast.SelectorExpr{
-			X:   ast.NewIdent(pkgName),
-			Sel: ast.NewIdent(t.Obj().Name()),
-		}
-	case *types.Struct:
-		return ast.NewIdent(t.String())
-	case *types.Interface:
-		return ast.NewIdent(t.String())
-	default:
-		return nil
-	}
-}
-
 // StmtToInsertVarBefore returns the ast.Stmt before which we can safely insert a new variable.
 // Some examples:
 //