From f4014ddd282000808023514d58a198e040878228 Mon Sep 17 00:00:00 2001 From: Tom <73077675+tmzane@users.noreply.github.com> Date: Sat, 20 Apr 2024 21:41:04 +0500 Subject: [PATCH] feat: implement `-context-only=scope` (#33) --- README.md | 8 +- sloglint.go | 195 +++++++++++------- sloglint_internal_test.go | 4 + sloglint_test.go | 11 +- testdata/src/context_only/context_only.go | 33 --- .../src/context_only_all/context_only_all.go | 31 +++ .../context_only_scope/context_only_scope.go | 25 +++ 7 files changed, 190 insertions(+), 117 deletions(-) delete mode 100644 testdata/src/context_only/context_only.go create mode 100644 testdata/src/context_only_all/context_only_all.go create mode 100644 testdata/src/context_only_scope/context_only_scope.go diff --git a/README.md b/README.md index 7f6455c..c080c2b 100644 --- a/README.md +++ b/README.md @@ -89,14 +89,10 @@ For them to work properly, you need to pass a context to all logger calls. The `context-only` option causes `sloglint` to report the use of methods without a context: ```go -slog.Info("a user has logged in") // sloglint: methods without a context should not be used +slog.Info("a user has logged in") // sloglint: InfoContext should be used instead ``` -This report can be fixed by using the equivalent method with the `Context` suffix: - -```go -slog.InfoContext(ctx, "a user has logged in") -``` +Possible values are `all` (report all contextless calls) and `scope` (report only if a context exists in the scope of the outermost function). ### Static messages diff --git a/sloglint.go b/sloglint.go index ac4fe87..d3b0176 100644 --- a/sloglint.go +++ b/sloglint.go @@ -24,7 +24,7 @@ type Options struct { KVOnly bool // Enforce using key-value pairs only (overrides NoMixedArgs, incompatible with AttrOnly). AttrOnly bool // Enforce using attributes only (overrides NoMixedArgs, incompatible with KVOnly). NoGlobal string // Enforce not using global loggers ("all" or "default"). - ContextOnly bool // Enforce using methods that accept a context. + ContextOnly string // Enforce using methods that accept a context ("all" or "scope"). StaticMsg bool // Enforce using static log messages. NoRawKeys bool // Enforce using constants instead of raw keys. KeyNamingCase string // Enforce a single key naming convention ("snake", "kebab", "camel", or "pascal"). @@ -36,6 +36,7 @@ func New(opts *Options) *analysis.Analyzer { if opts == nil { opts = &Options{NoMixedArgs: true} } + return &analysis.Analyzer{ Name: "sloglint", Doc: "ensure consistent code style when using log/slog", @@ -52,6 +53,12 @@ func New(opts *Options) *analysis.Analyzer { return nil, fmt.Errorf("sloglint: Options.NoGlobal=%s: %w", opts.NoGlobal, errInvalidValue) } + switch opts.ContextOnly { + case "", "all", "scope": + default: + return nil, fmt.Errorf("sloglint: Options.ContextOnly=%s: %w", opts.ContextOnly, errInvalidValue) + } + switch opts.KeyNamingCase { case "", snakeCase, kebabCase, camelCase, pascalCase: default: @@ -91,7 +98,7 @@ func flags(opts *Options) flag.FlagSet { boolVar(&opts.KVOnly, "kv-only", "enforce using key-value pairs only (overrides -no-mixed-args, incompatible with -attr-only)") boolVar(&opts.AttrOnly, "attr-only", "enforce using attributes only (overrides -no-mixed-args, incompatible with -kv-only)") strVar(&opts.NoGlobal, "no-global", "enforce not using global loggers (all|default)") - boolVar(&opts.ContextOnly, "context-only", "enforce using methods that accept a context") + strVar(&opts.ContextOnly, "context-only", "enforce using methods that accept a context (all|scope)") boolVar(&opts.StaticMsg, "static-msg", "enforce using static log messages") boolVar(&opts.NoRawKeys, "no-raw-keys", "enforce using constants instead of raw keys") strVar(&opts.KeyNamingCase, "key-naming-case", "enforce a single key naming convention (snake|kebab|camel|pascal)") @@ -142,96 +149,116 @@ const ( ) func run(pass *analysis.Pass, opts *Options) { - visit := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + visitor := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) filter := []ast.Node{(*ast.CallExpr)(nil)} - visit.Preorder(filter, func(node ast.Node) { - call := node.(*ast.CallExpr) + // WithStack is ~2x slower than Preorder, use it only when stack is needed. + if opts.ContextOnly == "scope" { + visitor.WithStack(filter, func(node ast.Node, _ bool, stack []ast.Node) bool { + visit(pass, opts, node, stack) + return false + }) + return + } - fn := typeutil.StaticCallee(pass.TypesInfo, call) - if fn == nil { - return - } + visitor.Preorder(filter, func(node ast.Node) { + visit(pass, opts, node, nil) + }) +} - name := fn.FullName() - argsPos, ok := slogFuncs[name] - if !ok { - return - } +// NOTE: stack is nil if Preorder is used. +func visit(pass *analysis.Pass, opts *Options, node ast.Node, stack []ast.Node) { + call := node.(*ast.CallExpr) - switch opts.NoGlobal { - case "all": - if strings.HasPrefix(name, "log/slog.") || globalLoggerUsed(pass.TypesInfo, call.Fun) { - pass.Reportf(call.Pos(), "global logger should not be used") - } - case "default": - if strings.HasPrefix(name, "log/slog.") { - pass.Reportf(call.Pos(), "default logger should not be used") - } - } + fn := typeutil.StaticCallee(pass.TypesInfo, call) + if fn == nil { + return + } - if opts.ContextOnly { - typ := pass.TypesInfo.TypeOf(call.Args[0]) - if typ != nil && typ.String() != "context.Context" { - pass.Reportf(call.Pos(), "methods without a context should not be used") - } - } + name := fn.FullName() + argsPos, ok := slogFuncs[name] + if !ok { + return + } - if opts.StaticMsg && !staticMsg(call.Args[argsPos-1]) { - pass.Reportf(call.Pos(), "message should be a string literal or a constant") + switch opts.NoGlobal { + case "all": + if strings.HasPrefix(name, "log/slog.") || globalLoggerUsed(pass.TypesInfo, call.Fun) { + pass.Reportf(call.Pos(), "global logger should not be used") } + case "default": + if strings.HasPrefix(name, "log/slog.") { + pass.Reportf(call.Pos(), "default logger should not be used") + } + } - // NOTE: we assume that the arguments have already been validated by govet. - args := call.Args[argsPos:] - if len(args) == 0 { - return + switch opts.ContextOnly { + case "all": + typ := pass.TypesInfo.TypeOf(call.Args[0]) + if typ != nil && typ.String() != "context.Context" { + pass.Reportf(call.Pos(), "%sContext should be used instead", fn.Name()) + } + case "scope": + typ := pass.TypesInfo.TypeOf(call.Args[0]) + if typ != nil && typ.String() != "context.Context" && hasContextInScope(pass.TypesInfo, stack) { + pass.Reportf(call.Pos(), "%sContext should be used instead", fn.Name()) } + } - var keys []ast.Expr - var attrs []ast.Expr + if opts.StaticMsg && !staticMsg(call.Args[argsPos-1]) { + pass.Reportf(call.Pos(), "message should be a string literal or a constant") + } - for i := 0; i < len(args); i++ { - typ := pass.TypesInfo.TypeOf(args[i]) - if typ == nil { - continue - } - switch typ.String() { - case "string": - keys = append(keys, args[i]) - i++ // skip the value. - case "log/slog.Attr": - attrs = append(attrs, args[i]) - } - } + // NOTE: we assume that the arguments have already been validated by govet. + args := call.Args[argsPos:] + if len(args) == 0 { + return + } - switch { - case opts.KVOnly && len(attrs) > 0: - pass.Reportf(call.Pos(), "attributes should not be used") - case opts.AttrOnly && len(attrs) < len(args): - pass.Reportf(call.Pos(), "key-value pairs should not be used") - case opts.NoMixedArgs && 0 < len(attrs) && len(attrs) < len(args): - pass.Reportf(call.Pos(), "key-value pairs and attributes should not be mixed") - } + var keys []ast.Expr + var attrs []ast.Expr - if opts.NoRawKeys && rawKeysUsed(pass.TypesInfo, keys, attrs) { - pass.Reportf(call.Pos(), "raw keys should not be used") + for i := 0; i < len(args); i++ { + typ := pass.TypesInfo.TypeOf(args[i]) + if typ == nil { + continue } - - if opts.ArgsOnSepLines && argsOnSameLine(pass.Fset, call, keys, attrs) { - pass.Reportf(call.Pos(), "arguments should be put on separate lines") + switch typ.String() { + case "string": + keys = append(keys, args[i]) + i++ // skip the value. + case "log/slog.Attr": + attrs = append(attrs, args[i]) } + } - switch { - case opts.KeyNamingCase == snakeCase && badKeyNames(pass.TypesInfo, strcase.ToSnake, keys, attrs): - pass.Reportf(call.Pos(), "keys should be written in snake_case") - case opts.KeyNamingCase == kebabCase && badKeyNames(pass.TypesInfo, strcase.ToKebab, keys, attrs): - pass.Reportf(call.Pos(), "keys should be written in kebab-case") - case opts.KeyNamingCase == camelCase && badKeyNames(pass.TypesInfo, strcase.ToCamel, keys, attrs): - pass.Reportf(call.Pos(), "keys should be written in camelCase") - case opts.KeyNamingCase == pascalCase && badKeyNames(pass.TypesInfo, strcase.ToPascal, keys, attrs): - pass.Reportf(call.Pos(), "keys should be written in PascalCase") - } - }) + switch { + case opts.KVOnly && len(attrs) > 0: + pass.Reportf(call.Pos(), "attributes should not be used") + case opts.AttrOnly && len(attrs) < len(args): + pass.Reportf(call.Pos(), "key-value pairs should not be used") + case opts.NoMixedArgs && 0 < len(attrs) && len(attrs) < len(args): + pass.Reportf(call.Pos(), "key-value pairs and attributes should not be mixed") + } + + if opts.NoRawKeys && rawKeysUsed(pass.TypesInfo, keys, attrs) { + pass.Reportf(call.Pos(), "raw keys should not be used") + } + + if opts.ArgsOnSepLines && argsOnSameLine(pass.Fset, call, keys, attrs) { + pass.Reportf(call.Pos(), "arguments should be put on separate lines") + } + + switch { + case opts.KeyNamingCase == snakeCase && badKeyNames(pass.TypesInfo, strcase.ToSnake, keys, attrs): + pass.Reportf(call.Pos(), "keys should be written in snake_case") + case opts.KeyNamingCase == kebabCase && badKeyNames(pass.TypesInfo, strcase.ToKebab, keys, attrs): + pass.Reportf(call.Pos(), "keys should be written in kebab-case") + case opts.KeyNamingCase == camelCase && badKeyNames(pass.TypesInfo, strcase.ToCamel, keys, attrs): + pass.Reportf(call.Pos(), "keys should be written in camelCase") + case opts.KeyNamingCase == pascalCase && badKeyNames(pass.TypesInfo, strcase.ToPascal, keys, attrs): + pass.Reportf(call.Pos(), "keys should be written in PascalCase") + } } func globalLoggerUsed(info *types.Info, expr ast.Expr) bool { @@ -247,6 +274,24 @@ func globalLoggerUsed(info *types.Info, expr ast.Expr) bool { return obj.Parent() == obj.Pkg().Scope() } +func hasContextInScope(info *types.Info, stack []ast.Node) bool { + for i := len(stack) - 1; i >= 0; i-- { + decl, ok := stack[i].(*ast.FuncDecl) + if !ok { + continue + } + params := decl.Type.Params + if len(params.List) == 0 || len(params.List[0].Names) == 0 { + continue + } + typ := info.TypeOf(params.List[0].Names[0]) + if typ != nil && typ.String() == "context.Context" { + return true + } + } + return false +} + func staticMsg(expr ast.Expr) bool { switch msg := expr.(type) { case *ast.BasicLit: // e.g. slog.Info("msg") diff --git a/sloglint_internal_test.go b/sloglint_internal_test.go index 2b4478d..3b9368b 100644 --- a/sloglint_internal_test.go +++ b/sloglint_internal_test.go @@ -18,6 +18,10 @@ func TestOptions(t *testing.T) { opts: Options{NoGlobal: "-"}, err: errInvalidValue, }, + "ContextOnly: invalid value": { + opts: Options{ContextOnly: "-"}, + err: errInvalidValue, + }, "KeyNamingCase: invalid value": { opts: Options{KeyNamingCase: "-"}, err: errInvalidValue, diff --git a/sloglint_test.go b/sloglint_test.go index 8d6055d..90622e7 100644 --- a/sloglint_test.go +++ b/sloglint_test.go @@ -35,9 +35,14 @@ func TestAnalyzer(t *testing.T) { analysistest.Run(t, testdata, analyzer, "no_global_default") }) - t.Run("context only", func(t *testing.T) { - analyzer := sloglint.New(&sloglint.Options{ContextOnly: true}) - analysistest.Run(t, testdata, analyzer, "context_only") + t.Run("context only (all)", func(t *testing.T) { + analyzer := sloglint.New(&sloglint.Options{ContextOnly: "all"}) + analysistest.Run(t, testdata, analyzer, "context_only_all") + }) + + t.Run("context only (scope)", func(t *testing.T) { + analyzer := sloglint.New(&sloglint.Options{ContextOnly: "scope"}) + analysistest.Run(t, testdata, analyzer, "context_only_scope") }) t.Run("static message", func(t *testing.T) { diff --git a/testdata/src/context_only/context_only.go b/testdata/src/context_only/context_only.go deleted file mode 100644 index 1b88568..0000000 --- a/testdata/src/context_only/context_only.go +++ /dev/null @@ -1,33 +0,0 @@ -package context_only - -import ( - "context" - "log/slog" -) - -func tests() { - logger := slog.New(nil) - ctx := context.Background() - - slog.Log(ctx, slog.LevelInfo, "msg") - slog.DebugContext(ctx, "msg") - slog.InfoContext(ctx, "msg") - slog.WarnContext(ctx, "msg") - slog.ErrorContext(ctx, "msg") - - logger.Log(ctx, slog.LevelInfo, "msg") - logger.DebugContext(ctx, "msg") - logger.InfoContext(ctx, "msg") - logger.WarnContext(ctx, "msg") - logger.ErrorContext(ctx, "msg") - - slog.Debug("msg") // want `methods without a context should not be used` - slog.Info("msg") // want `methods without a context should not be used` - slog.Warn("msg") // want `methods without a context should not be used` - slog.Error("msg") // want `methods without a context should not be used` - - logger.Debug("msg") // want `methods without a context should not be used` - logger.Info("msg") // want `methods without a context should not be used` - logger.Warn("msg") // want `methods without a context should not be used` - logger.Error("msg") // want `methods without a context should not be used` -} diff --git a/testdata/src/context_only_all/context_only_all.go b/testdata/src/context_only_all/context_only_all.go new file mode 100644 index 0000000..171a2c7 --- /dev/null +++ b/testdata/src/context_only_all/context_only_all.go @@ -0,0 +1,31 @@ +package context_only_all + +import ( + "context" + "log/slog" +) + +func tests(ctx context.Context) { + slog.Log(ctx, slog.LevelInfo, "msg") + slog.DebugContext(ctx, "msg") + slog.InfoContext(ctx, "msg") + slog.WarnContext(ctx, "msg") + slog.ErrorContext(ctx, "msg") + + slog.Debug("msg") // want `DebugContext should be used instead` + slog.Info("msg") // want `InfoContext should be used instead` + slog.Warn("msg") // want `WarnContext should be used instead` + slog.Error("msg") // want `ErrorContext should be used instead` + + logger := slog.New(nil) + logger.Log(ctx, slog.LevelInfo, "msg") + logger.DebugContext(ctx, "msg") + logger.InfoContext(ctx, "msg") + logger.WarnContext(ctx, "msg") + logger.ErrorContext(ctx, "msg") + + logger.Debug("msg") // want `DebugContext should be used instead` + logger.Info("msg") // want `InfoContext should be used instead` + logger.Warn("msg") // want `WarnContext should be used instead` + logger.Error("msg") // want `ErrorContext should be used instead` +} diff --git a/testdata/src/context_only_scope/context_only_scope.go b/testdata/src/context_only_scope/context_only_scope.go new file mode 100644 index 0000000..5720df2 --- /dev/null +++ b/testdata/src/context_only_scope/context_only_scope.go @@ -0,0 +1,25 @@ +package context_only_scope + +import ( + "context" + "log/slog" +) + +func tests(ctx context.Context) { + slog.Info("msg") // want `InfoContext should be used instead` + slog.InfoContext(ctx, "msg") + + if true { + slog.Info("msg") // want `InfoContext should be used instead` + slog.InfoContext(ctx, "msg") + } + + _ = func() { + slog.Info("msg") // want `InfoContext should be used instead` + slog.InfoContext(ctx, "msg") + } +} + +func noctx() { + slog.Info("msg") +}