Merge pull request #2 from go-critic/quasilyte/port_more_checkers

port several checkers from go-critic to lintpack

blankParam_checker.go

@@ -0,0 +1,71 @@
+package checkers
+
+import (
+	"go/ast"
+
+	"github.com/go-lintpack/lintpack"
+	"github.com/go-lintpack/lintpack/astwalk"
+)
+
+func init() {
+	var info lintpack.CheckerInfo
+	info.Name = "blankParam"
+	info.Tags = []string{"style", "opinionated", "experimental"}
+	info.Summary = "Detects unused params and suggests to name them as `_` (blank)"
+	info.Before = `func f(a int, b float64) // b isn't used inside function body`
+	info.After = `func f(a int, _ float64) // everything is cool`
+
+	collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
+		return astwalk.WalkerForFuncDecl(&blankParamChecker{ctx: ctx})
+	})
+}
+
+type blankParamChecker struct {
+	astwalk.WalkHandler
+	ctx *lintpack.CheckerContext
+}
+
+func (c *blankParamChecker) VisitFuncDecl(decl *ast.FuncDecl) {
+	params := decl.Type.Params
+	if decl.Body == nil || params == nil || params.NumFields() == 0 {
+		return
+	}
+
+	// collect all params to map
+	objToIdent := make(map[*ast.Object]*ast.Ident)
+	for _, p := range params.List {
+		if len(p.Names) == 0 {
+			c.warnUnnamed(p)
+			return
+		}
+		for _, id := range p.Names {
+			if id.Name != "_" {
+				objToIdent[id.Obj] = id
+			}
+		}
+	}
+
+	// remove used params
+	// TODO(cristaloleg): we might want to have less iterations here.
+	for id := range c.ctx.TypesInfo.Uses {
+		if _, ok := objToIdent[id.Obj]; ok {
+			delete(objToIdent, id.Obj)
+			if len(objToIdent) == 0 {
+				return
+			}
+		}
+	}
+
+	// all params that are left are unused
+	for _, id := range objToIdent {
+		c.warn(id)
+	}
+}
+
+func (c *blankParamChecker) warn(param *ast.Ident) {
+	c.ctx.Warn(param, "rename `%s` to `_`", param)
+}
+
+func (c *blankParamChecker) warnUnnamed(n ast.Node) {
+	c.ctx.Warn(n, "consider to name parameters as `_`")
+}

boolFuncPrefix_checker.go

@@ -0,0 +1,68 @@
+package checkers
+
+import (
+	"go/ast"
+	"go/types"
+	"strings"
+
+	"github.com/go-lintpack/lintpack"
+	"github.com/go-lintpack/lintpack/astwalk"
+)
+
+func init() {
+	var info lintpack.CheckerInfo
+	info.Name = "boolFuncPrefix"
+	info.Tags = []string{"style", "experimental", "opinionated"}
+	info.Summary = "Detects function returning only bool and suggests to add Is/Has/Contains prefix to it's name"
+	info.Before = "func Enabled() bool"
+	info.After = "func IsEnabled() bool"
+
+	collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
+		return astwalk.WalkerForFuncDecl(&boolFuncPrefixChecker{ctx: ctx})
+	})
+}
+
+type boolFuncPrefixChecker struct {
+	astwalk.WalkHandler
+	ctx *lintpack.CheckerContext
+}
+
+func (c *boolFuncPrefixChecker) VisitFuncDecl(decl *ast.FuncDecl) {
+	params := decl.Type.Params
+	results := decl.Type.Results
+
+	if params.NumFields() > 0 ||
+		results.NumFields() != 1 ||
+		!c.isBoolType(results.List[0].Type) ||
+		c.hasProperPrefix(decl.Name.Name) {
+		return
+	}
+	c.warn(decl)
+}
+
+func (c *boolFuncPrefixChecker) warn(fn *ast.FuncDecl) {
+	c.ctx.Warn(fn, "consider to add Is/Has/Contains prefix to function name")
+}
+
+func (c *boolFuncPrefixChecker) isBoolType(expr ast.Expr) bool {
+	typ, ok := c.ctx.TypesInfo.TypeOf(expr).(*types.Basic)
+	return ok && typ.Kind() == types.Bool
+}
+
+func (c *boolFuncPrefixChecker) hasProperPrefix(name string) bool {
+	name = strings.ToLower(name)
+	excluded := []string{"exit", "quit"}
+	for _, ex := range excluded {
+		if name == ex {
+			return true
+		}
+	}
+
+	prefixes := []string{"is", "has", "contains", "check", "get", "should", "need", "may", "should"}
+	for _, p := range prefixes {
+		if strings.HasPrefix(name, p) {
+			return true
+		}
+	}
+	return false
+}

