diff --git a/contrib/tools/workflowcheck/determinism/checker.go b/contrib/tools/workflowcheck/determinism/checker.go index cba084b6d..939b1c2cf 100644 --- a/contrib/tools/workflowcheck/determinism/checker.go +++ b/contrib/tools/workflowcheck/determinism/checker.go @@ -31,6 +31,10 @@ type Config struct { EnableObjectFacts bool // Map `package -> function names` with functions making any argument deterministic AcceptsNonDeterministicParameters map[string][]string + // Map of fully-qualified function name to the argument index that must not + // be an anonymous function (function literal). If an anonymous function is + // passed at the specified index, it is flagged as non-deterministic. + RejectsAnonymousFuncArgs map[string]int } // Checker is a checker that can run analysis passes to check for @@ -282,6 +286,17 @@ func (c *collector) collectFuncInfo(fn *types.Func, decl *ast.FuncDecl) { case *ast.CallExpr: // Get the callee if callee, _ := typeutil.Callee(c.pass.TypesInfo, n).(*types.Func); callee != nil { + // Check if this call rejects anonymous function arguments + if argIdx, ok := c.checker.RejectsAnonymousFuncArgs[callee.FullName()]; ok && argIdx < len(n.Args) { + if isAnonymousFunc(n.Args[argIdx], n.Pos(), decl, c.pass.TypesInfo) { + c.checker.debugf("Marking %v as non-deterministic because it passes anonymous function to %v", fn.FullName(), callee.Name()) + pos := c.pass.Fset.Position(n.Pos()) + info.reasons = append(info.reasons, &ReasonAnonymousFunc{ + SourcePos: &pos, + FuncName: callee.Name(), + }) + } + } if callee.Pkg() != nil && slices.Contains(c.checker.AcceptsNonDeterministicParameters[callee.Pkg().Path()], callee.Name()) { return false } else if c.pass.Pkg != callee.Pkg() { @@ -430,6 +445,96 @@ func (c *collector) applyFuncNonDeterminisms(f *funcInfo, p PackageNonDeterminis } } +// isAnonymousFunc checks if expr is a function literal or an identifier +// that could hold an anonymous function at the call site. For straight-line +// code, the latest assignment before callPos wins. For branches (if/else), +// if any branch assigns an anonymous function, it is flagged conservatively. +func isAnonymousFunc(expr ast.Expr, callPos token.Pos, decl *ast.FuncDecl, info *types.Info) bool { + if _, ok := expr.(*ast.FuncLit); ok { + return true + } + ident, ok := expr.(*ast.Ident) + if !ok || decl.Body == nil { + return false + } + obj := info.ObjectOf(ident) + if obj == nil { + return false + } + return stmtsHaveAnonForVar(decl.Body.List, obj, callPos, info) +} + +// stmtsHaveAnonForVar walks statements in order and determines if the variable +// identified by obj could hold an anonymous function at callPos. +// Direct assignments override the state (latest wins). Assignments inside +// branches (if/else/for/switch) conservatively flag if any is anonymous. +func stmtsHaveAnonForVar(stmts []ast.Stmt, obj types.Object, callPos token.Pos, info *types.Info) bool { + isAnon := false + for _, stmt := range stmts { + if stmt.Pos() >= callPos { + break + } + switch s := stmt.(type) { + case *ast.AssignStmt: + if _, isFuncLit := assignsToVar(s, obj, info); isFuncLit != nil { + isAnon = *isFuncLit + } + case *ast.DeclStmt: + if genDecl, ok := s.Decl.(*ast.GenDecl); ok { + for _, spec := range genDecl.Specs { + if vs, ok := spec.(*ast.ValueSpec); ok { + for i, name := range vs.Names { + if info.ObjectOf(name) == obj && i < len(vs.Values) { + _, isAnon = vs.Values[i].(*ast.FuncLit) + } + } + } + } + } + default: + if containsAnonAssignToVar(stmt, obj, callPos, info) { + isAnon = true + } + } + } + return isAnon +} + +func assignsToVar(s *ast.AssignStmt, obj types.Object, info *types.Info) (ast.Expr, *bool) { + for i, lhs := range s.Lhs { + if lhsIdent, ok := lhs.(*ast.Ident); ok && info.ObjectOf(lhsIdent) == obj && i < len(s.Rhs) { + _, isFuncLit := s.Rhs[i].(*ast.FuncLit) + return s.Rhs[i], &isFuncLit + } + } + return nil, nil +} + +func containsAnonAssignToVar(node ast.Node, obj types.Object, callPos token.Pos, info *types.Info) bool { + found := false + ast.Inspect(node, func(n ast.Node) bool { + if found || n == nil || n.Pos() >= callPos { + return false + } + // Don't descend into function literals — assignments inside closures + // that are only defined (not invoked) should not count. + if _, ok := n.(*ast.FuncLit); ok { + return false + } + if assign, ok := n.(*ast.AssignStmt); ok { + for i, lhs := range assign.Lhs { + if lhsIdent, ok := lhs.(*ast.Ident); ok && info.ObjectOf(lhsIdent) == obj && i < len(assign.Rhs) { + if _, ok := assign.Rhs[i].(*ast.FuncLit); ok { + found = true + } + } + } + } + return !found + }) + return found +} + // PackageLookupCache caches fact lookups across packages. type PackageLookupCache struct { pass *analysis.Pass diff --git a/contrib/tools/workflowcheck/determinism/reason.go b/contrib/tools/workflowcheck/determinism/reason.go index 90111c357..1e829a236 100644 --- a/contrib/tools/workflowcheck/determinism/reason.go +++ b/contrib/tools/workflowcheck/determinism/reason.go @@ -203,6 +203,21 @@ func (r *ReasonMapRange) String() string { return "iterates over map" } +// ReasonAnonymousFunc represents passing an anonymous function to an API +// that requires deterministic function names. +type ReasonAnonymousFunc struct { + SourcePos *token.Position + FuncName string +} + +// Pos returns the source position. +func (r *ReasonAnonymousFunc) Pos() *token.Position { return r.SourcePos } + +// String returns the reason. +func (r *ReasonAnonymousFunc) String() string { + return "anonymous function passed to " + r.FuncName + " that has a non-deterministic function name" +} + func init() { // Needed for go vet usage gob.Register(&ReasonDecl{}) @@ -210,4 +225,5 @@ func init() { gob.Register(&ReasonVarAccess{}) gob.Register(&ReasonConcurrency{}) gob.Register(&ReasonMapRange{}) + gob.Register(&ReasonAnonymousFunc{}) } diff --git a/contrib/tools/workflowcheck/workflow/checker.go b/contrib/tools/workflowcheck/workflow/checker.go index d62f90748..d1ff7cae3 100644 --- a/contrib/tools/workflowcheck/workflow/checker.go +++ b/contrib/tools/workflowcheck/workflow/checker.go @@ -84,6 +84,9 @@ func NewChecker(config Config) *Checker { Debug: config.DeterminismDebug, EnableObjectFacts: config.EnableObjectFacts, AcceptsNonDeterministicParameters: map[string][]string{"go.temporal.io/sdk/workflow": {"SideEffect", "MutableSideEffect"}}, + RejectsAnonymousFuncArgs: map[string]int{ + "go.temporal.io/sdk/workflow.ExecuteLocalActivity": 1, + }, }), } } @@ -144,12 +147,19 @@ func (c *Checker) Run(pass *analysis.Pass) error { determinism.UpdateIgnoreMap(pass.Fset, file, ignoreMap) ast.Inspect(file, func(n ast.Node) bool { - // Only handle calls with followable function pointers _, isIgnored := ignoreMap[n] for k := range ignoreMap { - asExprStmt, _ := k.(*ast.ExprStmt) - if asExprStmt != nil && asExprStmt.X == n { - isIgnored = true + switch stmt := k.(type) { + case *ast.ExprStmt: + if stmt.X == n { + isIgnored = true + } + case *ast.AssignStmt: + for _, rhs := range stmt.Rhs { + if rhs == n { + isIgnored = true + } + } } } funcDecl, _ := n.(*ast.FuncDecl) diff --git a/contrib/tools/workflowcheck/workflow/testdata/src/a/workflow.go b/contrib/tools/workflowcheck/workflow/testdata/src/a/workflow.go index 39f1793ad..77e5feb2c 100644 --- a/contrib/tools/workflowcheck/workflow/testdata/src/a/workflow.go +++ b/contrib/tools/workflowcheck/workflow/testdata/src/a/workflow.go @@ -68,6 +68,144 @@ func NotWorkflow(ctx context.Context) { time.Now() } +// --- Anonymous function in ExecuteLocalActivity tests --- + +func WorkflowLocalActivityLiteral(ctx workflow.Context) error { // want "a.WorkflowLocalActivityLiteral is non-deterministic, reason: anonymous function passed to ExecuteLocalActivity that has a non-deterministic function name" + workflow.ExecuteLocalActivity(ctx, func(ctx context.Context) error { return nil }) + return nil +} + +func WorkflowLocalActivityVarLiteral(ctx workflow.Context) error { // want "a.WorkflowLocalActivityVarLiteral is non-deterministic, reason: anonymous function passed to ExecuteLocalActivity that has a non-deterministic function name" + f := func(ctx context.Context) error { return nil } + workflow.ExecuteLocalActivity(ctx, f) + return nil +} + +func WorkflowLocalActivityVarReassignedToNamed(ctx workflow.Context) error { + f := func(ctx context.Context) error { return nil } + f = myLocalActivity + workflow.ExecuteLocalActivity(ctx, f) + return nil +} + +func WorkflowLocalActivityVarReassignedToAnon(ctx workflow.Context) error { // want "a.WorkflowLocalActivityVarReassignedToAnon is non-deterministic, reason: anonymous function passed to ExecuteLocalActivity that has a non-deterministic function name" + f := myLocalActivity + f = func(ctx context.Context) error { return nil } + workflow.ExecuteLocalActivity(ctx, f) + return nil +} + +func WorkflowLocalActivityBranchAnon(ctx workflow.Context) error { // want "a.WorkflowLocalActivityBranchAnon is non-deterministic, reason: anonymous function passed to ExecuteLocalActivity that has a non-deterministic function name" + f := myLocalActivity + if true { + f = func(ctx context.Context) error { return nil } + } + workflow.ExecuteLocalActivity(ctx, f) + return nil +} + +func WorkflowLocalActivityIfElseAnon(ctx workflow.Context) error { // want "a.WorkflowLocalActivityIfElseAnon is non-deterministic, reason: anonymous function passed to ExecuteLocalActivity that has a non-deterministic function name" + var f func(ctx context.Context) error + if true { + f = func(ctx context.Context) error { return nil } + } else { + f = myLocalActivity + } + workflow.ExecuteLocalActivity(ctx, f) + return nil +} + +func WorkflowLocalActivityIfElseAllNamed(ctx workflow.Context) error { + var f func(ctx context.Context) error + if true { + f = myLocalActivity + } else { + f = myLocalActivity + } + workflow.ExecuteLocalActivity(ctx, f) + return nil +} + +func WorkflowLocalActivityIfElseOverridden(ctx workflow.Context) error { + var f func(ctx context.Context) error + if true { + f = func(ctx context.Context) error { return nil } + } else { + f = myLocalActivity + } + f = myLocalActivity + workflow.ExecuteLocalActivity(ctx, f) + return nil +} + +func WorkflowLocalActivityAnonInClosure(ctx workflow.Context) error { + f := myLocalActivity + _ = func() { + f = func(ctx context.Context) error { return nil } + _ = f + } + workflow.ExecuteLocalActivity(ctx, f) + return nil +} + +func WorkflowLocalActivityFuncParam(ctx workflow.Context, f func(ctx context.Context) error) error { + workflow.ExecuteLocalActivity(ctx, f) + return nil +} + +func WorkflowLocalActivityBranchOverridden(ctx workflow.Context) error { + if true { + f := func(ctx context.Context) error { return nil } + _ = f + } + // f here is a different variable; this uses myLocalActivity directly + workflow.ExecuteLocalActivity(ctx, myLocalActivity) + return nil +} + +func myLocalActivity(ctx context.Context) error { return nil } + +func WorkflowLocalActivityNamed(ctx workflow.Context) error { + workflow.ExecuteLocalActivity(ctx, myLocalActivity) + return nil +} + +type ActivityStruct struct{} + +func (a *ActivityStruct) Run(ctx context.Context) error { return nil } + +func WorkflowLocalActivityMethodValue(ctx workflow.Context) error { + a := &ActivityStruct{} + workflow.ExecuteLocalActivity(ctx, a.Run) + return nil +} + +func WorkflowLocalActivityString(ctx workflow.Context) error { + workflow.ExecuteLocalActivity(ctx, "MyActivity") + return nil +} + +func WorkflowLocalActivityMethodExpr(ctx workflow.Context) error { + workflow.ExecuteLocalActivity(ctx, (*ActivityStruct).Run) + return nil +} + +func WorkflowLocalActivityNamedVar(ctx workflow.Context) error { + f := myLocalActivity + workflow.ExecuteLocalActivity(ctx, f) + return nil +} + +func WorkflowLocalActivityIgnored(ctx workflow.Context) error { + workflow.ExecuteLocalActivity(ctx, func(ctx context.Context) error { return nil }) //workflowcheck:ignore + return nil +} + +func WorkflowLocalActivityAssignIgnored(ctx workflow.Context) error { + _ = workflow.ExecuteLocalActivity(ctx, func(ctx context.Context) error { return nil }) //workflowcheck:ignore + return nil +} + func WorkflowWithSearchAttributes(workflow.Context) { sa := temporal.SearchAttributes{} _ = sa.Copy() diff --git a/contrib/tools/workflowcheck/workflow/testdata/src/go.temporal.io/sdk/internal/internal.go b/contrib/tools/workflowcheck/workflow/testdata/src/go.temporal.io/sdk/internal/internal.go index 1595630f8..37453e16d 100644 --- a/contrib/tools/workflowcheck/workflow/testdata/src/go.temporal.io/sdk/internal/internal.go +++ b/contrib/tools/workflowcheck/workflow/testdata/src/go.temporal.io/sdk/internal/internal.go @@ -1,3 +1,7 @@ package internal type Context interface{} + +func ExecuteLocalActivity(ctx Context, activity interface{}, args ...interface{}) interface{} { + return nil +} diff --git a/contrib/tools/workflowcheck/workflow/testdata/src/go.temporal.io/sdk/workflow/workflow.go b/contrib/tools/workflowcheck/workflow/testdata/src/go.temporal.io/sdk/workflow/workflow.go index f2821af98..8d141df6e 100644 --- a/contrib/tools/workflowcheck/workflow/testdata/src/go.temporal.io/sdk/workflow/workflow.go +++ b/contrib/tools/workflowcheck/workflow/testdata/src/go.temporal.io/sdk/workflow/workflow.go @@ -19,3 +19,7 @@ func AwaitWithTimeout(ctx Context, timeout time.Duration, condition func() bool) func SideEffect(ctx Context, f func(ctx Context) interface{}) interface{} { return nil } + +func ExecuteLocalActivity(ctx Context, activity interface{}, args ...interface{}) interface{} { + return nil +}