From f2d91ba51ab6b186fdddce48faeba051c9ac11bd Mon Sep 17 00:00:00 2001 From: Timon Wong Date: Tue, 30 Aug 2022 13:38:19 +0800 Subject: [PATCH] feat: add special handling for zap fix #27 Signed-off-by: Timon Wong --- internal/checkers/checker.go | 22 ++++ internal/checkers/common.go | 85 ++++++++++++++ internal/checkers/general.go | 36 ++++++ internal/checkers/zap.go | 58 ++++++++++ loggercheck.go | 124 ++++----------------- staticrules.go | 57 +++++----- testdata/src/a/all/example.go | 6 + testdata/src/a/requirestringkey/example.go | 7 ++ 8 files changed, 266 insertions(+), 129 deletions(-) create mode 100644 internal/checkers/checker.go create mode 100644 internal/checkers/common.go create mode 100644 internal/checkers/general.go create mode 100644 internal/checkers/zap.go diff --git a/internal/checkers/checker.go b/internal/checkers/checker.go new file mode 100644 index 0000000..88357db --- /dev/null +++ b/internal/checkers/checker.go @@ -0,0 +1,22 @@ +package checkers + +import ( + "go/ast" + "go/types" + + "golang.org/x/tools/go/analysis" +) + +type Config struct { + RequireStringKey bool +} + +type CallContext struct { + Expr *ast.CallExpr + Func *types.Func + Signature *types.Signature +} + +type Checker interface { + CheckAndReport(pass *analysis.Pass, call CallContext, cfg Config) +} diff --git a/internal/checkers/common.go b/internal/checkers/common.go new file mode 100644 index 0000000..5109283 --- /dev/null +++ b/internal/checkers/common.go @@ -0,0 +1,85 @@ +package checkers + +import ( + "fmt" + "go/ast" + "go/printer" + "go/token" + "go/types" + "unicode/utf8" + + "golang.org/x/tools/go/analysis" + + "github.com/timonwong/loggercheck/internal/bytebufferpool" + "github.com/timonwong/loggercheck/internal/stringutil" +) + +// getStringValueFromArg returns true if the argument is string literal or string constant. +func getStringValueFromArg(pass *analysis.Pass, arg ast.Expr) (value string, ok bool) { + switch arg := arg.(type) { + case *ast.BasicLit: // literals, must be string + if arg.Kind == token.STRING { + return arg.Value, true + } + case *ast.Ident: // identifiers, we require constant string key + if arg.Obj != nil && arg.Obj.Kind == ast.Con { + typeAndValue := pass.TypesInfo.Types[arg] + if typ, ok := typeAndValue.Type.(*types.Basic); ok { + if typ.Kind() == types.String { + return typeAndValue.Value.ExactString(), true + } + } + } + } + + return "", false +} + +func checkLoggingKey(pass *analysis.Pass, keyValuesArgs []ast.Expr) { + for i := 0; i < len(keyValuesArgs); i += 2 { + arg := keyValuesArgs[i] + if value, ok := getStringValueFromArg(pass, arg); ok { + if stringutil.IsASCII(value) { + continue + } + + pass.Report(analysis.Diagnostic{ + Pos: arg.Pos(), + End: arg.End(), + Category: "logging", + Message: fmt.Sprintf( + "logging keys are expected to be alphanumeric strings, please remove any non-latin characters from %s", + value), + }) + } else { + pass.Report(analysis.Diagnostic{ + Pos: arg.Pos(), + End: arg.End(), + Category: "logging", + Message: fmt.Sprintf( + "logging keys are expected to be inlined constant strings, please replace %q provided with string", + renderNodeEllipsis(pass.Fset, arg)), + }) + } + } +} + +func renderNodeEllipsis(fset *token.FileSet, v interface{}) string { + const maxLen = 20 + + buf := bytebufferpool.Get() + defer bytebufferpool.Put(buf) + + _ = printer.Fprint(buf, fset, v) + s := buf.String() + if utf8.RuneCountInString(s) > maxLen { + // Copied from go/constant/value.go + i := 0 + for n := 0; n < maxLen-3; n++ { + _, size := utf8.DecodeRuneInString(s[i:]) + i += size + } + s = s[:i] + "..." + } + return s +} diff --git a/internal/checkers/general.go b/internal/checkers/general.go new file mode 100644 index 0000000..ed700a2 --- /dev/null +++ b/internal/checkers/general.go @@ -0,0 +1,36 @@ +package checkers + +import ( + "golang.org/x/tools/go/analysis" +) + +type General struct{} + +var _ Checker = (*General)(nil) + +func (g General) CheckAndReport(pass *analysis.Pass, call CallContext, cfg Config) { + args := call.Expr.Args + params := call.Signature.Params() + + nparams := params.Len() // variadic => nonzero + startIndex := nparams - 1 + nargs := len(args) + + // Check the argument count + variadicLen := nargs - startIndex + if variadicLen%2 != 0 { + firstArg := args[startIndex] + lastArg := args[nargs-1] + pass.Report(analysis.Diagnostic{ + Pos: firstArg.Pos(), + End: lastArg.End(), + Category: "logging", + Message: "odd number of arguments passed as key-value pairs for logging", + }) + } + + // Check the "key" type + if cfg.RequireStringKey { + checkLoggingKey(pass, args[startIndex:]) + } +} diff --git a/internal/checkers/zap.go b/internal/checkers/zap.go new file mode 100644 index 0000000..e20c814 --- /dev/null +++ b/internal/checkers/zap.go @@ -0,0 +1,58 @@ +package checkers + +import ( + "go/ast" + "go/types" + + "golang.org/x/tools/go/analysis" +) + +type Zap struct { + General +} + +var _ Checker = (*Zap)(nil) + +func (z Zap) CheckAndReport(pass *analysis.Pass, call CallContext, cfg Config) { + args := call.Expr.Args + params := call.Signature.Params() + + nparams := params.Len() // variadic => nonzero + startIndex := nparams - 1 + nargs := len(args) + + // Check the argument count + keyValuesArgs := make([]ast.Expr, 0, nargs-startIndex) + for i := startIndex; i < nargs; i++ { + // This is a strongly-typed field. Consume it and move on. + arg := args[i] + switch arg := arg.(type) { + case *ast.CallExpr, *ast.Ident: + typ := pass.TypesInfo.TypeOf(arg) + switch typ := typ.(type) { + case *types.Named: + obj := typ.Obj() + if obj != nil && obj.Name() == "Field" { + continue + } + } + } + keyValuesArgs = append(keyValuesArgs, arg) + } + + if len(keyValuesArgs)%2 != 0 { + firstArg := args[startIndex] + lastArg := args[nargs-1] + pass.Report(analysis.Diagnostic{ + Pos: firstArg.Pos(), + End: lastArg.End(), + Category: "logging", + Message: "odd number of arguments passed as key-value pairs for logging", + }) + } + + // Check the "key" type + if cfg.RequireStringKey { + checkLoggingKey(pass, keyValuesArgs) + } +} diff --git a/loggercheck.go b/loggercheck.go index ae3f273..1213eba 100644 --- a/loggercheck.go +++ b/loggercheck.go @@ -4,21 +4,17 @@ import ( "flag" "fmt" "go/ast" - "go/printer" - "go/token" "go/types" "os" - "unicode/utf8" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/go/types/typeutil" - "github.com/timonwong/loggercheck/internal/bytebufferpool" + "github.com/timonwong/loggercheck/internal/checkers" "github.com/timonwong/loggercheck/internal/rules" "github.com/timonwong/loggercheck/internal/sets" - "github.com/timonwong/loggercheck/internal/stringutil" ) const Doc = `Checks key valur pairs for common logger libraries (logr,klog,zap).` @@ -66,10 +62,10 @@ func (l *loggercheck) isCheckerDisabled(name string) bool { return l.disable.Has(name) } -func (l *loggercheck) isValidLoggerFunc(fn *types.Func) bool { +func (l *loggercheck) getCheckerForFunc(fn *types.Func) checkers.Checker { pkg := fn.Pkg() if pkg == nil { - return false + return nil } for i := range l.rulesetList { @@ -79,33 +75,18 @@ func (l *loggercheck) isValidLoggerFunc(fn *types.Func) bool { continue } - if rs.Match(fn, pkg) { - return true + if !rs.Match(fn, pkg) { + continue } - } - - return false -} -// getStringValueFromArg returns true if the argument is string literal or string constant. -func getStringValueFromArg(pass *analysis.Pass, arg ast.Expr) (value string, ok bool) { - switch arg := arg.(type) { - case *ast.BasicLit: // literals, must be string - if arg.Kind == token.STRING { - return arg.Value, true - } - case *ast.Ident: // identifiers, we require constant string key - if arg.Obj != nil && arg.Obj.Kind == ast.Con { - typeAndValue := pass.TypesInfo.Types[arg] - if typ, ok := typeAndValue.Type.(*types.Basic); ok { - if typ.Kind() == types.String { - return typeAndValue.Value.ExactString(), true - } - } + checker := checkerByRulesetName[rs.Name] + if checker == nil { + return checkers.General{} } + return checker } - return "", false + return nil } func (l *loggercheck) checkLoggerArguments(pass *analysis.Pass, call *ast.CallExpr) { @@ -119,68 +100,23 @@ func (l *loggercheck) checkLoggerArguments(pass *analysis.Pass, call *ast.CallEx return // not variadic } - if !l.isValidLoggerFunc(fn) { - return - } - // ellipsis args is hard, just skip if call.Ellipsis.IsValid() { return } - params := sig.Params() - nparams := params.Len() // variadic => nonzero - args := params.At(nparams - 1) - iface, ok := args.Type().(*types.Slice).Elem().(*types.Interface) - if !ok || !iface.Empty() { - return // final (args) param is not ...interface{} - } - - startIndex := nparams - 1 - nargs := len(call.Args) - - // Check the argument count - variadicLen := nargs - startIndex - if variadicLen%2 != 0 { - firstArg := call.Args[startIndex] - lastArg := call.Args[nargs-1] - pass.Report(analysis.Diagnostic{ - Pos: firstArg.Pos(), - End: lastArg.End(), - Category: "logging", - Message: "odd number of arguments passed as key-value pairs for logging", - }) + checker := l.getCheckerForFunc(fn) + if checker == nil { + return } - // Check the "key" type - if l.requireStringKey { - for i := startIndex; i < nargs; i += 2 { - arg := call.Args[i] - if value, ok := getStringValueFromArg(pass, arg); ok { - if stringutil.IsASCII(value) { - continue - } - - pass.Report(analysis.Diagnostic{ - Pos: arg.Pos(), - End: arg.End(), - Category: "logging", - Message: fmt.Sprintf( - "logging keys are expected to be alphanumeric strings, please remove any non-latin characters from %s", - value), - }) - } else { - pass.Report(analysis.Diagnostic{ - Pos: arg.Pos(), - End: arg.End(), - Category: "logging", - Message: fmt.Sprintf( - "logging keys are expected to be inlined constant strings, please replace %q provided with string", - renderNodeEllipsis(pass.Fset, arg)), - }) - } - } - } + checker.CheckAndReport(pass, checkers.CallContext{ + Expr: call, + Func: fn, + Signature: sig, + }, checkers.Config{ + RequireStringKey: l.requireStringKey, + }) } func (l *loggercheck) processConfig() error { @@ -231,23 +167,3 @@ func (l *loggercheck) run(pass *analysis.Pass) (interface{}, error) { return nil, nil } - -func renderNodeEllipsis(fset *token.FileSet, v interface{}) string { - const maxLen = 20 - - buf := bytebufferpool.Get() - defer bytebufferpool.Put(buf) - - _ = printer.Fprint(buf, fset, v) - s := buf.String() - if utf8.RuneCountInString(s) > maxLen { - // Copied from go/constant/value.go - i := 0 - for n := 0; n < maxLen-3; n++ { - _, size := utf8.DecodeRuneInString(s[i:]) - i += size - } - s = s[:i] + "..." - } - return s -} diff --git a/staticrules.go b/staticrules.go index 1080aab..2755b7e 100644 --- a/staticrules.go +++ b/staticrules.go @@ -4,34 +4,41 @@ import ( "errors" "fmt" + "github.com/timonwong/loggercheck/internal/checkers" "github.com/timonwong/loggercheck/internal/rules" ) -var staticRuleList = []rules.Ruleset{ - mustNewStaticRuleSet("logr", []string{ - "(github.com/go-logr/logr.Logger).Error", - "(github.com/go-logr/logr.Logger).Info", - "(github.com/go-logr/logr.Logger).WithValues", - }), - mustNewStaticRuleSet("klog", []string{ - "k8s.io/klog/v2.InfoS", - "k8s.io/klog/v2.InfoSDepth", - "k8s.io/klog/v2.ErrorS", - "(k8s.io/klog/v2.Verbose).InfoS", - "(k8s.io/klog/v2.Verbose).InfoSDepth", - "(k8s.io/klog/v2.Verbose).ErrorS", - }), - mustNewStaticRuleSet("zap", []string{ - "(*go.uber.org/zap.SugaredLogger).With", - "(*go.uber.org/zap.SugaredLogger).Debugw", - "(*go.uber.org/zap.SugaredLogger).Infow", - "(*go.uber.org/zap.SugaredLogger).Warnw", - "(*go.uber.org/zap.SugaredLogger).Errorw", - "(*go.uber.org/zap.SugaredLogger).DPanicw", - "(*go.uber.org/zap.SugaredLogger).Panicw", - "(*go.uber.org/zap.SugaredLogger).Fatalw", - }), -} +var ( + staticRuleList = []rules.Ruleset{ + mustNewStaticRuleSet("logr", []string{ + "(github.com/go-logr/logr.Logger).Error", + "(github.com/go-logr/logr.Logger).Info", + "(github.com/go-logr/logr.Logger).WithValues", + }), + mustNewStaticRuleSet("klog", []string{ + "k8s.io/klog/v2.InfoS", + "k8s.io/klog/v2.InfoSDepth", + "k8s.io/klog/v2.ErrorS", + "(k8s.io/klog/v2.Verbose).InfoS", + "(k8s.io/klog/v2.Verbose).InfoSDepth", + "(k8s.io/klog/v2.Verbose).ErrorS", + }), + mustNewStaticRuleSet("zap", []string{ + "(*go.uber.org/zap.SugaredLogger).With", + "(*go.uber.org/zap.SugaredLogger).Debugw", + "(*go.uber.org/zap.SugaredLogger).Infow", + "(*go.uber.org/zap.SugaredLogger).Warnw", + "(*go.uber.org/zap.SugaredLogger).Errorw", + "(*go.uber.org/zap.SugaredLogger).DPanicw", + "(*go.uber.org/zap.SugaredLogger).Panicw", + "(*go.uber.org/zap.SugaredLogger).Fatalw", + }), + } + checkerByRulesetName = map[string]checkers.Checker{ + // by default, checkers.General will be used. + "zap": checkers.Zap{}, + } +) // mustNewStaticRuleSet only called at init, catch errors during development. // In production it will not panic. diff --git a/testdata/src/a/all/example.go b/testdata/src/a/all/example.go index 9aaf96e..0a97631 100644 --- a/testdata/src/a/all/example.go +++ b/testdata/src/a/all/example.go @@ -82,6 +82,12 @@ func ExampleZap() { zap.S().With("with_key1", "with_value1").Infow("message", "key1", "value1", "key2", "value2") zap.S().Infow("message", "key1", "value1", "key2", "value2", "key3") // want `odd number of arguments passed as key-value pairs for logging` + zap.S().Infow("message", zap.String("key1", "value1")) + field := zap.String("key1", "value1") + field2 := zap.Int("key2", 2) + field3 := zap.Bool("key3", true) + zap.S().Infow("message", field, field2, field3) + zap.S().Errorw("message", "err", err, "key1", "value1") zap.S().Errorw("message", err, "message", "key1") // want `odd number of arguments passed as key-value pairs for logging` } diff --git a/testdata/src/a/requirestringkey/example.go b/testdata/src/a/requirestringkey/example.go index b45c380..da9c48f 100644 --- a/testdata/src/a/requirestringkey/example.go +++ b/testdata/src/a/requirestringkey/example.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/go-logr/logr" + "go.uber.org/zap" ) func ExampleRequireStringKey() { @@ -34,4 +35,10 @@ func ExampleRequireStringKey() { log.Error(err, "message", "键1", "value1") // want `logging keys are expected to be alphanumeric strings, please remove any non-latin characters from "键1"` const KeyNonASCII = "键1" log.Error(err, "message", KeyNonASCII, "value1") // want `logging keys are expected to be alphanumeric strings, please remove any non-latin characters from "键1"` + + field := zap.String("key1", "value1") + field2 := zap.Int("key2", 2) + field3 := zap.Bool("key3", true) + const Key4Int = 4 + zap.S().Infow("message", field, field2, field3, Key4Int, "value4") // want `logging keys are expected to be inlined constant strings, please replace "Key4Int" provided with string` }