Skip to content

Commit

Permalink
empty: adjust report for NotEmpty+len cases (#189)
Browse files Browse the repository at this point in the history 
* empty: adjust report for NotEmpty+len cases

* refactoring of diagnostic helpers

* self-review fixes

* self-review fixes
  • Loading branch information
@Antonboom
Antonboom authored Oct 3, 2024
1 parent 8b28677 commit 44419da
Show file tree
Hide file tree
Showing 26 changed files with 218 additions and 166 deletions.
16 changes: 12 additions & 4 deletions analyzer/testdata/src/checkers-default/empty/empty_test.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

112 changes: 60 additions & 52 deletions analyzer/testdata/src/checkers-default/empty/empty_test.go.golden
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,40 +14,44 @@ func TestEmptyChecker(t *testing.T) {
// assert.Empty cases.
{
// Invalid.
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, []string{"e"}) // want "empty: use assert\\.Empty"
assert.Emptyf(t, []string{"e"}, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: remove unnecessary len"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: remove unnecessary len"
assert.Empty(t, []string{"e"}) // want "empty: remove unnecessary len"
assert.Emptyf(t, []string{"e"}, "msg with args %d %s", 42, "42") // want "empty: remove unnecessary len"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"
assert.Empty(t, elems) // want "empty: use assert\\.Empty"
assert.Emptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.Emptyf"

// Valid.
assert.Empty(t, elems)
Expand All @@ -57,24 +61,28 @@ func TestEmptyChecker(t *testing.T) {
// assert.NotEmpty cases.
{
// Invalid.
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, []string{"e"}) // want "empty: use assert\\.NotEmpty"
assert.NotEmptyf(t, []string{"e"}, "msg with args %d %s", 42, "42") // want "empty: use assert\\.NotEmptyf"
assert.NotEmpty(t, elems) // want "empty: remove unnecessary len"
assert.NotEmptyf(t, elems, "msg with args %d %s", 42, "42") // want "empty: remove unnecessary len"
assert.NotEmpty(t, []string{"e"}) // want "empty: remove unnecessary len"
assert.NotEmptyf(t, []string{"e"}, "msg with args %d %s", 42, "42") // want "empty: remove unnecessary len"

// Valid.
assert.NotEmpty(t, elems)
Expand Down
2 changes: 1 addition & 1 deletion internal/checkers/blank_import.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func (checker BlankImport) Check(pass *analysis.Pass, _ *inspector.Inspector) (d
}

msg := fmt.Sprintf("avoid blank import of %s as it does nothing", pkg)
diagnostics = append(diagnostics, *newDiagnostic(checker.Name(), imp, msg, nil))
diagnostics = append(diagnostics, *newDiagnostic(checker.Name(), imp, msg))
}
}
return diagnostics
Expand Down
14 changes: 6 additions & 8 deletions internal/checkers/bool_compare.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,11 @@ func (checker BoolCompare) Check(pass *analysis.Pass, call *CallMeta) *analysis.
}
survivingArg = newBoolCast(survivingArg)
}
return newUseFunctionDiagnostic(checker.Name(), call, proposed,
newSuggestedFuncReplacement(call, proposed, analysis.TextEdit{
Pos: replaceStart,
End: replaceEnd,
NewText: analysisutil.NodeBytes(pass.Fset, survivingArg),
}),
)
return newUseFunctionDiagnostic(checker.Name(), call, proposed, analysis.TextEdit{
Pos: replaceStart,
End: replaceEnd,
NewText: analysisutil.NodeBytes(pass.Fset, survivingArg),
})
}

