Workflowcheck enforce anonymous func isn't used as local activity#2299
Workflowcheck enforce anonymous func isn't used as local activity#2299Quinn-With-Two-Ns wants to merge 2 commits intotemporalio:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d397981ca4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Quinn-With-Two-Ns
force-pushed
the
issue-1341
branch
from
c26811e to
356a149
Compare
| return false | ||
| } | ||
| // Don't descend into function literals — assignments inside closures | ||
| // that are only defined (not invoked) should not count. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 true { | ||
| f := func(ctx context.Context) error { return nil } | ||
| _ = f | ||
| } | ||
| // f here is a different variable; this uses myLocalActivity directly | ||
| workflow.ExecuteLocalActivity(ctx, myLocalActivity) |
There was a problem hiding this comment.
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
| 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) |
2 participants
What was changed
Change
workflowchecktool to check anyExecuteLocalActivityin a workflow to make sure a user isn't trying to use an anonymous function as a local activity. Check is essentially best effortWhy?
Anonymous function names are not deterministic so they cannot be used as local activities in workflows.
Checklist
Closes WorkflowChecker should flag anonymous functions in local activities as non deterministic #1341
How was this tested:
Note
Medium Risk
Adds new static-analysis rules and AST flow heuristics that may introduce false positives/negatives when determining whether a variable holds an anonymous function at a call site.
Overview
workflowchecknow treats passing an anonymous function toworkflow.ExecuteLocalActivityas non-deterministic by adding a configurable rule (RejectsAnonymousFuncArgs) to the determinism analyzer and emitting a newReasonAnonymousFuncdiagnostic.The analysis includes best-effort tracking of whether an identifier argument could contain a func literal via prior assignments/decls (and conservatively through branches), and expands ignore handling to cover ignored RHS assignment call expressions. Testdata is extended with many
ExecuteLocalActivityscenarios plus stubExecuteLocalActivitydefinitions in workflow/internal packages to compile the new cases.Reviewed by Cursor Bugbot for commit 356a149. Bugbot is set up for automated code reviews on this repo. Configure here.