Skip to content

Commit da78760

Browse files
dominikhdominikh
committed
S1025: don't simplify for types that are formatters
Closes gh-898
1 parent e9f5151 commit da78760

File tree

3 files changed

+82
-14
lines changed

3 files changed

+82
-14
lines changed

‎simple/lint.go

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1407,6 +1407,31 @@ func isStringer(T types.Type, msCache *gotypeutil.MethodSetCache) bool {
14071407
return true
14081408
}
14091409

1410+
func isFormatter(T types.Type, msCache *gotypeutil.MethodSetCache) bool {
1411+
// TODO(dh): this function also exists in staticcheck/lint.go – deduplicate.
1412+
1413+
ms := msCache.MethodSet(T)
1414+
sel := ms.Lookup(nil, "Format")
1415+
if sel == nil {
1416+
return false
1417+
}
1418+
fn, ok := sel.Obj().(*types.Func)
1419+
if !ok {
1420+
// should be unreachable
1421+
return false
1422+
}
1423+
sig := fn.Type().(*types.Signature)
1424+
if sig.Params().Len() != 2 {
1425+
return false
1426+
}
1427+
// TODO(dh): check the types of the arguments for more
1428+
// precision
1429+
if sig.Results().Len() != 0 {
1430+
return false
1431+
}
1432+
return true
1433+
}
1434+
14101435
var checkRedundantSprintfQ = pattern.MustParse(`(CallExpr (Function "fmt.Sprintf") [format arg])`)
14111436

