starlark: return EvalErr for errors originating in built-ins (google#188

)

Previously, only errors originating in Starlark code would be
represented as EvalErr. The backtrace would include frames for
built-in functions that the exception passed through, but not
for the originating frame.

Also, report "<builtin>" not "<builtin>:1" for the position
of such frames. Position.Line==0 no longer implies !IsValid.
Clients cannot rely on valid position strings containing a colon.

Added test of errors originating in a built-in frame.

starlark/eval.go

@@ -157,7 +157,7 @@ func (fr *Frame) Position() syntax.Position {
 		// a Position method, use it.
 		return c.Position()
 	}
-	return syntax.MakePosition(&builtinFilename, 1, 0)
+	return syntax.MakePosition(&builtinFilename, 0, 0)
 }
 
 var builtinFilename = "<builtin>"
@@ -991,15 +991,23 @@ func Call(thread *Thread, fn Value, args Tuple, kwargs []Tuple) (Value, error) {
 		return nil, fmt.Errorf("invalid call of non-function (%s)", fn.Type())
 	}
 
-	thread.frame = NewFrame(thread.frame, c)
+	fr := NewFrame(thread.frame, c)
+	thread.frame = fr
 	thread.beginProfSpan()
 	result, err := c.CallInternal(thread, args, kwargs)
 	thread.endProfSpan()
 	thread.frame = thread.frame.parent
 
 	// Sanity check: nil is not a valid Starlark value.
 	if result == nil && err == nil {
-		return nil, fmt.Errorf("internal error: nil (not None) returned from %s", fn)
+		err = fmt.Errorf("internal error: nil (not None) returned from %s", fn)
+	}
+
+	// Always return an EvalError with an accurate frame.
+	if err != nil {
+		if _, ok := err.(*EvalError); !ok {
+			err = fr.errorf(fr.Position(), "%s", err.Error())
+		}
 	}
 
 	return result, err

starlark/eval_test.go

@@ -482,6 +482,18 @@ func TestInt(t *testing.T) {
 }
 
 func TestBacktrace(t *testing.T) {
+	getBacktrace := func(err error) string {
+		switch err := err.(type) {
+		case *starlark.EvalError:
+			return err.Backtrace()
+		case nil:
+			t.Fatalf("ExecFile succeeded unexpectedly")
+		default:
+			t.Fatalf("ExecFile failed with %v, wanted *EvalError", err)
+		}
+		panic("unreachable")
+	}
+
 	// This test ensures continuity of the stack of active Starlark
 	// functions, including propagation through built-ins such as 'min'.
 	const src = `
@@ -493,25 +505,44 @@ i()
 `
 	thread := new(starlark.Thread)
 	_, err := starlark.ExecFile(thread, "crash.star", src, nil)
-	switch err := err.(type) {
-	case *starlark.EvalError:
-		got := err.Backtrace()
-		// Compiled code currently has no column information.
-		const want = `Traceback (most recent call last):
+	// Compiled code currently has no column information.
+	const want = `Traceback (most recent call last):
   crash.star:6: in <toplevel>
   crash.star:5: in i
   crash.star:4: in h
-  <builtin>:1: in min
+  <builtin>: in min
   crash.star:3: in g
   crash.star:2: in f
 Error: floored division by zero`
-		if got != want {
+	if got := getBacktrace(err); got != want {
+		t.Errorf("error was %s, want %s", got, want)
+	}
+
+	// Additionally, ensure that errors originating in
+	// Starlark and/or Go each have an accurate frame.
+	//
+	// This program fails in Starlark (f) if x==0,
+	// or in Go (string.join) if x is non-zero.
+	const src2 = `
+def f(): ''.join([1//i])
+f()
+`
+	for i, want := range []string{
+		0: `Traceback (most recent call last):
+  crash.star:3: in <toplevel>
+  crash.star:2: in f
+Error: floored division by zero`,
+		1: `Traceback (most recent call last):
+  crash.star:3: in <toplevel>
+  crash.star:2: in f
+  <builtin>: in join
+Error: join: in list, want string, got int`,
+	} {
+		globals := starlark.StringDict{"i": starlark.MakeInt(i)}
+		_, err := starlark.ExecFile(thread, "crash.star", src2, globals)
+		if got := getBacktrace(err); got != want {
 			t.Errorf("error was %s, want %s", got, want)
 		}
-	case nil:
-		t.Error("ExecFile succeeded unexpectedly")
-	default:
-		t.Errorf("ExecFile failed with %v, wanted *EvalError", err)
 	}
 }
 

starlark/interp.go

@@ -576,12 +576,6 @@ loop:
 		iter.Done()
 	}
 
-	if err != nil {
-		if _, ok := err.(*EvalError); !ok {
-			err = fr.errorf(f.Position(fr.pc), "%s", err.Error())
-		}
-	}
-
 	fr.locals = nil
 
 	return result, err

syntax/scan.go

@@ -185,21 +185,19 @@ var tokenNames = [...]string{
 // A Position describes the location of a rune of input.
 type Position struct {
 	file *string // filename (indirect for compactness)
-	Line int32   // 1-based line number
-	Col  int32   // 1-based column number (strictly: rune)
+	Line int32   // 1-based line number; 0 if line unknown
+	Col  int32   // 1-based column (rune) number; 0 if column unknown
 }
 
 // IsValid reports whether the position is valid.
-func (p Position) IsValid() bool {
-	return p.Line >= 1
-}
+func (p Position) IsValid() bool { return p.file != nil }
 
 // Filename returns the name of the file containing this position.
 func (p Position) Filename() string {
 	if p.file != nil {
 		return *p.file
 	}
-	return "<unknown>"
+	return "<invalid>"
 }
 
 // MakePosition returns position with the specified components.
@@ -217,10 +215,14 @@ func (p Position) add(s string) Position {
 }
 
 func (p Position) String() string {
-	if p.Col > 0 {
-		return fmt.Sprintf("%s:%d:%d", p.Filename(), p.Line, p.Col)
+	file := p.Filename()
+	if p.Line > 0 {
+		if p.Col > 0 {
+			return fmt.Sprintf("%s:%d:%d", file, p.Line, p.Col)
+		}
+		return fmt.Sprintf("%s:%d", file, p.Line)
 	}
-	return fmt.Sprintf("%s:%d", p.Filename(), p.Line)
+	return file
 }
 
 func (p Position) isBefore(q Position) bool {