floatCompare_checker.go

@@ -0,0 +1,107 @@
+package checkers
+
+import (
+	"go/ast"
+	"go/token"
+	"go/types"
+
+	"github.com/go-lintpack/lintpack"
+	"github.com/go-lintpack/lintpack/astwalk"
+	"github.com/go-toolsmith/astequal"
+	"github.com/go-toolsmith/astp"
+	"golang.org/x/tools/go/ast/astutil"
+)
+
+func init() {
+	var info lintpack.CheckerInfo
+	info.Name = "floatCompare"
+	info.Tags = []string{"diagnostic", "experimental"}
+	info.Summary = "Detects fragile float variables comparisons"
+	info.Before = `
+// x and y are floats
+return x == y`
+	info.After = `
+// x and y are floats
+return math.Abs(x - y) < eps`
+
+	collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
+		return astwalk.WalkerForLocalExpr(&floatCompareChecker{ctx: ctx})
+	})
+}
+
+type floatCompareChecker struct {
+	astwalk.WalkHandler
+	ctx *lintpack.CheckerContext
+}
+
+func (c *floatCompareChecker) VisitLocalExpr(expr ast.Expr) {
+	binexpr, ok := expr.(*ast.BinaryExpr)
+	if !ok {
+		return
+	}
+	if binexpr.Op == token.EQL || binexpr.Op == token.NEQ {
+		if c.isFloatExpr(binexpr) &&
+			c.isMultiBinaryExpr(binexpr) &&
+			!c.isNaNCheckExpr(binexpr) &&
+			!c.isInfCheckExpr(binexpr) {
+			c.warn(binexpr)
+		}
+	}
+}
+
+func (c *floatCompareChecker) isFloatExpr(binexpr *ast.BinaryExpr) bool {
+	exprx := astutil.Unparen(binexpr.X)
+	typx, ok := c.ctx.TypesInfo.TypeOf(exprx).(*types.Basic)
+	return ok && typx.Info()&types.IsFloat != 0
+}
+
+func (c *floatCompareChecker) isNaNCheckExpr(binexpr *ast.BinaryExpr) bool {
+	exprx := astutil.Unparen(binexpr.X)
+	expry := astutil.Unparen(binexpr.Y)
+	return astequal.Expr(exprx, expry)
+}
+
+func (c *floatCompareChecker) isInfCheckExpr(binexpr *ast.BinaryExpr) bool {
+	binx := astutil.Unparen(binexpr.X)
+	biny := astutil.Unparen(binexpr.Y)
+	expr, bin, ok := c.identExpr(binx, biny)
+	if !ok {
+		return false
+	}
+	x := astutil.Unparen(expr)
+	y := astutil.Unparen(bin.X)
+	z := astutil.Unparen(bin.Y)
+	return astequal.Expr(x, y) && astequal.Expr(y, z)
+}
+
+func (c *floatCompareChecker) isMultiBinaryExpr(binexpr *ast.BinaryExpr) bool {
+	exprx := astutil.Unparen(binexpr.X)
+	expry := astutil.Unparen(binexpr.Y)
+	return astp.IsBinaryExpr(exprx) || astp.IsBinaryExpr(expry)
+}
+
+func (c *floatCompareChecker) identExpr(x, y ast.Node) (ast.Expr, *ast.BinaryExpr, bool) {
+	expr1, ok1 := x.(*ast.BinaryExpr)
+	expr2, ok2 := y.(*ast.BinaryExpr)
+	switch {
+	case ok1 && !ok2:
+		return y.(ast.Expr), expr1, true
+	case !ok1 && ok2:
+		return x.(ast.Expr), expr2, true
+
+	default:
+		return nil, nil, false
+	}
+}
+
+func (c *floatCompareChecker) warn(expr *ast.BinaryExpr) {
+	op := ">="
+	if expr.Op == token.EQL {
+		op = "<"
+	}
+	format := "change `%s` to `math.Abs(%s - %s) %s eps`"
+	if astp.IsBinaryExpr(expr.Y) {
+		format = "change `%s` to `math.Abs(%s - (%s)) %s eps`"
+	}
+	c.ctx.Warn(expr, format, expr, expr.X, expr.Y, op)
+}

