Skip to content

Commit d397981

Browse files
Workflowcheck enforce any. func isn't used as local activity
1 parent aadf2e8 commit d397981

6 files changed

Lines changed: 266 additions & 4 deletions

File tree

contrib/tools/workflowcheck/determinism/checker.go

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@ type Config struct {
3131
EnableObjectFacts bool
3232
// Map `package -> function names` with functions making any argument deterministic
3333
AcceptsNonDeterministicParameters map[string][]string
34+
// Map of fully-qualified function name to the argument index that must not
35+
// be an anonymous function (function literal). If an anonymous function is
36+
// passed at the specified index, it is flagged as non-deterministic.
37+
RejectsAnonymousFuncArgs map[string]int
3438
}
3539

3640
// 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) {
282286
case *ast.CallExpr:
283287
// Get the callee
284288
if callee, _ := typeutil.Callee(c.pass.TypesInfo, n).(*types.Func); callee != nil {
289+
// Check if this call rejects anonymous function arguments
290+
if argIdx, ok := c.checker.RejectsAnonymousFuncArgs[callee.FullName()]; ok && argIdx < len(n.Args) {
291+
if isAnonymousFunc(n.Args[argIdx], n.Pos(), decl, c.pass.TypesInfo) {
292+
c.checker.debugf("Marking %v as non-deterministic because it passes anonymous function to %v", fn.FullName(), callee.Name())
293+
pos := c.pass.Fset.Position(n.Pos())
294+
info.reasons = append(info.reasons, &ReasonAnonymousFunc{
295+
SourcePos: &pos,
296+
FuncName: callee.Name(),
297+
})
298+
}
299+
}
285300
if callee.Pkg() != nil && slices.Contains(c.checker.AcceptsNonDeterministicParameters[callee.Pkg().Path()], callee.Name()) {
286301
return false
287302
} else if c.pass.Pkg != callee.Pkg() {
@@ -430,6 +445,91 @@ func (c *collector) applyFuncNonDeterminisms(f *funcInfo, p PackageNonDeterminis
430445
}
431446
}
432447

448+
// isAnonymousFunc checks if expr is a function literal or an identifier
449+
// that could hold an anonymous function at the call site. For straight-line
450+
// code, the latest assignment before callPos wins. For branches (if/else),
451+
// if any branch assigns an anonymous function, it is flagged conservatively.
452+
func isAnonymousFunc(expr ast.Expr, callPos token.Pos, decl *ast.FuncDecl, info *types.Info) bool {
453+
if _, ok := expr.(*ast.FuncLit); ok {
454+
return true
455+
}
456+
ident, ok := expr.(*ast.Ident)
457+
if !ok || decl.Body == nil {
458+
return false
459+
}
460+
obj := info.ObjectOf(ident)
461+
if obj == nil {
462+
return false
463+
}
464+
return stmtsHaveAnonForVar(decl.Body.List, obj, callPos, info)
465+
}
466+
467+
// stmtsHaveAnonForVar walks statements in order and determines if the variable
468+
// identified by obj could hold an anonymous function at callPos.
469+
// Direct assignments override the state (latest wins). Assignments inside
470+
// branches (if/else/for/switch) conservatively flag if any is anonymous.
471+
func stmtsHaveAnonForVar(stmts []ast.Stmt, obj types.Object, callPos token.Pos, info *types.Info) bool {
472+
isAnon := false
473+
for _, stmt := range stmts {
474+
if stmt.Pos() >= callPos {
475+
break
476+
}
477+
switch s := stmt.(type) {
478+
case *ast.AssignStmt:
479+
if _, isFuncLit := assignsToVar(s, obj, info); isFuncLit != nil {
480+
isAnon = *isFuncLit
481+
}
482+
case *ast.DeclStmt:
483+
if genDecl, ok := s.Decl.(*ast.GenDecl); ok {
484+
for _, spec := range genDecl.Specs {
485+
if vs, ok := spec.(*ast.ValueSpec); ok {
486+
for i, name := range vs.Names {
487+
if info.ObjectOf(name) == obj && i < len(vs.Values) {
488+
_, isAnon = vs.Values[i].(*ast.FuncLit)
489+
}
490+
}
491+
}
492+
}
493+
}
494+
default:
495+
if containsAnonAssignToVar(stmt, obj, callPos, info) {
496+
isAnon = true
497+
}
498+
}
499+
}
500+
return isAnon
501+
}
502+
503+
func assignsToVar(s *ast.AssignStmt, obj types.Object, info *types.Info) (ast.Expr, *bool) {
504+
for i, lhs := range s.Lhs {
505+
if lhsIdent, ok := lhs.(*ast.Ident); ok && info.ObjectOf(lhsIdent) == obj && i < len(s.Rhs) {
506+
_, isFuncLit := s.Rhs[i].(*ast.FuncLit)
507+
return s.Rhs[i], &isFuncLit
508+
}
509+
}
510+
return nil, nil
511+
}
512+
513+
func containsAnonAssignToVar(node ast.Node, obj types.Object, callPos token.Pos, info *types.Info) bool {
514+
found := false
515+
ast.Inspect(node, func(n ast.Node) bool {
516+
if found || n == nil || n.Pos() >= callPos {
517+
return false
518+
}
519+
if assign, ok := n.(*ast.AssignStmt); ok {
520+
for i, lhs := range assign.Lhs {
521+
if lhsIdent, ok := lhs.(*ast.Ident); ok && info.ObjectOf(lhsIdent) == obj && i < len(assign.Rhs) {
522+
if _, ok := assign.Rhs[i].(*ast.FuncLit); ok {
523+
found = true
524+
}
525+
}
526+
}
527+
}
528+
return !found
529+
})
530+
return found
531+
}
532+
433533
// PackageLookupCache caches fact lookups across packages.
434534
type PackageLookupCache struct {
435535
pass *analysis.Pass

contrib/tools/workflowcheck/determinism/reason.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,11 +203,27 @@ func (r *ReasonMapRange) String() string {
203203
return "iterates over map"
204204
}
205205

206+
// ReasonAnonymousFunc represents passing an anonymous function to an API
207+
// that requires deterministic function names.
208+
type ReasonAnonymousFunc struct {
209+
SourcePos *token.Position
210+
FuncName string
211+
}
212+
213+
// Pos returns the source position.
214+
func (r *ReasonAnonymousFunc) Pos() *token.Position { return r.SourcePos }
215+
216+
// String returns the reason.
217+
func (r *ReasonAnonymousFunc) String() string {
218+
return "anonymous function passed to " + r.FuncName + " that has a non-deterministic function name"
219+
}
220+
206221
func init() {
207222
// Needed for go vet usage
208223
gob.Register(&ReasonDecl{})
209224
gob.Register(&ReasonFuncCall{})
210225
gob.Register(&ReasonVarAccess{})
211226
gob.Register(&ReasonConcurrency{})
212227
gob.Register(&ReasonMapRange{})
228+
gob.Register(&ReasonAnonymousFunc{})
213229
}

contrib/tools/workflowcheck/workflow/checker.go

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,9 @@ func NewChecker(config Config) *Checker {
8484
Debug: config.DeterminismDebug,
8585
EnableObjectFacts: config.EnableObjectFacts,
8686
AcceptsNonDeterministicParameters: map[string][]string{"go.temporal.io/sdk/workflow": {"SideEffect", "MutableSideEffect"}},
87+
RejectsAnonymousFuncArgs: map[string]int{
88+
"go.temporal.io/sdk/workflow.ExecuteLocalActivity": 1,
89+
},
8790
}),
8891
}
8992
}
@@ -144,12 +147,19 @@ func (c *Checker) Run(pass *analysis.Pass) error {
144147
determinism.UpdateIgnoreMap(pass.Fset, file, ignoreMap)
145148

146149
ast.Inspect(file, func(n ast.Node) bool {
147-
// Only handle calls with followable function pointers
148150
_, isIgnored := ignoreMap[n]
149151
for k := range ignoreMap {
150-
asExprStmt, _ := k.(*ast.ExprStmt)
151-
if asExprStmt != nil && asExprStmt.X == n {
152-
isIgnored = true
152+
switch stmt := k.(type) {
153+
case *ast.ExprStmt:
154+
if stmt.X == n {
155+
isIgnored = true
156+
}
157+
case *ast.AssignStmt:
158+
for _, rhs := range stmt.Rhs {
159+
if rhs == n {
160+
isIgnored = true
161+
}
162+
}
153163
}
154164
}
155165
funcDecl, _ := n.(*ast.FuncDecl)

contrib/tools/workflowcheck/workflow/testdata/src/a/workflow.go

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,134 @@ func NotWorkflow(ctx context.Context) {
6868
time.Now()
6969
}
7070

71+
// --- Anonymous function in ExecuteLocalActivity tests ---
72+
73+
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"
74+
workflow.ExecuteLocalActivity(ctx, func(ctx context.Context) error { return nil })
75+
return nil
76+
}
77+
78+
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"
79+
f := func(ctx context.Context) error { return nil }
80+
workflow.ExecuteLocalActivity(ctx, f)
81+
return nil
82+
}
83+
84+
func WorkflowLocalActivityVarReassignedToNamed(ctx workflow.Context) error {
85+
f := func(ctx context.Context) error { return nil }
86+
f = myLocalActivity
87+
workflow.ExecuteLocalActivity(ctx, f)
88+
return nil
89+
}
90+
91+
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"
92+
f := myLocalActivity
93+
f = func(ctx context.Context) error { return nil }
94+
workflow.ExecuteLocalActivity(ctx, f)
95+
return nil
96+
}
97+
98+
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"
99+
f := myLocalActivity
100+
if true {
101+
f = func(ctx context.Context) error { return nil }
102+
}
103+
workflow.ExecuteLocalActivity(ctx, f)
104+
return nil
105+
}
106+
107+
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"
108+
var f func(ctx context.Context) error
109+
if true {
110+
f = func(ctx context.Context) error { return nil }
111+
} else {
112+
f = myLocalActivity
113+
}
114+
workflow.ExecuteLocalActivity(ctx, f)
115+
return nil
116+
}
117+
118+
func WorkflowLocalActivityIfElseAllNamed(ctx workflow.Context) error {
119+
var f func(ctx context.Context) error
120+
if true {
121+
f = myLocalActivity
122+
} else {
123+
f = myLocalActivity
124+
}
125+
workflow.ExecuteLocalActivity(ctx, f)
126+
return nil
127+
}
128+
129+
func WorkflowLocalActivityIfElseOverridden(ctx workflow.Context) error {
130+
var f func(ctx context.Context) error
131+
if true {
132+
f = func(ctx context.Context) error { return nil }
133+
} else {
134+
f = myLocalActivity
135+
}
136+
f = myLocalActivity
137+
workflow.ExecuteLocalActivity(ctx, f)
138+
return nil
139+
}
140+
141+
func WorkflowLocalActivityFuncParam(ctx workflow.Context, f func(ctx context.Context) error) error {
142+
workflow.ExecuteLocalActivity(ctx, f)
143+
return nil
144+
}
145+
146+
func WorkflowLocalActivityBranchOverridden(ctx workflow.Context) error {
147+
if true {
148+
f := func(ctx context.Context) error { return nil }
149+
_ = f
150+
}
151+
// f here is a different variable; this uses myLocalActivity directly
152+
workflow.ExecuteLocalActivity(ctx, myLocalActivity)
153+
return nil
154+
}
155+
156+
func myLocalActivity(ctx context.Context) error { return nil }
157+
158+
func WorkflowLocalActivityNamed(ctx workflow.Context) error {
159+
workflow.ExecuteLocalActivity(ctx, myLocalActivity)
160+
return nil
161+
}
162+
163+
type ActivityStruct struct{}
164+
165+
func (a *ActivityStruct) Run(ctx context.Context) error { return nil }
166+
167+
func WorkflowLocalActivityMethodValue(ctx workflow.Context) error {
168+
a := &ActivityStruct{}
169+
workflow.ExecuteLocalActivity(ctx, a.Run)
170+
return nil
171+
}
172+
173+
func WorkflowLocalActivityString(ctx workflow.Context) error {
174+
workflow.ExecuteLocalActivity(ctx, "MyActivity")
175+
return nil
176+
}
177+
178+
func WorkflowLocalActivityMethodExpr(ctx workflow.Context) error {
179+
workflow.ExecuteLocalActivity(ctx, (*ActivityStruct).Run)
180+
return nil
181+
}
182+
183+
func WorkflowLocalActivityNamedVar(ctx workflow.Context) error {
184+
f := myLocalActivity
185+
workflow.ExecuteLocalActivity(ctx, f)
186+
return nil
187+
}
188+
189+
func WorkflowLocalActivityIgnored(ctx workflow.Context) error {
190+
workflow.ExecuteLocalActivity(ctx, func(ctx context.Context) error { return nil }) //workflowcheck:ignore
191+
return nil
192+
}
193+
194+
func WorkflowLocalActivityAssignIgnored(ctx workflow.Context) error {
195+
_ = workflow.ExecuteLocalActivity(ctx, func(ctx context.Context) error { return nil }) //workflowcheck:ignore
196+
return nil
197+
}
198+
71199
func WorkflowWithSearchAttributes(workflow.Context) {
72200
sa := temporal.SearchAttributes{}
73201
_ = sa.Copy()
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
11
package internal
22

33
type Context interface{}
4+
5+
func ExecuteLocalActivity(ctx Context, activity interface{}, args ...interface{}) interface{} {
6+
return nil
7+
}

contrib/tools/workflowcheck/workflow/testdata/src/go.temporal.io/sdk/workflow/workflow.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,3 +19,7 @@ func AwaitWithTimeout(ctx Context, timeout time.Duration, condition func() bool)
1919
func SideEffect(ctx Context, f func(ctx Context) interface{}) interface{} {
2020
return nil
2121
}
22+
23+
func ExecuteLocalActivity(ctx Context, activity interface{}, args ...interface{}) interface{} {
24+
return nil
25+
}

0 commit comments

Comments
 (0)