14121437
func CheckRedundantSprintf(pass *analysis.Pass) (interface{}, error) {
@@ -1425,9 +1450,20 @@ func CheckRedundantSprintf(pass *analysis.Pass) (interface{}, error) {
14251450
return
14261451
}
14271452
typ := pass.TypesInfo.TypeOf(arg)
1428-
14291453
irpkg := pass.ResultOf[buildir.Analyzer].(*buildir.IR).Pkg
1430-
if types.TypeString(typ, nil) != "reflect.Value" && isStringer(typ, &irpkg.Prog.MethodSets) {
1454+
1455+
if types.TypeString(typ, nil) == "reflect.Value" {
1456+
// printing with %s produces output different from using
1457+
// the String method
1458+
return
1459+
}
1460+
1461+
if isFormatter(typ, &irpkg.Prog.MethodSets) {
1462+
// the type may choose to handle %s in arbitrary ways
1463+
return
1464+
}
1465+
1466+
if isStringer(typ, &irpkg.Prog.MethodSets) {
14311467
replacement := &ast.CallExpr{
14321468
Fun: &ast.SelectorExpr{
14331469
X: arg,
@@ -1436,20 +1472,18 @@ func CheckRedundantSprintf(pass *analysis.Pass) (interface{}, error) {
14361472
}
14371473
report.Report(pass, node, "should use String() instead of fmt.Sprintf",
14381474
report.Fixes(edit.Fix("replace with call to String method", edit.ReplaceWithNode(pass.Fset, node, replacement))))
1475+
} else if typ == types.Universe.Lookup("string").Type() {
1476+
report.Report(pass, node, "the argument is already a string, there's no need to use fmt.Sprintf",
1477+
report.FilterGenerated(),
1478+
report.Fixes(edit.Fix("remove unnecessary call to fmt.Sprintf", edit.ReplaceWithNode(pass.Fset, node, arg))))
14391479
} else if typ.Underlying() == types.Universe.Lookup("string").Type() {
1440-
if typ == types.Universe.Lookup("string").Type() {
1441-
report.Report(pass, node, "the argument is already a string, there's no need to use fmt.Sprintf",
1442-
report.FilterGenerated(),
1443-
report.Fixes(edit.Fix("remove unnecessary call to fmt.Sprintf", edit.ReplaceWithNode(pass.Fset, node, arg))))
1444-
} else {
1445-
replacement := &ast.CallExpr{
1446-
Fun: &ast.Ident{Name: "string"},
1447-
Args: []ast.Expr{arg},
1448-
}
1449-
report.Report(pass, node, "the argument's underlying type is a string, should use a simple conversion instead of fmt.Sprintf",
1450-
report.FilterGenerated(),
1451-
report.Fixes(edit.Fix("replace with conversion to string", edit.ReplaceWithNode(pass.Fset, node, replacement))))
1480+
replacement := &ast.CallExpr{
1481+
Fun: &ast.Ident{Name: "string"},
1482+
Args: []ast.Expr{arg},
14521483
}
1484+
report.Report(pass, node, "the argument's underlying type is a string, should use a simple conversion instead of fmt.Sprintf",
1485+
report.FilterGenerated(),
1486+
report.Fixes(edit.Fix("replace with conversion to string", edit.ReplaceWithNode(pass.Fset, node, replacement))))
14531487
} else if slice, ok := typ.Underlying().(*types.Slice); ok && slice.Elem() == types.Universe.Lookup("byte").Type() {
14541488
// Note that we check slice.Elem(), not slice.Elem().Underlying, because of https://github.com/golang/go/issues/23536
14551489
replacement := &ast.CallExpr{

‎simple/testdata/src/CheckRedundantSprintf/LintRedundantSprintf.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,22 @@ type T6 string
1111
type T7 []byte
1212
type T8 []MyByte
1313

14+
type T9 string
15+
type T10 []byte
16+
type T11 int
17+
1418
type MyByte byte
1519

1620
func (T3) String() string { return "" }
1721
func (T6) String() string { return "" }
1822
func (T4) String(arg int) string { return "" }
1923
func (T5) String() {}
2024

25+
func (T9) Format(f fmt.State, c rune) {}
26+
func (T10) Format(f fmt.State, c rune) {}
27+
func (T11) Format(f fmt.State, c rune) {}
28+
func (T11) String() string { return "" }
29+
2130
func fn() {
2231
var t1 T1
2332
var t2 T2
@@ -27,6 +36,9 @@ func fn() {
2736
var t6 T6
2837
var t7 T7
2938
var t8 T8
39+
var t9 T9
40+
var t10 T10
41+
var t11 T11
3042
_ = fmt.Sprintf("%s", "test") // want `is already a string`
3143
_ = fmt.Sprintf("%s", t1) // want `is a string`
3244
_ = fmt.Sprintf("%s", t2) // want `is a string`
@@ -39,4 +51,9 @@ func fn() {
3951
_ = fmt.Sprintf("%s", t6) // want `should use String\(\) instead of fmt\.Sprintf`
4052
_ = fmt.Sprintf("%s", t7) // want `underlying type is a slice of bytes`
4153
_ = fmt.Sprintf("%s", t8)
54+
55+
// don't simplify types that implement fmt.Formatter
56+
_ = fmt.Sprintf("%s", t9)
57+
_ = fmt.Sprintf("%s", t10)
58+
_ = fmt.Sprintf("%s", t11)
4259
}

‎simple/testdata/src/CheckRedundantSprintf/LintRedundantSprintf.go.golden

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,22 @@ type T6 string
1111
type T7 []byte
1212
type T8 []MyByte
1313

14+
type T9 string
15+
type T10 []byte
16+
type T11 int
17+
1418
type MyByte byte
1519

1620
func (T3) String() string { return "" }
1721
func (T6) String() string { return "" }
1822
func (T4) String(arg int) string { return "" }
1923
func (T5) String() {}
2024

25+
func (T9) Format(f fmt.State, c rune) {}
26+
func (T10) Format(f fmt.State, c rune) {}
27+
func (T11) Format(f fmt.State, c rune) {}
28+
func (T11) String() string { return "" }
29+
2130
func fn() {
2231
var t1 T1
2332
var t2 T2
@@ -27,6 +36,9 @@ func fn() {
2736
var t6 T6
2837
var t7 T7
2938
var t8 T8
39+
var t9 T9
40+
var t10 T10
41+
var t11 T11
3042
_ = "test" // want `is already a string`
3143
_ = string(t1) // want `is a string`
3244
_ = string(t2) // want `is a string`
@@ -39,4 +51,9 @@ func fn() {
3951
_ = t6.String() // want `should use String\(\) instead of fmt\.Sprintf`
4052
_ = string(t7) // want `underlying type is a slice of bytes`
4153
_ = fmt.Sprintf("%s", t8)
54+
55+
// don't simplify types that implement fmt.Formatter
56+
_ = fmt.Sprintf("%s", t9)
57+
_ = fmt.Sprintf("%s", t10)
58+
_ = fmt.Sprintf("%s", t11)
4259
}

0 commit comments

Comments
 (0)