diff --git a/sloglint.go b/sloglint.go index f3b06c9..dde1b1c 100644 --- a/sloglint.go +++ b/sloglint.go @@ -261,18 +261,19 @@ func visit(pass *analysis.Pass, opts *Options, node ast.Node, stack []ast.Node) pass.Reportf(call.Pos(), "arguments should be put on separate lines") } - if len(opts.ForbiddenKeys) > 0 && badKeyNames(pass.TypesInfo, forbiddenKeysUsed(opts.ForbiddenKeys), keys, attrs) { - pass.Reportf(call.Pos(), "keys include forbidden values") + // NOTE: checkForbiddenKeys returns early with found=false if opts.ForbiddenKeys is empty. + if name, found := checkForbiddenKeys(pass.TypesInfo, opts.ForbiddenKeys, keys, attrs); found { + pass.Reportf(call.Pos(), "%q key is forbidden and should not be used", name) } switch { - case opts.KeyNamingCase == snakeCase && badKeyNames(pass.TypesInfo, valueChanged(strcase.ToSnake), keys, attrs): + 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, valueChanged(strcase.ToKebab), keys, attrs): + 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, valueChanged(strcase.ToCamel), keys, attrs): + 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, valueChanged(strcase.ToPascal), keys, attrs): + case opts.KeyNamingCase == pascalCase && badKeyNames(pass.TypesInfo, strcase.ToPascal, keys, attrs): pass.Reportf(call.Pos(), "keys should be written in PascalCase") } } @@ -367,9 +368,9 @@ func rawKeysUsed(info *types.Info, keys, attrs []ast.Expr) bool { return false } -func badKeyNames(info *types.Info, caseFn func(string) bool, keys, attrs []ast.Expr) bool { +func badKeyNames(info *types.Info, caseFn func(string) string, keys, attrs []ast.Expr) bool { for _, key := range keys { - if name, ok := getKeyName(key); ok && caseFn(name) { + if name, ok := getKeyName(key); ok && name != caseFn(name) { return true } } @@ -405,7 +406,7 @@ func badKeyNames(info *types.Info, caseFn func(string) bool, keys, attrs []ast.E } } - if name, ok := getKeyName(expr); ok && caseFn(name) { + if name, ok := getKeyName(expr); ok && name != caseFn(name) { return true } } @@ -413,6 +414,56 @@ func badKeyNames(info *types.Info, caseFn func(string) bool, keys, attrs []ast.E return false } +// checkForbiddenKeys returns the first forbidden key found in keys or attrs or an empty string. +func checkForbiddenKeys(info *types.Info, forbiddenKeys []string, keys, attrs []ast.Expr) (string, bool) { + if len(forbiddenKeys) == 0 { + return "", false + } + for _, key := range keys { + if name, ok := getKeyName(key); ok && slices.Contains(forbiddenKeys, name) { + return name, true + } + } + + for _, attr := range attrs { + var expr ast.Expr + + switch attr := attr.(type) { + case *ast.CallExpr: // e.g. slog.Int() + fn := typeutil.StaticCallee(info, attr) + if fn == nil { + continue + } + if _, ok := attrFuncs[fn.FullName()]; !ok { + continue + } + expr = attr.Args[0] + + case *ast.CompositeLit: // slog.Attr{} + switch len(attr.Elts) { + case 1: // slog.Attr{Key: ...} | slog.Attr{Value: ...} + if kv := attr.Elts[0].(*ast.KeyValueExpr); kv.Key.(*ast.Ident).Name == "Key" { + expr = kv.Value + } + case 2: // slog.Attr{..., ...} | slog.Attr{Key: ..., Value: ...} + expr = attr.Elts[0] + if kv1, ok := attr.Elts[0].(*ast.KeyValueExpr); ok && kv1.Key.(*ast.Ident).Name == "Key" { + expr = kv1.Value + } + if kv2, ok := attr.Elts[1].(*ast.KeyValueExpr); ok && kv2.Key.(*ast.Ident).Name == "Key" { + expr = kv2.Value + } + } + } + + if name, ok := getKeyName(expr); ok && slices.Contains(forbiddenKeys, name) { + return name, true + } + } + + return "", false +} + func getKeyName(expr ast.Expr) (string, bool) { if expr == nil { return "", false @@ -427,7 +478,10 @@ func getKeyName(expr ast.Expr) (string, bool) { } } if lit, ok := expr.(*ast.BasicLit); ok && lit.Kind == token.STRING { - return lit.Value, true + // For string literals, we need to trim the quotes to get the actual key name. + // e.g. "foo" -> foo, 'foo' -> foo, `foo` -> foo + // This is necessary because the lit.Value is always quoted. + return trimQuotes(lit.Value), true } return "", false } @@ -455,20 +509,6 @@ func argsOnSameLine(fset *token.FileSet, call ast.Expr, keys, attrs []ast.Expr) return false } -func forbiddenKeysUsed(forbiddenKeys []string) func(string) bool { - return func(s string) bool { - // Strip quotes if any. - unquoted := trimQuotes(s) - return slices.Contains(forbiddenKeys, unquoted) - } -} - -func valueChanged(handler func(string) string) func(string) bool { - return func(s string) bool { - return handler(s) != s - } -} - func trimQuotes(s string) string { if len(s) >= 2 { if c := s[len(s)-1]; s[0] == c && (c == '"' || c == '\'' || c == '`') { diff --git a/sloglint_test.go b/sloglint_test.go index e82402c..a84c70b 100644 --- a/sloglint_test.go +++ b/sloglint_test.go @@ -65,8 +65,8 @@ func TestAnalyzer(t *testing.T) { analysistest.Run(t, testdata, analyzer, "args_on_sep_lines") }) - t.Run("key foo_bar is forbbiden case", func(t *testing.T) { + t.Run("forbidden keys", func(t *testing.T) { analyzer := sloglint.New(&sloglint.Options{ForbiddenKeys: []string{"foo_bar"}}) - analysistest.Run(t, testdata, analyzer, "key_forbidden") + analysistest.Run(t, testdata, analyzer, "forbidden_keys") }) } diff --git a/testdata/src/forbidden_keys/forbidden_keys.go b/testdata/src/forbidden_keys/forbidden_keys.go new file mode 100644 index 0000000..c410393 --- /dev/null +++ b/testdata/src/forbidden_keys/forbidden_keys.go @@ -0,0 +1,30 @@ +package forbidden_keys + +import "log/slog" + +const ( + snakeKey = "foo_bar" +) + +func tests() { + slog.Info("msg") + slog.Info("msg", "foo-bar", 1) + slog.Info("msg", "foo_bar", 1) // want `"foo_bar" key is forbidden and should not be used` + slog.Info("msg", snakeKey, 1) // want `"foo_bar" key is forbidden and should not be used` + slog.Info("msg", slog.Int("foo_bar", 1)) // want `"foo_bar" key is forbidden and should not be used` + slog.Info("msg", slog.Int(snakeKey, 1)) // want `"foo_bar" key is forbidden and should not be used` + slog.Info("msg", slog.Attr{}) + slog.Info("msg", slog.Attr{"foo_bar", slog.IntValue(1)}) // want `"foo_bar" key is forbidden and should not be used` + slog.Info("msg", slog.Attr{snakeKey, slog.IntValue(1)}) // want `"foo_bar" key is forbidden and should not be used` + slog.Info("msg", slog.Attr{Key: "foo_bar"}) // want `"foo_bar" key is forbidden and should not be used` + slog.Info("msg", slog.Attr{Key: snakeKey}) // want `"foo_bar" key is forbidden and should not be used` + slog.Info("msg", slog.Attr{Key: "foo_bar", Value: slog.IntValue(1)}) // want `"foo_bar" key is forbidden and should not be used` + slog.Info("msg", slog.Attr{Key: snakeKey, Value: slog.IntValue(1)}) // want `"foo_bar" key is forbidden and should not be used` + slog.Info("msg", slog.Attr{Value: slog.IntValue(1), Key: "foo_bar"}) // want `"foo_bar" key is forbidden and should not be used` + slog.Info("msg", slog.Attr{Value: slog.IntValue(1), Key: snakeKey}) // want `"foo_bar" key is forbidden and should not be used` + slog.With(slog.Attr{"foo_bar", slog.IntValue(1)}).Info("msg") // want `"foo_bar" key is forbidden and should not be used` + slog.With(slog.Attr{snakeKey, slog.IntValue(1)}) // want `"foo_bar" key is forbidden and should not be used` + slog.With(snakeKey, slog.IntValue(1)) // want `"foo_bar" key is forbidden and should not be used` + slog.With("foo_bar", slog.IntValue(1)) // want `"foo_bar" key is forbidden and should not be used` + slog.Info("msg", slog.Attr{Value: slog.IntValue(1), Key: `foo_bar`}) // want `"foo_bar" key is forbidden and should not be used` +} diff --git a/testdata/src/key_forbidden/key_forbidden.go b/testdata/src/key_forbidden/key_forbidden.go deleted file mode 100644 index c709c12..0000000 --- a/testdata/src/key_forbidden/key_forbidden.go +++ /dev/null @@ -1,30 +0,0 @@ -package key_forbidden - -import "log/slog" - -const ( - snakeKey = "foo_bar" -) - -func tests() { - slog.Info("msg") - slog.Info("msg", "foo-bar", 1) - slog.Info("msg", "foo_bar", 1) // want `keys include forbidden values` - slog.Info("msg", snakeKey, 1) // want `keys include forbidden values` - slog.Info("msg", slog.Int("foo_bar", 1)) // want `keys include forbidden values` - slog.Info("msg", slog.Int(snakeKey, 1)) // want `keys include forbidden values` - slog.Info("msg", slog.Attr{}) - slog.Info("msg", slog.Attr{"foo_bar", slog.IntValue(1)}) // want `keys include forbidden values` - slog.Info("msg", slog.Attr{snakeKey, slog.IntValue(1)}) // want `keys include forbidden values` - slog.Info("msg", slog.Attr{Key: "foo_bar"}) // want `keys include forbidden values` - slog.Info("msg", slog.Attr{Key: snakeKey}) // want `keys include forbidden values` - slog.Info("msg", slog.Attr{Key: "foo_bar", Value: slog.IntValue(1)}) // want `keys include forbidden values` - slog.Info("msg", slog.Attr{Key: snakeKey, Value: slog.IntValue(1)}) // want `keys include forbidden values` - slog.Info("msg", slog.Attr{Value: slog.IntValue(1), Key: "foo_bar"}) // want `keys include forbidden values` - slog.Info("msg", slog.Attr{Value: slog.IntValue(1), Key: snakeKey}) // want `keys include forbidden values` - slog.With(slog.Attr{"foo_bar", slog.IntValue(1)}).Info("msg") // want `keys include forbidden values` - slog.With(slog.Attr{snakeKey, slog.IntValue(1)}) // want `keys include forbidden values` - slog.With(snakeKey, slog.IntValue(1)) // want `keys include forbidden values` - slog.With("foo_bar", slog.IntValue(1)) // want `keys include forbidden values` - slog.Info("msg", slog.Attr{Value: slog.IntValue(1), Key: `foo_bar`}) // want `keys include forbidden values` -}