From a2b3d351b44e2fe74929cfc866dd47e6a380d490 Mon Sep 17 00:00:00 2001 From: "Iskander (Alex) Sharipov" Date: Wed, 20 Feb 2019 00:36:13 +0300 Subject: [PATCH] checkers: add newDeref checker (#806) Finds *new(T) expressions and suggests to simplify them by using T zero value instead. For example: *new(string) => "" *new(bool) => false *new(myInt) => myInt(0) Signed-off-by: Iskander Sharipov --- checkers/internal/lintutil/zero_value.go | 93 +++++++++++++++++++ checkers/newDeref_checker.go | 45 +++++++++ .../_importable/examplepkg/examplepkg.go | 10 ++ .../check_main_only/linttest.golden | 1 + .../_integration/check_main_only/main.go | 4 + checkers/testdata/newDeref/negative_tests.go | 31 +++++++ checkers/testdata/newDeref/positive_tests.go | 91 ++++++++++++++++++ 7 files changed, 275 insertions(+) create mode 100644 checkers/internal/lintutil/zero_value.go create mode 100644 checkers/newDeref_checker.go create mode 100644 checkers/testdata/_importable/examplepkg/examplepkg.go create mode 100644 checkers/testdata/newDeref/negative_tests.go create mode 100644 checkers/testdata/newDeref/positive_tests.go diff --git a/checkers/internal/lintutil/zero_value.go b/checkers/internal/lintutil/zero_value.go new file mode 100644 index 000000000..d0c1badf2 --- /dev/null +++ b/checkers/internal/lintutil/zero_value.go @@ -0,0 +1,93 @@ +package lintutil + +import ( + "go/ast" + "go/constant" + "go/token" + "go/types" +) + +// IsZeroValue reports whether x represents zero value of its type. +// +// The functions is conservative and may return false for zero values +// if some cases are not handled in a comprehensive way +// but is should never return true for something that's not a proper zv. +func IsZeroValue(info *types.Info, x ast.Expr) bool { + switch x := x.(type) { + case *ast.BasicLit: + typ := info.TypeOf(x).Underlying().(*types.Basic) + v := info.Types[x].Value + var z constant.Value + switch { + case typ.Kind() == types.String: + z = constant.MakeString("") + case typ.Info()&types.IsInteger != 0: + z = constant.MakeInt64(0) + case typ.Info()&types.IsUnsigned != 0: + z = constant.MakeUint64(0) + case typ.Info()&types.IsFloat != 0: + z = constant.MakeFloat64(0) + default: + return false + } + return constant.Compare(v, token.EQL, z) + + case *ast.CompositeLit: + return len(x.Elts) == 0 + + default: + // Note that this function is not comprehensive. + return false + } +} + +// ZeroValueOf returns a zero value expression for typeExpr of type typ. +// If function can't find such a value, nil is returned. +func ZeroValueOf(typeExpr ast.Expr, typ types.Type) ast.Expr { + switch utyp := typ.Underlying().(type) { + case *types.Basic: + info := utyp.Info() + var zv ast.Expr + switch { + case info&types.IsInteger != 0: + zv = &ast.BasicLit{Kind: token.INT, Value: "0"} + case info&types.IsFloat != 0: + zv = &ast.BasicLit{Kind: token.FLOAT, Value: "0.0"} + case info&types.IsString != 0: + zv = &ast.BasicLit{Kind: token.STRING, Value: `""`} + case info&types.IsBoolean != 0: + zv = &ast.Ident{Name: "false"} + } + if isDefaultLiteralType(typ) { + return zv + } + return &ast.CallExpr{ + Fun: typeExpr, + Args: []ast.Expr{zv}, + } + + case *types.Slice, *types.Map, *types.Pointer, *types.Interface: + return &ast.CallExpr{ + Fun: typeExpr, + Args: []ast.Expr{&ast.Ident{Name: "nil"}}, + } + + case *types.Array, *types.Struct: + return &ast.CompositeLit{Type: typeExpr} + } + + return nil +} + +func isDefaultLiteralType(typ types.Type) bool { + btyp, ok := typ.(*types.Basic) + if !ok { + return false + } + switch btyp.Kind() { + case types.Bool, types.Int, types.Float64, types.String: + return true + default: + return false + } +} diff --git a/checkers/newDeref_checker.go b/checkers/newDeref_checker.go new file mode 100644 index 000000000..75e7f6428 --- /dev/null +++ b/checkers/newDeref_checker.go @@ -0,0 +1,45 @@ +package checkers + +import ( + "go/ast" + + "github.com/go-critic/go-critic/checkers/internal/lintutil" + "github.com/go-lintpack/lintpack" + "github.com/go-lintpack/lintpack/astwalk" + "github.com/go-toolsmith/astcast" + "golang.org/x/tools/go/ast/astutil" +) + +func init() { + var info lintpack.CheckerInfo + info.Name = "newDeref" + info.Tags = []string{"style", "experimental"} + info.Summary = "Detects immediate dereferencing of `new` expressions" + info.Before = `x := *new(bool)` + info.After = `x := false` + + collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker { + return astwalk.WalkerForExpr(&newDerefChecker{ctx: ctx}) + }) +} + +type newDerefChecker struct { + astwalk.WalkHandler + ctx *lintpack.CheckerContext +} + +func (c *newDerefChecker) VisitExpr(expr ast.Expr) { + deref := astcast.ToStarExpr(expr) + call := astcast.ToCallExpr(deref.X) + if astcast.ToIdent(call.Fun).Name == "new" { + typ := c.ctx.TypesInfo.TypeOf(call.Args[0]) + zv := lintutil.ZeroValueOf(astutil.Unparen(call.Args[0]), typ) + if zv != nil { + c.warn(expr, zv) + } + } +} + +func (c *newDerefChecker) warn(cause, suggestion ast.Expr) { + c.ctx.Warn(cause, "replace `%s` with `%s`", cause, suggestion) +} diff --git a/checkers/testdata/_importable/examplepkg/examplepkg.go b/checkers/testdata/_importable/examplepkg/examplepkg.go new file mode 100644 index 000000000..a0ed95125 --- /dev/null +++ b/checkers/testdata/_importable/examplepkg/examplepkg.go @@ -0,0 +1,10 @@ +package examplepkg + +type StructType struct { + A int + B int +} + +type InterfaceType interface { + Method() int +} diff --git a/checkers/testdata/_integration/check_main_only/linttest.golden b/checkers/testdata/_integration/check_main_only/linttest.golden index 1090f94c9..77e19d94f 100644 --- a/checkers/testdata/_integration/check_main_only/linttest.golden +++ b/checkers/testdata/_integration/check_main_only/linttest.golden @@ -29,6 +29,7 @@ exit status 1 ./main.go:123:19: importShadow: shadow of imported package 'flag' ./main.go:126:6: indexAlloc: consider replacing strings.Index(string(s), sub) with bytes.Index(s, []byte(sub)) ./main.go:130:6: methodExprCall: consider to change `point.String` to `p.String` +./main.go:272:6: newDeref: replace `*new(string)` with `""` ./main.go:135:3: nilValReturn: returned expr is always nil; replace x with nil ./main.go:234:9: offBy1: index expr always panics; maybe you wanted xs[len(xs)-1]? ./main.go:140:1: paramTypeCombine: func(x int, y int) could be replaced with func(x, y int) diff --git a/checkers/testdata/_integration/check_main_only/main.go b/checkers/testdata/_integration/check_main_only/main.go index 2427e3f83..4c78e6d0e 100644 --- a/checkers/testdata/_integration/check_main_only/main.go +++ b/checkers/testdata/_integration/check_main_only/main.go @@ -268,5 +268,9 @@ func argOrder(s string) { _ = strings.HasPrefix("$", s) } +func newDeref() { + _ = *new(string) +} + func main() { } diff --git a/checkers/testdata/newDeref/negative_tests.go b/checkers/testdata/newDeref/negative_tests.go new file mode 100644 index 000000000..440332f2f --- /dev/null +++ b/checkers/testdata/newDeref/negative_tests.go @@ -0,0 +1,31 @@ +package checker_test + +import ( + "github.com/go-critic/go-critic/checkers/testdata/_importable/examplepkg" +) + +func fixedNewExpressions() { + _ = false + _ = "" + _ = 0 + _ = 0.0 + _ = int32(0) + _ = float32(0.0) + _ = []int(nil) + _ = myInt(0) + _ = point{} + _ = []*point(nil) + _ = point{} + _ = [4][2]int{} + _ = map[int]int(nil) + _ = []map[int][]int(nil) + _ = (*int)(nil) + _ = examplepkg.StructType{} + + _ = interface{}(nil) + _ = myEface(nil) + _ = nonEmptyIface(nil) + _ = underlyingIface(nil) + _ = interface{}(nil) + _ = examplepkg.InterfaceType(nil) +} diff --git a/checkers/testdata/newDeref/positive_tests.go b/checkers/testdata/newDeref/positive_tests.go new file mode 100644 index 000000000..dc61bcdbf --- /dev/null +++ b/checkers/testdata/newDeref/positive_tests.go @@ -0,0 +1,91 @@ +package checker_test + +import ( + "github.com/go-critic/go-critic/checkers/testdata/_importable/examplepkg" +) + +type point struct { + x float64 + y float64 +} + +type myInt int + +func badNewExpressions() { + /*! replace `*new(bool)` with `false` */ + _ = *new(bool) + + /*! replace `*new(string)` with `""` */ + _ = *new(string) + + /*! replace `*new(int)` with `0` */ + _ = *new(int) + + /*! replace `*new(float64)` with `0.0` */ + _ = *new(float64) + + /*! replace `*new(int32)` with `int32(0)` */ + _ = *new(int32) + + /*! replace `*new(float32)` with `float32(0.0)` */ + _ = *new(float32) + + /*! replace `*new([]int)` with `[]int(nil)` */ + _ = *new([]int) + + /*! replace `*new(myInt)` with `myInt(0)` */ + _ = *new(myInt) + + /*! replace `*new(point)` with `point{}` */ + _ = *new(point) + + /*! replace `*new([]*point)` with `[]*point(nil)` */ + _ = *new([]*point) + + /*! replace `*new((point))` with `point{}` */ + _ = *new((point)) + + /*! replace `*new([4][2]int)` with `[4][2]int{}` */ + _ = *new([4][2]int) + + /*! replace `*new(map[int]int)` with `map[int]int(nil)` */ + _ = *new(map[int]int) + + /*! replace `*new([]map[int][]int)` with `[]map[int][]int(nil)` */ + _ = *new([]map[int][]int) + + /*! replace `*new(*int)` with `(*int)(nil)` */ + _ = *new(*int) + + /*! replace `*new(examplepkg.StructType)` with `examplepkg.StructType{}` */ + _ = *new(examplepkg.StructType) +} + +type myEface interface{} + +type nonEmptyIface interface { + Foo() + Bar() +} + +type underlyingIface nonEmptyIface + +func interfaceDeref() { + /*! replace `*new(interface{})` with `interface{}(nil)` */ + _ = *new(interface{}) + + /*! replace `*new(myEface)` with `myEface(nil)` */ + _ = *new(myEface) + + /*! replace `*new(nonEmptyIface)` with `nonEmptyIface(nil)` */ + _ = *new(nonEmptyIface) + + /*! replace `*new(underlyingIface)` with `underlyingIface(nil)` */ + _ = *new(underlyingIface) + + /*! replace `*new(interface{})` with `interface{}(nil)` */ + _ = *new(interface{}) + + /*! replace `*new(examplepkg.InterfaceType)` with `examplepkg.InterfaceType(nil)` */ + _ = *new(examplepkg.InterfaceType) +}