logcheck: support banning non-contextual functions and methods

Sometimes there are APIs with different variants where one variant is fine in
code which doesn't support contextual logging but should be replaced with some
other variant if that is the goal.

For example, k8s.io/apimachinery/pkg/util/runtime.HandleError is the old-style
function. We cannot deprecate it because it works fine in most existing
code. But the upcoming HandleErrorWithContext is what code in Kubernetes that
has (or will be) converted to contextual logging should use.

We also cannot forbid it through forbidigo, because then the logcheck.conf with
its definition of which code is "contextual" would be ignored. Besides,
maintaining a config for forbidigo would be continual effort that would need to
be replicated by other projects consuming the package with these APIs.

A better solution is to:
- mark these non-contextual APIs through a `//logcheck:context // <why>` comment
- detect those comments in logcheck
- warn about using them in contextual code

logcheck/README.md

@@ -56,6 +56,22 @@ through that.
 klog calls that are needed to manage contextual logging, for example
 `klog.Background`, are still allowed.
 
+Which of the klog functions are allowed is compiled into the logcheck binary.
+For functions or methods defined elsewhere, a special `//logcheck:context` can
+be added to trigger a warning about usage of such an API when contextual
+checking is enabled. Here is an example:
+
+    //logcheck:context // Foo should not be used in code which supports contextual logging.
+    func Foo() { ... }
+
+The additional explanation is optional. The default is the text above. It is recommended
+to mention what should be used instead, for example like this:
+
+    //logcheck:context // FooWithContext should be used instead of Foo in code which supports contextual logging.
+    func Foo() { ... }
+
+    func FooWithContext(ctx context.Context) { .... }
+
 ## parameters (enabled by default)
 
 This ensures that if certain logging functions are allowed and are used, those

logcheck/main_test.go

@@ -150,6 +150,13 @@ func TestAnalyzer(t *testing.T) {
 			},
 			testPackage: "stringer",
 		},
