Repository: google/starlark-go
Pull request #434 (2022-10-20 14:56:16)

lib/json: fix trivially broken tests
Also, use uppercase for "JSON" in the same error message.


Patch:
diff --git a/lib/json/json.go b/lib/json/json.go
index 80b6f70..6d9e99e 100644
--- a/lib/json/json.go
+++ b/lib/json/json.go
@@ -112,7 +112,7 @@ func encode(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, k
 		// cases did not show significant improvement on the benchmarks.
 		if ptr := pointer(x); ptr != nil {
 			if pathContains(path, ptr) {
-				return fmt.Errorf("cycle in json structure")
+				return fmt.Errorf("cycle in JSON structure")
 			}
 
 			path = append(path, ptr)
diff --git a/starlark/eval.go b/starlark/eval.go
index 379d038..23614eb 100644
--- a/starlark/eval.go
+++ b/starlark/eval.go
@@ -1233,8 +1233,22 @@ func Call(thread *Thread, fn Value, args Tuple, kwargs []Tuple) (Value, error) {
 	fr.callable = c
 
 	thread.beginProfSpan()
+
+	// Use defer to ensure that panics from built-ins
+	// pass through the interpreter without leaving
+	// it in a bad state.
+	defer func() {
+		thread.endProfSpan()
+
+		// clear out any references
+		// TODO(adonovan): opt: zero fr.Locals and
+		// reuse it if it is large enough.
+		*fr = frame{}
+
+		thread.stack = thread.stack[:len(thread.stack)-1] // pop
+	}()
+
 	result, err := c.CallInternal(thread, args, kwargs)
-	thread.endProfSpan()
 
 	// Sanity check: nil is not a valid Starlark value.
 	if result == nil && err == nil {
@@ -1248,9 +1262,6 @@ func Call(thread *Thread, fn Value, args Tuple, kwargs []Tuple) (Value, error) {
 		}
 	}
 
-	*fr = frame{}                                     // clear out any references
-	thread.stack = thread.stack[:len(thread.stack)-1] // pop
-
 	return result, err
 }
 
diff --git a/starlark/hashtable.go b/starlark/hashtable.go
index 9fab60e..fed7c3a 100644
--- a/starlark/hashtable.go
+++ b/starlark/hashtable.go
@@ -372,6 +372,8 @@ func (it *keyIterator) Done() {
 	}
 }
 
+// TODO(adonovan): use go1.19's maphash.String.
+
 // hashString computes the hash of s.
 func hashString(s string) uint32 {
 	if len(s) >= 12 {
diff --git a/starlark/interp.go b/starlark/interp.go
index 8b073bd..b41905a 100644
--- a/starlark/interp.go
+++ b/starlark/interp.go
@@ -81,6 +81,17 @@ func (fn *Function) CallInternal(thread *Thread, args Tuple, kwargs []Tuple) (Va
 
 	var iterstack []Iterator // stack of active iterators
 
+	// Use defer so that application panics can pass through
+	// interpreter without leaving thread in a bad state.
+	defer func() {
+		// ITERPOP the rest of the iterator stack.
+		for _, iter := range iterstack {
+			iter.Done()
+		}
+
+		fr.locals = nil
+	}()
+
 	sp := 0
 	var pc uint32
 	var result Value
@@ -646,14 +657,7 @@ loop:
 			break loop
 		}
 	}
-
-	// ITERPOP the rest of the iterator stack.
-	for _, iter := range iterstack {
-		iter.Done()
-	}
-
-	fr.locals = nil
-
+	// (deferred cleanup runs here)
 	return result, err
 }
 


Test patch:
diff --git a/starlark/eval_test.go b/starlark/eval_test.go
index 448401e..5378615 100644
--- a/starlark/eval_test.go
+++ b/starlark/eval_test.go
@@ -1002,3 +1002,52 @@ func TestDeps(t *testing.T) {
 		}
 	}
 }
+
+// TestPanicSafety ensures that a panic from an application-defined
+// built-in may traverse the interpreter safely; see issue #411.
+func TestPanicSafety(t *testing.T) {
+	predeclared := starlark.StringDict{
+		"panic": starlark.NewBuiltin("panic", func(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {
+			panic(args[0])
+		}),
+		"list": starlark.NewList([]starlark.Value{starlark.MakeInt(0)}),
+	}
+
+	// This program is executed twice, using the same Thread,
+	// and panics both times, with values 1 and 2, while
+	// main is on the stack and a for-loop is active.
+	//
+	// It mutates list, a predeclared variable.
+	// This operation would fail if the previous
+	// for-loop failed to close its iterator during the panic.
+	//
+	// It also calls main a second time without recursion enabled.
+	// This operation would fail if the previous
+	// call failed to pop main from the stack during the panic.
+	const src = `
+list[0] += 1
+
+def main():
+    for x in list:
+        panic(x)
+
+main()
+`
+	thread := new(starlark.Thread)
+	for _, i := range []int{1, 2} {
+		// Use a func to limit the scope of recover.
+		func() {
+			defer func() {
+				if got := fmt.Sprint(recover()); got != fmt.Sprint(i) {
+					t.Fatalf("recover: got %v, want %v", got, i)
+				}
+			}()
+			v, err := starlark.ExecFile(thread, "panic.star", src, predeclared)
+			if err != nil {
+				t.Fatalf("ExecFile returned error %q, expected panic", err)
+			} else {
+				t.Fatalf("ExecFile returned %v, expected panic", v)
+			}
+		}()
+	}
+}
diff --git a/starlark/testdata/json.star b/starlark/testdata/json.star
index 9e222db..4f66637 100644
--- a/starlark/testdata/json.star
+++ b/starlark/testdata/json.star
@@ -39,15 +39,15 @@ encode_error({1: 2}, 'dict has int key, want string')
 
 recursive_map = {}
 recursive_map["r"] = recursive_map
-encode_error(recursive_map, 'json.encode: in dict key "r": Detected cycle in json structure')
+encode_error(recursive_map, 'json.encode: in dict key "r": cycle in JSON structure')
 
 recursive_list = []
 recursive_list.append(recursive_list)
-encode_error(recursive_list, 'json.encode: at list index 0: Detected cycle in json structure')
+encode_error(recursive_list, 'json.encode: at list index 0: cycle in JSON structure')
 
 recursive_tuple = (1, 2, [])
 recursive_tuple[2].append(recursive_tuple)
-encode_error(recursive_tuple, 'json.encode: at tuple index 2: at list index 0: Detected cycle in json structure')
+encode_error(recursive_tuple, 'json.encode: at tuple index 2: at list index 0: cycle in JSON structure')
 
 ## json.decode