Skip to content

Commit

Permalink
Fix the compare different type rule, to allow using WithTransform
Browse files Browse the repository at this point in the history
The `WithTransform` matcher can be used to transform the actual value
type. This PR fix a false positive when using this matcher to change the
actual value type to match the matcher type.

For example:
```go
Expect(uint(5)).Should(WithTransform(func(i uint) int { return int(i) }, Equal(5)))
```

In this case, the test should work because the transform function (the
first parameter of the `WithTransform` matcher) casts the `uint` actual
value to `int`, to match the `Equal(5)` matcher.
  • Loading branch information
nunnatsa committed Oct 3, 2023
1 parent 1785e17 commit 8a24a39
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 6 deletions.
31 changes: 28 additions & 3 deletions ginkgo_linter.go
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,8 @@ func checkEqualDifferentTypes(pass *analysis.Pass, matcher *ast.CallExpr, actual
return false
}

actualType := pass.TypesInfo.TypeOf(actualArg)

switch matcherFuncName {
case equal, beIdenticalTo: // continue
case and, or:
Expand All @@ -354,7 +356,16 @@ func checkEqualDifferentTypes(pass *analysis.Pass, matcher *ast.CallExpr, actual
return false
}

return checkEqualDifferentTypes(pass, nested, actualArg, handler, old, parentPointer)
if t := getFuncType(pass, matcher.Args[0]); t != nil {
actualType = t
matcher = nested
matcherFuncName, ok = handler.GetActualFuncName(nested)
if !ok {
return false
}
} else {
return checkEqualDifferentTypes(pass, nested, actualArg, handler, old, parentPointer)
}

case not:
nested, ok := matcher.Args[0].(*ast.CallExpr)
Expand All @@ -377,8 +388,6 @@ func checkEqualDifferentTypes(pass *analysis.Pass, matcher *ast.CallExpr, actual

matcherValue := matcher.Args[0]

actualType := pass.TypesInfo.TypeOf(actualArg)

switch act := actualType.(type) {
case *gotypes.Tuple:
actualType = act.At(0).Type()
Expand All @@ -403,6 +412,22 @@ func checkEqualDifferentTypes(pass *analysis.Pass, matcher *ast.CallExpr, actual
return false
}

func getFuncType(pass *analysis.Pass, expr ast.Expr) gotypes.Type {
switch f := expr.(type) {
case *ast.FuncLit:
if f.Type != nil && f.Type.Results != nil && len(f.Type.Results.List) > 0 {
return pass.TypesInfo.TypeOf(f.Type.Results.List[0].Type)
}
case *ast.Ident:
a := pass.TypesInfo.TypeOf(f)
if sig, ok := a.(*gotypes.Signature); ok && sig.Results().Len() > 0 {
return sig.Results().At(0).Type()
}
}

return nil
}

func isImplementing(ifs, impl gotypes.Type) bool {
if gotypes.IsInterface(ifs) {

Expand Down
14 changes: 14 additions & 0 deletions testdata/src/a/comparetypes/comparetypes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ import (
. "github.com/onsi/gomega"
)

func uint2int(u uint) int {
return int(u)
}

var _ = Describe("compare different types", func() {
It("find false positive check", func() {
a := 5
Expand Down Expand Up @@ -54,4 +58,14 @@ var _ = Describe("compare different types", func() {
Expect(a).Should(Equal(b))
Expect(b).Should(Equal(a))
})

It("test WithTransform", func() {
a := uint(5)
Expect(uint(5)).Should(WithTransform(func(i uint) int { return int(i) }, Equal(5)))
Expect(uint(5)).Should(WithTransform(func(i uint) uint64 { return uint64(i) }, Equal(5))) // want `ginkgo-linter: use Equal with different types: Comparing uint64 with int; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
Expect(a).Should(WithTransform(func(i uint) int { return int(i) }, Equal(5)))
Expect(a).Should(WithTransform(uint2int, Equal(5)))
Expect(a).Should(WithTransform(uint2int, Equal(uint64(5)))) // want `ginkgo-linter: use Equal with different types: Comparing int with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
Expect(5).Should(WithTransform(func(i int) myinf { return imp1(i) }, Equal(imp1(5))))
})
})
6 changes: 3 additions & 3 deletions tests/e2e.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ cp ginkgolinter testdata/src/a
cd testdata/src/a

# no suppress
[[ $(./ginkgolinter a/... 2>&1 | wc -l) == 2462 ]]
[[ $(./ginkgolinter a/... 2>&1 | wc -l) == 2464 ]]
# suppress all but nil
[[ $(./ginkgolinter --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 1452 ]]
# suppress all but len
Expand All @@ -20,8 +20,8 @@ cd testdata/src/a
# suppress all but focus
[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-focus-container=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 143 ]]
# suppress all but compare different types
[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-compare-assertion=true a/... 2>&1 | wc -l) == 209 ]]
[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-compare-assertion=true a/... 2>&1 | wc -l) == 211 ]]
# allow HaveLen(0)
[[ $(./ginkgolinter --allow-havelen-0=true a/... 2>&1 | wc -l) == 2449 ]]
[[ $(./ginkgolinter --allow-havelen-0=true a/... 2>&1 | wc -l) == 2451 ]]
# suppress all - should only return the few non-suppressble
[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-focus-container=false --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 88 ]]

0 comments on commit 8a24a39

Please sign in to comment.