+		{
+			name: "logcheck facts",
+			enabled: map[string]string{
+				"contextual": "true",
+			},
+			testPackage: "k8s.io/apimachinery/pkg/util/runtime",
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {

logcheck/pkg/logcheck.go

@@ -124,10 +124,20 @@ klog methods (Info, Infof, Error, Errorf, Warningf, etc).`)
 		Run: func(pass *analysis.Pass) (interface{}, error) {
 			return run(pass, &c)
 		},
-		Flags: logcheckFlags,
+		Flags:     logcheckFlags,
+		FactTypes: []analysis.Fact{new(warnContextual)},
 	}, &c
 }
 
+// warnContextual is a fact that is set for methods or functions which have the
+// `logcheck:context // <comment>` comment. The value stored here is that
+// comment.
+type warnContextual string
+
+func (w warnContextual) AFact() {}
+
+func (w warnContextual) String() string { return string(w) }
+
 func run(pass *analysis.Pass, c *Config) (interface{}, error) {
 	for _, file := range pass.Files {
 		ast.Inspect(file, func(n ast.Node) bool {
@@ -140,6 +150,14 @@ func run(pass *analysis.Pass, c *Config) (interface{}, error) {
 				checkForContextAndLogger(n, n.Params, pass, c)
 			case *ast.IfStmt:
 				checkForIfEnabled(n, pass, c)
+			case *ast.FuncDecl:
+				checkForComments(pass.TypesInfo.ObjectOf(n.Name), n.Doc, pass)
+			case *ast.InterfaceType:
+				for _, method := range n.Methods.List {
+					for _, name := range method.Names {
+						checkForComments(pass.TypesInfo.ObjectOf(name), method.Doc, pass)
+					}
+				}
 			}
 
 			return true
@@ -152,6 +170,22 @@ func run(pass *analysis.Pass, c *Config) (interface{}, error) {
 func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *Config) {
 	fun := fexpr.Fun
 	args := fexpr.Args
+	filename := pass.Pkg.Path() + "/" + path.Base(pass.Fset.Position(fexpr.Pos()).Filename)
+	contextualCheckEnabled := c.isEnabled(contextualCheck, filename)
+
+	// Some function that is banned for contextual logging through comment?
+	if contextualCheckEnabled {
+		if ident, ok := fun.(*ast.Ident); ok {
+			object := pass.TypesInfo.ObjectOf(ident)
+			var why warnContextual
+			if pass.ImportObjectFact(object, &why) {
+				pass.Report(analysis.Diagnostic{
+					Pos:     fun.Pos(),
+					Message: string(why),
+				})
+			}
+		}
+	}
 
 	/* we are extracting external package function calls e.g. klog.Infof fmt.Printf
 	   and eliminating calls like setLocalHost()
@@ -161,11 +195,22 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *Config) {
 		// extracting function Name like Infof
 		fName := selExpr.Sel.Name
 
-		filename := pass.Pkg.Path() + "/" + path.Base(pass.Fset.Position(fexpr.Pos()).Filename)
 		valueCheckEnabled := c.isEnabled(valueCheck, filename)
 		keyCheckEnabled := c.isEnabled(keyCheck, filename)
 		parametersCheckEnabled := c.isEnabled(parametersCheck, filename)
 
+		// Some method that is banned for contextual logging through comment?
+		if contextualCheckEnabled {
+			object := pass.TypesInfo.ObjectOf(selExpr.Sel)
+			var why warnContextual
+			if pass.ImportObjectFact(object, &why) {
+				pass.Report(analysis.Diagnostic{
+					Pos:     selExpr.Sel.Pos(),
+					Message: string(why),
+				})
+			}
+		}
+
 		// Now we need to determine whether it is coming from klog.
 		if isKlog(selExpr.X, pass) {
 			if c.isEnabled(contextualCheck, filename) && !isContextualCall(fName) {
@@ -702,3 +747,41 @@ func isWrapperStruct(t types.Type) bool {
 
 	return false
 }
+
+func checkForComments(object types.Object, doc *ast.CommentGroup, pass *analysis.Pass) {
+	if object == nil || doc == nil {
+		return
+	}
+
+	for _, comment := range doc.List {
+		text := comment.Text
+		text, found := strings.CutPrefix(text, logcheckPrefix)
+		if !found {
+			continue
+		}
+		text, found = strings.CutPrefix(text, contextKeyword)
+		if !found {
+			pass.Report(analysis.Diagnostic{
+				Pos:     comment.Pos(),
+				Message: "unknown logcheck keyword in comment",
+			})
+			continue
+		}
+		text = strings.TrimSpace(text)
+		why := warnContextual(fmt.Sprintf("%s should not be used in code which supports contextual logging.", object.Name()))
+		text, found = strings.CutPrefix(text, commentSep)
+		if found {
+			text = strings.TrimSpace(text)
+			if len(text) > 0 {
+				why = warnContextual(text)
+			}
+		}
+		pass.ExportObjectFact(object, &why)
+	}
+}
+
+const (
+	logcheckPrefix = "//logcheck:"
+	contextKeyword = "context"
+	commentSep     = "//"
+)

logcheck/testdata/src/allowUnstructuredLogs/allowUnstructuredLogs.go

@@ -22,6 +22,7 @@ limitations under the License.
 package allowUnstructuredLogs
 
 import (
+	"k8s.io/apimachinery/pkg/util/runtime"
 	klog "k8s.io/klog/v2"
 )
 
@@ -64,4 +65,5 @@ func allowUnstructuredLogs() {
 	klog.ExitDepth(1, "test log")
 	klog.Exitln("test log")
 	klog.Exitf("test log")
+	runtime.HandleError(nil)
 }

logcheck/testdata/src/k8s.io/apimachinery/pkg/util/runtime/runtime.go

@@ -0,0 +1,51 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+// This fake package is created as package golang.org/x/tools/go/analysis/analysistest
+// expects test data dependency to be testdata/src
+package runtime
+
+import "context"
+
+//logcheck:context // Use HandleErrorWithContext instead in code which supports contextual logging.
+func HandleError(err error) { // want HandleError:"Use HandleErrorWithContext instead in code which supports contextual logging."
+}
+
+func HandleErrorWithContext(ctx context.Context, err error) {
+}
+
+//logcheck:xyz // want "unknown logcheck keyword in comment"
+func Foo() {
+}
+
+//logcheck:context
+func Bar() { // want Bar:"Bar should not be used in code which supports contextual logging."
+}
+
+type Handler interface {
+	//logcheck:context//Use HandleErrorWithContext instead in code which supports contextual logging.
+	HandleError(err error) // want HandleError:"Use HandleErrorWithContext instead in code which supports contextual logging."
+
+	HandleErrorWithContext(ctx context.Context, err error)
+}
+
+func foo(ctx context.Context, err error) {
+	HandleError(nil) // want "Use HandleErrorWithContext instead in code which supports contextual logging."
+	HandleErrorWithContext(ctx, err)
+	var h Handler
+	h.HandleError(err) // want "Use HandleErrorWithContext instead in code which supports contextual logging."
+	h.HandleErrorWithContext(ctx, err)
+}

logcheck/testdata/src/onlyAllowContextual/onlyAllowContextual.go

@@ -22,6 +22,9 @@ limitations under the License.
 package onlyallowcontextual
 
 import (
+	"context"
+
+	"k8s.io/apimachinery/pkg/util/runtime"
 	klog "k8s.io/klog/v2"
 )
 
@@ -33,3 +36,11 @@ func doNotAlllowKlog() {
 	klog.KObjs(nil)                                // want `Detected usage of deprecated helper "KObjs". Please switch to "KObjSlice" instead.`
 	klog.InfoS("test log", "key", klog.KObjs(nil)) // want `function "InfoS" should not be used, convert to contextual logging` `Detected usage of deprecated helper "KObjs". Please switch to "KObjSlice" instead.`
 }
+
+func doNotAllNonContext(ctx context.Context) {
+	runtime.HandleError(nil) // want `Use HandleErrorWithContext instead in code which supports contextual logging.`
+	runtime.HandleErrorWithContext(ctx, nil)
+	var handler runtime.Handler
+	handler.HandleError(nil) // want `Use HandleErrorWithContext instead in code which supports contextual logging.`
+	handler.HandleErrorWithContext(ctx, nil)
+}