Skip to content

Commit b2ac48b

Browse files
committed
Adds a linter for the ocm logger
Linting issues in using ocm logger were not detected by the linter because the logger methods are not ending with `f` and the first parameter is not the string format. This PR adds a custom linter to detect them.
1 parent 9747853 commit b2ac48b

File tree

11 files changed

+310
-7
lines changed

11 files changed

+310
-7
lines changed

.custom-gcl.yml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
version: v2.1.6
2+
3+
name: ocm-lint
4+
5+
destination: ./bin
6+
7+
plugins:
8+
- module: 'github.com/openshift-online/ocm-sdk-go'
9+
import: 'github.com/openshift-online/ocm-sdk-go/linters'
10+
path: .

.golangci.yml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,13 @@ version: "2"
22
run:
33
issues-exit-code: 1
44
linters:
5+
settings:
6+
custom:
7+
ocmlogger:
8+
type: "module"
9+
description: "none"
10+
enable:
11+
- ocmlogger
512
exclusions:
613
generated: lax
714
presets:

Makefile

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,8 @@ fmt:
9797
.PHONY: lint
9898
lint:
9999
golangci-lint --version
100-
golangci-lint run
100+
golangci-lint custom
101+
$(LOCAL_BIN_PATH)/ocm-lint run
101102

102103
# set "normal" alias matching kube and ocp
103104
.PHONY: update

examples/go.mod

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,11 @@ require (
5050
github.com/prometheus/procfs v0.7.3 // indirect
5151
github.com/skratchdot/open-golang v0.0.0-20200116055534-eef842397966 // indirect
5252
github.com/zalando/go-keyring v0.2.3 // indirect
53-
golang.org/x/net v0.21.0 // indirect
53+
golang.org/x/net v0.39.0 // indirect
5454
golang.org/x/oauth2 v0.15.0 // indirect
55-
golang.org/x/sys v0.17.0 // indirect
56-
golang.org/x/term v0.17.0 // indirect
57-
golang.org/x/text v0.14.0 // indirect
55+
golang.org/x/sys v0.32.0 // indirect
56+
golang.org/x/term v0.31.0 // indirect
57+
golang.org/x/text v0.24.0 // indirect
5858
google.golang.org/appengine v1.6.7 // indirect
5959
google.golang.org/protobuf v1.31.0 // indirect
6060
gopkg.in/inf.v0 v0.9.1 // indirect

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,4 +70,4 @@ require (
7070
golang.org/x/tools v0.14.0 // indirect
7171
google.golang.org/appengine v1.6.7 // indirect
7272
google.golang.org/protobuf v1.31.0 // indirect
73-
)
73+
)

go.sum

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -677,4 +677,4 @@ honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9
677677
honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
678678
rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=
679679
rsc.io/quote/v3 v3.1.0/go.mod h1:yEA65RcK8LyAZtP9Kv3t0HmxON59tX3rD+tICJqUlj0=
680-
rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA=
680+
rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA=

linters/linters_suite_test.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
package linters_test
2+
3+
import (
4+
"testing"
5+
6+
. "github.com/onsi/ginkgo/v2"
7+
. "github.com/onsi/gomega"
8+
)
9+
10+
func TestLinters(t *testing.T) {
11+
RegisterFailHandler(Fail)
12+
RunSpecs(t, "Linters Suite")
13+
}

