Skip to content

Commit 4d3fdcc

Browse files
AntonboomAntonboom
authored
negative-positive: remove untyping, ignore Negative for len comparisons (#226)
* negative-positive: ignore Negative for len comparisons * negative-positive: remove untyping support (but keep typed zeros support) * useless-assert: support meaningless len comparions * self-review fixes
1 parent 38fb805 commit 4d3fdcc

File tree

9 files changed

+460
-306
lines changed

9 files changed

+460
-306
lines changed

‎README.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1064,6 +1064,11 @@ assert.True(t, true)
10641064
assert.Zero(t, 0)
10651065
assert.Zero(t, "")
10661066
assert.Zero(t, nil)
1067+
1068+
assert.GreaterOrEqual(t, uintVal, 0)
1069+
assert.LessOrEqual(t, 0, uintVal)
1070+
assert.GreaterOrEqual(t, len(x), 0)
1071+
assert.LessOrEqual(t, 0, len(x))
10671072
```
10681073

10691074
**Autofix**: false. <br>

‎analyzer/testdata/src/checkers-default/negative-positive/negative_positive_test.go

Lines changed: 99 additions & 61 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎analyzer/testdata/src/checkers-default/negative-positive/negative_positive_test.go.golden

Lines changed: 245 additions & 207 deletions
Large diffs are not rendered by default.

‎analyzer/testdata/src/checkers-default/useless-assert/useless_assert_test.go

Lines changed: 29 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/checkers/helpers_basic_type.go

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -176,12 +176,3 @@ func hasStringType(pass *analysis.Pass, e ast.Expr) bool {
176176
basicType, ok := pass.TypesInfo.TypeOf(e).(*types.Basic)
177177
return ok && basicType.Kind() == types.String
178178
}
179-
180-
// untype returns v from type(v) expression or v itself if there is no type conversion.
181-
func untype(e ast.Expr) ast.Expr {
182-
ce, ok := e.(*ast.CallExpr)
183-
if !ok || len(ce.Args) != 1 {
184-
return e
185-
}
186-
return ce.Args[0]
187-
}

‎internal/checkers/negative_positive.go

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ func (checker NegativePositive) checkNegative(pass *analysis.Pass, call *CallMet
5555
})
5656
}
5757

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

6060
switch call.Fn.NameFTrimmed {
6161
case "Less":
@@ -64,8 +64,8 @@ func (checker NegativePositive) checkNegative(pass *analysis.Pass, call *CallMet
6464
}
6565
a, b := call.Args[0], call.Args[1]
6666

67-
if isSignedNotZero(pass, a) && isZeroOrSignedZero(b) {
68-
return newUseNegativeDiagnostic(a.Pos(), b.End(), untype(a))
67+
if canBeNegative(pass, a) && isZeroOrSignedZero(b) {
68+
return newUseNegativeDiagnostic(a.Pos(), b.End(), a)
6969
}
7070

7171
case "Greater":
@@ -74,8 +74,8 @@ func (checker NegativePositive) checkNegative(pass *analysis.Pass, call *CallMet
7474
}
7575
a, b := call.Args[0], call.Args[1]
7676

77-
if isZeroOrSignedZero(a) && isSignedNotZero(pass, b) {
78-
return newUseNegativeDiagnostic(a.Pos(), b.End(), untype(b))
77+
if isZeroOrSignedZero(a) && canBeNegative(pass, b) {
78+
return newUseNegativeDiagnostic(a.Pos(), b.End(), b)
7979
}
8080

8181
case "True":
@@ -84,12 +84,12 @@ func (checker NegativePositive) checkNegative(pass *analysis.Pass, call *CallMet
8484
}
8585
expr := call.Args[0]
8686

87-
a, _, ok1 := isStrictComparisonWith(pass, expr, isSignedNotZero, token.LSS, p(isZeroOrSignedZero)) // a < 0
88-
_, b, ok2 := isStrictComparisonWith(pass, expr, p(isZeroOrSignedZero), token.GTR, isSignedNotZero) // 0 > a
87+
a, _, ok1 := isStrictComparisonWith(pass, expr, canBeNegative, token.LSS, p(isZeroOrSignedZero)) // a < 0
88+
_, b, ok2 := isStrictComparisonWith(pass, expr, p(isZeroOrSignedZero), token.GTR, canBeNegative) // 0 > a
8989

9090
survivingArg, ok := anyVal([]bool{ok1, ok2}, a, b)
9191
if ok {
92-
return newUseNegativeDiagnostic(expr.Pos(), expr.End(), untype(survivingArg))
92+
return newUseNegativeDiagnostic(expr.Pos(), expr.End(), survivingArg)
9393
}
9494

9595
case "False":
@@ -98,12 +98,12 @@ func (checker NegativePositive) checkNegative(pass *analysis.Pass, call *CallMet
9898
}
9999
expr := call.Args[0]
100100

101-
a, _, ok1 := isStrictComparisonWith(pass, expr, isSignedNotZero, token.GEQ, p(isZeroOrSignedZero)) // a >= 0
102-
_, b, ok2 := isStrictComparisonWith(pass, expr, p(isZeroOrSignedZero), token.LEQ, isSignedNotZero) // 0 <= a
101+
a, _, ok1 := isStrictComparisonWith(pass, expr, canBeNegative, token.GEQ, p(isZeroOrSignedZero)) // a >= 0
102+
_, b, ok2 := isStrictComparisonWith(pass, expr, p(isZeroOrSignedZero), token.LEQ, canBeNegative) // 0 <= a
103103

104104
survivingArg, ok := anyVal([]bool{ok1, ok2}, a, b)
105105
if ok {
106-
return newUseNegativeDiagnostic(expr.Pos(), expr.End(), untype(survivingArg))
106+
return newUseNegativeDiagnostic(expr.Pos(), expr.End(), survivingArg)
107107
}
108108
}
109109
return nil
@@ -128,7 +128,7 @@ func (checker NegativePositive) checkPositive(pass *analysis.Pass, call *CallMet
128128
a, b := call.Args[0], call.Args[1]
129129

130130
if isNotAnyZero(a) && isAnyZero(b) {
131-
return newUsePositiveDiagnostic(a.Pos(), b.End(), untype(a))
131+
return newUsePositiveDiagnostic(a.Pos(), b.End(), a)
132132
}
133133

134134
case "Less":
@@ -138,7 +138,7 @@ func (checker NegativePositive) checkPositive(pass *analysis.Pass, call *CallMet
138138
a, b := call.Args[0], call.Args[1]
139139

140140
if isAnyZero(a) && isNotAnyZero(b) {
141-
return newUsePositiveDiagnostic(a.Pos(), b.End(), untype(b))
141+
return newUsePositiveDiagnostic(a.Pos(), b.End(), b)
142142
}
143143

144144
case "True":
@@ -152,7 +152,7 @@ func (checker NegativePositive) checkPositive(pass *analysis.Pass, call *CallMet
152152

153153
survivingArg, ok := anyVal([]bool{ok1, ok2}, a, b)
154154
if ok {
155-
return newUsePositiveDiagnostic(expr.Pos(), expr.End(), untype(survivingArg))
155+
return newUsePositiveDiagnostic(expr.Pos(), expr.End(), survivingArg)
156156
}
157157

158158
case "False":
@@ -166,8 +166,13 @@ func (checker NegativePositive) checkPositive(pass *analysis.Pass, call *CallMet
166166

167167
survivingArg, ok := anyVal([]bool{ok1, ok2}, a, b)
168168
if ok {
169-
return newUsePositiveDiagnostic(expr.Pos(), expr.End(), untype(survivingArg))
169+
return newUsePositiveDiagnostic(expr.Pos(), expr.End(), survivingArg)
170170
}
171171
}
172172
return nil
173173
}
174+
175+
func canBeNegative(pass *analysis.Pass, e ast.Expr) bool {
176+
_, isLen := isBuiltinLenCall(pass, e)
177+
return isSignedNotZero(pass, e) && !isLen
178+
}

‎internal/checkers/useless_assert.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,11 @@ import (
4444
// assert.Zero(t, 0)
4545
// assert.Zero(t, "")
4646
// assert.Zero(t, nil)
47+
//
48+
// assert.GreaterOrEqual(t, uintVal, 0)
49+
// assert.LessOrEqual(t, 0, uintVal)
50+
// assert.GreaterOrEqual(t, len(x), 0)
51+
// assert.LessOrEqual(t, 0, len(x))
4752
type UselessAssert struct{}
4853

4954
// NewUselessAssert constructs UselessAssert checker.
@@ -63,6 +68,9 @@ func (checker UselessAssert) Check(pass *analysis.Pass, call *CallMeta) *analysi
6368
case "False":
6469
isMeaningless = (len(call.Args) >= 1) && isUntypedFalse(pass, call.Args[0])
6570

71+
case "GreaterOrEqual":
72+
isMeaningless = (len(call.Args) >= 2) && isAnyZero(call.Args[1]) && canNotBeNegative(pass, call.Args[0])
73+
6674
case "Implements":
6775
if len(call.Args) < 2 {
6876
return nil
@@ -71,6 +79,9 @@ func (checker UselessAssert) Check(pass *analysis.Pass, call *CallMeta) *analysi
7179
elem, ok := isPointer(pass, call.Args[0])
7280
isMeaningless = ok && isEmptyInterfaceType(elem)
7381

82+
case "LessOrEqual":
83+
isMeaningless = (len(call.Args) >= 2) && isAnyZero(call.Args[0]) && canNotBeNegative(pass, call.Args[1])
84+
7485
case "Negative":
7586
isMeaningless = (len(call.Args) >= 1) && isNegativeIntNumber(call.Args[0])
7687

@@ -165,3 +176,8 @@ func (checker UselessAssert) checkSameVars(pass *analysis.Pass, call *CallMeta)
165176
}
166177
return nil
167178
}
179+
180+
func canNotBeNegative(pass *analysis.Pass, e ast.Expr) bool {
181+
_, isLen := isBuiltinLenCall(pass, e)
182+
return isLen || isUnsigned(pass, e)
183+
}

‎internal/testgen/gen_negative_positive.go

Lines changed: 39 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,12 @@ func (g NegativePositiveTestsGenerator) TemplateData() any {
3333
}
3434

3535
invalidAssertions = append(invalidAssertions,
36-
Assertion{Fn: "Less", Argsf: "a, " + zero, ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: "a"},
37-
Assertion{Fn: "Greater", Argsf: zero + ", a", ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: "a"},
38-
Assertion{Fn: "True", Argsf: v + " < " + zero, ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: "a"},
39-
Assertion{Fn: "True", Argsf: zero + " > " + v, ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: "a"},
40-
Assertion{Fn: "False", Argsf: v + " >= " + zero, ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: "a"},
41-
Assertion{Fn: "False", Argsf: zero + " <= " + v, ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: "a"},
36+
Assertion{Fn: "Less", Argsf: v + ", " + zero, ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: v},
37+
Assertion{Fn: "Greater", Argsf: zero + ", " + v, ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: v},
38+
Assertion{Fn: "True", Argsf: v + " < " + zero, ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: v},
39+
Assertion{Fn: "True", Argsf: zero + " > " + v, ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: v},
40+
Assertion{Fn: "False", Argsf: v + " >= " + zero, ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: v},
41+
Assertion{Fn: "False", Argsf: zero + " <= " + v, ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: v},
4242
)
4343
}
4444

@@ -54,15 +54,22 @@ func (g NegativePositiveTestsGenerator) TemplateData() any {
5454
}
5555

5656
invalidAssertions = append(invalidAssertions,
57-
Assertion{Fn: "Greater", Argsf: "a, " + zero, ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: "a"},
58-
Assertion{Fn: "Less", Argsf: zero + ", a", ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: "a"},
59-
Assertion{Fn: "True", Argsf: v + " > " + zero, ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: "a"},
60-
Assertion{Fn: "True", Argsf: zero + " < " + v, ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: "a"},
61-
Assertion{Fn: "False", Argsf: v + " <= " + zero, ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: "a"},
62-
Assertion{Fn: "False", Argsf: zero + " >= " + v, ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: "a"},
57+
Assertion{Fn: "Greater", Argsf: v + ", " + zero, ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: v},
58+
Assertion{Fn: "Less", Argsf: zero + ", " + v, ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: v},
59+
Assertion{Fn: "True", Argsf: v + " > " + zero, ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: v},
60+
Assertion{Fn: "True", Argsf: zero + " < " + v, ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: v},
61+
Assertion{Fn: "False", Argsf: v + " <= " + zero, ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: v},
62+
Assertion{Fn: "False", Argsf: zero + " >= " + v, ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: v},
6363
)
6464
}
6565

66+
invalidAssertions = append(invalidAssertions,
67+
Assertion{Fn: "True", Argsf: "len(x) > 0", ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: "len(x)"},
68+
Assertion{Fn: "Greater", Argsf: "len(x), 0", ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: "len(x)"},
69+
Assertion{Fn: "Less", Argsf: "f(a), 0", ReportMsgf: report, ProposedFn: "Negative", ProposedArgsf: "f(a)"},
70+
Assertion{Fn: "False", Argsf: "0 >= f(a)", ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: "f(a)"},
71+
)
72+
6673
var ignoredAssertions []Assertion
6774

6875
for _, fn := range []string{"Equal", "NotEqual", "GreaterOrEqual", "LessOrEqual"} {
@@ -192,16 +199,27 @@ func (g NegativePositiveTestsGenerator) TemplateData() any {
192199
Assertion{Fn: "False", Argsf: "-1 <= -1"},
193200
Assertion{Fn: "False", Argsf: "-1 == -1"},
194201
Assertion{Fn: "False", Argsf: "-1 != -1"},
202+
Assertion{Fn: "GreaterOrEqual", Argsf: "len(x), 0"},
203+
Assertion{Fn: "LessOrEqual", Argsf: "0, len(x)"},
195204
)
196205

197206
// These one will be reported by incorrect-assert.
198207
ignoredAssertions = append(ignoredAssertions,
199208
Assertion{Fn: "Negative", Argsf: "uint(a)"},
200209
Assertion{Fn: "Less", Argsf: "uint(a), 0"},
210+
Assertion{Fn: "Greater", Argsf: "0, uint(a)"},
201211
Assertion{Fn: "True", Argsf: "uint(a) < 0"},
202212
Assertion{Fn: "True", Argsf: "0 > uint(a)"},
203213
Assertion{Fn: "False", Argsf: "uint(a) >= 0"},
204214
Assertion{Fn: "False", Argsf: "0 <= uint(a)"},
215+
216+
Assertion{Fn: "Negative", Argsf: "len(x)"},
217+
Assertion{Fn: "Less", Argsf: "len(x), 0"},
218+
Assertion{Fn: "Greater", Argsf: "0, len(x)"},
219+
Assertion{Fn: "True", Argsf: "len(x) < 0"},
220+
Assertion{Fn: "True", Argsf: "0 > len(x)"},
221+
Assertion{Fn: "False", Argsf: "len(x) >= 0"},
222+
Assertion{Fn: "False", Argsf: "0 <= len(x)"},
205223
)
206224

207225
return struct {
@@ -215,7 +233,10 @@ func (g NegativePositiveTestsGenerator) TemplateData() any {
215233
InvalidAssertions: invalidAssertions,
216234
ValidAssertions: []Assertion{
217235
{Fn: "Negative", Argsf: "a"},
236+
{Fn: "Negative", Argsf: "f(a)"},
218237
{Fn: "Positive", Argsf: "a"},
238+
{Fn: "Positive", Argsf: "len(x)"},
239+
{Fn: "Positive", Argsf: "f(a)"},
219240
},
220241
IgnoredAssertions: ignoredAssertions,
221242
RealLifeUintExamples: []Assertion{
@@ -237,7 +258,7 @@ func (g NegativePositiveTestsGenerator) TemplateData() any {
237258
},
238259
{
239260
Fn: "Greater", Argsf: "uint64(state.LastUpdatedEpoch), uint64(0)",
240-
ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: "state.LastUpdatedEpoch",
261+
ReportMsgf: report, ProposedFn: "Positive", ProposedArgsf: "uint64(state.LastUpdatedEpoch)",
241262
},
242263
{
243264
Fn: "True", Argsf: `uint64(0) < prod["last_claim_time"].(uint64)`,
@@ -272,7 +293,11 @@ import (
272293
)
273294
274295
func {{ .CheckerName.AsTestName }}(t *testing.T) {
275-
var a int
296+
var (
297+
a int
298+
x []int
299+
f func(d int) int
300+
)
276301
277302
// Invalid.
278303
{

‎internal/testgen/gen_useless_assert.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,12 @@ func (g UselessAssertTestsGenerator) TemplateData() any {
104104
{Fn: "Zero", Argsf: "0", ReportMsgf: defaultReport},
105105
{Fn: "Zero", Argsf: `""`, ReportMsgf: defaultReport},
106106
{Fn: "Zero", Argsf: "nil", ReportMsgf: defaultReport},
107+
108+
{Fn: "GreaterOrEqual", Argsf: "uint(42), 0", ReportMsgf: defaultReport},
109+
{Fn: "LessOrEqual", Argsf: "0, uint(42)", ReportMsgf: defaultReport},
110+
111+
{Fn: "GreaterOrEqual", Argsf: "len(x), 0", ReportMsgf: defaultReport},
112+
{Fn: "LessOrEqual", Argsf: "0, len(x)", ReportMsgf: defaultReport},
107113
},
108114
InvalidAssertions: twoSideAssertions,
109115
ValidAssertions: []Assertion{
@@ -159,6 +165,7 @@ func {{ .CheckerName.AsTestName }}(t *testing.T) {
159165
var num int
160166
var b bool
161167
var tc testCase
168+
var x []int
162169
163170
// Invalid.
164171
{

0 commit comments

Comments
 (0)