logcheck: detect logger.V().Error

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

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",
 		},

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

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.`
+
 }