Skip to content

Commit 1601df7

Browse files
Add custom Linter to not allow improper usage of logger.Error(nil, ...) calls
1 parent dcf50b8 commit 1601df7

File tree

5 files changed

+127
-1
lines changed

5 files changed

+127
-1
lines changed

Makefile

+10-1
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,18 @@ help-extended: #HELP Display extended help.
9595
#SECTION Development
9696

9797
.PHONY: lint
98-
lint: $(GOLANGCI_LINT) #HELP Run golangci linter.
98+
lint: lint-custom $(GOLANGCI_LINT) #HELP Run golangci linter.
9999
$(GOLANGCI_LINT) run --build-tags $(GO_BUILD_TAGS) $(GOLANGCI_LINT_ARGS)
100100

101+
.PHONY: custom-linter-build
102+
LINTER_DIR := ./hack/ci/custom-linters/cmd
103+
custom-linter-build: # HELP Build custom linter
104+
cd $(LINTER_DIR) && go build -tags '$(GO_BUILD_TAGS)' -o $(ROOT_DIR)/bin/custom-linter
105+
106+
.PHONY: lint-custom #HELP Call custom linter for the project
107+
lint-custom: custom-linter-build
108+
go vet -tags=$(GO_BUILD_TAGS) -vettool=./bin/custom-linter ./...
109+
101110
.PHONY: tidy
102111
tidy: #HELP Update dependencies.
103112
# Force tidy to use the version already in go.mod
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
package analyzers
2+
3+
import (
4+
"bytes"
5+
"fmt"
6+
"go/ast"
7+
"go/format"
8+
"go/token"
9+
"go/types"
10+
11+
"golang.org/x/tools/go/analysis"
12+
)
13+
14+
var SetupLogNilErrorCheck = &analysis.Analyzer{
15+
Name: "setuplognilerrorcheck",
16+
Doc: "Detects improper usage of logger.Error() calls, ensuring the first argument is a non-nil error.",
17+
Run: runSetupLogNilErrorCheck,
18+
}
19+
20+
func runSetupLogNilErrorCheck(pass *analysis.Pass) (interface{}, error) {
21+
for _, f := range pass.Files {
22+
ast.Inspect(f, func(n ast.Node) bool {
23+
callExpr, ok := n.(*ast.CallExpr)
24+
if !ok {
25+
return true
26+
}
27+
28+
// Ensure function being called is logger.Error
29+
selectorExpr, ok := callExpr.Fun.(*ast.SelectorExpr)
30+
if !ok || selectorExpr.Sel.Name != "Error" {
31+
return true
32+
}
33+
34+
// Ensure receiver (logger) is identified
35+
ident, ok := selectorExpr.X.(*ast.Ident)
36+
if !ok {
37+
return true
38+
}
39+
40+
// Verify if the receiver is logr.Logger
41+
obj := pass.TypesInfo.ObjectOf(ident)
42+
if obj == nil {
43+
return true
44+
}
45+
46+
named, ok := obj.Type().(*types.Named)
47+
if !ok || named.Obj().Pkg() == nil || named.Obj().Pkg().Path() != "github.com/go-logr/logr" || named.Obj().Name() != "Logger" {
48+
return true
49+
}
50+
51+
if len(callExpr.Args) == 0 {
52+
return true
53+
}
54+
55+
// Check if the first argument of the error log is nil
56+
firstArg, ok := callExpr.Args[0].(*ast.Ident)
57+
if !ok || firstArg.Name != "nil" {
58+
return true
59+
}
60+
61+
// Extract error message for the suggestion
62+
suggestedError := "errors.New(\"kind error (i.e. configuration error)\")"
63+
suggestedMessage := "\"error message describing the failed operation\""
64+
65+
if len(callExpr.Args) > 1 {
66+
if msgArg, ok := callExpr.Args[1].(*ast.BasicLit); ok && msgArg.Kind == token.STRING {
67+
suggestedMessage = msgArg.Value
68+
}
69+
}
70+
71+
// Get the actual source code line where the issue occurs
72+
var srcBuffer bytes.Buffer
73+
if err := format.Node(&srcBuffer, pass.Fset, callExpr); err == nil {
74+
sourceLine := srcBuffer.String()
75+
pass.Reportf(callExpr.Pos(),
76+
"Incorrect usage of 'logger.Error(nil, ...)'. The first argument must be a non-nil 'error'. "+
77+
"Passing 'nil' results in silent failures, making debugging harder.\n\n"+
78+
"\U0001F41B **What is wrong?**\n"+
79+
fmt.Sprintf(" %s\n\n", sourceLine)+
80+
"\U0001F4A1 **How to solve? Return the error, i.e.:**\n"+
81+
fmt.Sprintf(" logger.Error(%s, %s, \"key\", value)\n\n", suggestedError, suggestedMessage))
82+
}
83+
84+
return true
85+
})
86+
}
87+
return nil, nil
88+
}

hack/ci/custom-linters/cmd/main.go

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
package main
2+
3+
import (
4+
"github.com/operator-framework/operator-controller/hack/ci/custom-linters/analyzers"
5+
"golang.org/x/tools/go/analysis"
6+
"golang.org/x/tools/go/analysis/unitchecker"
7+
)
8+
9+
// Define the custom Linters implemented in the project
10+
var customLinters = []*analysis.Analyzer{
11+
analyzers.SetupLogNilErrorCheck,
12+
}
13+
14+
func main() {
15+
unitchecker.Main(customLinters...)
16+
}

hack/ci/custom-linters/go.mod

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
module github.com/operator-framework/operator-controller/hack/ci/custom-linters
2+
3+
go 1.23.4
4+
5+
require golang.org/x/tools v0.29.0

hack/ci/custom-linters/go.sum

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
2+
github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
3+
golang.org/x/mod v0.22.0 h1:D4nJWe9zXqHOmWqj4VMOJhvzj7bEZg4wEYa759z1pH4=
4+
golang.org/x/mod v0.22.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY=
5+
golang.org/x/sync v0.10.0 h1:3NQrjDixjgGwUOCaF8w2+VYHv0Ve/vGYSbdkTa98gmQ=
6+
golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
7+
golang.org/x/tools v0.29.0 h1:Xx0h3TtM9rzQpQuR4dKLrdglAmCEN5Oi+P74JdhdzXE=
8+
golang.org/x/tools v0.29.0/go.mod h1:KMQVMRsVxU6nHCFXrBPhDB8XncLNLM0lIy/F14RP588=

0 commit comments

Comments
 (0)