diff --git a/tools/analyzers/cmd/analyzers/main.go b/tools/analyzers/cmd/analyzers/main.go index f878e6f93b..899b6a065e 100644 --- a/tools/analyzers/cmd/analyzers/main.go +++ b/tools/analyzers/cmd/analyzers/main.go @@ -17,6 +17,7 @@ import ( "github.com/authzed/spicedb/tools/analyzers/paniccheck" "github.com/authzed/spicedb/tools/analyzers/protomarshalcheck" "github.com/authzed/spicedb/tools/analyzers/singleflightcheck" + "github.com/authzed/spicedb/tools/analyzers/staticformatcheck" "github.com/authzed/spicedb/tools/analyzers/telemetryconvcheck" "github.com/authzed/spicedb/tools/analyzers/zerologmarshalcheck" ) @@ -34,6 +35,7 @@ func main() { protomarshalcheck.Analyzer(), zerologmarshalcheck.Analyzer(), singleflightcheck.Analyzer(), + staticformatcheck.Analyzer(), telemetryconvcheck.Analyzer(), } diff --git a/tools/analyzers/staticformatcheck/staticformatcheck.go b/tools/analyzers/staticformatcheck/staticformatcheck.go new file mode 100644 index 0000000000..95aa89746d --- /dev/null +++ b/tools/analyzers/staticformatcheck/staticformatcheck.go @@ -0,0 +1,84 @@ +package staticformatcheck + +import ( + "go/ast" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +// checkedFuncNames holds the names of the format-style functions whose format +// string argument must be a compile-time constant. The format string is +// expected to be the last argument before the variadic format arguments. +// +// Matching is intentionally by name only, not scoped to a type or package, so +// names here must be distinctive enough not to collide with unrelated +// same-named methods. WithSourceErrorf is unique to the schema compiler's +// dslNode; a generic name like Errorf would over-match fmt.Errorf, +// testing.T.Errorf, and others and cannot be added without type scoping. +var checkedFuncNames = map[string]struct{}{ + "WithSourceErrorf": {}, +} + +func Analyzer() *analysis.Analyzer { + return &analysis.Analyzer{ + Name: "staticformatcheck", + Doc: "reports calls to WithSourceErrorf whose format string argument is not a compile-time constant", + Run: run, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + } +} + +func run(pass *analysis.Pass) (any, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} + inspect.Preorder(nodeFilter, func(n ast.Node) { + call := n.(*ast.CallExpr) + + selectorExpr, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return + } + + if _, ok := checkedFuncNames[selectorExpr.Sel.Name]; !ok { + return + } + + // Ensure the callee is a function and derive the format string index + // from the call site's signature: the format string is the parameter + // immediately before the variadic format arguments. For method + // expressions the receiver appears as the first parameter, which shifts + // the format string index accordingly. + if _, ok := pass.TypesInfo.Uses[selectorExpr.Sel].(*types.Func); !ok { + return + } + + signature, ok := pass.TypesInfo.TypeOf(call.Fun).(*types.Signature) + if !ok || !signature.Variadic() { + return + } + + formatArgIndex := signature.Params().Len() - 2 + if formatArgIndex < 0 || len(call.Args) <= formatArgIndex { + return + } + + formatParamType, ok := signature.Params().At(formatArgIndex).Type().(*types.Basic) + if !ok || formatParamType.Kind() != types.String { + return + } + + formatArg := call.Args[formatArgIndex] + if tv, ok := pass.TypesInfo.Types[formatArg]; ok && tv.Value != nil { + // The format string is a compile-time constant. + return + } + + pass.Reportf(formatArg.Pos(), "format string argument to `%s` must be a static string", selectorExpr.Sel.Name) + }) + + return nil, nil +} diff --git a/tools/analyzers/staticformatcheck/staticformatcheck_test.go b/tools/analyzers/staticformatcheck/staticformatcheck_test.go new file mode 100644 index 0000000000..b997b53ad0 --- /dev/null +++ b/tools/analyzers/staticformatcheck/staticformatcheck_test.go @@ -0,0 +1,15 @@ +package staticformatcheck + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAnalyzer(t *testing.T) { + analyzer := Analyzer() + + testdata := analysistest.TestData() + analysistest.Run(t, testdata, analyzer, "disallowedformat") + analysistest.Run(t, testdata, analyzer, "validformat") +} diff --git a/tools/analyzers/staticformatcheck/testdata/src/disallowedformat/disallowedformat.go b/tools/analyzers/staticformatcheck/testdata/src/disallowedformat/disallowedformat.go new file mode 100644 index 0000000000..4cb90941ba --- /dev/null +++ b/tools/analyzers/staticformatcheck/testdata/src/disallowedformat/disallowedformat.go @@ -0,0 +1,28 @@ +package disallowedformat + +// NOTE: this def is just to get us to a `WithSourceErrorf` call without +// needing to import things, because importing in these tests is difficult. +type someNode struct{} + +func (n *someNode) WithSourceErrorf(sourceCode string, message string, args ...any) error { + return nil +} + +func dynamicFormat(n *someNode, name string) error { + message := "found dynamic message: " + name + return n.WithSourceErrorf(name, message) // want "format string argument to `WithSourceErrorf` must be a static string" +} + +func dynamicConcatFormat(n *someNode, name string) error { + return n.WithSourceErrorf(name, "found dynamic message: "+name) // want "format string argument to `WithSourceErrorf` must be a static string" +} + +func dynamicNestedFormat(n *someNode, name string) []error { + message := "found dynamic message: " + name + return append([]error{}, n.WithSourceErrorf(name, message)) // want "format string argument to `WithSourceErrorf` must be a static string" +} + +func dynamicMethodExprFormat(n *someNode, name string) error { + message := "found dynamic message: " + name + return (*someNode).WithSourceErrorf(n, name, message) // want "format string argument to `WithSourceErrorf` must be a static string" +} diff --git a/tools/analyzers/staticformatcheck/testdata/src/validformat/validformat.go b/tools/analyzers/staticformatcheck/testdata/src/validformat/validformat.go new file mode 100644 index 0000000000..6804cafbac --- /dev/null +++ b/tools/analyzers/staticformatcheck/testdata/src/validformat/validformat.go @@ -0,0 +1,39 @@ +package validformat + +// NOTE: this def is just to get us to a `WithSourceErrorf` call without +// needing to import things, because importing in these tests is difficult. +type someNode struct{} + +func (n *someNode) WithSourceErrorf(sourceCode string, message string, args ...any) error { + return nil +} + +// unrelated has a non-format signature and must not be checked, even though +// its method shares the checked name. +type unrelated struct{} + +func (u *unrelated) WithSourceErrorf(message string) error { + return nil +} + +const errTemplate = "found duplicate name: %s" + +func literalFormat(n *someNode, name string) error { + return n.WithSourceErrorf(name, "found duplicate name: %s", name) +} + +func constantFormat(n *someNode, name string) error { + return n.WithSourceErrorf(name, errTemplate, name) +} + +func literalConcatFormat(n *someNode, name string) error { + return n.WithSourceErrorf(name, "found duplicate "+"name: %s", name) +} + +func methodExprFormat(n *someNode, name string) error { + return (*someNode).WithSourceErrorf(n, name, "found duplicate name: %s", name) +} + +func unrelatedSignature(u *unrelated, name string) error { + return u.WithSourceErrorf("found dynamic message: " + name) +}