Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 105 additions & 0 deletions contrib/tools/workflowcheck/determinism/checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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
Comment thread
Quinn-With-Two-Ns marked this conversation as resolved.
}
// Don't descend into function literals — assignments inside closures
// that are only defined (not invoked) should not count.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment isn't entirely accurate, right? The function literal could, in fact, be invoked down the line? In the tests below, we're missing this scenario, which I think would fail due to us unconditionally ignoring function literals

  f := myLocalActivity
  g := func() {
      f = func(ctx context.Context) error { return nil }
  }
  g()
  workflow.ExecuteLocalActivity(ctx, f)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is true, but this would add significant complexity to the check since we would need to check if the closure is ever invoked and that can get very complex.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's valid. But I would recommend making this comment a little more clear that we're consciously choosing not to evaluate inside of closures, regardless of if they're invoked or not, due to complexity

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
Expand Down
16 changes: 16 additions & 0 deletions contrib/tools/workflowcheck/determinism/reason.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,11 +203,27 @@ 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{})
gob.Register(&ReasonFuncCall{})
gob.Register(&ReasonVarAccess{})
gob.Register(&ReasonConcurrency{})
gob.Register(&ReasonMapRange{})
gob.Register(&ReasonAnonymousFunc{})
}
18 changes: 14 additions & 4 deletions contrib/tools/workflowcheck/workflow/checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
}),
}
}
Expand Down Expand Up @@ -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)
Expand Down
138 changes: 138 additions & 0 deletions contrib/tools/workflowcheck/workflow/testdata/src/a/workflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines +157 to +162
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test isn't doing what was intended, it seems like you wanted to test shadowing f, but ExecuteLocalActivity is calling myLocalActivity directly, not an outer f

Suggested change
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)
f := myLocalActivity
if true {
f := func(ctx context.Context) error { return nil }
_ = f
}
// f here is a different variable; this uses myLocalActivity directly
workflow.ExecuteLocalActivity(ctx, f)

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()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
package internal

type Context interface{}

func ExecuteLocalActivity(ctx Context, activity interface{}, args ...interface{}) interface{} {
return nil
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Loading