Merge pull request #907 from josephschorr/zerolog-lint-check

Add a lint check for zerolog expression statements without Send or Msg calls

.github/workflows/lint.yaml

@@ -48,7 +48,14 @@ jobs:
         with:
           working_directory: "tools/analyzers"
       - name: "Run custom analyzers"
-        run: './tools/analyzers/analyzers -skip-pkg "github.com/authzed/spicedb/pkg/proto/dispatch/v1" -disallowed-nil-return-type-paths "*github.com/authzed/spicedb/pkg/proto/dispatch/v1.DispatchCheckResponse,*github.com/authzed/spicedb/pkg/proto/dispatch/v1.DispatchExpandResponse,*github.com/authzed/spicedb/pkg/proto/dispatch/v1.DispatchLookupResponse" ./...'
+        run: >
+          ./tools/analyzers/analyzers
+          -nilvaluecheck
+          -nilvaluecheck.skip-pkg "github.com/authzed/spicedb/pkg/proto/dispatch/v1"
+          -nilvaluecheck.disallowed-nil-return-type-paths "*github.com/authzed/spicedb/pkg/proto/dispatch/v1.DispatchCheckResponse,*github.com/authzed/spicedb/pkg/proto/dispatch/v1.DispatchExpandResponse,*github.com/authzed/spicedb/pkg/proto/dispatch/v1.DispatchLookupResponse"
+          -exprstatementcheck
+          -exprstatementcheck.disallowed-expr-statement-types "*github.com/rs/zerolog.Event:MarshalZerologObject:missing Send or Msg on zerolog log Event"
+          ./...
       - uses: "authzed/actions/govulncheck@main"
 
   extra-lint:

tools/analyzers/cmd/analyzers/main.go

@@ -1,10 +1,11 @@
 package main
 
 import (
+	"github.com/authzed/spicedb/tools/analyzers/exprstatementcheck"
 	"github.com/authzed/spicedb/tools/analyzers/nilvaluecheck"
-	"golang.org/x/tools/go/analysis/singlechecker"
+	"golang.org/x/tools/go/analysis/multichecker"
 )
 
 func main() {
-	singlechecker.Main(nilvaluecheck.Analyzer())
+	multichecker.Main(nilvaluecheck.Analyzer(), exprstatementcheck.Analyzer())
 }

tools/analyzers/exprstatementcheck/exprstatementcheck.go

@@ -0,0 +1,123 @@
+package exprstatementcheck
+
+import (
+	"flag"
+	"fmt"
+	"go/ast"
+	"strings"
+
+	"github.com/jzelinskie/stringz"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+func sliceMap(s []string, f func(value string) string) []string {
+	mapped := make([]string, 0, len(s))
+	for _, value := range s {
+		mapped = append(mapped, f(value))
+	}
+	return mapped
+}
+
+type disallowedExprStatementConfig struct {
+	fullTypePath       string
+	errorMessage       string
+	ignoredMethodNames []string
+}
+
+func Analyzer() *analysis.Analyzer {
+	flagSet := flag.NewFlagSet("exprstatementcheck", flag.ExitOnError)
+	disallowedExprTypes := flagSet.String(
+		"disallowed-expr-statement-types",
+		"",
+		`semicolon delimited configuration of the types that should be disallowed as expression statements. 
+			Formats:
+				- "full type path:error message"
+				- "full type path:MethodNameUnderWhichToIgnore:error message"
+
+			Example: "*github.com/rs/zerolog.Event:MarshalZerologObject:error message here"
+		`,
+	)
+	skip := flagSet.String("skip-pkg", "", "package(s) to skip for linting")
+
+	return &analysis.Analyzer{
+		Name: "exprstatementcheck",
+		Doc:  "reports expression statements that have the disallowed value types",
+		Run: func(pass *analysis.Pass) (any, error) {
+			// Check for a skipped package.
+			if len(*skip) > 0 {
+				skipped := sliceMap(strings.Split(*skip, ","), strings.TrimSpace)
+				for _, s := range skipped {
+					if strings.Contains(pass.Pkg.Path(), s) {
+						return nil, nil
+					}
+				}
+			}
+
+			entries := strings.Split(*disallowedExprTypes, ";")
+			typePathsAndMessages := map[string]disallowedExprStatementConfig{}
+			for _, entry := range entries {
+				if len(entry) == 0 {
+					continue
+				}
+
+				parts := strings.Split(entry, ":")
+				if len(parts) == 2 {
+					typePathsAndMessages[parts[0]] = disallowedExprStatementConfig{
+						fullTypePath: parts[0],
+						errorMessage: parts[1],
+					}
+				} else if len(parts) == 3 {
+					typePathsAndMessages[parts[0]] = disallowedExprStatementConfig{
+						fullTypePath:       parts[0],
+						ignoredMethodNames: strings.Split(parts[1], ","),
+						errorMessage:       parts[2],
+					}
+				} else {
+					return nil, fmt.Errorf("invalid value for disallowed-expr-statement-types: `%s`", entry)
+				}
+			}
+
+			inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+			nodeFilter := []ast.Node{
+				(*ast.ExprStmt)(nil),
+			}
+
+			inspect.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool {
+			switchStatement:
+				switch s := n.(type) {
+				case *ast.ExprStmt:
+					foundType := pass.TypesInfo.TypeOf(s.X)
+					if foundType == nil {
+						return false
+					}
+
+					if config, ok := typePathsAndMessages[foundType.String()]; ok {
+						if len(config.ignoredMethodNames) > 0 {
+							for _, parent := range stack {
+								if funcDecl, ok := parent.(*ast.FuncDecl); ok {
+									if stringz.SliceContains(config.ignoredMethodNames, funcDecl.Name.Name) {
+										break switchStatement
+									}
+								}
+							}
+						}
+
+						pass.Reportf(s.Pos(), "In package %s: %s", pass.Pkg.Path(), config.errorMessage)
+					}
+
+					return false
+				}
+
+				return true
+			})
+
+			return nil, nil
+		},
+		Requires: []*analysis.Analyzer{inspect.Analyzer},
+		Flags:    *flagSet,
+	}
+}

tools/analyzers/exprstatementcheck/exprstatementcheck_test.go

@@ -0,0 +1,19 @@
+package exprstatementcheck
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestAnalyzer(t *testing.T) {
+	analyzer := Analyzer()
+	analyzer.Flags.Set("disallowed-expr-statement-types", "*missingsend.Event:missing Send or Msg for zerolog log statement")
+
+	// NOTE: importing of external packages is currently broken for analysistest,
+	// so we cannot check zerolog itself.
+	// See: https://github.com/golang/go/issues/46041
+
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, analyzer, "missingsend")
+}

tools/analyzers/exprstatementcheck/testdata/src/missingsend/log.go

@@ -0,0 +1,10 @@
+package _missingsend
+
+type Event struct{}
+
+func Err(err error) *Event {
+	return nil
+}
+
+func (e *Event) Send() {
+}

tools/analyzers/exprstatementcheck/testdata/src/missingsend/missingsend.go

@@ -0,0 +1,13 @@
+package _missingsend
+
+import (
+	"errors"
+)
+
+func SomeFunction() {
+	Err(errors.New("foo")) // want "missing Send or Msg for zerolog log statement"
+}
+
+func AnotherFunction() {
+	Err(errors.New("foo")).Send()
+}