Skip to content

Commit

Permalink
feat: implement -context-only=scope (#33)
Browse files Browse the repository at this point in the history
  • Loading branch information
tmzane authored Apr 20, 2024
1 parent c397610 commit f4014dd
Show file tree
Hide file tree
Showing 7 changed files with 190 additions and 117 deletions.
8 changes: 2 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
195 changes: 120 additions & 75 deletions sloglint.go
Original file line number Diff line number Diff line change
Expand Up @@ -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").
Expand All @@ -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",
Expand All @@ -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:
Expand Down Expand Up @@ -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)")
Expand Down Expand Up @@ -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 {
Expand All @@ -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")
Expand Down
4 changes: 4 additions & 0 deletions sloglint_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
11 changes: 8 additions & 3 deletions sloglint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
33 changes: 0 additions & 33 deletions testdata/src/context_only/context_only.go

This file was deleted.

31 changes: 31 additions & 0 deletions testdata/src/context_only_all/context_only_all.go
Original file line number Diff line number Diff line change
@@ -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`
}
25 changes: 25 additions & 0 deletions testdata/src/context_only_scope/context_only_scope.go
Original file line number Diff line number Diff line change
@@ -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")
}

0 comments on commit f4014dd

Please sign in to comment.