simple: flag unnecessary uses of fmt.Sprint

Closes gh-143

simple/analysis.go

@@ -137,4 +137,12 @@ var Analyzers = lintutil.InitializeAnalyzers(Docs, map[string]*analysis.Analyzer
 		Run:      CheckElaborateSleep,
 		Requires: []*analysis.Analyzer{inspect.Analyzer, facts.Generated},
 	},
+	"S1038": {
+		Run:      CheckPrintSprintf,
+		Requires: []*analysis.Analyzer{inspect.Analyzer, facts.Generated},
+	},
+	"S1039": {
+		Run:      CheckSprintLiteral,
+		Requires: []*analysis.Analyzer{inspect.Analyzer, facts.Generated},
+	},
 })

simple/doc.go

@@ -470,4 +470,14 @@ from the result of time.After is a very elaborate way of sleeping that
 can much simpler be expressed with a simple call to time.Sleep.`,
 		Since: "Unreleased",
 	},
+
+	"S1038": {
+		Title: "Unnecessarily complex way of printing formatted string",
+		Since: "Unreleased",
+	},
+
+	"S1039": {
+		Title: "Unnecessary use of fmt.Sprint",
+		Since: "Unreleased",
+	},
 }

simple/lint.go

@@ -1786,3 +1786,83 @@ func CheckElaborateSleep(pass *analysis.Pass) (interface{}, error) {
 	code.Preorder(pass, fn, (*ast.SelectStmt)(nil))
 	return nil, nil
 }
+
+var checkPrintSprintQ = pattern.MustParse(`
+	(Or
+		(CallExpr
+			fn@(Or
+				(Function "fmt.Print")
+				(Function "fmt.Sprint")
+				(Function "fmt.Println")
+				(Function "fmt.Sprintln"))
+			[(CallExpr (Function "fmt.Sprintf") f:_)])
+		(CallExpr
+			fn@(Or
+				(Function "fmt.Fprint")
+				(Function "fmt.Fprintln"))
+			[_ (CallExpr (Function "fmt.Sprintf") f:_)]))`)
+
+func CheckPrintSprintf(pass *analysis.Pass) (interface{}, error) {
+	fn := func(node ast.Node) {
+		m, ok := Match(pass, checkPrintSprintQ, node)
+		if !ok {
+			return
+		}
+
+		name := m.State["fn"].(*types.Func).Name()
+		var msg string
+		switch name {
+		case "Print", "Fprint", "Sprint":
+			newname := name + "f"
+			msg = fmt.Sprintf("should use fmt.%s instead of fmt.%s(fmt.Sprintf(...))", newname, name)
+		case "Println", "Fprintln", "Sprintln":
+			if _, ok := m.State["f"].(*ast.BasicLit); !ok {
+				// This may be an instance of
+				// fmt.Println(fmt.Sprintf(arg, ...)) where arg is an
+				// externally provided format string and the caller
+				// cannot guarantee that the format string ends with a
+				// newline.
+				return
+			}
+			newname := name[:len(name)-2] + "f"
+			msg = fmt.Sprintf("should use fmt.%s instead of fmt.%s(fmt.Sprintf(...)) (but don't forget the newline)", newname, name)
+		}
+		report.Report(pass, node, msg,
+			report.FilterGenerated())
+	}
+	code.Preorder(pass, fn, (*ast.CallExpr)(nil))
+	return nil, nil
+}
+
+var checkSprintLiteralQ = pattern.MustParse(`
+	(CallExpr
+		fn@(Or
+			(Function "fmt.Sprint")
+			(Function "fmt.Sprintf"))
+		[lit@(BasicLit "STRING" _)])`)
+
+func CheckSprintLiteral(pass *analysis.Pass) (interface{}, error) {
+	// We only flag calls with string literals, not expressions of
+	// type string, because some people use fmt.Sprint(s) as a pattern
+	// for copying strings, which may be useful when extracing a small
+	// substring from a large string.
+	fn := func(node ast.Node) {
+		m, ok := Match(pass, checkSprintLiteralQ, node)
+		if !ok {
+			return
+		}
+		callee := m.State["fn"].(*types.Func)
+		lit := m.State["lit"].(*ast.BasicLit)
+		if callee.Name() == "Sprintf" {
+			if strings.ContainsRune(lit.Value, '%') {
+				// This might be a format string
+				return
+			}
+		}
+		report.Report(pass, node, fmt.Sprintf("unnecessary use of fmt.%s", callee.Name()),
+			report.FilterGenerated(),
+			report.Fixes(edit.Fix("Replace with string literal", edit.ReplaceWithNode(pass.Fset, node, lit))))
+	}
+	code.Preorder(pass, fn, (*ast.CallExpr)(nil))
+	return nil, nil
+}

simple/lint_test.go

@@ -40,6 +40,8 @@ func TestAll(t *testing.T) {
 		"S1035": {{Dir: "CheckRedundantCanonicalHeaderKey"}},
 		"S1036": {{Dir: "CheckUnnecessaryGuard"}},
 		"S1037": {{Dir: "CheckElaborateSleep"}},
+		"S1038": {{Dir: "CheckPrintSprintf"}},
+		"S1039": {{Dir: "CheckSprintLiteral"}},
 	}
 
 	testutil.Run(t, Analyzers, checks)

simple/testdata/src/CheckPrintSprintf/CheckPrintSprintf.go

@@ -0,0 +1,15 @@
+package pkg
+
+import "fmt"
+
+func fn() {
+	fmt.Print(fmt.Sprintf("%d", 1))         // want `should use fmt\.Printf`
+	fmt.Println(fmt.Sprintf("%d", 1))       // want `don't forget the newline`
+	fmt.Fprint(nil, fmt.Sprintf("%d", 1))   // want `should use fmt\.Fprintf`
+	fmt.Fprintln(nil, fmt.Sprintf("%d", 1)) // want `don't forget the newline`
+	fmt.Sprint(fmt.Sprintf("%d", 1))        // want `should use fmt\.Sprintf`
+	fmt.Sprintln(fmt.Sprintf("%d", 1))      // want `don't forget the newline`
+
+	arg := "%d"
+	fmt.Println(fmt.Sprintf(arg, 1))
+}

simple/testdata/src/CheckSprintLiteral/CheckSprintLiteral.go

@@ -0,0 +1,13 @@
+package pkg
+
+import "fmt"
+
+func fn() {
+	fmt.Sprint("foo")  // want `unnecessary use of fmt\.Sprint`
+	fmt.Sprintf("foo") // want `unnecessary use of fmt\.Sprintf`
+	fmt.Sprintf("foo %d")
+	fmt.Sprintf("foo %d", 1)
+
+	var x string
+	fmt.Sprint(x)
+}