lint: improve type parenthesis checker warning messages

cmd/gocritic/testdata/parenthesis/negative_tests.go

@@ -1,10 +1,10 @@
 package checker_test
 
-const ALAST = 1
+const length = 1
 
-var opindex [(ALAST + 1) & 3]*int
+var opindex [(length + 1) & 3]*int
 
-var _ [(ALAST + 100 - 20*5)]*int
+var _ [(length + 100 - 20*5)]*int
 
 var _ func(int, string)
 
@@ -17,3 +17,11 @@ func convertPtr(x string) *myString {
 func multipleReturn() (int, bool) {
 	return 1, true
 }
+
+type goodMap1 map[string]string
+
+type goodMap2 map[[5][5]string]map[string]string
+
+var _ = [4]*int{}
+
+var _ = func() []func() { return nil }

cmd/gocritic/testdata/parenthesis/positive_tests.go

@@ -1,30 +1,29 @@
 package checker_test
 
-/// could simplify (func()) to func()
+/// could simplify [](func()) to []func()
 func badReturn() [](func()) {
 	return nil
 }
 
-//TODO: could simplify [](func[](func())) to []func([]func())
-/// could simplify (func([](func()))) to func([](func()))
+/// could simplify [](func([](func()))) to []func([]func())
 func veryBadReturn() [](func([](func()))) {
 	return nil
 }
 
-/// could simplify (func()) to func()
+/// could simplify [](func()) to []func()
 var _ [](func())
 
-/// could simplify (*int) to *int
+/// could simplify [5](*int) to [5]*int
 var _ [5](*int)
 
-/// could simplify (func()) to func()
+/// could simplify [](func()) to []func()
 var _ [](func())
 
 var (
 	_ int
-	/// could simplify (*int) to *int
+	/// could simplify [5](*int) to [5]*int
 	_ [5](*int)
-	/// could simplify (func()) to func()
+	/// could simplify [](func()) to []func()
 	_ [](func())
 )
 
@@ -43,10 +42,10 @@ type myStruct1 struct {
 }
 
 type myInterface1 interface {
-	/// could simplify (int) to int
+	/// could simplify [](int) to []int
 	foo([](int))
 
-	/// could simplify (func() string) to func() string
+	/// could simplify [](func() string) to []func() string
 	bar() [](func() string)
 }
 
@@ -76,3 +75,15 @@ func myFunc1() {
 	_ = localVar1
 	_ = localVar2
 }
+
+/// could simplify map[(string)](string) to map[string]string
+type mapType1 map[(string)](string)
+
+/// could simplify map[[5][5](string)]map[(string)](string) to map[[5][5]string]map[string]string
+type mapType2 map[[5][5](string)]map[(string)](string)
+
+/// could simplify [4](*int) to [4]*int
+var _ = [4](*int){}
+
+/// could simplify [](func()) to []func()
+var _ = func() [](func()) { return nil }

lint/appendCombine_checker.go

@@ -4,7 +4,7 @@ import (
 	"go/ast"
 	"go/token"
 
-	"github.com/Quasilyte/astcmp"
+	"github.com/go-toolsmith/astequal"
 )
 
 func init() {
@@ -78,15 +78,15 @@ func (c *appendCombineChecker) matchAppend(stmt ast.Stmt, slice ast.Expr) *ast.C
 		cond := ok &&
 			qualifiedName(call.Fun) == "append" &&
 			call.Ellipsis == token.NoPos &&
-			astcmp.EqualExpr(assign.Lhs[0], call.Args[0])
+			astequal.Expr(assign.Lhs[0], call.Args[0])
 		if !cond {
 			return nil
 		}
 	}
 
 	// Check that current append slice match previous append slice.
 	// Otherwise we should break the chain.
-	if slice == nil || astcmp.EqualExpr(slice, call.Args[0]) {
+	if slice == nil || astequal.Expr(slice, call.Args[0]) {
 		return call
 	}
 	return nil

lint/paramDuplication_checker.go

@@ -3,7 +3,7 @@ package lint
 import (
 	"go/ast"
 
-	"github.com/Quasilyte/astcmp"
+	"github.com/go-toolsmith/astequal"
 )
 
 func init() {
@@ -22,7 +22,7 @@ func (c paramDuplicationChecker) New(ctx *context) func(*ast.File) {
 
 func (c *paramDuplicationChecker) CheckFuncDecl(decl *ast.FuncDecl) {
 	typ := c.optimizeFuncType(decl.Type)
-	if !astcmp.EqualExpr(typ, decl.Type) {
+	if !astequal.Expr(typ, decl.Type) {
 		c.warn(decl.Type, typ)
 	}
 }
@@ -48,7 +48,7 @@ func (c *paramDuplicationChecker) optimizeParams(params *ast.FieldList) *ast.Fie
 	for i, p := range params.List[1:] {
 		names = make([]*ast.Ident, len(p.Names))
 		copy(names, p.Names)
-		if astcmp.EqualExpr(p.Type, params.List[i].Type) {
+		if astequal.Expr(p.Type, params.List[i].Type) {
 			list[len(list)-1].Names =
 				append(list[len(list)-1].Names, names...)
 		} else {

lint/parenthesis_checker.go

@@ -2,6 +2,9 @@ package lint
 
 import (
 	"go/ast"
+
+	"github.com/go-toolsmith/astcopy"
+	"golang.org/x/tools/go/ast/astutil"
 )
 
 func init() {
@@ -19,16 +22,43 @@ func (c parenthesisChecker) New(ctx *context) func(*ast.File) {
 }
 
 func (c *parenthesisChecker) CheckTypeExpr(expr ast.Expr) {
-	// TODO: improve suggestions for complex cases like (func([](func()))).
-	// TODO: print outermost cause instead of innermost.
-	ast.Inspect(expr, func(n ast.Node) bool {
-		expr, ok := n.(*ast.ParenExpr)
-		if !ok {
-			return true
+	// Arrays require extra care: we don't want to unparen
+	// length expression as they are not type expressions.
+	if arr, ok := expr.(*ast.ArrayType); ok {
+		if !c.hasParens(arr.Elt) {
+			return
 		}
-		c.ctx.Warn(expr, "could simplify %s to %s",
-			nodeString(c.ctx.FileSet, expr),
-			nodeString(c.ctx.FileSet, expr.X))
-		return false
+		noParens := astcopy.Expr(arr).(*ast.ArrayType)
+		noParens.Elt = c.unparenExpr(noParens.Elt)
+		c.warn(expr, noParens)
+		return
+	}
+	if !c.hasParens(expr) {
+		return
+	}
+	c.warn(expr, c.unparenExpr(astcopy.Expr(expr)))
+}
+
+func (c *parenthesisChecker) hasParens(x ast.Expr) bool {
+	// TODO: could use astp.IsParenExpr. Refs #200
+	return nil != findNode(x, func(x ast.Node) bool {
+		_, ok := x.(*ast.ParenExpr)
+		return ok
 	})
 }
+
+func (c *parenthesisChecker) unparenExpr(x ast.Expr) ast.Expr {
+	// Replace every paren expr with expression it encloses.
+	return astutil.Apply(x, nil, func(cur *astutil.Cursor) bool {
+		if paren, ok := cur.Node().(*ast.ParenExpr); ok {
+			cur.Replace(paren.X)
+		}
+		return true
+	}).(ast.Expr)
+}
+
+func (c *parenthesisChecker) warn(cause, noParens ast.Expr) {
+	c.ctx.Warn(cause, "could simplify %s to %s",
+		nodeString(c.ctx.FileSet, cause),
+		nodeString(c.ctx.FileSet, noParens))
+}

lint/stddef_checker.go

@@ -10,7 +10,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/Quasilyte/astcmp"
+	"github.com/go-toolsmith/astequal"
 )
 
 // TODO(quasilyte): if we were tracking expression context, it would be possible
@@ -135,7 +135,7 @@ func (c *stddefChecker) CheckExpr(expr ast.Expr) {
 		c.checkBasicLit(expr, val)
 	case *ast.BinaryExpr:
 		for suggestion, y := range c.suggestionToExpression {
-			if astcmp.EqualExpr(expr, y) {
+			if astequal.Expr(expr, y) {
 				c.warn(expr, suggestion)
 				return
 			}

lint/typeGuard_checker.go

@@ -3,8 +3,7 @@ package lint
 import (
 	"go/ast"
 
-	"github.com/Quasilyte/astcmp"
-	"golang.org/x/tools/go/ast/astutil"
+	"github.com/go-toolsmith/astequal"
 )
 
 func init() {
@@ -49,8 +48,8 @@ func (c *typeGuardChecker) checkTypeSwitch(root *ast.TypeSwitchStmt) {
 		// Create artificial node just for matching.
 		assert1 := ast.TypeAssertExpr{X: expr, Type: clause.List[0]}
 		for _, stmt := range clause.Body {
-			assert2 := c.find(stmt, func(x ast.Node) bool {
-				return astcmp.Equal(&assert1, x)
+			assert2 := findNode(stmt, func(x ast.Node) bool {
+				return astequal.Node(&assert1, x)
 			})
 			if object == c.ctx.TypesInfo.ObjectOf(c.identOf(assert2)) {
 				c.warn(root, i)
@@ -64,23 +63,6 @@ func (c *typeGuardChecker) warn(node ast.Node, caseIndex int) {
 	c.ctx.Warn(node, "case %d can benefit from type switch with assignment", caseIndex)
 }
 
-// find applies pred for root and all it's childs until it returns true.
-// Matched node is returned.
-// If none of the nodes matched predicate, nil is returned.
-//
-// TODO: is this generally useful and can be placed in util.go?
-func (c *typeGuardChecker) find(root ast.Node, pred func(ast.Node) bool) ast.Node {
-	var found ast.Node
-	astutil.Apply(root, nil, func(cur *astutil.Cursor) bool {
-		if pred(cur.Node()) {
-			found = cur.Node()
-			return false
-		}
-		return true
-	})
-	return found
-}
-
 // identOf returns identifier for x that can be used to obtain associated types.Object.
 // Returns nil for expressions that yield temporary results, like `f().field`.
 //

lint/util.go

@@ -7,6 +7,8 @@ import (
 	"go/token"
 	"go/types"
 	"strings"
+
+	"golang.org/x/tools/go/ast/astutil"
 )
 
 func nodeString(fset *token.FileSet, x ast.Node) string {
@@ -51,3 +53,18 @@ func qualifiedName(x ast.Expr) string {
 		return ""
 	}
 }
+
+// findNode applies pred for root and all it's childs until it returns true.
+// Matched node is returned.
+// If none of the nodes matched predicate, nil is returned.
+func findNode(root ast.Node, pred func(ast.Node) bool) ast.Node {
+	var found ast.Node
+	astutil.Apply(root, nil, func(cur *astutil.Cursor) bool {
+		if pred(cur.Node()) {
+			found = cur.Node()
+			return false
+		}
+		return true
+	})
+	return found
+}

lint/wrappers.go

@@ -219,15 +219,18 @@ func wrapTypeExprChecker(c typeExprChecker) func(*ast.File) {
 
 	checkExpr = func(x ast.Expr) {
 		switch x := x.(type) {
+		case *ast.CompositeLit:
+			checkExpr(x.Type)
 		case *ast.StructType:
 			checkStructType(x)
 		case *ast.InterfaceType:
 			checkFieldList(x.Methods.List)
 		case *ast.FuncType:
 			checkFuncType(x)
 		case *ast.ArrayType:
-			// TODO: handle slices separately.
-			checkExpr(x.Elt)
+			c.CheckTypeExpr(x)
+		case *ast.FuncLit:
+			checkExpr(x.Type)
 		default:
 			c.CheckTypeExpr(x)
 		}
@@ -240,6 +243,9 @@ func wrapTypeExprChecker(c typeExprChecker) func(*ast.File) {
 				if spec.Type != nil {
 					checkExpr(spec.Type)
 				}
+				for _, expr := range spec.Values {
+					checkExpr(expr)
+				}
 			case *ast.TypeSpec:
 				checkExpr(spec.Type)
 			default: