Check all functions with workflow.Context as first parameter (tempora…

…lio#1215)

Check all functions with workflow.Context as first parameter

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

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 "<built-in>" }

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()
 }

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",
 	)
 }