Skip to content

Commit

Permalink
negative-positive: support typed 0 (#126)
Browse files Browse the repository at this point in the history
* negative-positive: support typed 0

Fixes #94

* negative-positive: support signed zeros only

* negative-positive: refactor debug tests

* negative-positive: support typed 0

Fixes #94

* negative-positive: support signed zeros only

* negative-positive: refactor debug tests

* negative-positive: support Positive for uints

---------

Co-authored-by: Anton Telyshev <anton.telishev@yandex.ru>
  • Loading branch information
ccoVeille and Antonboom authored Jun 20, 2024
1 parent 679ad66 commit 473abe0
Show file tree
Hide file tree
Showing 9 changed files with 2,244 additions and 398 deletions.
8 changes: 7 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type TimeCompare struct{}

// NewTimeCompare constructs TimeCompare checker.
func NewTimeCompare() TimeCompare { return TimeCompare{} }
func (TimeCompare) Name() string { return "TimeCompare" }
func (TimeCompare) Name() string { return "time-compare" }
```

The above code is enough to satisfy the `checkers.Checker` interface.
Expand Down Expand Up @@ -121,6 +121,12 @@ Install...

Fix linter issues and broken tests (probably related to the checkers registry).

To run checker default tests you can use `task test:checker -- {checker-name}`, e.g.

```bash
$ task test:checker -- time-compare
```

### 10) Update `README.md`, commit the changes and submit a pull request 🔥

Describe a new checker in [checkers section](./README.md#checkers).
Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,8 @@ assert.Positive(t, a)
**Enabled by default**: true <br>
**Reason**: More appropriate `testify` API with clearer failure message.

Typed zeros (like `int8(0)`, ..., `uint64(0)`) are also supported.

---

### nil-compare
Expand Down
6 changes: 6 additions & 0 deletions Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ tasks:
- echo "Test..."
- go test ./...

# task test:checker -- negative-positive
test:checker:
deps: [ test:gen ]
cmds:
- go test -count 1 -run TestTestifyLint_CheckersDefault/{{.CLI_ARGS}} ./analyzer

test:coverage:
env:
GOEXPERIMENT: nocoverageredesign # https://github.com/golang/go/issues/65653#issuecomment-1955872134
Expand Down

Large diffs are not rendered by default.

Large diffs are not rendered by default.

52 changes: 46 additions & 6 deletions analyzer/testdata/src/debug/negative_positive_replacements_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
func TestNegativeReplacements(t *testing.T) {
var tm tMock

negativeAssertions := []func(v int) bool{
negativeAssertionsInt := []func(v int) bool{
func(v int) bool { return assert.Less(tm, v, 0) },
func(v int) bool { return assert.Greater(tm, 0, v) },
func(v int) bool { return assert.True(tm, v < 0) },
Expand All @@ -19,8 +19,8 @@ func TestNegativeReplacements(t *testing.T) {
func(v int) bool { return assert.False(tm, 0 <= v) },
}

t.Run("assert.Negative", func(t *testing.T) {
for i, original := range negativeAssertions {
t.Run("int", func(t *testing.T) {
for i, original := range negativeAssertionsInt {
for _, v := range []int{-1, 0, 1} {
t.Run(fmt.Sprintf("%d_%d", i, v), func(t *testing.T) {
replacement := assert.Negative(tm, v)
Expand All @@ -29,12 +29,32 @@ func TestNegativeReplacements(t *testing.T) {
}
}
})

negativeAssertionsUint := []func(v uint8) bool{
func(v uint8) bool { return assert.Less(tm, v, uint8(0)) },
func(v uint8) bool { return assert.Greater(tm, uint8(0), v) },
func(v uint8) bool { return assert.True(tm, v < uint8(0)) },
func(v uint8) bool { return assert.True(tm, uint8(0) > v) },
func(v uint8) bool { return assert.False(tm, v >= uint8(0)) },
func(v uint8) bool { return assert.False(tm, uint8(0) <= v) },
}

t.Run("uint", func(t *testing.T) {
for i, original := range negativeAssertionsUint {
for _, v := range []uint8{ /* -1, */ 0, 1} { // constant -1 overflows uint8
t.Run(fmt.Sprintf("%d_%d", i, v), func(t *testing.T) {
replacement := assert.Negative(tm, v)
assert.Equal(t, original(v), replacement, "not an equivalent replacement")
})
}
}
})
}

func TestPositiveReplacements(t *testing.T) {
var tm tMock

positiveAssertions := []func(v int) bool{
positiveAssertionsInt := []func(v int) bool{
func(v int) bool { return assert.Greater(tm, v, 0) },
func(v int) bool { return assert.Less(tm, 0, v) },
func(v int) bool { return assert.True(tm, v > 0) },
Expand All @@ -43,8 +63,8 @@ func TestPositiveReplacements(t *testing.T) {
func(v int) bool { return assert.False(tm, 0 >= v) },
}

t.Run("assert.Positive", func(t *testing.T) {
for i, original := range positiveAssertions {
t.Run("int", func(t *testing.T) {
for i, original := range positiveAssertionsInt {
for _, v := range []int{-1, 0, 1} {
t.Run(fmt.Sprintf("%d_%d", i, v), func(t *testing.T) {
replacement := assert.Positive(tm, v)
Expand All @@ -53,4 +73,24 @@ func TestPositiveReplacements(t *testing.T) {
}
}
})

positiveAssertionsUint := []func(v uint8) bool{
func(v uint8) bool { return assert.Greater(tm, v, uint8(0)) },
func(v uint8) bool { return assert.Less(tm, uint8(0), v) },
func(v uint8) bool { return assert.True(tm, v > uint8(0)) },
func(v uint8) bool { return assert.True(tm, uint8(0) < v) },
func(v uint8) bool { return assert.False(tm, v <= uint8(0)) },
func(v uint8) bool { return assert.False(tm, uint8(0) >= v) },
}

t.Run("uint", func(t *testing.T) {
for i, original := range positiveAssertionsUint {
for _, v := range []uint8{ /* -1, */ 0, 1} { // constant -1 overflows uint8
t.Run(fmt.Sprintf("%d_%d", i, v), func(t *testing.T) {
replacement := assert.Positive(tm, v)
assert.Equal(t, original(v), replacement, "not an equivalent replacement")
})
}
}
})
}
86 changes: 69 additions & 17 deletions internal/checkers/helpers_basic_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,51 @@ import (

func isZero(e ast.Expr) bool { return isIntNumber(e, 0) }

func isNotZero(e ast.Expr) bool { return !isZero(e) }

func isOne(e ast.Expr) bool { return isIntNumber(e, 1) }

func isAnyZero(e ast.Expr) bool {
return isIntNumber(e, 0) || isTypedSignedIntNumber(e, 0) || isTypedUnsignedIntNumber(e, 0)
}

func isNotAnyZero(e ast.Expr) bool {
return !isAnyZero(e)
}

func isZeroOrSignedZero(e ast.Expr) bool {
return isIntNumber(e, 0) || isTypedSignedIntNumber(e, 0)
}

func isSignedNotZero(pass *analysis.Pass, e ast.Expr) bool {
return !isUnsigned(pass, e) && !isZeroOrSignedZero(e)
}

func isTypedSignedIntNumber(e ast.Expr, v int) bool {
return isTypedIntNumber(e, v, "int", "int8", "int16", "int32", "int64")
}

func isTypedUnsignedIntNumber(e ast.Expr, v int) bool {
return isTypedIntNumber(e, v, "uint", "uint8", "uint16", "uint32", "uint64")
}

func isTypedIntNumber(e ast.Expr, v int, types ...string) bool {
ce, ok := e.(*ast.CallExpr)
if !ok || len(ce.Args) != 1 {
return false
}

fn, ok := ce.Fun.(*ast.Ident)
if !ok {
return false
}

for _, t := range types {
if fn.Name == t {
return isIntNumber(ce.Args[0], v)
}
}
return false
}

func isIntNumber(e ast.Expr, v int) bool {
bl, ok := e.(*ast.BasicLit)
return ok && bl.Kind == token.INT && bl.Value == fmt.Sprintf("%d", v)
Expand All @@ -30,32 +71,43 @@ func isIntBasicLit(e ast.Expr) bool {
return ok && bl.Kind == token.INT
}

func isUntypedConst(p *analysis.Pass, e ast.Expr) bool {
t := p.TypesInfo.TypeOf(e)
if t == nil {
return false
}

b, ok := t.(*types.Basic)
return ok && b.Info()&types.IsUntyped > 0
func isUntypedConst(pass *analysis.Pass, e ast.Expr) bool {
return isUnderlying(pass, e, types.IsUntyped)
}

func isTypedConst(p *analysis.Pass, e ast.Expr) bool {
tt, ok := p.TypesInfo.Types[e]
func isTypedConst(pass *analysis.Pass, e ast.Expr) bool {
tt, ok := pass.TypesInfo.Types[e]
return ok && tt.IsValue() && tt.Value != nil
}

func isFloat(pass *analysis.Pass, expr ast.Expr) bool {
t := pass.TypesInfo.TypeOf(expr)
func isFloat(pass *analysis.Pass, e ast.Expr) bool {
return isUnderlying(pass, e, types.IsFloat)
}

func isUnsigned(pass *analysis.Pass, e ast.Expr) bool {
return isUnderlying(pass, e, types.IsUnsigned)
}

func isUnderlying(pass *analysis.Pass, e ast.Expr, flag types.BasicInfo) bool {
t := pass.TypesInfo.TypeOf(e)
if t == nil {
return false
}

bt, ok := t.Underlying().(*types.Basic)
return ok && (bt.Info()&types.IsFloat > 0)
return ok && (bt.Info()&flag > 0)
}

func isPointer(pass *analysis.Pass, expr ast.Expr) bool {
_, ok := pass.TypesInfo.TypeOf(expr).(*types.Pointer)
func isPointer(pass *analysis.Pass, e ast.Expr) bool {
_, ok := pass.TypesInfo.TypeOf(e).(*types.Pointer)
return ok
}

// untype returns v from type(v) expression or v itself if there is no type cast.
func untype(e ast.Expr) ast.Expr {
ce, ok := e.(*ast.CallExpr)
if !ok || len(ce.Args) != 1 {
return e
}
return ce.Args[0]
}
44 changes: 24 additions & 20 deletions internal/checkers/negative_postive.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import (
//
// assert.Negative(t, value)
// assert.Positive(t, value)
//
// Typed zeros (like `int8(0)`, ..., `uint64(0)`) are also supported.
type NegativePositive struct{}

// NewNegativePositive constructs NegativePositive checker.
Expand All @@ -54,15 +56,17 @@ func (checker NegativePositive) checkNegative(pass *analysis.Pass, call *CallMet
)
}

// NOTE(a.telyshev): We ignore uint-asserts as being no sense for assert.Negative.

switch call.Fn.NameFTrimmed {
case "Less":
if len(call.Args) < 2 {
return nil
}
a, b := call.Args[0], call.Args[1]

if isNotZero(a) && isZero(b) {
return newUseNegativeDiagnostic(a.Pos(), b.End(), a)
if isSignedNotZero(pass, a) && isZeroOrSignedZero(b) {
return newUseNegativeDiagnostic(a.Pos(), b.End(), untype(a))
}

case "Greater":
Expand All @@ -71,8 +75,8 @@ func (checker NegativePositive) checkNegative(pass *analysis.Pass, call *CallMet
}
a, b := call.Args[0], call.Args[1]

if isZero(a) && isNotZero(b) {
return newUseNegativeDiagnostic(a.Pos(), b.End(), b)
if isZeroOrSignedZero(a) && isSignedNotZero(pass, b) {
return newUseNegativeDiagnostic(a.Pos(), b.End(), untype(b))
}

case "True":
Expand All @@ -81,12 +85,12 @@ func (checker NegativePositive) checkNegative(pass *analysis.Pass, call *CallMet
}
expr := call.Args[0]

a, _, ok1 := isStrictComparisonWith(pass, expr, p(isNotZero), token.LSS, p(isZero)) // a < 0
_, b, ok2 := isStrictComparisonWith(pass, expr, p(isZero), token.GTR, p(isNotZero)) // 0 > a
a, _, ok1 := isStrictComparisonWith(pass, expr, isSignedNotZero, token.LSS, p(isZeroOrSignedZero)) // a < 0
_, b, ok2 := isStrictComparisonWith(pass, expr, p(isZeroOrSignedZero), token.GTR, isSignedNotZero) // 0 > a

survivingArg, ok := anyVal([]bool{ok1, ok2}, a, b)
if ok {
return newUseNegativeDiagnostic(expr.Pos(), expr.End(), survivingArg)
return newUseNegativeDiagnostic(expr.Pos(), expr.End(), untype(survivingArg))
}

case "False":
Expand All @@ -95,12 +99,12 @@ func (checker NegativePositive) checkNegative(pass *analysis.Pass, call *CallMet
}
expr := call.Args[0]

a, _, ok1 := isStrictComparisonWith(pass, expr, p(isNotZero), token.GEQ, p(isZero)) // a >= 0
_, b, ok2 := isStrictComparisonWith(pass, expr, p(isZero), token.LEQ, p(isNotZero)) // 0 <= a
a, _, ok1 := isStrictComparisonWith(pass, expr, isSignedNotZero, token.GEQ, p(isZeroOrSignedZero)) // a >= 0
_, b, ok2 := isStrictComparisonWith(pass, expr, p(isZeroOrSignedZero), token.LEQ, isSignedNotZero) // 0 <= a

survivingArg, ok := anyVal([]bool{ok1, ok2}, a, b)
if ok {
return newUseNegativeDiagnostic(expr.Pos(), expr.End(), survivingArg)
return newUseNegativeDiagnostic(expr.Pos(), expr.End(), untype(survivingArg))
}
}
return nil
Expand All @@ -125,8 +129,8 @@ func (checker NegativePositive) checkPositive(pass *analysis.Pass, call *CallMet
}
a, b := call.Args[0], call.Args[1]

if isNotZero(a) && isZero(b) {
return newUsePositiveDiagnostic(a.Pos(), b.End(), a)
if isNotAnyZero(a) && isAnyZero(b) {
return newUsePositiveDiagnostic(a.Pos(), b.End(), untype(a))
}

case "Less":
Expand All @@ -135,8 +139,8 @@ func (checker NegativePositive) checkPositive(pass *analysis.Pass, call *CallMet
}
a, b := call.Args[0], call.Args[1]

if isZero(a) && isNotZero(b) {
return newUsePositiveDiagnostic(a.Pos(), b.End(), b)
if isAnyZero(a) && isNotAnyZero(b) {
return newUsePositiveDiagnostic(a.Pos(), b.End(), untype(b))
}

case "True":
Expand All @@ -145,12 +149,12 @@ func (checker NegativePositive) checkPositive(pass *analysis.Pass, call *CallMet
}
expr := call.Args[0]

a, _, ok1 := isStrictComparisonWith(pass, expr, p(isNotZero), token.GTR, p(isZero)) // a > 0
_, b, ok2 := isStrictComparisonWith(pass, expr, p(isZero), token.LSS, p(isNotZero)) // 0 < a
a, _, ok1 := isStrictComparisonWith(pass, expr, p(isNotAnyZero), token.GTR, p(isAnyZero)) // a > 0
_, b, ok2 := isStrictComparisonWith(pass, expr, p(isAnyZero), token.LSS, p(isNotAnyZero)) // 0 < a

survivingArg, ok := anyVal([]bool{ok1, ok2}, a, b)
if ok {
return newUsePositiveDiagnostic(expr.Pos(), expr.End(), survivingArg)
return newUsePositiveDiagnostic(expr.Pos(), expr.End(), untype(survivingArg))
}

case "False":
Expand All @@ -159,12 +163,12 @@ func (checker NegativePositive) checkPositive(pass *analysis.Pass, call *CallMet
}
expr := call.Args[0]

a, _, ok1 := isStrictComparisonWith(pass, expr, p(isNotZero), token.LEQ, p(isZero)) // a <= 0
_, b, ok2 := isStrictComparisonWith(pass, expr, p(isZero), token.GEQ, p(isNotZero)) // 0 >= a
a, _, ok1 := isStrictComparisonWith(pass, expr, p(isNotAnyZero), token.LEQ, p(isAnyZero)) // a <= 0
_, b, ok2 := isStrictComparisonWith(pass, expr, p(isAnyZero), token.GEQ, p(isNotAnyZero)) // 0 >= a

survivingArg, ok := anyVal([]bool{ok1, ok2}, a, b)
if ok {
return newUsePositiveDiagnostic(expr.Pos(), expr.End(), survivingArg)
return newUsePositiveDiagnostic(expr.Pos(), expr.End(), untype(survivingArg))
}
}
return nil
Expand Down
Loading

0 comments on commit 473abe0

Please sign in to comment.