linters/ocm_logging_linter.go

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
package linters
2+
3+
import (
4+
"go/ast"
5+
"go/token"
6+
"slices"
7+
"strconv"
8+
"strings"
9+
10+
"github.com/golangci/plugin-module-register/register"
11+
"golang.org/x/tools/go/analysis"
12+
)
13+
14+
func init() {
15+
register.Plugin("ocmlogger", New)
16+
}
17+
18+
// OcmLoggerLinter checks that calls to the OCM logger (e.g. `logger.Info`, `logger.Warn`, etc.) use format strings correctly.
19+
//
20+
// Specifically, it verifies that the number of format specifiers (e.g. `%v`, `%s`, `%d`, ...)
21+
// in the log message matches the number of arguments passed after the format string.
22+
//
23+
// Example of a valid call:
24+
//
25+
// logger.Warn(ctx, "failed to create resource %s: %v", name, err)
26+
//
27+
// Example of an invalid call (missing one argument):
28+
//
29+
// logger.Warn(ctx, "failed to create resource %s: %v", name)
30+
//
31+
// The analyzer only runs on calls whose receiver is of type
32+
// `github.com/openshift-online/ocm-sdk-go/logging.Logger` (or a pointer to it).
33+
//
34+
// To disable the check for a specific line, add one of the following comments:
35+
//
36+
// //nolint:ocmlogger
37+
// // ocm-linter:ignore
38+
//
39+
// Example:
40+
//
41+
// logger.Info(ctx, "%s %d", name) //nolint:ocmlogger
42+
//
43+
// Comments can be placed on the same line (or the line immediately above) the call.
44+
//
45+
// var OcmLoggerLinter = &analysis.Analyzer{
46+
// Name: "ocmlogger",
47+
// Doc: "checks that log calls have matching format strings and arguments",
48+
// Run: run,
49+
// }
50+
type OcmLoggerLinter struct{}
51+
52+
func (f *OcmLoggerLinter) GetLoadMode() string {
53+
return register.LoadModeSyntax
54+
}
55+
56+
func New(settings any) (register.LinterPlugin, error) {
57+
return &OcmLoggerLinter{}, nil
58+
}
59+
60+
func (f *OcmLoggerLinter) BuildAnalyzers() ([]*analysis.Analyzer, error) {
61+
return []*analysis.Analyzer{
62+
{
63+
Name: "ocmlogger",
64+
Doc: "find ocm logging usage errors",
65+
Run: f.run,
66+
},
67+
}, nil
68+
}
69+
70+
func (f *OcmLoggerLinter) isDisabled(pass *analysis.Pass, node ast.Node) bool {
71+
pos := pass.Fset.Position(node.Pos())
72+
for _, f := range pass.Files {
73+
for _, cg := range f.Comments {
74+
for _, c := range cg.List {
75+
cpos := pass.Fset.Position(c.Pos())
76+
if cpos.Filename != pos.Filename {
77+
continue
78+
}
79+
// same line or line above
80+
if cpos.Line == pos.Line || cpos.Line == pos.Line-1 {
81+
txt := c.Text // ex: "//nolint:ocmlogger" or "/* ... */"
82+
if strings.Contains(txt, "nolint:ocmlogger") || strings.Contains(txt, "ocm-linter:ignore") {
83+
return true
84+
}
85+
}
86+
}
87+
}
88+
}
89+
return false
90+
}
91+
92+
func (f *OcmLoggerLinter) extractString(expr ast.Expr) (string, bool) {
93+
switch e := expr.(type) {
94+
case *ast.BasicLit:
95+
if e.Kind == token.STRING {
96+
s, err := strconv.Unquote(e.Value)
97+
if err != nil {
98+
return "", false
99+
}
100+
return s, true
101+
}
102+
case *ast.BinaryExpr:
103+
if e.Op == token.ADD {
104+
left, ok1 := f.extractString(e.X)
105+
right, ok2 := f.extractString(e.Y)
106+
if ok1 && ok2 {
107+
return left + right, true
108+
}
109+
}
110+
}
111+
return "", false
112+
}
113+
114+
func (f *OcmLoggerLinter) countPlaceholders(fmtString string) int {
115+
count := 0
116+
fmtStringLength := len(fmtString)
117+
for i := 0; i < fmtStringLength; i++ {
118+
if fmtString[i] != '%' {
119+
continue
120+
}
121+
// skip "%%"
122+
if i+1 < fmtStringLength && fmtString[i+1] == '%' {
123+
i++ // skip both
124+
continue
125+
}
126+
// placeholder found
127+
count++
128+
}
129+
return count
130+
}
131+
132+
func (f *OcmLoggerLinter) run(pass *analysis.Pass) (interface{}, error) {
133+
134+
loggerMethods := []string{"Debug", "Info", "Warn", "Error", "Fatal"}
135+
136+
for _, file := range pass.Files {
137+
ast.Inspect(file, func(n ast.Node) bool {
138+
call, ok := n.(*ast.CallExpr)
139+
if !ok {
140+
return true
141+
}
142+
143+
sel, ok := call.Fun.(*ast.SelectorExpr)
144+
if !ok {
145+
return true
146+
}
147+
148+
if !slices.Contains(loggerMethods, sel.Sel.Name) {
149+
return true
150+
}
151+
152+
// Get the static type of the receiver
153+
recvType := pass.TypesInfo.TypeOf(sel.X)
154+
if recvType == nil {
155+
return true
156+
}
157+
158+
// Check that the type is exactly github.com/openshift-online/ocm-sdk-go/logging.Logger or a pointer to that type
159+
typeString := recvType.String()
160+
if typeString != "github.com/openshift-online/ocm-sdk-go/logging.Logger" &&
161+
typeString != "*github.com/openshift-online/ocm-sdk-go/logging.Logger" {
162+
return true
163+
}
164+
165+
if len(call.Args) < 2 {
166+
// If the number of parameters is less than 2, it is a compilation error
167+
return true
168+
}
169+
170+
// The second argument must be a string literal. If it is not, we cannot validate the number of
171+
// format arguments, so we ignore the line
172+
formatArg := call.Args[1]
173+
fmtString, ok := f.extractString(formatArg)
174+
if !ok {
175+
return true
176+
}
177+
178+
// Count the placeholders
179+
countPlaceholders := f.countPlaceholders(fmtString)
180+
181+
// Number of variadic arguments (after ctx and format)
182+
variadicCount := len(call.Args) - 2
183+
184+
if countPlaceholders != variadicCount && !f.isDisabled(pass, call) {
185+
pass.Reportf(call.Pos(),
186+
"number of format placeholders (%d) does not match number of arguments (%d)",
187+
countPlaceholders, variadicCount)
188+
}
189+
190+
return true
191+
})
192+
}
193+
return nil, nil
194+
}