indexOnlyLoop_checker.go

@@ -0,0 +1,83 @@
+package checkers
+
+import (
+	"go/ast"
+	"go/types"
+
+	"github.com/go-lintpack/lintpack"
+	"github.com/go-lintpack/lintpack/astwalk"
+	"github.com/go-toolsmith/astequal"
+	"github.com/go-toolsmith/typep"
+)
+
+func init() {
+	var info lintpack.CheckerInfo
+	info.Name = "indexOnlyLoop"
+	info.Tags = []string{"style", "experimental"}
+	info.Summary = "Detects for loops that can benefit from rewrite to range loop"
+	info.Details = "Suggests to use for key, v := range container form."
+	info.Before = `
+for i := range files {
+	if files[i] != nil {
+		files[i].Close()
+	}
+}`
+	info.After = `
+for _, f := range files {
+	if f != nil {
+		f.Close()
+	}
+}`
+
+	collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
+		return astwalk.WalkerForStmt(&indexOnlyLoopChecker{ctx: ctx})
+	})
+}
+
+type indexOnlyLoopChecker struct {
+	astwalk.WalkHandler
+	ctx *lintpack.CheckerContext
+}
+
+func (c *indexOnlyLoopChecker) VisitStmt(stmt ast.Stmt) {
+	rng, ok := stmt.(*ast.RangeStmt)
+	if !ok || rng.Key == nil || rng.Value != nil {
+		return
+	}
+	iterated := c.ctx.TypesInfo.ObjectOf(identOf(rng.X))
+	if iterated == nil || !c.elemTypeIsPtr(iterated) {
+		return // To avoid redundant traversals
+	}
+	count := 0
+	ast.Inspect(rng.Body, func(n ast.Node) bool {
+		if n, ok := n.(*ast.IndexExpr); ok {
+			if !astequal.Expr(n.Index, rng.Key) {
+				return true
+			}
+			if iterated == c.ctx.TypesInfo.ObjectOf(identOf(n.X)) {
+				count++
+			}
+		}
+		// Stop DFS traverse if we found more than one usage.
+		return count < 2
+	})
+	if count > 1 {
+		c.warn(stmt, rng.Key, iterated.Name())
+	}
+}
+
+func (c *indexOnlyLoopChecker) elemTypeIsPtr(obj types.Object) bool {
+	switch typ := obj.Type().(type) {
+	case *types.Slice:
+		return typep.IsPointer(typ.Elem())
+	case *types.Array:
+		return typep.IsPointer(typ.Elem())
+	default:
+		return false
+	}
+}
+
+func (c *indexOnlyLoopChecker) warn(x, key ast.Node, iterated string) {
+	c.ctx.Warn(x, "%s occurs more than once in the loop; consider using for _, value := range %s",
+		key, iterated)
+}