Code inside workflow.SideEffect is deterministic (#1230)

Code inside workflow.SideEffect is deterministic

contrib/tools/workflowcheck/determinism/checker.go

@@ -34,6 +34,7 @@ import (
 	"strings"
 	"sync"
 
+	"golang.org/x/exp/slices"
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/types/typeutil"
 )
@@ -50,6 +51,8 @@ type Config struct {
 	Debug bool
 	// Whether to export a *NonDeterminisms fact per object.
 	EnableObjectFacts bool
+	// Map `package -> function names` with functions making any argument deterministic
+	AcceptsNonDeterministicParameters map[string][]string
 }
 
 // Checker is a checker that can run analysis passes to check for
@@ -301,8 +304,10 @@ 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 {
-				// If it's in a different package, check externals
-				if c.pass.Pkg != callee.Pkg() {
+				if callee.Pkg() != nil && slices.Contains(c.checker.AcceptsNonDeterministicParameters[callee.Pkg().Path()], callee.Name()) {
+					return false
+				} else if c.pass.Pkg != callee.Pkg() {
+					// If it's in a different package, check externals
 					calleeReasons := c.externalFuncNonDeterminisms(callee)
 					if len(calleeReasons) > 0 {
 						c.checker.debugf("Marking %v as non-deterministic because it calls %v", fn.FullName(), callee.FullName())

contrib/tools/workflowcheck/determinism/determinism_test.go

@@ -41,11 +41,12 @@ func Test(t *testing.T) {
 		t,
 		analysistest.TestData(),
 		determinism.NewChecker(determinism.Config{
-			IdentRefs:         identRefs,
-			SkipFiles:         []*regexp.Regexp{regexp.MustCompile(`.*/should_skip\.go`)},
-			Debug:             false, // Set to true to see details
-			DebugfFunc:        t.Logf,
-			EnableObjectFacts: true,
+			IdentRefs:                         identRefs,
+			SkipFiles:                         []*regexp.Regexp{regexp.MustCompile(`.*/should_skip\.go`)},
+			Debug:                             false, // Set to true to see details
+			DebugfFunc:                        t.Logf,
+			EnableObjectFacts:                 true,
+			AcceptsNonDeterministicParameters: map[string][]string{"a": {"DeterministicWrapper"}},
 		}).NewAnalyzer(),
 		"a",
 	)

contrib/tools/workflowcheck/determinism/testdata/src/a/calls.go

@@ -62,3 +62,28 @@ func CallsHTTP() { // want CallsHTTP:"calls non-deterministic function net/http.
 func SafeFmtCall() {
 	fmt.Sprintf("foo bar")
 }
+
+func NotSafeFmtCall() { // want NotSafeFmtCall:"calls non-deterministic function time.Now"
+	fmt.Sprintf("%d", time.Now())
+}
+
+func SafeMathRandomCall() {
+	DeterministicWrapper(func() {
+		mathrand.Int()
+	})
+}
+
+func NotSafeMathRandomCall() { // want NotSafeMathRandomCall:"calls non-deterministic function math/rand.Int"
+	DeterministicWrapper(func() {
+		mathrand.Int()
+	})
+	NonDeterministicWrapper(func() {
+		mathrand.Int()
+	})
+}
+
+func DeterministicWrapper(func()) {
+}
+
+func NonDeterministicWrapper(func()) {
+}

contrib/tools/workflowcheck/go.mod

@@ -3,13 +3,14 @@ module go.temporal.io/sdk/contrib/tools/workflowcheck
 go 1.20
 
 require (
-	golang.org/x/tools v0.10.0
+	golang.org/x/exp v0.0.0-20230905200255-921286631fa9
+	golang.org/x/tools v0.13.0
 	gopkg.in/yaml.v2 v2.4.0
 )
 
 require (
 	github.com/kr/pretty v0.1.0 // indirect
-	golang.org/x/mod v0.11.0 // indirect
-	golang.org/x/sys v0.9.0 // indirect
+	golang.org/x/mod v0.12.0 // indirect
+	golang.org/x/sys v0.12.0 // indirect
 	gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 // indirect
 )

contrib/tools/workflowcheck/go.sum

@@ -3,13 +3,15 @@ github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORN
 github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
 github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
-golang.org/x/mod v0.11.0 h1:bUO06HqtnRcc/7l71XBe4WcqTZ+3AH1J59zWDDwLKgU=
-golang.org/x/mod v0.11.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs=
+golang.org/x/exp v0.0.0-20230905200255-921286631fa9 h1:GoHiUyI/Tp2nVkLI2mCxVkOjsbSXD66ic0XW0js0R9g=
+golang.org/x/exp v0.0.0-20230905200255-921286631fa9/go.mod h1:S2oDrQGGwySpoQPVqRShND87VCbxmc6bL1Yd2oYrm6k=
+golang.org/x/mod v0.12.0 h1:rmsUpXtvNzj340zd98LZ4KntptpfRHwpFOHG188oHXc=
+golang.org/x/mod v0.12.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs=
 golang.org/x/sync v0.3.0 h1:ftCYgMx6zT/asHUrPw8BLLscYtGznsLAnjq5RH9P66E=
-golang.org/x/sys v0.9.0 h1:KS/R3tvhPqvJvwcKfnBHJwwthS11LRhmM5D59eEXa0s=
-golang.org/x/sys v0.9.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/tools v0.10.0 h1:tvDr/iQoUqNdohiYm0LmmKcBk+q86lb9EprIUFhHHGg=
-golang.org/x/tools v0.10.0/go.mod h1:UJwyiVBsOA2uwvK/e5OY3GTpDUJriEd+/YlqAwLPmyM=
+golang.org/x/sys v0.12.0 h1:CM0HF96J0hcLAwsHPJZjfdNzs0gftsLfgKt57wWHJ0o=
+golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/tools v0.13.0 h1:Iey4qkscZuv0VvIt8E0neZjtPVQFSc870HQ448QgEmQ=
+golang.org/x/tools v0.13.0/go.mod h1:HvlwmtVNQAhOuCjW7xxvovg8wbNq7LwfXh/k7wXUl58=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY=
 gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=

contrib/tools/workflowcheck/workflow/checker.go

@@ -92,10 +92,11 @@ func NewChecker(config Config) *Checker {
 		Debug:               config.Debug,
 		IncludePosOnMessage: config.IncludePosOnMessage,
 		Determinism: determinism.NewChecker(determinism.Config{
-			IdentRefs:         config.IdentRefs,
-			DebugfFunc:        config.DebugfFunc,
-			Debug:             config.DeterminismDebug,
-			EnableObjectFacts: config.EnableObjectFacts,
+			IdentRefs:                         config.IdentRefs,
+			DebugfFunc:                        config.DebugfFunc,
+			Debug:                             config.DeterminismDebug,
+			EnableObjectFacts:                 config.EnableObjectFacts,
+			AcceptsNonDeterministicParameters: map[string][]string{"go.temporal.io/sdk/workflow": {"SideEffect", "MutableSideEffect"}},
 		}),
 	}
 }

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

@@ -17,6 +17,21 @@ func WorkflowCallTime(ctx workflow.Context) error { // want "a.WorkflowCallTime
 	return nil
 }
 
+func WorkflowCallTimeInSideEffect(ctx workflow.Context) error {
+	workflow.SideEffect(ctx, func(ctx workflow.Context) interface{} {
+		return time.Now()
+	})
+	return nil
+}
+
+func WorkflowCallTimeInSideEffectAndNot(ctx workflow.Context) error { // want "a.WorkflowCallTimeInSideEffectAndNot is non-deterministic, reason: calls non-deterministic function time.Now"
+	workflow.SideEffect(ctx, func(ctx workflow.Context) interface{} {
+		return time.Now()
+	})
+	time.Now()
+	return nil
+}
+
 func WorkflowCallTimeTransitively(ctx workflow.Context) error { // want "a.WorkflowCallTimeTransitively is non-deterministic, reason: calls non-deterministic function a.SomeTimeCall"
 	SomeTimeCall()
 	return nil

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

@@ -11,3 +11,7 @@ func AwaitWithTimeout(ctx Context, timeout time.Duration, condition func() bool)
 	time.Sleep(10 * time.Second)
 	return false, nil
 }
+
+func SideEffect(ctx Context, f func(ctx Context) interface{}) interface{} {
+	return nil
+}