From 058e2f95025b10f41734f2fc92ec58beceb682d7 Mon Sep 17 00:00:00 2001 From: Matthew Dowdell Date: Sat, 21 Oct 2023 09:31:58 +0100 Subject: [PATCH] Add -context-only option As described in #13, third-party implementations of `slog.Handler` can choose to take values out of a context to supplement the attributes already used. An example of this in my day job is adding trace IDs to enhance serviceability. This change adds the ability to optionally identify `slog` methods that do not take a context and so prevent the ability to take values from that context. Any matches can be resolved by appending `Context` to the method. For example, `slog.Info` becomes `slog.Context`. --- README.md | 18 ++++++++++++ sloglint.go | 11 +++++++ sloglint_test.go | 5 ++++ testdata/src/context_only/context_only.go | 35 +++++++++++++++++++++++ 4 files changed, 69 insertions(+) create mode 100644 testdata/src/context_only/context_only.go diff --git a/README.md b/README.md index 8f26f3a..d16a3e2 100644 --- a/README.md +++ b/README.md @@ -26,6 +26,7 @@ The linter has several options, so you can adjust it to your own code style. * Enforce using either key-value pairs or attributes for the entire project (optional) * Enforce using constants instead of raw keys (optional) * Enforce putting arguments on separate lines (optional) +* Enforce using methods that take a context (optional) ## 📦 Install @@ -98,5 +99,22 @@ slog.Info("a user has logged in", ) ``` +### Context only + +The `-context-only` flag causes `sloglint` to report the use of any methods that do not take a `context.Context`. + +```go +slog.Info("a user has logged in") // sloglint: methods that do not take a context should not be used" +``` + +This report can be fixed by using the equivalent method with a `Context` suffix. + +```go +slog.InfoContext(ctx, "a user has logged in") +``` + +The `slog.Handler` implementations in the standard library do not currently utilise the given +context. However, third-party implementations can create `slog.Attr` instances from the context. + [1]: https://github.com/go-simpler/sloglint/releases [2]: https://github.com/go-simpler/sloggen diff --git a/sloglint.go b/sloglint.go index 4e45206..96edea4 100644 --- a/sloglint.go +++ b/sloglint.go @@ -8,6 +8,7 @@ import ( "go/token" "go/types" "strconv" + "strings" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" @@ -21,6 +22,7 @@ type Options struct { AttrOnly bool // Enforce using attributes only (incompatible with KVOnly). NoRawKeys bool // Enforce using constants instead of raw keys. ArgsOnSepLines bool // Enforce putting arguments on separate lines. + ContextOnly bool // Enforce using *Context methods only. } // New creates a new sloglint analyzer. @@ -58,6 +60,7 @@ func flags(opts *Options) flag.FlagSet { boolVar(&opts.AttrOnly, "attr-only", "enforce using attributes only (incompatible with -kv-only)") boolVar(&opts.NoRawKeys, "no-raw-keys", "enforce using constants instead of raw keys") boolVar(&opts.ArgsOnSepLines, "args-on-sep-lines", "enforce putting arguments on separate lines") + boolVar(&opts.ContextOnly, "context-only", "enforce using methods that take a context") return *fs } @@ -113,6 +116,14 @@ func run(pass *analysis.Pass, opts *Options) { return } + if opts.ContextOnly { + fnName := fn.FullName() + + if !strings.HasSuffix(fnName, ".Log") && !strings.HasSuffix(fnName, "Context") { + pass.Reportf(call.Pos(), "methods that do not take a context should not be used") + } + } + // NOTE: we assume that the arguments have already been validated by govet. args := call.Args[argsPos:] if len(args) == 0 { diff --git a/sloglint_test.go b/sloglint_test.go index 8d9aa20..6e1fa34 100644 --- a/sloglint_test.go +++ b/sloglint_test.go @@ -34,4 +34,9 @@ func TestAnalyzer(t *testing.T) { analyzer := sloglint.New(&sloglint.Options{ArgsOnSepLines: true}) analysistest.Run(t, testdata, analyzer, "args_on_sep_lines") }) + + 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/context_only/context_only.go b/testdata/src/context_only/context_only.go new file mode 100644 index 0000000..0d491c6 --- /dev/null +++ b/testdata/src/context_only/context_only.go @@ -0,0 +1,35 @@ +package context_only + +import ( + "context" + "io" + "log/slog" +) + +func tests() { + ctx := context.Background() + + slog.Debug("msg") // want `methods that do not take a context should not be used` + slog.Info("msg") // want `methods that do not take a context should not be used` + slog.Warn("msg") // want `methods that do not take a context should not be used` + slog.Error("msg") // want `methods that do not take a context should not be used` + + slog.Log(ctx, slog.LevelInfo, "msg") + slog.DebugContext(ctx, "msg") + slog.InfoContext(ctx, "msg") + slog.WarnContext(ctx, "msg") + slog.ErrorContext(ctx, "msg") + + logger := slog.New(slog.NewJSONHandler(io.Discard, &slog.HandlerOptions{})) + + logger.Debug("msg") // want `methods that do not take a context should not be used` + logger.Info("msg") // want `methods that do not take a context should not be used` + logger.Warn("msg") // want `methods that do not take a context should not be used` + logger.Error("msg") // want `methods that do not take a context should not be used` + + logger.Log(ctx, slog.LevelInfo, "msg") + logger.DebugContext(ctx, "msg") + logger.InfoContext(ctx, "msg") + logger.WarnContext(ctx, "msg") + logger.ErrorContext(ctx, "msg") +}