Builtin: push a Frame on the Thread's stack even when calling Builtins (#112)

* Builtin: push a Frame on the Thread's stack even when calling Builtins

This makes the stack trace more accurate.

Breaking API change: (*Frame).Function is superseded by (*Frame).Callable.

4 files changed, 50 insertions, 35 deletions

diff --git a/eval.go b/eval.go
index b11285b..7a7444a 100644
--- a/eval.go
+++ b/eval.go
@@ -62,11 +62,12 @@ func (thread *Thread) Local(key string) interface{} {
 	return thread.locals[key]
 }
 
-// Caller returns the frame of the innermost enclosing Skylark function.
+// Caller returns the frame of the caller of the current function.
 // It should only be used in built-ins called from Skylark code.
-func (thread *Thread) Caller() *Frame {
-	return thread.frame
-}
+func (thread *Thread) Caller() *Frame { return thread.frame.parent }
+
+// TopFrame returns the topmost stack frame.
+func (thread *Thread) TopFrame() *Frame { return thread.frame }
 
 // A StringDict is a mapping from names to values, and represents
 // an environment such as the global variables of a module.
@@ -104,15 +105,19 @@ func (d StringDict) Freeze() {
 // Has reports whether the dictionary contains the specified key.
 func (d StringDict) Has(key string) bool { _, ok := d[key]; return ok }
 
-// A Frame holds the execution state of a single Skylark function call
-// or module toplevel.
+// A Frame records a call to a Skylark function (including module toplevel)
+// or a built-in function or method.
 type Frame struct {
-	parent *Frame          // caller's frame (or nil)
-	posn   syntax.Position // source position of PC, set during error
-	callpc uint32          // PC of position of active call, set during call
-	fn     *Function       // current function (or toplevel)
+	parent   *Frame          // caller's frame (or nil)
+	callable Callable        // current function (or toplevel) or built-in
+	posn     syntax.Position // source position of PC, set during error
+	callpc   uint32          // PC of position of active call, set during call
 }
 
+// The Frames of a thread are structured as a spaghetti stack, not a
+// slice, so that an EvalError can copy a stack efficiently and immutably.
+// In hindsight using a slice would have led to a more convenient API.
+
 func (fr *Frame) errorf(posn syntax.Position, format string, args ...interface{}) *EvalError {
 	fr.posn = posn
 	msg := fmt.Sprintf(format, args...)
@@ -124,11 +129,16 @@ func (fr *Frame) Position() syntax.Position {
 	if fr.posn.IsValid() {
 		return fr.posn // leaf frame only (the error)
 	}
-	return fr.fn.funcode.Position(fr.callpc) // position of active call
+	if fn, ok := fr.callable.(*Function); ok {
+		return fn.funcode.Position(fr.callpc) // position of active call
+	}
+	return syntax.MakePosition(&builtinFilename, 1, 0)
 }
 
-// Function returns the frame's function, or nil for the top-level of a module.
-func (fr *Frame) Function() *Function { return fr.fn }
+var builtinFilename = "<builtin>"
+
+// Function returns the frame's function or built-in.
+func (fr *Frame) Callable() Callable { return fr.callable }
 
 // Parent returns the frame of the enclosing function call, if any.
 func (fr *Frame) Parent() *Frame { return fr.parent }
@@ -157,7 +167,7 @@ func (fr *Frame) WriteBacktrace(out *bytes.Buffer) {
 	print = func(fr *Frame) {
 		if fr != nil {
 			print(fr.parent)
-			fmt.Fprintf(out, "  %s: in %s\n", fr.Position(), fr.fn.Name())
+			fmt.Fprintf(out, "  %s: in %s\n", fr.Position(), fr.Callable().Name())
 		}
 	}
 	print(fr)
diff --git a/eval_test.go b/eval_test.go
index 500c77d..8edd8c0 100644
--- a/eval_test.go
+++ b/eval_test.go
@@ -322,7 +322,7 @@ f()
 	print := func(thread *skylark.Thread, msg string) {
 		caller := thread.Caller()
 		fmt.Fprintf(buf, "%s: %s: %s\n",
-			caller.Position(), caller.Function().Name(), msg)
+			caller.Position(), caller.Callable().Name(), msg)
 	}
 	thread := &skylark.Thread{Print: print}
 	if _, err := skylark.ExecFile(thread, "foo.go", src, nil); err != nil {
@@ -420,17 +420,18 @@ def i(): return h()
 i()
 `
 	thread := new(skylark.Thread)
-	_, err := skylark.ExecFile(thread, "crash.go", src, nil)
+	_, err := skylark.ExecFile(thread, "crash.sky", src, nil)
 	switch err := err.(type) {
 	case *skylark.EvalError:
 		got := err.Backtrace()
 		// Compiled code currently has no column information.
 		const want = `Traceback (most recent call last):
-  crash.go:6: in <toplevel>
-  crash.go:5: in i
-  crash.go:4: in h
-  crash.go:3: in g
-  crash.go:2: in f
+  crash.sky:6: in <toplevel>
+  crash.sky:5: in i
+  crash.sky:4: in h
+  <builtin>:1: in min
+  crash.sky:3: in g
+  crash.sky:2: in f
 Error: floored division by zero`
 		if got != want {
 			t.Errorf("error was %s, want %s", got, want)
diff --git a/interp.go b/interp.go
index 60d4890..baa83c1 100644
--- a/interp.go
+++ b/interp.go
@@ -28,12 +28,12 @@ func (fn *Function) Call(thread *Thread, args Tuple, kwargs []Tuple) (Value, err
 		// We look for the same function code,
 		// not function value, otherwise the user could
 		// defeat the check by writing the Y combinator.
-		if fr.fn != nil && fr.fn.funcode == fn.funcode {
+		if frfn, ok := fr.Callable().(*Function); ok && frfn.funcode == fn.funcode {
 			return nil, fmt.Errorf("function %s called recursively", fn.Name())
 		}
 	}
 
-	thread.frame = &Frame{parent: thread.frame, fn: fn}
+	thread.frame = &Frame{parent: thread.frame, callable: fn}
 	result, err := call(thread, args, kwargs)
 	thread.frame = thread.frame.parent
 	return result, err
@@ -41,15 +41,16 @@ func (fn *Function) Call(thread *Thread, args Tuple, kwargs []Tuple) (Value, err
 
 func call(thread *Thread, args Tuple, kwargs []Tuple) (Value, error) {
 	fr := thread.frame
-	f := fr.fn.funcode
+	fn := fr.callable.(*Function)
+	f := fn.funcode
 	nlocals := len(f.Locals)
 	stack := make([]Value, nlocals+f.MaxStack)
 	locals := stack[:nlocals:nlocals] // local variables, starting with parameters
 	stack = stack[nlocals:]
 
-	err := setArgs(locals, fr.fn, args, kwargs)
+	err := setArgs(locals, fn, args, kwargs)
 	if err != nil {
-		return nil, fr.errorf(fr.fn.Position(), "%v", err)
+		return nil, fr.errorf(fr.Position(), "%v", err)
 	}
 
 	if vmdebug {
@@ -432,7 +433,7 @@ loop:
 			sp--
 
 		case compile.CONSTANT:
-			stack[sp] = fr.fn.constants[arg]
+			stack[sp] = fn.constants[arg]
 			sp++
 
 		case compile.MAKETUPLE:
@@ -458,9 +459,9 @@ loop:
 			sp -= 2
 			stack[sp] = &Function{
 				funcode:     funcode,
-				predeclared: fr.fn.predeclared,
-				globals:     fr.fn.globals,
-				constants:   fr.fn.constants,
+				predeclared: fn.predeclared,
+				globals:     fn.globals,
+				constants:   fn.constants,
 				defaults:    defaults,
 				freevars:    freevars,
 			}
@@ -497,7 +498,7 @@ loop:
 			sp--
 
 		case compile.SETGLOBAL:
-			fr.fn.globals[arg] = stack[sp-1]
+			fn.globals[arg] = stack[sp-1]
 			sp--
 
 		case compile.LOCAL:
@@ -510,11 +511,11 @@ loop:
 			sp++
 
 		case compile.FREE:
-			stack[sp] = fr.fn.freevars[arg]
+			stack[sp] = fn.freevars[arg]
 			sp++
 
 		case compile.GLOBAL:
-			x := fr.fn.globals[arg]
+			x := fn.globals[arg]
 			if x == nil {
 				err = fmt.Errorf("global variable %s referenced before assignment", f.Prog.Globals[arg].Name)
 				break loop
@@ -524,7 +525,7 @@ loop:
 
 		case compile.PREDECLARED:
 			name := f.Prog.Names[arg]
-			x := fr.fn.predeclared[name]
+			x := fn.predeclared[name]
 			if x == nil {
 				err = fmt.Errorf("internal error: predeclared variable %s is uninitialized", name)
 				break loop
diff --git a/value.go b/value.go
index bd2aa34..eefc201 100644
--- a/value.go
+++ b/value.go
@@ -559,7 +559,10 @@ func (b *Builtin) Receiver() Value { return b.recv }
 func (b *Builtin) String() string  { return toString(b) }
 func (b *Builtin) Type() string    { return "builtin_function_or_method" }
 func (b *Builtin) Call(thread *Thread, args Tuple, kwargs []Tuple) (Value, error) {
-	return b.fn(thread, b, args, kwargs)
+	thread.frame = &Frame{parent: thread.frame, callable: b}
+	result, err := b.fn(thread, b, args, kwargs)
+	thread.frame = thread.frame.parent
+	return result, err
 }
 func (b *Builtin) Truth() Bool { return true }