feature: new rule unnecessary-format to spot unnecessary use of formatting functions (#1372)

README.md

@@ -540,6 +540,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | [`unexported-naming`](./RULES_DESCRIPTIONS.md#unexported-naming)          |  n/a   |  Warns on wrongly named un-exported symbols       |    no    |  no   |
 | [`unexported-return`](./RULES_DESCRIPTIONS.md#unexported-return)   |  n/a   | Warns when a public return is from unexported type.              |   yes    |  yes  |
 | [`unhandled-error`](./RULES_DESCRIPTIONS.md#unhandled-error)   | []string   | Warns on unhandled errors returned by function calls    |    no    |  yes   |
+| [`unnecessary-format`](./RULES_DESCRIPTIONS.md#unnecessary-format)    |  n/a   | Identifies calls to formatting functions where the format string does not contain any formatting verbs          |    no    |  no   |
 | [`unnecessary-stmt`](./RULES_DESCRIPTIONS.md#unnecessary-stmt)    |  n/a   | Suggests removing or simplifying unnecessary statements          |    no    |  no   |
 | [`unreachable-code`](./RULES_DESCRIPTIONS.md#unreachable-code)    |  n/a   | Warns on unreachable code                                        |    no    |  no   |
 | [`unused-parameter`](./RULES_DESCRIPTIONS.md#unused-parameter)    |  n/a   | Suggests to rename or remove unused function parameters          |    no    |  no   |

RULES_DESCRIPTIONS.md

@@ -78,6 +78,7 @@ List of all available rules.
   - [unexported-naming](#unexported-naming)
   - [unexported-return](#unexported-return)
   - [unhandled-error](#unhandled-error)
+  - [unnecessary-format](#unnecessary-format)
   - [unnecessary-stmt](#unnecessary-stmt)
   - [unreachable-code](#unreachable-code)
   - [unused-parameter](#unused-parameter)
@@ -1191,6 +1192,13 @@ arguments = [
 ]
 ```
 
+## unnecessary-format
+
+_Description_: This rule identifies calls to formatting functions where the format string does not contain any formatting verbs
+and recommends switching to the non-formatting, more efficient alternative.
+
+_Configuration_: N/A
+
 ## unnecessary-stmt
 
 _Description_: This rule suggests to remove redundant statements like a `break` at the end of a case block, for improving the code's readability.

config/config.go

@@ -102,6 +102,7 @@ var allRules = append([]lint.Rule{
 	&rule.RedundantBuildTagRule{},
 	&rule.UseErrorsNewRule{},
 	&rule.RedundantTestMainExitRule{},
+	&rule.UnnecessaryFormatRule{},
 }, defaultRules...)
 
 // allFormatters is a list of all available formatters to output the linting results.

internal/astutils/ast_utils.go

@@ -3,6 +3,7 @@ package astutils
 
 import (
 	"go/ast"
+	"go/token"
 	"slices"
 )
 
@@ -76,3 +77,10 @@ func getFieldTypeName(typ ast.Expr) string {
 		return "UNHANDLED_TYPE"
 	}
 }
+
+// IsStringLiteral returns true if the given expression is a string literal, false otherwise
+func IsStringLiteral(e ast.Expr) bool {
+	sl, ok := e.(*ast.BasicLit)
+
+	return ok && sl.Kind == token.STRING
+}

rule/unnecessary_format.go

@@ -0,0 +1,129 @@
+package rule
+
+import (
+	"fmt"
+	"go/ast"
+	"strings"
+
+	"github.com/mgechev/revive/internal/astutils"
+	"github.com/mgechev/revive/lint"
+)
+
+// UnnecessaryFormatRule spots calls to formatting functions without leveraging formatting directives.
+type UnnecessaryFormatRule struct{}
+
+// Apply applies the rule to given file.
+func (*UnnecessaryFormatRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	fileAst := file.AST
+	walker := lintUnnecessaryFormat{
+		onFailure: func(failure lint.Failure) {
+			failures = append(failures, failure)
+		},
+	}
+
+	ast.Walk(walker, fileAst)
+
+	return failures
+}
+
+// Name returns the rule name.
+func (*UnnecessaryFormatRule) Name() string {
+	return "unnecessary-format"
+}
+
+type lintUnnecessaryFormat struct {
+	onFailure func(lint.Failure)
+}
+
+type formattingSpec struct {
+	formatArgPosition byte
+	alternative       string
+}
+
+var formattingFuncs = map[string]formattingSpec{
+	"fmt.Appendf": {1, `"fmt.Append"`},
+	"fmt.Errorf":  {0, `"errors.New"`},
+	"fmt.Fprintf": {1, `"fmt.Fprint"`},
+	"fmt.Fscanf":  {1, `"fmt.Fscan" or "fmt.Fscanln"`},
+	"fmt.Printf":  {0, `"fmt.Print" or "fmt.Println"`},
+	"fmt.Scanf":   {0, `"fmt.Scan"`},
+	"fmt.Sprintf": {0, `"fmt.Sprint" or just the string itself"`},
+	"fmt.Sscanf":  {1, `"fmt.Sscan"`},
+	// standard logging functions
+	"log.Fatalf": {0, `"log.Fatal"`},
+	"log.Panicf": {0, `"log.Panic"`},
+	"log.Printf": {0, `"log.Print"`},
+	// Variable logger possibly being the std logger
+	// Will trigger a false positive if all the following holds:
+	//   1. the variable is not the std logger
+	//   2. the Fatalf/Panicf/Printf method expects a string as first argument
+	//      but the string is not expected to be a format string
+	//   3. the actual first argument is a string literal
+	//   4. the actual first argument does not contain a %
+	"logger.Fatalf": {0, `"logger.Fatal"`},
+	"logger.Panicf": {0, `"logger.Panic"`},
+	"logger.Printf": {0, `"logger.Print"`},
+	// standard testing functions
+	// Variable t possibly being a testing.T
+	// False positive risk: see comment on logger
+	"t.Errorf": {0, `"t.Error"`},
+	"t.Fatalf": {0, `"t.Fatal"`},
+	"t.Logf":   {0, `"t.Log"`},
+	"t.Skipf":  {0, `"t.Skip"`},
+	// Variable b possibly being a testing.B
+	// False positive risk: see comment on logger
+	"b.Errorf": {0, `"b.Error"`},
+	"b.Fatalf": {0, `"b.Fatal"`},
+	"b.Logf":   {0, `"b.Log"`},
+	"b.Skipf":  {0, `"b.Skip"`},
+	// Variable f possibly being a testing.F
+	// False positive risk: see comment on logger
+	"f.Errorf": {0, `"f.Error"`},
+	"f.Fatalf": {0, `"f.Fatal"`},
+	"f.Logf":   {0, `"f.Log"`},
+	"f.Skipf":  {0, `"f.Skip"`},
+	// standard trace functions
+	"trace.Logf": {2, `"trace.Log"`},
+}
+
+func (w lintUnnecessaryFormat) Visit(n ast.Node) ast.Visitor {
+	ce, ok := n.(*ast.CallExpr)
+	if !ok || len(ce.Args) < 1 {
+		return w
+	}
+
+	funcName := gofmt(ce.Fun)
+	spec, ok := formattingFuncs[funcName]
+	if !ok {
+		return w
+	}
+
+	pos := int(spec.formatArgPosition)
+	if len(ce.Args) <= pos {
+		return w // not enough params /!\
+	}
+
+	arg := ce.Args[pos]
+	if !astutils.IsStringLiteral(arg) {
+		return w
+	}
+
+	format := gofmt(arg)
+
+	if strings.Contains(format, `%`) {
+		return w
+	}
+
+	failure := lint.Failure{
+		Category:   lint.FailureCategoryOptimization,
+		Node:       ce.Fun,
+		Confidence: 0.8,
+		Failure:    fmt.Sprintf("unnecessary use of formatting function %q, you can replace it with %s", funcName, spec.alternative),
+	}
+
+	w.onFailure(failure)
+
+	return w
+}

test/unnecessary_format_test.go

@@ -0,0 +1,12 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/lint"
+	"github.com/mgechev/revive/rule"
+)
+
+func TestUnnecessaryFormat(t *testing.T) {
+	testRule(t, "unnecessary_format", &rule.UnnecessaryFormatRule{}, &lint.RuleConfig{})
+}

testdata/unnecessary_format.go

@@ -0,0 +1,81 @@
+package fixtures
+
+import (
+	"fmt"
+	"log"
+	"runtime/trace"
+	"testing"
+)
+
+func unnecessaryFormat(t *testing.T, b *testing.B, f *testing.F) {
+	logger := log.New(nil, "", 0)
+
+	fmt.Appendf(nil, "no format") // MATCH /unnecessary use of formatting function "fmt.Appendf", you can replace it with "fmt.Append"/
+	fmt.Errorf("no format")       // MATCH /unnecessary use of formatting function "fmt.Errorf", you can replace it with "errors.New"/
+	fmt.Fprintf(nil, "no format") // MATCH /unnecessary use of formatting function "fmt.Fprintf", you can replace it with "fmt.Fprint"/
+	fmt.Fscanf(nil, "no format")  // MATCH /unnecessary use of formatting function "fmt.Fscanf", you can replace it with "fmt.Fscan" or "fmt.Fscanln"/
+	fmt.Printf("no format")       // MATCH /unnecessary use of formatting function "fmt.Printf", you can replace it with "fmt.Print" or "fmt.Println"/
+	fmt.Scanf("no format")        // MATCH /unnecessary use of formatting function "fmt.Scanf", you can replace it with "fmt.Scan"/
+	fmt.Sprintf("no format")      // MATCH /unnecessary use of formatting function "fmt.Sprintf", you can replace it with "fmt.Sprint" or just the string itself"/
+	fmt.Sscanf("", "no format")   // MATCH /unnecessary use of formatting function "fmt.Sscanf", you can replace it with "fmt.Sscan"/
+	// standard logging functions
+	log.Fatalf("no format")    // MATCH /unnecessary use of formatting function "log.Fatalf", you can replace it with "log.Fatal"/
+	log.Panicf("no format")    // MATCH /unnecessary use of formatting function "log.Panicf", you can replace it with "log.Panic"/
+	log.Printf("no format")    // MATCH /unnecessary use of formatting function "log.Printf", you can replace it with "log.Print"/
+	logger.Fatalf("no format") // MATCH /unnecessary use of formatting function "logger.Fatalf", you can replace it with "logger.Fatal"/
+	logger.Panicf("no format") // MATCH /unnecessary use of formatting function "logger.Panicf", you can replace it with "logger.Panic"/
+	logger.Printf("no format") // MATCH /unnecessary use of formatting function "logger.Printf", you can replace it with "logger.Print"/
+	// standard testing functions
+	t.Errorf("no format") // MATCH /unnecessary use of formatting function "t.Errorf", you can replace it with "t.Error"/
+	t.Fatalf("no format") // MATCH /unnecessary use of formatting function "t.Fatalf", you can replace it with "t.Fatal"/
+	t.Logf("no format")   // MATCH /unnecessary use of formatting function "t.Logf", you can replace it with "t.Log"/
+	t.Skipf("no format")  // MATCH /unnecessary use of formatting function "t.Skipf", you can replace it with "t.Skip"/
+	b.Errorf("no format") // MATCH /unnecessary use of formatting function "b.Errorf", you can replace it with "b.Error"/
+	b.Fatalf("no format") // MATCH /unnecessary use of formatting function "b.Fatalf", you can replace it with "b.Fatal"/
+	b.Logf("no format")   // MATCH /unnecessary use of formatting function "b.Logf", you can replace it with "b.Log"/
+	b.Skipf("no format")  // MATCH /unnecessary use of formatting function "b.Skipf", you can replace it with "b.Skip"/
+	f.Errorf("no format") // MATCH /unnecessary use of formatting function "f.Errorf", you can replace it with "f.Error"/
+	f.Fatalf("no format") // MATCH /unnecessary use of formatting function "f.Fatalf", you can replace it with "f.Fatal"/
+	f.Logf("no format")   // MATCH /unnecessary use of formatting function "f.Logf", you can replace it with "f.Log"/
+	f.Skipf("no format")  // MATCH /unnecessary use of formatting function "f.Skipf", you can replace it with "f.Skip"/
+	// standard trace functions
+	trace.Logf(nil, "http", "no format", nil) // MATCH /unnecessary use of formatting function "trace.Logf", you can replace it with "trace.Log"/
+
+	fmt.Appendf(nil, "format %d", 0)
+	fmt.Errorf("format %d", 0)
+	fmt.Fprintf(nil, "format %d", 0)
+	fmt.Fscanf(nil, "format %d", 0)
+	fmt.Printf("format %d", 0)
+	fmt.Scanf("format %d", 0)
+	fmt.Sprintf("format %d", 0)
+	fmt.Sscanf("", "format %d", 0)
+	// standard logging functions
+	log.Fatalf("format %d", 0)
+	log.Panicf("format %d", 0)
+	log.Printf("format %d", 0)
+	logger.Fatalf("format %d", 0)
+	logger.Panicf("format %d", 0)
+	logger.Printf("format %d", 0)
+	// standard testing functions
+	t.Errorf("format %d", 0)
+	t.Fatalf("format %d", 0)
+	t.Logf("format %d", 0)
+	t.Skipf("format %d", 0)
+	b.Errorf("format %d", 0)
+	b.Fatalf("format %d", 0)
+	b.Logf("format %d", 0)
+	b.Skipf("format %d", 0)
+	f.Errorf("format %d", 0)
+	f.Fatalf("format %d", 0)
+	f.Logf("format %d", 0)
+	f.Skipf("format %d", 0)
+	// standard trace functions
+	trace.Logf(nil, "http", "format %d", nil)
+
+	// test with multiline string argument
+	// MATCH:77 /unnecessary use of formatting function "fmt.Appendf", you can replace it with "fmt.Append"/
+	fmt.Appendf(nil, `no 
+		format`)
+	fmt.Appendf(nil, `format 
+	%d`, 0)
+}