newUseTrueDiagnostic := func(survivingArg ast.Expr, replaceStart, replaceEnd token.Pos) *analysis.Diagnostic {
Expand All @@ -74,7 +72,7 @@ func (checker BoolCompare) Check(pass *analysis.Pass, call *CallMeta) *analysis.
survivingArg = newBoolCast(survivingArg)
}
return newDiagnostic(checker.Name(), call, "need to simplify the assertion",
&analysis.SuggestedFix{
analysis.SuggestedFix{
Message: "Simplify the assertion",
TextEdits: []analysis.TextEdit{{
Pos: replaceStart,
Expand Down
5 changes: 2 additions & 3 deletions internal/checkers/compares.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,11 @@ func (checker Compares) Check(pass *analysis.Pass, call *CallMeta) *analysis.Dia

a, b := be.X, be.Y
return newUseFunctionDiagnostic(checker.Name(), call, proposedFn,
newSuggestedFuncReplacement(call, proposedFn, analysis.TextEdit{
analysis.TextEdit{
Pos: be.X.Pos(),
End: be.Y.End(),
NewText: formatAsCallArgs(pass, a, b),
}),
)
})
}

var tokenToProposedFnInsteadOfTrue = map[token.Token]string{
Expand Down
5 changes: 2 additions & 3 deletions internal/checkers/contains.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,9 @@ func (checker Contains) Check(pass *analysis.Pass, call *CallMeta) *analysis.Dia
}

return newUseFunctionDiagnostic(checker.Name(), call, proposed,
newSuggestedFuncReplacement(call, proposed, analysis.TextEdit{
analysis.TextEdit{
Pos: call.Args[0].Pos(),
End: call.Args[0].End(),
NewText: formatAsCallArgs(pass, ce.Args[0], ce.Args[1]),
}),
)
})
}
34 changes: 20 additions & 14 deletions internal/checkers/empty.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,25 +53,28 @@ func (checker Empty) checkEmpty(pass *analysis.Pass, call *CallMeta) *analysis.D
newUseEmptyDiagnostic := func(replaceStart, replaceEnd token.Pos, replaceWith ast.Expr) *analysis.Diagnostic {
const proposed = "Empty"
return newUseFunctionDiagnostic(checker.Name(), call, proposed,
newSuggestedFuncReplacement(call, proposed, analysis.TextEdit{
analysis.TextEdit{
Pos: replaceStart,
End: replaceEnd,
NewText: analysisutil.NodeBytes(pass.Fset, replaceWith),
}),
)
})
}

if len(call.Args) == 0 {
return nil
}

a := call.Args[0]

switch call.Fn.NameFTrimmed {
case "Zero", "Empty":
lenArg, ok := isBuiltinLenCall(pass, a)
if ok {
case "Zero":
if lenArg, ok := isBuiltinLenCall(pass, a); ok {
return newUseEmptyDiagnostic(a.Pos(), a.End(), lenArg)
}

case "Empty":
if lenArg, ok := isBuiltinLenCall(pass, a); ok {
return newRemoveLenDiagnostic(pass, checker.Name(), call, a, lenArg)
}
}

if len(call.Args) < 2 {
Expand Down Expand Up @@ -120,25 +123,28 @@ func (checker Empty) checkNotEmpty(pass *analysis.Pass, call *CallMeta) *analysi
newUseNotEmptyDiagnostic := func(replaceStart, replaceEnd token.Pos, replaceWith ast.Expr) *analysis.Diagnostic {
const proposed = "NotEmpty"
return newUseFunctionDiagnostic(checker.Name(), call, proposed,
newSuggestedFuncReplacement(call, proposed, analysis.TextEdit{
analysis.TextEdit{
Pos: replaceStart,
End: replaceEnd,
NewText: analysisutil.NodeBytes(pass.Fset, replaceWith),
}),
)
})
}

if len(call.Args) == 0 {
return nil
}

a := call.Args[0]

switch call.Fn.NameFTrimmed {
case "NotZero", "NotEmpty", "Positive":
lenArg, ok := isBuiltinLenCall(pass, a)
if ok {
case "NotZero", "Positive":
if lenArg, ok := isBuiltinLenCall(pass, a); ok {
return newUseNotEmptyDiagnostic(a.Pos(), a.End(), lenArg)
}

case "NotEmpty":
if lenArg, ok := isBuiltinLenCall(pass, a); ok {
return newRemoveLenDiagnostic(pass, checker.Name(), call, a, lenArg)
}
}

if len(call.Args) < 2 {
Expand Down
8 changes: 5 additions & 3 deletions internal/checkers/encoded_compare.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,17 @@ func (checker EncodedCompare) Check(pass *analysis.Pass, call *CallMeta) *analys

if proposed != "" {
return newUseFunctionDiagnostic(checker.Name(), call, proposed,
newSuggestedFuncReplacement(call, proposed, analysis.TextEdit{
analysis.TextEdit{
Pos: lhs.Pos(),
End: lhs.End(),
NewText: formatWithStringCastForBytes(pass, a),
}, analysis.TextEdit{
},
analysis.TextEdit{
Pos: rhs.Pos(),
End: rhs.End(),
NewText: formatWithStringCastForBytes(pass, b),
}))
},
)
}
return nil
}
Expand Down
Loading

0 comments on commit 44419da

Please sign in to comment.