Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check all functions with workflow.Context as first parameter #1215

Merged
merged 2 commits into from
Aug 30, 2023
Merged
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
2 changes: 1 addition & 1 deletion contrib/tools/workflowcheck/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
49 changes: 23 additions & 26 deletions contrib/tools/workflowcheck/workflow/checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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())
Expand All @@ -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
Expand All @@ -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>" }
Expand Down
33 changes: 13 additions & 20 deletions contrib/tools/workflowcheck/workflow/testdata/src/a/workflow.go
Original file line number Diff line number Diff line change
@@ -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)
}

Expand All @@ -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()
}
4 changes: 1 addition & 3 deletions contrib/tools/workflowcheck/workflow/workflow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
)
}
Loading