Repository: google/starlark-go

Issue:
AllowRecursion doesn't allow while loops
This page:

https://pkg.go.dev/go.starlark.net/resolve?utm_source=godoc

seems to suggest that setting AllowRecursion = true will allow while loops. However, if I enable it and then run the code below, I get a "this Starlark dialect does not support while loops" error:

    x = 0
    while x <= 800:
        x = x + 1
    dump(x)


Discussion:
@alistair-cunningham: Thank you for updating the documentation. I've set AllowGlobalReassign = true, and while loops now appear to be working.

Solution patch:
diff --git a/cmd/starlark/starlark.go b/cmd/starlark/starlark.go
index 8076deb..f84ef03 100644
--- a/cmd/starlark/starlark.go
+++ b/cmd/starlark/starlark.go
@@ -38,13 +38,13 @@ func init() {
 	flag.BoolVar(&compile.Disassemble, "disassemble", compile.Disassemble, "show disassembly during compilation of each function")
 
 	// non-standard dialect flags
-	flag.BoolVar(&resolve.AllowSet, "set", resolve.AllowSet, "allow set data type")
 	flag.BoolVar(&resolve.AllowRecursion, "recursion", resolve.AllowRecursion, "allow while statements and recursive functions")
 	flag.BoolVar(&resolve.AllowGlobalReassign, "globalreassign", resolve.AllowGlobalReassign, "allow reassignment of globals, and if/for/while statements at top level")
 
-	// flags that are now standard
-	flag.BoolVar(&resolve.AllowFloat, "float", resolve.AllowFloat, "obsolete; no effect")
-	flag.BoolVar(&resolve.AllowLambda, "lambda", resolve.AllowLambda, "obsolete; no effect")
+	// obsolete flags for features that are now standard
+	flag.BoolVar(&resolve.AllowSet, "set", true, "obsolete; no effect")
+	flag.BoolVar(&resolve.AllowFloat, "float", true, "obsolete; no effect")
+	flag.BoolVar(&resolve.AllowLambda, "lambda", true, "obsolete; no effect")
 }
 
 func main() {
diff --git a/lib/proto/cmd/star2proto/star2proto.go b/lib/proto/cmd/star2proto/star2proto.go
index dffa603..e3b7b9c 100644
--- a/lib/proto/cmd/star2proto/star2proto.go
+++ b/lib/proto/cmd/star2proto/star2proto.go
@@ -37,14 +37,12 @@ var (
 	descriptors = flag.String("descriptors", "", "comma-separated list of names of files containing proto.FileDescriptorProto messages")
 )
 
-// Starlark dialect flags
+// obsolete Starlark dialect flags
 func init() {
-	flag.BoolVar(&resolve.AllowSet, "set", resolve.AllowSet, "allow set data type")
-
-	// obsolete, no effect:
-	flag.BoolVar(&resolve.AllowFloat, "fp", true, "allow floating-point numbers")
-	flag.BoolVar(&resolve.AllowLambda, "lambda", resolve.AllowLambda, "allow lambda expressions")
-	flag.BoolVar(&resolve.AllowNestedDef, "nesteddef", resolve.AllowNestedDef, "allow nested def statements")
+	flag.BoolVar(&resolve.AllowFloat, "fp", true, "obsolete; no effect")
+	flag.BoolVar(&resolve.AllowLambda, "lambda", true, "obsolete; no effect")
+	flag.BoolVar(&resolve.AllowNestedDef, "nesteddef", true, "obsolete; no effect")
+	flag.BoolVar(&resolve.AllowSet, "set", true, "obsolete; no effect")
 }
 
 func main() {
diff --git a/resolve/resolve.go b/resolve/resolve.go
index e00d2b3..c2ca7eb 100644
--- a/resolve/resolve.go
+++ b/resolve/resolve.go
@@ -94,23 +94,29 @@ import (
 const debug = false
 const doesnt = "this Starlark dialect does not "
 
-// global options
-// These features are either not standard Starlark (yet), or deprecated
-// features of the BUILD language, so we put them behind flags.
+// Global options: these features are either not standard Starlark
+// (yet), or deprecated features of the BUILD language, so we put them
+// behind flags.
 //
 // Deprecated: use an explicit [syntax.FileOptions] argument instead,
-// as it avoids all the usual problems of global variables.
+// as it avoids all the usual problems of global variables,
+// and permits finer control.
+//
+// For example The legacy AllowGlobalReassign flag controls three
+// FileOptions: the availability of 'while' loops at all; the use of
+// if/for/while constructs at top level; and the ability to reassign
+// a global variable.
 var (
-	AllowSet            = false // allow the 'set' built-in
-	AllowGlobalReassign = false // allow reassignment to top-level names; also, allow if/for/while at top-level
-	AllowRecursion      = false // allow while statements and recursive functions
+	AllowGlobalReassign = false // allow reassignment to top-level names; while loops; and if/for/while at top-level
+	AllowRecursion      = false // allow recursive functions
 	LoadBindsGlobally   = false // load creates global not file-local bindings (deprecated)
 
 	// obsolete flags for features that are now standard. No effect.
-	AllowNestedDef = true
-	AllowLambda    = true
-	AllowFloat     = true
 	AllowBitwise   = true
+	AllowFloat     = true
+	AllowLambda    = true
+	AllowNestedDef = true
+	AllowSet       = true
 )
 
 // File resolves the specified file and records information about the
diff --git a/starlark/interp.go b/starlark/interp.go
index 261077f..a0af4cb 100644
--- a/starlark/interp.go
+++ b/starlark/interp.go
@@ -24,7 +24,17 @@ func (fn *Function) CallInternal(thread *Thread, args Tuple, kwargs []Tuple) (Va
 	// but allows CALL to avoid a copy.
 
 	f := fn.funcode
-	if !f.Prog.Recursion {
+	if f.Prog.Recursion {
+		// prevent stack overflow
+		//
+		// Each CallInternal recursion (via Call) uses ~1.4KB,
+		// but the stack limit is on the order of 1GB, so a
+		// maximum of about 700K recursive calls is possible.
+		// Limit it to much less here.
+		if len(thread.stack) > 100_000 {
+			return nil, fmt.Errorf("Starlark stack overflow")
+		}
+	} else {
 		// detect recursion
 		for _, fr := range thread.stack[:len(thread.stack)-1] {
 			// We look for the same function code,
diff --git a/starlark/library.go b/starlark/library.go
index 78de542..2ff2544 100644
--- a/starlark/library.go
+++ b/starlark/library.go
@@ -63,7 +63,7 @@ func init() {
 		"range":     NewBuiltin("range", range_),
 		"repr":      NewBuiltin("repr", repr),
 		"reversed":  NewBuiltin("reversed", reversed),
-		"set":       NewBuiltin("set", set), // requires resolve.AllowSet
+		"set":       NewBuiltin("set", set),
 		"sorted":    NewBuiltin("sorted", sorted),
 		"str":       NewBuiltin("str", str),
 		"tuple":     NewBuiltin("tuple", tuple),



Test patch:
diff --git a/starlark/testdata/recursion.star b/starlark/testdata/recursion.star
index 7029ea0..ebaabc5 100644
--- a/starlark/testdata/recursion.star
+++ b/starlark/testdata/recursion.star
@@ -12,3 +12,10 @@ def fib(n):
 	return fib(n-1) + fib(n-2)
 
 assert.eq(fib(5), 8)
+
+def runaway():
+	return runaway()
+
+# Runaway recursion should not overflow the Go stack (#617).
+# The interpreter imposes a limit long before then.
+assert.fails(runaway, "Starlark stack overflow")
\ No newline at end of file