Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement -context-only=scope #33

Merged
merged 4 commits into from
Apr 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@
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 @@
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 @@
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 @@
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 @@
)

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

Check warning on line 224 in sloglint.go

View check run for this annotation

Codecov / codecov/patch

sloglint.go#L224

Added line #L224 was not covered by tests
}

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")

Check warning on line 260 in sloglint.go

View check run for this annotation

Codecov / codecov/patch

sloglint.go#L255-L260

Added lines #L255 - L260 were not covered by tests
}
}

func globalLoggerUsed(info *types.Info, expr ast.Expr) bool {
Expand All @@ -247,6 +274,24 @@
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")
}
Loading