Skip to content

Commit 38fb805

Browse files
AntonboomAntonboom
authored
formatter: warn on empty message (#225)
* formatter: warn on empty message * self-review fixes * self-review fixes
1 parent 3418337 commit 38fb805

File tree

9 files changed

+213
-23
lines changed

9 files changed

+213
-23
lines changed

‎README.md

+4-1
Original file line numberDiff line numberDiff line change
@@ -595,14 +595,17 @@ in `v2` of `testify`.
595595
#### 4)
596596

597597
Validating the first argument of `msgAndArgs` has `string` type (based on `testify`'s
598-
maintainer's [feedback](https://github.com/stretchr/testify/issues/1679#issuecomment-2480629257)):
598+
maintainer's [feedback](https://github.com/stretchr/testify/issues/1679#issuecomment-2480629257)) and this string is
599+
not empty:
599600

600601
```go
601602
❌
602603
assert.Equal(t, 1, strings.Count(b.String(), "hello"), tc)
604+
assert.Equalf(t, want, got, "")
603605

604606
✅
605607
assert.Equal(t, 1, strings.Count(b.String(), "hello"), "%+v", tc)
608+
assert.Equal(t, want, got)
606609
```
607610

608611
You can disable this behaviour with the `--formatter.require-string-msg=false` flag.

‎analyzer/testdata/src/checkers-default/formatter/formatter_test.go

+19
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎analyzer/testdata/src/checkers-default/formatter/formatter_test.go.golden

+19
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,25 @@ func TestFormatterChecker_PrintfChecks(t *testing.T) {
124124
assert.Equalf(t, 1, 2, "msg with args %s %s", "42") // want "formatter: assert\\.Equalf format %s reads arg #2, but call has 1 arg$"
125125
}
126126

127+
func TestFormatterChecker_EmptyMessage(t *testing.T) {
128+
var want, got any
129+
assertObj := assert.New(t)
130+
assert.Equal(t, want, got) // want "formatter: empty message"
131+
assertObj.Equal(want, got) // want "formatter: empty message"
132+
assert.Equal(t, want, got) // want "formatter: empty message"
133+
assertObj.Equal(want, got) // want "formatter: empty message"
134+
assert.Equal(t, want, got, "", 1, 2) // want "formatter: empty message"
135+
assertObj.Equal(want, got, "", 1, 2) // want "formatter: empty message"
136+
assert.Equalf(t, want, got, "", 1, 2) // want "formatter: empty message"
137+
assertObj.Equalf(want, got, "", 1, 2) // want "formatter: empty message"
138+
assert.Equal(t, want, got, "boom!")
139+
assertObj.Equal(want, got, "boom!")
140+
assert.Equal(t, want, got, "%v != %v", 1, 2)
141+
assertObj.Equal(want, got, "%v != %v", 1, 2)
142+
assert.Equalf(t, want, got, "%v != %v", 1, 2)
143+
assertObj.Equalf(want, got, "%v != %v", 1, 2)
144+
}
145+
127146
type FormatterCheckerSuite struct {
128147
suite.Suite
129148
}

‎analyzer/testdata/src/formatter-not-defaults/formatter_not_defaults_test.go

+19
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎analyzer/testdata/src/formatter-not-defaults/formatter_not_defaults_test.go.golden

+19
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,25 @@ func TestFormatterChecker(t *testing.T) {
4242
assert.Equalf(t, 1, 2, fmt.Sprintf("msg with arg %d", 42), "42")
4343
}
4444

45+
func TestFormatterChecker_EmptyMessage(t *testing.T) {
46+
var want, got any
47+
assertObj := assert.New(t)
48+
assert.Equal(t, want, got) // want "formatter: empty message"
49+
assertObj.Equal(want, got) // want "formatter: empty message"
50+
assert.Equal(t, want, got) // want "formatter: empty message"
51+
assertObj.Equal(want, got) // want "formatter: empty message"
52+
assert.Equal(t, want, got, "", 1, 2) // want "formatter: empty message"
53+
assertObj.Equal(want, got, "", 1, 2) // want "formatter: empty message"
54+
assert.Equalf(t, want, got, "", 1, 2) // want "formatter: empty message"
55+
assertObj.Equalf(want, got, "", 1, 2) // want "formatter: empty message"
56+
assert.Equalf(t, want, got, "boom!") // want "formatter: use assert\\.Equalf"
57+
assertObj.Equalf(want, got, "boom!") // want "formatter: use assertObj\\.Equalf"
58+
assert.Equalf(t, want, got, "%v != %v", 1, 2) // want "formatter: use assert\\.Equalf"
59+
assertObj.Equalf(want, got, "%v != %v", 1, 2) // want "formatter: use assertObj\\.Equalf"
60+
assert.Equalf(t, want, got, "%v != %v", 1, 2)
61+
assertObj.Equalf(want, got, "%v != %v", 1, 2)
62+
}
63+
4564
func TestFormatterChecker_AllAssertions(t *testing.T) {
4665
assert.Condition(t, nil)
4766
assert.Conditionf(t, nil, "msg") // want "formatter: use assert\\.Conditionf"

‎internal/checkers/formatter.go

+33-6
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package checkers
22

33
import (
4+
"fmt"
45
"go/types"
56
"strconv"
67
"strings"
@@ -28,7 +29,7 @@ import (
2829
// assert.Truef(t, targetTs.Equal(ts), "the timestamp should be as expected (%s) but was %s", targetTs, ts)
2930
//
3031
// It also checks that there are no arguments in `msgAndArgs` if the message is not a string,
31-
// and additionally checks that the first argument of `msgAndArgs` is a string.
32+
// and additionally checks that the first argument of `msgAndArgs` is a not empty string.
3233
//
3334
// Finally, it checks that failure message in Fail and FailNow is not used as a format string (which won't work).
3435
type Formatter struct {
@@ -100,6 +101,17 @@ func (checker Formatter) checkNotFmtAssertion(pass *analysis.Pass, call *CallMet
100101
}
101102

102103
if hasStringType(pass, msgAndArgs) { //nolint:nestif // This is the best option of code organization :(
104+
format, err := strconv.Unquote(analysisutil.NodeString(pass.Fset, msgAndArgs))
105+
if nil == err && format == "" { // Unquote failed for not string literals.
106+
var fixes []analysis.SuggestedFix
107+
if isSingleMsgAndArgElem {
108+
fixes = append(fixes, analysis.SuggestedFix{
109+
Message: "Remove empty message",
110+
TextEdits: []analysis.TextEdit{newRemoveLastArgTextEdit(pass, call.Args)},
111+
})
112+
}
113+
return newDiagnostic(checker.Name(), call, "empty message", fixes...)
114+
}
103115
if checker.requireFFuncs {
104116
return newUseFunctionDiagnostic(checker.Name(), call, call.Fn.Name+"f")
105117
}
@@ -135,25 +147,40 @@ func (checker Formatter) checkFmtAssertion(pass *analysis.Pass, call *CallMeta)
135147

136148
lastArgPos := len(call.ArgsRaw) - 1
137149
msg := call.ArgsRaw[formatPos]
150+
noFormatArgs := formatPos == lastArgPos
138151

139152
if formatPos == lastArgPos {
140153
if args, ok := isFmtSprintfCall(pass, msg); ok {
141154
return newRemoveSprintfDiagnostic(pass, checker.Name(), call, msg, args)
142155
}
143156
}
144157

158+
format, err := strconv.Unquote(analysisutil.NodeString(pass.Fset, msg))
159+
if err != nil {
160+
// Unreachable, because msg is a string literal in formatted assertion.
161+
return nil
162+
}
163+
if format == "" {
164+
var fixes []analysis.SuggestedFix
165+
if noFormatArgs {
166+
fixes = append(fixes, analysis.SuggestedFix{
167+
Message: fmt.Sprintf("Remove empty message and use `%s`", call.Fn.NameFTrimmed),
168+
TextEdits: []analysis.TextEdit{
169+
newReplaceFnTextEdit(call.Fn, call.Fn.NameFTrimmed),
170+
newRemoveLastArgTextEdit(pass, call.Args),
171+
},
172+
})
173+
}
174+
return newDiagnostic(checker.Name(), call, "empty message", fixes...)
175+
}
176+
145177
if checker.checkFormatString {
146178
report := pass.Report
147179
defer func() { pass.Report = report }()
148180

149181
pass.Report = func(d analysis.Diagnostic) {
150182
result = newDiagnostic(checker.Name(), call, d.Message)
151183
}
152-
153-
format, err := strconv.Unquote(analysisutil.NodeString(pass.Fset, msg))
154-
if err != nil {
155-
return nil
156-
}
157184
printf.CheckPrintf(pass, call.Call, call.String(), format, formatPos)
158185
}
159186
return result

‎internal/checkers/helpers_diagnostic.go

+18-8
Original file line numberDiff line numberDiff line change
@@ -131,13 +131,23 @@ func newSuggestedFuncReplacement(
131131
proposedFn += "f"
132132
}
133133
return analysis.SuggestedFix{
134-
Message: fmt.Sprintf("Replace `%s` with `%s`", call.Fn.Name, proposedFn),
135-
TextEdits: append([]analysis.TextEdit{
136-
{
137-
Pos: call.Fn.Pos(),
138-
End: call.Fn.End(),
139-
NewText: []byte(proposedFn),
140-
},
141-
}, additionalEdits...),
134+
Message: fmt.Sprintf("Replace `%s` with `%s`", call.Fn.Name, proposedFn),
135+
TextEdits: append([]analysis.TextEdit{newReplaceFnTextEdit(call.Fn, proposedFn)}, additionalEdits...),
136+
}
137+
}
138+
139+
func newReplaceFnTextEdit(callFn analysis.Range, proposedFn string) analysis.TextEdit {
140+
return analysis.TextEdit{
141+
Pos: callFn.Pos(),
142+
End: callFn.End(),
143+
NewText: []byte(proposedFn),
144+
}
145+
}
146+
147+
func newRemoveLastArgTextEdit(pass *analysis.Pass, callArgs []ast.Expr) analysis.TextEdit {
148+
return analysis.TextEdit{
149+
Pos: callArgs[0].Pos(),
150+
End: callArgs[len(callArgs)-1].End(),
151+
NewText: formatAsCallArgs(pass, callArgs[0:len(callArgs)-1]...),
142152
}
143153
}

‎internal/testgen/gen_formatter.go

+37
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ func (g FormatterTestsGenerator) TemplateData() any {
2424
": using msgAndArgs with non-string first element (msg) causes panic"
2525
reportFailureMsgIsNotFmtString = checker +
2626
": failure message is not a format string, use msgAndArgs instead"
27+
reportEmptyMessage = checker +
28+
": empty message"
2729
)
2830

2931
baseAssertion := Assertion{Fn: "Equal", Argsf: "1, 2"}
@@ -171,6 +173,29 @@ func (g FormatterTestsGenerator) TemplateData() any {
171173
},
172174
}
173175

176+
emptyMsgAssertions := []Assertion{
177+
{
178+
Fn: "Equal", Argsf: `want, got, ""`,
179+
ReportMsgf: reportEmptyMessage, ProposedArgsf: "want, got",
180+
},
181+
{
182+
Fn: "Equalf", Argsf: `want, got, ""`,
183+
ReportMsgf: reportEmptyMessage + "%.s%.s", ProposedFn: "Equal", ProposedArgsf: "want, got",
184+
},
185+
{
186+
Fn: "Equal", Argsf: `want, got, "", 1, 2`,
187+
ReportMsgf: reportEmptyMessage,
188+
},
189+
{
190+
Fn: "Equalf", Argsf: `want, got, "", 1, 2`,
191+
ReportMsgf: reportEmptyMessage,
192+
},
193+
194+
{Fn: "Equal", Argsf: `want, got, "boom!"`},
195+
{Fn: "Equal", Argsf: `want, got, "%v != %v", 1, 2`},
196+
{Fn: "Equalf", Argsf: `want, got, "%v != %v", 1, 2`},
197+
}
198+
174199
assertions := make([]Assertion, 0, len(allAssertions)*5)
175200
for _, a := range allAssertions {
176201
assertions = append(assertions,
@@ -204,13 +229,15 @@ func (g FormatterTestsGenerator) TemplateData() any {
204229
BaseAssertion Assertion
205230
NonStringMsgAssertions []Assertion
206231
SprintfAssertions []Assertion
232+
EmptyMsgAssertions []Assertion
207233
AllAssertions []Assertion
208234
IgnoredAssertions []Assertion
209235
}{
210236
CheckerName: CheckerName(g.Checker().Name()),
211237
BaseAssertion: baseAssertion,
212238
NonStringMsgAssertions: nonStringMsgAssertions,
213239
SprintfAssertions: sprintfAssertions,
240+
EmptyMsgAssertions: emptyMsgAssertions,
214241
AllAssertions: assertions,
215242
IgnoredAssertions: []Assertion{
216243
{Fn: "ObjectsAreEqual", Argsf: "nil, nil"},
@@ -304,6 +331,16 @@ func {{ .CheckerName.AsTestName }}_PrintfChecks(t *testing.T) {
304331
assert.Equalf(t, 1, 2, "msg with args %s %s", "42") // want "formatter: assert\\.Equalf format %s reads arg #2, but call has 1 arg$"
305332
}
306333
334+
func {{ .CheckerName.AsTestName }}_EmptyMessage(t *testing.T) {
335+
var want, got any
336+
assertObj := assert.New(t)
337+
338+
{{- range $ai, $assrn := $.EmptyMsgAssertions }}
339+
{{ NewAssertionExpander.NotFmtSingleMode.Expand $assrn "assert" "t" nil }}
340+
{{ NewAssertionExpander.NotFmtSingleMode.Expand $assrn "assertObj" "" nil }}
341+
{{- end }}
342+
}
343+
307344
{{ $suiteName := .CheckerName.AsSuiteName }}
308345
309346
type {{ $suiteName }} struct {

‎internal/testgen/gen_formatter_not_defaults.go

+45-8
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ func (g FormatterNotDefaultsTestsGenerator) TemplateData() any {
1919
": using msgAndArgs with non-string first element (msg) causes panic"
2020
reportFailureMsgIsNotFmtString = checker +
2121
": failure message is not a format string, use msgAndArgs instead"
22+
reportEmptyMessage = checker +
23+
": empty message"
2224
)
2325

2426
baseAssertions := []Assertion{
@@ -130,6 +132,29 @@ func (g FormatterNotDefaultsTestsGenerator) TemplateData() any {
130132
},
131133
}
132134

135+
emptyMsgAssertions := []Assertion{
136+
{
137+
Fn: "Equal", Argsf: `want, got, ""`,
138+
ReportMsgf: reportEmptyMessage, ProposedArgsf: "want, got",
139+
},
140+
{
141+
Fn: "Equalf", Argsf: `want, got, ""`,
142+
ReportMsgf: reportEmptyMessage + "%.s%.s", ProposedFn: "Equal", ProposedArgsf: "want, got",
143+
},
144+
{
145+
Fn: "Equal", Argsf: `want, got, "", 1, 2`,
146+
ReportMsgf: reportEmptyMessage,
147+
},
148+
{
149+
Fn: "Equalf", Argsf: `want, got, "", 1, 2`,
150+
ReportMsgf: reportEmptyMessage,
151+
},
152+
153+
{Fn: "Equal", Argsf: `want, got, "boom!"`, ReportMsgf: reportUse, ProposedFn: "Equalf"},
154+
{Fn: "Equal", Argsf: `want, got, "%v != %v", 1, 2`, ReportMsgf: reportUse, ProposedFn: "Equalf"},
155+
{Fn: "Equalf", Argsf: `want, got, "%v != %v", 1, 2`},
156+
}
157+
133158
assertions := make([]Assertion, 0, len(allAssertions)*5)
134159
for _, a := range allAssertions {
135160
assertions = append(assertions,
@@ -159,15 +184,17 @@ func (g FormatterNotDefaultsTestsGenerator) TemplateData() any {
159184
}
160185

161186
return struct {
162-
CheckerName CheckerName
163-
BaseAssertions []Assertion
164-
SprintfAssertions []Assertion
165-
AllAssertions []Assertion
187+
CheckerName CheckerName
188+
BaseAssertions []Assertion
189+
SprintfAssertions []Assertion
190+
EmptyMsgAssertions []Assertion
191+
AllAssertions []Assertion
166192
}{
167-
CheckerName: CheckerName(checker),
168-
BaseAssertions: baseAssertions,
169-
SprintfAssertions: sprintfAssertions,
170-
AllAssertions: assertions,
193+
CheckerName: CheckerName(checker),
194+
BaseAssertions: baseAssertions,
195+
SprintfAssertions: sprintfAssertions,
196+
EmptyMsgAssertions: emptyMsgAssertions,
197+
AllAssertions: assertions,
171198
}
172199
}
173200

@@ -210,6 +237,16 @@ func {{ .CheckerName.AsTestName }}(t *testing.T) {
210237
{{- end }}
211238
}
212239
240+
func {{ .CheckerName.AsTestName }}_EmptyMessage(t *testing.T) {
241+
var want, got any
242+
assertObj := assert.New(t)
243+
244+
{{- range $ai, $assrn := $.EmptyMsgAssertions }}
245+
{{ NewAssertionExpander.NotFmtSingleMode.Expand $assrn "assert" "t" nil }}
246+
{{ NewAssertionExpander.NotFmtSingleMode.Expand $assrn "assertObj" "" nil }}
247+
{{- end }}
248+
}
249+
213250
func {{ .CheckerName.AsTestName }}_AllAssertions(t *testing.T) {
214251
{{- range $ai, $assrn := $.AllAssertions }}
215252
{{ NewAssertionExpander.NotFmtSingleMode.Expand $assrn "assert" "t" nil }}

0 commit comments

Comments
 (0)