linters/ocm_logging_linter_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
package linters
2+
3+
import (
4+
"github.com/golangci/plugin-module-register/register"
5+
"github.com/onsi/gomega"
6+
"golang.org/x/tools/go/analysis/analysistest"
7+
8+
. "github.com/onsi/ginkgo/v2"
9+
)
10+
11+
var _ = Describe("ocmlogger analyzer", func() {
12+
It("reports only expected diagnostics according to // want comments", func() {
13+
td := analysistest.TestData()
14+
newPlugin, err := register.GetPlugin("ocmlogger")
15+
gomega.Expect(err).NotTo(gomega.HaveOccurred())
16+
17+
plugin, err := newPlugin(nil)
18+
gomega.Expect(err).NotTo(gomega.HaveOccurred())
19+
20+
analyzers, err := plugin.BuildAnalyzers()
21+
gomega.Expect(err).NotTo(gomega.HaveOccurred())
22+
analysistest.Run(GinkgoT(), td, analyzers[0], "data")
23+
})
24+
})

linters/testdata/src/data/data.go

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
package data
2+
3+
import (
4+
"context"
5+
6+
"github.com/openshift-online/ocm-sdk-go/logging"
7+
)
8+
9+
var (
10+
ctx = context.Background()
11+
log = logging.Logger{}
12+
)
13+
14+
func ok() {
15+
log.Info(ctx, "hello %s %d", "world", 42) // ok
16+
log.Warn(ctx, "pi is %.2f", 3.14) // ok (placeholder with precision)
17+
log.Fatal(ctx, `%% %d`, 1) // ok, percent symbol and one parameter
18+
log.Fatal(ctx, `%d %% %d`, 1, 2) // ok, percent symbol between two parameters
19+
log.Fatal(ctx, `%d %d %%`, 1, 2) // ok, percent symbol as last character
20+
}
21+
22+
func mismatch() {
23+
log.Error(ctx, "name=%s age=%d", "alice") // want "number of format placeholders .* does not match"
24+
log.Error(ctx, "name=%s %% age=%d", "alice") // want "number of format placeholders .* does not match"
25+
}
26+
27+
func dynamicFormatIgnored() {
28+
f := "x=%d"
29+
log.Info(ctx, f, 1) // format is not a literal -> no errors expected
30+
}
31+
32+
func nolintSameLine() {
33+
// linting is disabled: no error expected although the parameters are wrong
34+
log.Info(ctx, "u=%d v=%d", 1) //nolint:ocmlogger
35+
}
36+
37+
func nolintPrevLine() {
38+
// linting is disabled: no error expected although the parameters are wrong
39+
//nolint:ocmlogger
40+
log.Info(ctx, "x=%d y=%d", 1)
41+
}

0 commit comments

Comments
 (0)