diff --git a/README.md b/README.md index fc4d9de..7f6455c 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,7 @@ With `sloglint` you can enforce various rules for `log/slog` based on your prefe * Enforce not mixing key-value pairs and attributes (default) * Enforce using either key-value pairs only or attributes only (optional) +* Enforce not using global loggers (optional) * Enforce using methods that accept a context (optional) * Enforce using static log messages (optional) * Enforce using constants instead of raw keys (optional) @@ -70,6 +71,17 @@ In contrast, the `attr-only` option causes `sloglint` to report any use of key-v slog.Info("a user has logged in", "user_id", 42) // sloglint: key-value pairs should not be used ``` +### No global + +Some projects prefer to pass loggers as explicit dependencies. +The `no-global` option causes `sloglint` to report the usage of global loggers. + +```go +slog.Info("a user has logged in", "user_id", 42) // sloglint: global logger should not be used +``` + +Possible values are `all` (report all global loggers) and `default` (report only the default `slog` logger). + ### Context only Some `slog.Handler` implementations make use of the given `context.Context` (e.g. to access context values). diff --git a/sloglint.go b/sloglint.go index b7f72ce..35cac14 100644 --- a/sloglint.go +++ b/sloglint.go @@ -9,6 +9,7 @@ import ( "go/token" "go/types" "strconv" + "strings" "github.com/ettle/strcase" "golang.org/x/tools/go/analysis" @@ -22,6 +23,7 @@ type Options struct { NoMixedArgs bool // Enforce not mixing key-value pairs and attributes (default true). 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. StaticMsg bool // Enforce using static log messages. NoRawKeys bool // Enforce using constants instead of raw keys. @@ -43,11 +45,19 @@ func New(opts *Options) *analysis.Analyzer { if opts.KVOnly && opts.AttrOnly { return nil, fmt.Errorf("sloglint: Options.KVOnly and Options.AttrOnly: %w", errIncompatible) } + + switch opts.NoGlobal { + case "", "all", "default": + default: + return nil, fmt.Errorf("sloglint: Options.NoGlobal=%s: %w", opts.NoGlobal, errInvalidValue) + } + switch opts.KeyNamingCase { case "", snakeCase, kebabCase, camelCase, pascalCase: default: return nil, fmt.Errorf("sloglint: Options.KeyNamingCase=%s: %w", opts.KeyNamingCase, errInvalidValue) } + run(pass, opts) return nil, nil }, @@ -60,30 +70,34 @@ var ( ) func flags(opts *Options) flag.FlagSet { - fs := flag.NewFlagSet("sloglint", flag.ContinueOnError) + fset := flag.NewFlagSet("sloglint", flag.ContinueOnError) boolVar := func(value *bool, name, usage string) { - fs.Func(name, usage, func(s string) error { + fset.Func(name, usage, func(s string) error { v, err := strconv.ParseBool(s) *value = v return err }) } + strVar := func(value *string, name, usage string) { + fset.Func(name, usage, func(s string) error { + *value = s + return nil + }) + } + boolVar(&opts.NoMixedArgs, "no-mixed-args", "enforce not mixing key-value pairs and attributes (default true)") 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") 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)") boolVar(&opts.ArgsOnSepLines, "args-on-sep-lines", "enforce putting arguments on separate lines") - fs.Func("key-naming-case", "enforce a single key naming convention (snake|kebab|camel|pascal)", func(s string) error { - opts.KeyNamingCase = s - return nil - }) - - return *fs + return *fset } var slogFuncs = map[string]int{ // funcName:argsPos @@ -139,17 +153,30 @@ func run(pass *analysis.Pass, opts *Options) { return } - argsPos, ok := slogFuncs[fn.FullName()] + name := fn.FullName() + argsPos, ok := slogFuncs[name] if !ok { return } + 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") + } + } + 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") } } + if opts.StaticMsg && !staticMsg(call.Args[argsPos-1]) { pass.Reportf(call.Pos(), "message should be a string literal or a constant") } @@ -189,6 +216,7 @@ func run(pass *analysis.Pass, opts *Options) { 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") } @@ -206,6 +234,19 @@ func run(pass *analysis.Pass, opts *Options) { }) } +func globalLoggerUsed(info *types.Info, expr ast.Expr) bool { + selector, ok := expr.(*ast.SelectorExpr) + if !ok { + return false + } + ident, ok := selector.X.(*ast.Ident) + if !ok { + return false + } + obj := info.ObjectOf(ident) + return obj.Parent() == obj.Pkg().Scope() +} + 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 c9a6d47..2b4478d 100644 --- a/sloglint_internal_test.go +++ b/sloglint_internal_test.go @@ -10,11 +10,15 @@ func TestOptions(t *testing.T) { opts Options err error }{ - "incompatible": { + "KVOnly+AttrOnly: incompatible": { opts: Options{KVOnly: true, AttrOnly: true}, err: errIncompatible, }, - "invalid value": { + "NoGlobal: invalid value": { + opts: Options{NoGlobal: "-"}, + err: errInvalidValue, + }, + "KeyNamingCase: invalid value": { opts: Options{KeyNamingCase: "-"}, err: errInvalidValue, }, diff --git a/sloglint_test.go b/sloglint_test.go index 73b504f..8d6055d 100644 --- a/sloglint_test.go +++ b/sloglint_test.go @@ -25,6 +25,16 @@ func TestAnalyzer(t *testing.T) { analysistest.Run(t, testdata, analyzer, "attr_only") }) + t.Run("no global (all)", func(t *testing.T) { + analyzer := sloglint.New(&sloglint.Options{NoGlobal: "all"}) + analysistest.Run(t, testdata, analyzer, "no_global_all") + }) + + t.Run("no global (default)", func(t *testing.T) { + analyzer := sloglint.New(&sloglint.Options{NoGlobal: "default"}) + 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") diff --git a/testdata/src/no_global_all/no_global_all.go b/testdata/src/no_global_all/no_global_all.go new file mode 100644 index 0000000..b425fff --- /dev/null +++ b/testdata/src/no_global_all/no_global_all.go @@ -0,0 +1,10 @@ +package no_global_all + +import "log/slog" + +var logger = slog.New(nil) + +func tests() { + slog.Info("msg") // want `global logger should not be used` + logger.Info("msg") // want `global logger should not be used` +} diff --git a/testdata/src/no_global_default/no_global_default.go b/testdata/src/no_global_default/no_global_default.go new file mode 100644 index 0000000..4bdfb7a --- /dev/null +++ b/testdata/src/no_global_default/no_global_default.go @@ -0,0 +1,10 @@ +package no_global_default + +import "log/slog" + +var logger = slog.New(nil) + +func tests() { + slog.Info("msg") // want `default logger should not be used` + logger.Info("msg") +}