Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rules definition #41

Closed
wants to merge 11 commits into from
Closed

Rules definition #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
486e799
analyzer: add option integer-format
catenacyber Jan 29, 2025
f82c04f
analyzer: add option error-format
catenacyber Jan 29, 2025
aef67a1
analyzer: add options bool-format and hex-format
catenacyber Jan 29, 2025
278b251
analyzer: add option string-format
catenacyber Jan 29, 2025
e936ca5
doc: document the new set of options
catenacyber Jan 29, 2025
87f7e3c
analyzer: rename global to enabled
catenacyber Jan 30, 2025
2518a2c
analyzer: add missing cases of options usage
catenacyber Jan 30, 2025
0375e34
analyzer: consistency settings for options
catenacyber Jan 30, 2025
6c34683
analyzer: remove redundant checks
catenacyber Jan 30, 2025
b1b230a
analyzer: replace enabled by deactivated
catenacyber Jan 30, 2025
8686a80
fixup! analyzer: replace enabled by deactivated
catenacyber Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 slices with `hex.EncodeToString`)


To disable `fmt.Errorf` optimization, you can use the flag `-errorf=false`
This optimization is not always equivalent.
Expand Down
97 changes: 67 additions & 30 deletions analyzer/analyzer.go
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to write tests to ensure we don't break anything with all these new options?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

What is the best way ?

Have analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), a, "somedir") for all 10 options with a different value than the default one ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests caught an error with fmt.Errorf("toto") becoming just a string when disabling error-format ;-)

Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,41 @@ import (
"golang.org/x/tools/go/analysis"
)

type optionInt struct {
deactivated bool
intConv bool
}

type optionErr struct {
deactivated bool
errError bool
errorf bool
}

type optionStr struct {
deactivated 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{intConv: true},
errFormat: optionErr{errError: false, errorf: true},
strFormat: optionStr{sprintf1: true, strconcat: true},
boolFormat: true,
hexFormat: true,
fiximports: true,
strconcat: true,
}
}

Expand All @@ -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.deactivated, "integer-format", false, "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.deactivated, "error-format", false, "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.deactivated, "string-format", false, "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")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not need all of that. Your algorithm shall start by checking if a format is activated and then trigger check their option.
Let's try to follow KISS principle here

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But some default options being true, I still need to do the check

if n.intFormat.deactivated {
n.intFormat.intConv = false
}

if n.errFormat.deactivated {
n.errFormat.errError = false
n.errFormat.errorf = false
}
if n.strFormat.deactivated {
n.strFormat.sprintf1 = false
n.strFormat.strconcat = false
}

return r
}

Expand Down Expand Up @@ -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]
Expand All @@ -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]
Expand Down Expand Up @@ -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 {
catenacyber marked this conversation as resolved.
Show resolved Hide resolved
break
}
return
Expand All @@ -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.deactivated {
neededPackages[fname]["errors"] = struct{}{}
d = newAnalysisDiagnostic(
"", // TODO: precise checker
Expand All @@ -177,7 +214,7 @@ func (n *perfSprint) run(pass *analysis.Pass) (interface{}, error) {
},
},
)
} else {
} else if !n.strFormat.deactivated {
catenacyber marked this conversation as resolved.
Show resolved Hide resolved
d = newAnalysisDiagnostic(
"", // TODO: precise checker
call,
Expand All @@ -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()"
Expand All @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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.deactivated:
fname := pass.Fset.File(call.Pos()).Name()
removedFmtUsages[fname]++
if _, ok := neededPackages[fname]; !ok {
Expand All @@ -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.deactivated:
fname := pass.Fset.File(call.Pos()).Name()
removedFmtUsages[fname]++
if _, ok := neededPackages[fname]; !ok {
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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.deactivated:
base := []byte(", 10")
if verb == "%x" {
base = []byte(", 16")
Expand Down Expand Up @@ -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.deactivated:
var fix string
if strings.HasSuffix(verb, "%s") {
fix = strconv.Quote(verb[:len(verb)-2]) + "+" + formatNode(pass.Fset, value)
Expand Down