analyzer: add options to cover all cases (#42)

* analyzer: add options to cover all cases

integer-format
error-format
bool-format
hex-format
string-format

Author: catenacyber

README.md

@@ -17,7 +17,23 @@ go install github.com/catenacyber/perfsprint@latest
 perfsprint --fix ./...
 ```
 
-To disable int/uint cast, you can use the flag `-int-conversion=false`
+## Options
+
+The 5 following options cover all optimizations proposed by the linter.
+
+Some have suboptions for specific cases.
+
+- integer-format (formatting integer with the package `strconv`)
+    - int-conversion : disable when the optimization adds a int/uint cast (readability)
+- error-format (formatting errors)
+    - errorf : known behavior change avoiding panic
+    - err-error : known behavior change panicking for nil errors
+- string-format (formatting strings)
+   - sprintf1 : known behavior change avoiding panic
+   - strconcat : disable turning some `fmt.Sprintf` to a string concatenation (readability)
+- bool-format (formatting bool with `strconv.FormatBool`)
+- hex-format (formatting bytes with `hex.EncodeToString`)
+
 
 To disable `fmt.Errorf` optimization, you can use the flag `-errorf=false`
 This optimization is not always equivalent.

analyzer/analyzer.go

@@ -16,23 +16,41 @@ import (
 	"golang.org/x/tools/go/analysis"
 )
 
+type optionInt struct {
+	enabled bool
+	intConv bool
+}
+
+type optionErr struct {
+	enabled  bool
+	errError bool
+	errorf   bool
+}
+
+type optionStr struct {
+	enabled   bool
+	sprintf1  bool
+	strconcat bool
+}
+
 type perfSprint struct {
-	intConv    bool
-	errError   bool
-	errorf     bool
-	sprintf1   bool
+	intFormat optionInt
+	errFormat optionErr
+	strFormat optionStr
+
+	boolFormat bool
+	hexFormat  bool
 	fiximports bool
-	strconcat  bool
 }
 
 func newPerfSprint() *perfSprint {
 	return &perfSprint{
-		intConv:    true,
-		errError:   false,
-		errorf:     true,
-		sprintf1:   true,
+		intFormat:  optionInt{enabled: true, intConv: true},
+		errFormat:  optionErr{enabled: true, errError: false, errorf: true},
+		strFormat:  optionStr{enabled: true, sprintf1: true, strconcat: true},
+		boolFormat: true,
+		hexFormat:  true,
 		fiximports: true,
-		strconcat:  true,
 	}
 }
 
@@ -44,12 +62,31 @@ func New() *analysis.Analyzer {
 		Run:      n.run,
 		Requires: []*analysis.Analyzer{inspect.Analyzer},
 	}
-	r.Flags.BoolVar(&n.intConv, "int-conversion", true, "optimizes even if it requires an int or uint type cast")
-	r.Flags.BoolVar(&n.errError, "err-error", false, "optimizes into err.Error() even if it is only equivalent for non-nil errors")
-	r.Flags.BoolVar(&n.errorf, "errorf", true, "optimizes fmt.Errorf")
-	r.Flags.BoolVar(&n.sprintf1, "sprintf1", true, "optimizes fmt.Sprintf with only one argument")
+	r.Flags.BoolVar(&n.intFormat.enabled, "integer-format", true, "enable/disable optimization of integer formatting")
+	r.Flags.BoolVar(&n.intFormat.intConv, "int-conversion", true, "optimizes even if it requires an int or uint type cast")
+	r.Flags.BoolVar(&n.errFormat.enabled, "error-format", true, "enable/disable optimization of error formatting")
+	r.Flags.BoolVar(&n.errFormat.errError, "err-error", false, "optimizes into err.Error() even if it is only equivalent for non-nil errors")
+	r.Flags.BoolVar(&n.errFormat.errorf, "errorf", true, "optimizes fmt.Errorf")
+	r.Flags.BoolVar(&n.boolFormat, "bool-format", true, "enable/disable optimization of bool formatting")
+	r.Flags.BoolVar(&n.hexFormat, "hex-format", true, "enable/disable optimization of hex formatting")
+	r.Flags.BoolVar(&n.strFormat.enabled, "string-format", true, "enable/disable optimization of string formatting")
+	r.Flags.BoolVar(&n.strFormat.sprintf1, "sprintf1", true, "optimizes fmt.Sprintf with only one argument")
+	r.Flags.BoolVar(&n.strFormat.strconcat, "strconcat", true, "optimizes into strings concatenation")
 	r.Flags.BoolVar(&n.fiximports, "fiximports", true, "fix needed imports from other fixes")
-	r.Flags.BoolVar(&n.strconcat, "strconcat", true, "optimizes into strings concatenation")
+
+	if !n.intFormat.enabled {
+		n.intFormat.intConv = false
+	}
+
+	if !n.errFormat.enabled {
+		n.errFormat.errError = false
+		n.errFormat.errorf = false
+	}
+	if !n.strFormat.enabled {
+		n.strFormat.sprintf1 = false
+		n.strFormat.strconcat = false
+	}
+
 	return r
 }
 
@@ -102,7 +139,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 			err   error
 		)
 		switch {
-		case calledObj == fmtErrorfObj && len(call.Args) == 1 && n.errorf:
+		case calledObj == fmtErrorfObj && len(call.Args) == 1 && n.errFormat.errorf:
 			fn = "fmt.Errorf"
 			verb = "%s"
 			value = call.Args[0]
@@ -112,7 +149,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 			verb = "%v"
 			value = call.Args[0]
 
-		case calledObj == fmtSprintfObj && len(call.Args) == 1 && n.sprintf1:
+		case calledObj == fmtSprintfObj && len(call.Args) == 1 && n.strFormat.sprintf1:
 			fn = "fmt.Sprintf"
 			verb = "%s"
 			value = call.Args[0]
@@ -141,7 +178,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 
 		switch verb {
 		default:
-			if fn == "fmt.Sprintf" && isConcatable(verb) && n.strconcat {
+			if fn == "fmt.Sprintf" && isConcatable(verb) && n.strFormat.strconcat {
 				break
 			}
 			return
@@ -160,7 +197,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 				neededPackages[fname] = make(map[string]struct{})
 			}
 			removedFmtUsages[fname]++
-			if fn == "fmt.Errorf" {
+			if fn == "fmt.Errorf" && n.errFormat.enabled {
 				neededPackages[fname]["errors"] = struct{}{}
 				d = newAnalysisDiagnostic(
 					"", // TODO: precise checker
@@ -177,7 +214,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 						},
 					},
 				)
-			} else {
+			} else if fn != "fmt.Errorf" && n.strFormat.enabled {
 				d = newAnalysisDiagnostic(
 					"", // TODO: precise checker
 					call,
@@ -194,7 +231,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 					},
 				)
 			}
-		case types.Implements(valueType, errIface) && oneOf(verb, "%v", "%s") && n.errError:
+		case types.Implements(valueType, errIface) && oneOf(verb, "%v", "%s") && n.errFormat.errError:
 			// known false positive if this error is nil
 			// fmt.Sprint(nil) does not panic like nil.Error() does
 			errMethodCall := formatNode(pass.Fset, value) + ".Error()"
@@ -216,7 +253,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 				},
 			)
 
-		case isBasicType(valueType, types.Bool) && oneOf(verb, "%v", "%t"):
+		case isBasicType(valueType, types.Bool) && oneOf(verb, "%v", "%t") && n.boolFormat:
 			fname := pass.Fset.File(call.Pos()).Name()
 			removedFmtUsages[fname]++
 			if _, ok := neededPackages[fname]; !ok {
@@ -239,7 +276,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 				},
 			)
 
-		case isArray && isBasicType(a.Elem(), types.Uint8) && oneOf(verb, "%x"):
+		case isArray && isBasicType(a.Elem(), types.Uint8) && oneOf(verb, "%x") && n.hexFormat:
 			if _, ok := value.(*ast.Ident); !ok {
 				// Doesn't support array literals.
 				return
@@ -273,7 +310,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 					},
 				},
 			)
-		case isSlice && isBasicType(s.Elem(), types.Uint8) && oneOf(verb, "%x"):
+		case isSlice && isBasicType(s.Elem(), types.Uint8) && oneOf(verb, "%x") && n.hexFormat:
 			fname := pass.Fset.File(call.Pos()).Name()
 			removedFmtUsages[fname]++
 			if _, ok := neededPackages[fname]; !ok {
@@ -296,7 +333,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 				},
 			)
 
-		case isBasicType(valueType, types.Int8, types.Int16, types.Int32) && oneOf(verb, "%v", "%d") && n.intConv:
+		case isBasicType(valueType, types.Int8, types.Int16, types.Int32) && oneOf(verb, "%v", "%d") && n.intFormat.intConv:
 			fname := pass.Fset.File(call.Pos()).Name()
 			removedFmtUsages[fname]++
 			if _, ok := neededPackages[fname]; !ok {
@@ -325,7 +362,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 					},
 				},
 			)
-		case isBasicType(valueType, types.Int) && oneOf(verb, "%v", "%d"):
+		case isBasicType(valueType, types.Int) && oneOf(verb, "%v", "%d") && n.intFormat.enabled:
 			fname := pass.Fset.File(call.Pos()).Name()
 			removedFmtUsages[fname]++
 			if _, ok := neededPackages[fname]; !ok {
@@ -347,7 +384,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 					},
 				},
 			)
-		case isBasicType(valueType, types.Int64) && oneOf(verb, "%v", "%d"):
+		case isBasicType(valueType, types.Int64) && oneOf(verb, "%v", "%d") && n.intFormat.enabled:
 			fname := pass.Fset.File(call.Pos()).Name()
 			removedFmtUsages[fname]++
 			if _, ok := neededPackages[fname]; !ok {
@@ -377,7 +414,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 				},
 			)
 
-		case isBasicType(valueType, types.Uint8, types.Uint16, types.Uint32, types.Uint) && oneOf(verb, "%v", "%d", "%x") && n.intConv:
+		case isBasicType(valueType, types.Uint8, types.Uint16, types.Uint32, types.Uint) && oneOf(verb, "%v", "%d", "%x") && n.intFormat.intConv:
 			base := []byte("), 10")
 			if verb == "%x" {
 				base = []byte("), 16")
@@ -410,7 +447,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 					},
 				},
 			)
-		case isBasicType(valueType, types.Uint64) && oneOf(verb, "%v", "%d", "%x"):
+		case isBasicType(valueType, types.Uint64) && oneOf(verb, "%v", "%d", "%x") && n.intFormat.enabled:
 			base := []byte(", 10")
 			if verb == "%x" {
 				base = []byte(", 16")
@@ -443,7 +480,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
 					},
 				},
 			)
-		case isBasicType(valueType, types.String) && fn == "fmt.Sprintf" && isConcatable(verb):
+		case isBasicType(valueType, types.String) && fn == "fmt.Sprintf" && isConcatable(verb) && n.strFormat.enabled:
 			var fix string
 			if strings.HasSuffix(verb, "%s") {
 				fix = strconv.Quote(verb[:len(verb)-2]) + "+" + formatNode(pass.Fset, value)

analyzer/analyzer_test.go

@@ -2,6 +2,7 @@ package analyzer_test
 
 import (
 	"encoding/hex"
+	"flag"
 	"fmt"
 	"io"
 	"strconv"
@@ -14,31 +15,30 @@ import (
 func TestAnalyzer(t *testing.T) {
 	t.Parallel()
 	a := analyzer.New()
-	err := a.Flags.Set("err-error", "true")
-	if err != nil {
-		t.Fatalf("failed to set err-error flag")
-	}
-	analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), a, "p")
-}
-
-func TestAnalyzerNoErrError(t *testing.T) {
-	t.Parallel()
-	a := analyzer.New()
-	err := a.Flags.Set("err-error", "false")
-	if err != nil {
-		t.Fatalf("failed to set err-error flag")
-	}
-	analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), a, "nilerror")
-}
+	analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), a, "default")
+	a.Flags.VisitAll(func(f *flag.Flag) {
+		if f.Name == "fiximports" {
+			return
+		}
+		t.Run(f.Name, func(t *testing.T) {
+			changedVal := "false"
+			if f.DefValue == "false" {
+				changedVal = "true"
+			} else if f.DefValue != "true" {
+				t.Fatalf("default value neither false or true")
+			}
+			err := a.Flags.Set(f.Name, changedVal)
+			if err != nil {
+				t.Fatalf("failed to set %q flag", f.Name)
+			}
+			analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), a, f.Name)
+			err = a.Flags.Set(f.Name, f.DefValue)
+			if err != nil {
+				t.Fatalf("failed to set %q flag", f.Name)
+			}
+		})
+	})
 
-func TestAnalyzerNoConv(t *testing.T) {
-	t.Parallel()
-	a := analyzer.New()
-	err := a.Flags.Set("int-conversion", "false")
-	if err != nil {
-		t.Fatalf("failed to set int-conversion flag")
-	}
-	analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), a, "noconv")
 }
 
 func TestReplacements(t *testing.T) {