From 9345e81cd458332ce2bf19e4ad31d5efcbc637a1 Mon Sep 17 00:00:00 2001 From: Nikolay Tretyak Date: Wed, 30 Aug 2023 18:14:49 +0200 Subject: [PATCH] Check all functions with workflow.Context as first parameter (#1215) Check all functions with workflow.Context as first parameter --- contrib/tools/workflowcheck/README.md | 2 +- .../tools/workflowcheck/workflow/checker.go | 49 +++++++++---------- .../workflow/testdata/src/a/workflow.go | 33 +++++-------- .../workflowcheck/workflow/workflow_test.go | 4 +- 4 files changed, 38 insertions(+), 50 deletions(-) diff --git a/contrib/tools/workflowcheck/README.md b/contrib/tools/workflowcheck/README.md index 3d69a4f1c..04aa4154d 100644 --- a/contrib/tools/workflowcheck/README.md +++ b/contrib/tools/workflowcheck/README.md @@ -2,7 +2,7 @@ Temporal Workflow Check is a tool that statically analyzes Temporal [Workflow Definitions written in Go](https://docs.temporal.io/docs/go/how-to-write-a-workflow-definition-in-go) -(i.e. functions passed to `worker.RegisterWorkflow`) to check for non-deterministic code either directly in the function +(i.e. functions with `workflow.Context` as first argument) to check for non-deterministic code either directly in the function or in a function called by the Workflow. It is highly optimized to scan large codebases quickly. **NOTE: This will not catch all cases of non-determinism such as global var mutation. This is just a helper and diff --git a/contrib/tools/workflowcheck/workflow/checker.go b/contrib/tools/workflowcheck/workflow/checker.go index 40299fcc0..9432a920b 100644 --- a/contrib/tools/workflowcheck/workflow/checker.go +++ b/contrib/tools/workflowcheck/workflow/checker.go @@ -33,7 +33,6 @@ import ( "go.temporal.io/sdk/contrib/tools/workflowcheck/determinism" "golang.org/x/tools/go/analysis" - "golang.org/x/tools/go/types/typeutil" "gopkg.in/yaml.v2" ) @@ -165,32 +164,11 @@ func (c *Checker) Run(pass *analysis.Pass) error { isIgnored = true } } - callExpr, _ := n.(*ast.CallExpr) - if callExpr == nil || isIgnored { - return true - } - // Callee needs to be workflow registry - callee, _ := typeutil.Callee(pass.TypesInfo, callExpr).(*types.Func) - const regName = "(go.temporal.io/sdk/worker.WorkflowRegistry).RegisterWorkflow" - const regOptName = "(go.temporal.io/sdk/worker.WorkflowRegistry).RegisterWorkflowWithOptions" - if callee == nil || len(callExpr.Args) == 0 || (callee.FullName() != regName && callee.FullName() != regOptName) { - return true - } - // First param should be a function ident or a selector with ident as - // function - var fn *types.Func - switch arg := callExpr.Args[0].(type) { - case *ast.Ident: - fn, _ = pass.TypesInfo.ObjectOf(arg).(*types.Func) - case *ast.SelectorExpr: - fn, _ = pass.TypesInfo.ObjectOf(arg.Sel).(*types.Func) - } - // Report if couldn't get type - if fn == nil { - pass.Reportf(callExpr.Args[0].Pos(), - "unrecognized function reference format. We cannot follow this function reference to check for non-determinism.") + funcDecl, _ := n.(*ast.FuncDecl) + if funcDecl == nil || isIgnored || !isWorkflowFunc(funcDecl, pass) { return true } + fn, _ := pass.TypesInfo.ObjectOf(funcDecl.Name).(*types.Func) c.debugf("Checking workflow function %v", fn.FullName()) // Get non-determinisms of that package and check packageNonDeterminisms := lookupCache.PackageNonDeterminisms(fn.Pkg()) @@ -199,7 +177,7 @@ func (c *Checker) Run(pass *analysis.Pass) error { for _, reason := range nonDeterminisms { lines := determinism.NonDeterminisms{reason}.AppendChildReasonLines( fn.FullName(), nil, 0, depthRepeat, c.IncludePosOnMessage, fn.Pkg(), lookupCache, map[string]bool{}) - pass.Report(analysis.Diagnostic{Pos: callExpr.Pos(), Message: strings.Join(lines, hierarchySeparator)}) + pass.Report(analysis.Diagnostic{Pos: fn.Pos(), Message: strings.Join(lines, hierarchySeparator)}) } } return true @@ -208,6 +186,25 @@ func (c *Checker) Run(pass *analysis.Pass) error { return nil } +// isWorkflowFunc checks if f has workflow.Context as a first parameter. +func isWorkflowFunc(f *ast.FuncDecl, pass *analysis.Pass) bool { + if f.Type.Params == nil || len(f.Type.Params.List) == 0 { + return false + } + firstParam := f.Type.Params.List[0] + typeInfo := pass.TypesInfo.TypeOf(firstParam.Type) + named, _ := typeInfo.(*types.Named) + if named == nil { + return false + } + obj := named.Obj() + if obj.Pkg() == nil || obj.Name() != "Context" { + return false + } + path := obj.Pkg().Path() + return path == "go.temporal.io/sdk/workflow" || path == "go.temporal.io/sdk/internal" +} + type configFileFlag struct{ checker *determinism.Checker } func (configFileFlag) String() string { return "" } diff --git a/contrib/tools/workflowcheck/workflow/testdata/src/a/workflow.go b/contrib/tools/workflowcheck/workflow/testdata/src/a/workflow.go index d3bb9dc97..f466235c6 100644 --- a/contrib/tools/workflowcheck/workflow/testdata/src/a/workflow.go +++ b/contrib/tools/workflowcheck/workflow/testdata/src/a/workflow.go @@ -1,50 +1,39 @@ package a //want package:"\\d+ non-deterministic vars/funcs" import ( + "context" "text/template" "time" - "go.temporal.io/sdk/worker" "go.temporal.io/sdk/workflow" ) -func PrepWorkflow() { - var wrk worker.Worker - wrk.RegisterWorkflow(WorkflowNop) - wrk.RegisterWorkflow(WorkflowCallTime) // want "a.WorkflowCallTime is non-deterministic, reason: calls non-deterministic function time.Now" - wrk.RegisterWorkflow(WorkflowCallTimeTransitively) // want "a.WorkflowCallTimeTransitively is non-deterministic, reason: calls non-deterministic function a.SomeTimeCall" - wrk.RegisterWorkflow(WorkflowIterateMap) // want "a.WorkflowIterateMap is non-deterministic, reason: iterates over map" - wrk.RegisterWorkflow(WorkflowWithTemplate) // want "a.WorkflowWithTemplate is non-deterministic, reason: calls non-deterministic function \\(\\*text/template\\.Template\\)\\.Execute.*" - wrk.RegisterWorkflow(WorkflowWithAwait) - HigherOrderWorkflowCallIgnorable(wrk, WorkflowNop) -} - func WorkflowNop(ctx workflow.Context) error { return nil } -func WorkflowCallTime(ctx workflow.Context) error { // want WorkflowCallTime:"calls non-deterministic function time.Now" +func WorkflowCallTime(ctx workflow.Context) error { // want "a.WorkflowCallTime is non-deterministic, reason: calls non-deterministic function time.Now" time.Now() return nil } -func WorkflowCallTimeTransitively(ctx workflow.Context) error { // want WorkflowCallTimeTransitively:"calls non-deterministic function a.SomeTimeCall" +func WorkflowCallTimeTransitively(ctx workflow.Context) error { // want "a.WorkflowCallTimeTransitively is non-deterministic, reason: calls non-deterministic function a.SomeTimeCall" SomeTimeCall() return nil } -func SomeTimeCall() time.Time { // want SomeTimeCall:"calls non-deterministic function time.Now" +func SomeTimeCall() time.Time { return time.Now() } -func WorkflowIterateMap(ctx workflow.Context) error { // want WorkflowIterateMap:"iterates over map" +func WorkflowIterateMap(ctx workflow.Context) error { // want "a.WorkflowIterateMap is non-deterministic, reason: iterates over map" var m map[string]string for range m { } return nil } -func WorkflowWithTemplate(ctx workflow.Context) error { // want WorkflowWithTemplate:"calls non-deterministic function \\(\\*text/template\\.Template\\)\\.Execute.*" +func WorkflowWithTemplate(ctx workflow.Context) error { // want "a.WorkflowWithTemplate is non-deterministic, reason: calls non-deterministic function \\(\\*text/template\\.Template\\)\\.Execute.*" return template.New("mytmpl").Execute(nil, nil) } @@ -54,7 +43,11 @@ func WorkflowWithAwait(ctx workflow.Context) error { return err } -func HigherOrderWorkflowCallIgnorable(w worker.Worker, wfunc func(workflow.Context) error) { - //workflowcheck:ignore - w.RegisterWorkflow(wfunc) +func WorkflowWithUnnamedArgument(workflow.Context) error { // want "a.WorkflowWithUnnamedArgument is non-deterministic, reason: calls non-deterministic function time.Now" + time.Now() + return nil +} + +func NotWorkflow(ctx context.Context) { + time.Now() } diff --git a/contrib/tools/workflowcheck/workflow/workflow_test.go b/contrib/tools/workflowcheck/workflow/workflow_test.go index 169fce519..450a1d23a 100644 --- a/contrib/tools/workflowcheck/workflow/workflow_test.go +++ b/contrib/tools/workflowcheck/workflow/workflow_test.go @@ -35,9 +35,7 @@ func Test(t *testing.T) { analysistest.Run( t, analysistest.TestData(), - workflow.NewChecker(workflow.Config{ - EnableObjectFacts: true, - }).NewAnalyzer(), + workflow.NewChecker(workflow.Config{}).NewAnalyzer(), "a", ) }