From 06276f7db5276612741151f8d6582f8ba860db78 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 5 Jan 2026 18:33:36 +0100 Subject: [PATCH] logcheck: detect logger.V().Error This might be the result of a literal `klog.V().ErrorS` migration. But the semantic is different, which is worth calling out in the linter because it's not a compile error. --- logcheck/README.md | 14 ++++++++ logcheck/main_test.go | 12 ++++--- logcheck/pkg/logcheck.go | 47 +++++++++++++++++--------- logcheck/testdata/src/gologr/gologr.go | 3 +- 4 files changed, 54 insertions(+), 22 deletions(-) diff --git a/logcheck/README.md b/logcheck/README.md index e6fa9d8..83b2362 100644 --- a/logcheck/README.md +++ b/logcheck/README.md @@ -105,6 +105,20 @@ runtime in klog. This check flags all invocation of `klog.V(0)` or any of it's equivalent as errors +## verbosity-error (enabled by default) + +`logger.V(5).Error` for a `logr.Logger` instance is identical to `logger.Error` +because `logger.V(5)` is just another `Logger` instance and `Error` bypasses +all verbosity checks. This is different from `klog.V(5).ErrorS`, which does +the verbosity check. + +If the call really is an `Error` log call, i.e. with "an admin must know about +this" importance, then the preceding `V()` should be removed. If the semantic +from klog is desired, then the corresponding go-logr call is +`logger.V(5).Info`, with the error passed in a `"err", error` key/value +pair - at least in Kubernetes. Other projects may have different naming +conventions. + ## key (enabled by default) This check flags check whether name arguments are valid keys according to the diff --git a/logcheck/main_test.go b/logcheck/main_test.go index 6b715ad..ec1ebf4 100644 --- a/logcheck/main_test.go +++ b/logcheck/main_test.go @@ -112,17 +112,19 @@ func TestAnalyzer(t *testing.T) { { name: "Do not allow Verbosity Zero logs", enabled: map[string]string{ - "structured": "false", - "key": "false", + "structured": "false", + "key": "false", + "verbosity-error": "false", }, testPackage: "doNotAllowVerbosityZeroLogs", }, { name: "Allow Verbosity Zero logs", enabled: map[string]string{ - "structured": "false", - "verbosity-zero": "false", - "key": "false", + "structured": "false", + "verbosity-zero": "false", + "verbosity-error": "false", + "key": "false", }, testPackage: "allowVerbosityZeroLogs", }, diff --git a/logcheck/pkg/logcheck.go b/logcheck/pkg/logcheck.go index e655e02..2a1453c 100644 --- a/logcheck/pkg/logcheck.go +++ b/logcheck/pkg/logcheck.go @@ -34,14 +34,15 @@ import ( ) const ( - structuredCheck = "structured" - parametersCheck = "parameters" - contextualCheck = "contextual" - withHelpersCheck = "with-helpers" - verbosityZeroCheck = "verbosity-zero" - keyCheck = "key" - valueCheck = "value" - deprecationsCheck = "deprecations" + structuredCheck = "structured" + parametersCheck = "parameters" + contextualCheck = "contextual" + withHelpersCheck = "with-helpers" + verbosityZeroCheck = "verbosity-zero" + verbosityErrorCheck = "verbosity-error" + keyCheck = "key" + valueCheck = "value" + deprecationsCheck = "deprecations" ) type checks map[string]*bool @@ -72,14 +73,15 @@ func (c *Config) ParseConfig(configContent string) error { func Analyser() (*analysis.Analyzer, *Config) { c := Config{ enabled: checks{ - structuredCheck: new(bool), - parametersCheck: new(bool), - contextualCheck: new(bool), - withHelpersCheck: new(bool), - verbosityZeroCheck: new(bool), - keyCheck: new(bool), - valueCheck: new(bool), - deprecationsCheck: new(bool), + structuredCheck: new(bool), + parametersCheck: new(bool), + contextualCheck: new(bool), + withHelpersCheck: new(bool), + verbosityZeroCheck: new(bool), + verbosityErrorCheck: new(bool), + keyCheck: new(bool), + valueCheck: new(bool), + deprecationsCheck: new(bool), }, } c.fileOverrides.validChecks = map[string]bool{} @@ -94,6 +96,7 @@ klog methods (Info, Infof, Error, Errorf, Warningf, etc).`) logcheckFlags.BoolVar(c.enabled[contextualCheck], prefix+contextualCheck, false, `When true, logcheck will only allow log calls for contextual logging (retrieving a Logger from klog or the context and logging through that) and warn about all others.`) logcheckFlags.BoolVar(c.enabled[withHelpersCheck], prefix+withHelpersCheck, false, `When true, logcheck will warn about direct calls to WithName, WithValues and NewContext.`) logcheckFlags.BoolVar(c.enabled[verbosityZeroCheck], prefix+verbosityZeroCheck, true, `When true, logcheck will check whether the parameter for V() is 0.`) + logcheckFlags.BoolVar(c.enabled[verbosityErrorCheck], prefix+verbosityErrorCheck, true, `When true, logcheck will check for V() in front of a logr Error call.`) logcheckFlags.BoolVar(c.enabled[keyCheck], prefix+keyCheck, true, `When true, logcheck will check whether name arguments are valid keys according to the guidelines in (https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#name-arguments).`) logcheckFlags.BoolVar(c.enabled[valueCheck], prefix+valueCheck, false, `When true, logcheck will check for problematic values (for example, types that have an incomplete fmt.Stringer implementation).`) logcheckFlags.BoolVar(c.enabled[deprecationsCheck], prefix+deprecationsCheck, true, `When true, logcheck will analyze the usage of deprecated Klog function calls.`) @@ -293,6 +296,18 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *Config) { if c.isEnabled(verbosityZeroCheck, filename) { checkForVerbosityZero(fexpr, pass) } + + if fName == "Error" && c.isEnabled(verbosityErrorCheck, filename) { + // Is X itself the result of a selector expression with V as method name? + if innerCallExpr, ok := selExpr.X.(*ast.CallExpr); ok { + if innerSelExpr, ok := innerCallExpr.Fun.(*ast.SelectorExpr); ok && innerSelExpr.Sel.Name == "V" { + pass.Report(analysis.Diagnostic{ + Pos: innerSelExpr.Sel.Pos(), + Message: `V().Error ignores the verbosity and always logs. Use only Error if that is desired, otherwise V().Info(..., "err", err).`, + }) + } + } + } } else if fName == "NewContext" && isPackage(selExpr.X, "github.com/go-logr/logr", pass) && c.isEnabled(withHelpersCheck, filename) { diff --git a/logcheck/testdata/src/gologr/gologr.go b/logcheck/testdata/src/gologr/gologr.go index a3463e1..98d1130 100644 --- a/logcheck/testdata/src/gologr/gologr.go +++ b/logcheck/testdata/src/gologr/gologr.go @@ -33,7 +33,7 @@ func logging() { logger.WithValues("missing value") // want `Additional arguments to WithValues should always be Key Value pairs. Please check if there is any key or value missing.` logger.V(1).Info("hello", "missing value") // want `Additional arguments to Info should always be Key Value pairs. Please check if there is any key or value missing.` - logger.V(1).Error(nil, "hello", 1, 2) // want `Key positional arguments are expected to be inlined constant strings. Please replace 1 provided with string value` + logger.V(1).Error(nil, "hello", 1, 2) // want `Key positional arguments are expected to be inlined constant strings. Please replace 1 provided with string value` `V\(\).Error ignores the verbosity and always logs. Use only Error if that is desired, otherwise V\(\).Info\(..., "err", err\).` logger.V(1).WithValues("missing value") // want `Additional arguments to WithValues should always be Key Value pairs. Please check if there is any key or value missing.` // variadic input to logger.Info, logger.Error, logger.WithValues functions @@ -43,4 +43,5 @@ func logging() { logger.WithValues(kvs...) logger.WithValues(kvs) // want `Additional arguments to WithValues should always be Key Value pairs. Please check if there is any key or value missing.` logger.Error(nil, "foo error message", kvs) // want `Additional arguments to Error should always be Key Value pairs. Please check if there is any key or value missing.` + }