From 0f049eaa385f81f7b8f868bfbfbcce3c627415b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stanis=C5=82aw=20Barzowski?= Date: Wed, 4 Oct 2017 18:22:15 -0400 Subject: [PATCH] Basic tailstrict support --- evaluator.go | 15 ++- interpreter.go | 117 +++++++++++++--------- testdata/percent_format_str4.golden | 10 -- testdata/percent_format_str5.golden | 6 +- testdata/percent_format_str6.golden | 6 +- testdata/percent_format_str7.golden | 6 +- testdata/stackbug-regression-test.golden | 1 + testdata/stackbug-regression-test.jsonnet | 1 + testdata/tailstrict.golden | 1 + testdata/tailstrict.jsonnet | 2 + testdata/tailstrict2.golden | 15 +++ testdata/tailstrict2.jsonnet | 2 + testdata/tailstrict3.golden | 15 +++ testdata/tailstrict3.jsonnet | 2 + testdata/tailstrict4.golden | 1 + testdata/tailstrict4.jsonnet | 2 + testdata/tailstrict5.golden | 1 + testdata/tailstrict5.jsonnet | 7 ++ thunks.go | 21 +++- value.go | 1 + 20 files changed, 160 insertions(+), 72 deletions(-) create mode 100644 testdata/stackbug-regression-test.golden create mode 100644 testdata/stackbug-regression-test.jsonnet create mode 100644 testdata/tailstrict.golden create mode 100644 testdata/tailstrict.jsonnet create mode 100644 testdata/tailstrict2.golden create mode 100644 testdata/tailstrict2.jsonnet create mode 100644 testdata/tailstrict3.golden create mode 100644 testdata/tailstrict3.jsonnet create mode 100644 testdata/tailstrict4.golden create mode 100644 testdata/tailstrict4.jsonnet create mode 100644 testdata/tailstrict5.golden create mode 100644 testdata/tailstrict5.jsonnet diff --git a/evaluator.go b/evaluator.go index 984931c..43e80a4 100644 --- a/evaluator.go +++ b/evaluator.go @@ -42,6 +42,13 @@ func (e *evaluator) evaluate(ph potentialValue) (value, error) { return ph.getValue(e.i, e.trace) } +func (e *evaluator) evaluateTailCall(ph potentialValue, tc tailCallStatus) (value, error) { + if tc == tailCall { + e.i.stack.tailCallTrimStack() + } + return ph.getValue(e.i, e.trace) +} + func (e *evaluator) Error(s string) error { err := makeRuntimeError(s, e.i.getCurrentStackTrace(e.trace)) return err @@ -161,12 +168,12 @@ func (e *evaluator) evaluateObject(pv potentialValue) (valueObject, error) { return e.getObject(v) } -func (e *evaluator) evalInCurrentContext(a ast.Node) (value, error) { - return e.i.evaluate(a) +func (e *evaluator) evalInCurrentContext(a ast.Node, tc tailCallStatus) (value, error) { + return e.i.evaluate(a, tc) } -func (e *evaluator) evalInCleanEnv(env *environment, ast ast.Node) (value, error) { - return e.i.EvalInCleanEnv(e.trace, env, ast) +func (e *evaluator) evalInCleanEnv(env *environment, ast ast.Node, trimmable bool) (value, error) { + return e.i.EvalInCleanEnv(e.trace, env, ast, trimmable) } func (e *evaluator) lookUpVar(ident ast.Identifier) potentialValue { diff --git a/interpreter.go b/interpreter.go index 5b8a32c..036171e 100644 --- a/interpreter.go +++ b/interpreter.go @@ -74,17 +74,26 @@ type callFrame struct { // Tracing information about the place where it was called from. trace *TraceElement - /** Reuse this stack frame for the purpose of tail call optimization. */ - tailCall bool // TODO what is it? + // Whether this frame can be removed from the stack when it doesn't affect + // the evaluation result, but in case of an error, it won't appear on the + // stack trace. + // It's used for tail call optimization. + trimmable bool env environment } func dumpCallFrame(c *callFrame) string { - return fmt.Sprintf("", + var loc ast.LocationRange + if c.trace == nil || c.trace.loc == nil { + loc = ast.MakeLocationRangeMessage("?") + } else { + loc = *c.trace.loc + } + return fmt.Sprintf("", c.isCall, - *c.trace.loc, - c.tailCall, + loc, + c.trimmable, ) } @@ -105,24 +114,27 @@ func dumpCallStack(c *callStack) string { } func (s *callStack) top() *callFrame { - return s.stack[len(s.stack)-1] + r := s.stack[len(s.stack)-1] + return r } -func (s *callStack) pop() { - if s.top().isCall { - s.calls-- +// It might've been popped already by tail call optimization. +// We check if it was trimmed by comparing the current stack size to the position +// of the frame we want to pop. +func (s *callStack) popIfExists(whichFrame int) { + if len(s.stack) == whichFrame { + if s.top().isCall { + s.calls-- + } + s.stack = s.stack[:len(s.stack)-1] } - s.stack = s.stack[:len(s.stack)-1] } -// TODO(sbarzowski) I don't get this. When we have a tail call why can't we just -// pop the last call from stack before pushing our new thing. -// https://github.com/google/go-jsonnet/pull/24#pullrequestreview-58524217 -/** If there is a tailstrict annotated frame followed by some locals, pop them all. */ +/** If there is a trimmable frame followed by some locals, pop them all. */ func (s *callStack) tailCallTrimStack() { for i := len(s.stack) - 1; i >= 0; i-- { if s.stack[i].isCall { - if !s.stack[i].tailCall { // TODO(sbarzowski) we may need to check some more stuff + if !s.stack[i].trimmable { return } // Remove this stack frame and everything above it @@ -133,18 +145,23 @@ func (s *callStack) tailCallTrimStack() { } } -func (i *interpreter) newCall(trace *TraceElement, env environment) error { +type tailCallStatus int + +const ( + nonTailCall tailCallStatus = iota + tailCall +) + +func (i *interpreter) newCall(trace *TraceElement, env environment, trimmable bool) error { s := &i.stack - s.tailCallTrimStack() if s.calls >= s.limit { - // TODO(sbarzowski) add tracing information return makeRuntimeError("Max stack frames exceeded.", i.getCurrentStackTrace(trace)) } s.stack = append(s.stack, &callFrame{ - isCall: true, - trace: trace, - env: env, - tailCall: false, + isCall: true, + trace: trace, + env: env, + trimmable: trimmable, }) s.calls++ return nil @@ -239,7 +256,7 @@ func (i *interpreter) getCurrentEnv(ast ast.Node) environment { ) } -func (i *interpreter) evaluate(a ast.Node) (value, error) { +func (i *interpreter) evaluate(a ast.Node, tc tailCallStatus) (value, error) { e := &evaluator{ trace: &TraceElement{ loc: a.Loc(), @@ -290,7 +307,7 @@ func (i *interpreter) evaluate(a ast.Node) (value, error) { return result, nil case *ast.Conditional: - cond, err := e.evalInCurrentContext(ast.Cond) + cond, err := e.evalInCurrentContext(ast.Cond, nonTailCall) if err != nil { return nil, err } @@ -299,15 +316,15 @@ func (i *interpreter) evaluate(a ast.Node) (value, error) { return nil, err } if condBool.value { - return e.evalInCurrentContext(ast.BranchTrue) + return e.evalInCurrentContext(ast.BranchTrue, tc) } - return e.evalInCurrentContext(ast.BranchFalse) + return e.evalInCurrentContext(ast.BranchFalse, tc) case *ast.DesugaredObject: // Evaluate all the field names. Check for null, dups, etc. fields := make(simpleObjectFieldMap) for _, field := range ast.Fields { - fieldNameValue, err := e.evalInCurrentContext(field.Name) + fieldNameValue, err := e.evalInCurrentContext(field.Name, nonTailCall) if err != nil { return nil, err } @@ -339,7 +356,7 @@ func (i *interpreter) evaluate(a ast.Node) (value, error) { return makeValueSimpleObject(upValues, fields, asserts), nil case *ast.Error: - msgVal, err := e.evalInCurrentContext(ast.Expr) + msgVal, err := e.evalInCurrentContext(ast.Expr, nonTailCall) if err != nil { // error when evaluating error message return nil, err @@ -351,11 +368,11 @@ func (i *interpreter) evaluate(a ast.Node) (value, error) { return nil, e.Error(msg.getString()) case *ast.Index: - targetValue, err := e.evalInCurrentContext(ast.Target) + targetValue, err := e.evalInCurrentContext(ast.Target, nonTailCall) if err != nil { return nil, err } - index, err := e.evalInCurrentContext(ast.Index) + index, err := e.evalInCurrentContext(ast.Index, nonTailCall) if err != nil { return nil, err } @@ -372,7 +389,8 @@ func (i *interpreter) evaluate(a ast.Node) (value, error) { return nil, err } // TODO(https://github.com/google/jsonnet/issues/377): non-integer indexes should be an error - return e.evaluate(target.elements[int(indexInt.value)]) + return e.evaluateTailCall(target.elements[int(indexInt.value)], tc) + case *valueString: indexInt, err := e.getNumber(index) if err != nil { @@ -415,10 +433,12 @@ func (i *interpreter) evaluate(a ast.Node) (value, error) { bindEnv.upValues[bind.Variable] = th } i.newLocal(vars) + sz := len(i.stack.stack) // Add new stack frame, with new thunk for this variable // execute body WRT stack frame. - v, err := e.evalInCurrentContext(ast.Body) - i.stack.pop() + v, err := e.evalInCurrentContext(ast.Body, tc) + i.stack.popIfExists(sz) + return v, err case *ast.Self: @@ -426,10 +446,10 @@ func (i *interpreter) evaluate(a ast.Node) (value, error) { return sb.self, nil case *ast.Var: - return e.evaluate(e.lookUpVar(ast.Id)) + return e.evaluateTailCall(e.lookUpVar(ast.Id), tc) case *ast.SuperIndex: - index, err := e.evalInCurrentContext(ast.Index) + index, err := e.evalInCurrentContext(ast.Index, nonTailCall) if err != nil { return nil, err } @@ -440,7 +460,7 @@ func (i *interpreter) evaluate(a ast.Node) (value, error) { return objectIndex(e, i.stack.getSelfBinding().super(), indexStr.getString()) case *ast.InSuper: - index, err := e.evalInCurrentContext(ast.Index) + index, err := e.evalInCurrentContext(ast.Index, nonTailCall) if err != nil { return nil, err } @@ -458,7 +478,7 @@ func (i *interpreter) evaluate(a ast.Node) (value, error) { case *ast.Apply: // Eval target - target, err := e.evalInCurrentContext(ast.Target) + target, err := e.evalInCurrentContext(ast.Target, nonTailCall) if err != nil { return nil, err } @@ -469,10 +489,10 @@ func (i *interpreter) evaluate(a ast.Node) (value, error) { // environment in which we can evaluate arguments argEnv := i.getCurrentEnv(a) - arguments := callArguments{ positional: make([]potentialValue, len(ast.Arguments.Positional)), named: make([]namedCallArgument, len(ast.Arguments.Named)), + tailstrict: ast.TailStrict, } for i, arg := range ast.Arguments.Positional { arguments.positional[i] = makeThunk(argEnv, arg) @@ -481,8 +501,7 @@ func (i *interpreter) evaluate(a ast.Node) (value, error) { for i, arg := range ast.Arguments.Named { arguments.named[i] = namedCallArgument{name: arg.Name, pv: makeThunk(argEnv, arg.Arg)} } - - return e.evaluate(function.call(arguments)) + return e.evaluateTailCall(function.call(arguments), tc) default: return nil, e.Error(fmt.Sprintf("Executing this AST type not implemented yet: %v", reflect.TypeOf(a))) @@ -557,7 +576,7 @@ func (i *interpreter) manifestJSON(trace *TraceElement, v value) (interface{}, e case *valueArray: result := make([]interface{}, 0, len(v.elements)) for _, th := range v.elements { - elVal, err := th.getValue(i, trace) // TODO(sbarzowski) perhaps manifestJSON should just take potentialValue + elVal, err := e.evaluate(th) if err != nil { return nil, err } @@ -711,13 +730,17 @@ func (i *interpreter) manifestAndSerializeJSON(trace *TraceElement, v value, mul return buf.String(), nil } -func (i *interpreter) EvalInCleanEnv(fromWhere *TraceElement, env *environment, ast ast.Node) (value, error) { - err := i.newCall(fromWhere, *env) +func (i *interpreter) EvalInCleanEnv(fromWhere *TraceElement, env *environment, ast ast.Node, trimmable bool) (value, error) { + err := i.newCall(fromWhere, *env, trimmable) if err != nil { return nil, err } - val, err := i.evaluate(ast) - i.stack.pop() + stackSize := len(i.stack.stack) + + val, err := i.evaluate(ast, tailCall) + + i.stack.popIfExists(stackSize) + return val, err } @@ -750,7 +773,7 @@ func evaluateStd(i *interpreter) (value, error) { if err != nil { return nil, err } - return i.EvalInCleanEnv(evalTrace, &beforeStdEnv, node) + return i.EvalInCleanEnv(evalTrace, &beforeStdEnv, node, false) } func prepareExtVars(i *interpreter, ext vmExtMap, kind string) map[ast.Identifier]potentialValue { @@ -821,7 +844,7 @@ func evaluate(node ast.Node, ext vmExtMap, tla vmExtMap, maxStack int, importer loc: &evalLoc, } env := makeInitialEnv(node.Loc().FileName, i.baseStd) - result, err := i.EvalInCleanEnv(evalTrace, &env, node) + result, err := i.EvalInCleanEnv(evalTrace, &env, node, false) if err != nil { return "", err } diff --git a/testdata/percent_format_str4.golden b/testdata/percent_format_str4.golden index 7c8403b..008ea4b 100644 --- a/testdata/percent_format_str4.golden +++ b/testdata/percent_format_str4.golden @@ -9,16 +9,6 @@ RUNTIME ERROR: Too many values to format: 2, expected 1 format_codes_arr(codes, arr, i + 1, j, v + code) tailstrict -------------------------------------------------- - :568:21-74 function - - format_codes_arr(codes, arr, i + 1, j3, v + s_padded) tailstrict; - -------------------------------------------------- - :525:21-69 function - - format_codes_arr(codes, arr, i + 1, j, v + code) tailstrict - ------------------------------------------------- :612:13-52 function diff --git a/testdata/percent_format_str5.golden b/testdata/percent_format_str5.golden index 57ceae0..45c1b4a 100644 --- a/testdata/percent_format_str5.golden +++ b/testdata/percent_format_str5.golden @@ -18,11 +18,11 @@ RUNTIME ERROR: Not enough values to format, got 1 builtin function ------------------------------------------------- - ... (skipped 21 frames) + ... (skipped 14 frames) ------------------------------------------------- - :525:21-69 function + :568:21-74 function - format_codes_arr(codes, arr, i + 1, j, v + code) tailstrict + format_codes_arr(codes, arr, i + 1, j3, v + s_padded) tailstrict; ------------------------------------------------- :612:13-52 function diff --git a/testdata/percent_format_str6.golden b/testdata/percent_format_str6.golden index 5b7bbd8..83c5959 100644 --- a/testdata/percent_format_str6.golden +++ b/testdata/percent_format_str6.golden @@ -18,11 +18,11 @@ RUNTIME ERROR: Not enough values to format, got 1 builtin function ------------------------------------------------- - ... (skipped 21 frames) + ... (skipped 14 frames) ------------------------------------------------- - :525:21-69 function + :568:21-74 function - format_codes_arr(codes, arr, i + 1, j, v + code) tailstrict + format_codes_arr(codes, arr, i + 1, j3, v + s_padded) tailstrict; ------------------------------------------------- :616:13-54 function diff --git a/testdata/percent_format_str7.golden b/testdata/percent_format_str7.golden index e3f9cd3..841b851 100644 --- a/testdata/percent_format_str7.golden +++ b/testdata/percent_format_str7.golden @@ -21,11 +21,11 @@ RUNTIME ERROR: Format required number at 0, got string padding(w - std.length(str), s) + str; ------------------------------------------------- - ... (skipped 16 frames) + ... (skipped 11 frames) ------------------------------------------------- - :525:21-69 function + :568:21-74 function - format_codes_arr(codes, arr, i + 1, j, v + code) tailstrict + format_codes_arr(codes, arr, i + 1, j3, v + s_padded) tailstrict; ------------------------------------------------- :612:13-52 function diff --git a/testdata/stackbug-regression-test.golden b/testdata/stackbug-regression-test.golden new file mode 100644 index 0000000..27ba77d --- /dev/null +++ b/testdata/stackbug-regression-test.golden @@ -0,0 +1 @@ +true diff --git a/testdata/stackbug-regression-test.jsonnet b/testdata/stackbug-regression-test.jsonnet new file mode 100644 index 0000000..88518f4 --- /dev/null +++ b/testdata/stackbug-regression-test.jsonnet @@ -0,0 +1 @@ +local xxx=0; xxx==0 diff --git a/testdata/tailstrict.golden b/testdata/tailstrict.golden new file mode 100644 index 0000000..3006dac --- /dev/null +++ b/testdata/tailstrict.golden @@ -0,0 +1 @@ +642 diff --git a/testdata/tailstrict.jsonnet b/testdata/tailstrict.jsonnet new file mode 100644 index 0000000..6f71c75 --- /dev/null +++ b/testdata/tailstrict.jsonnet @@ -0,0 +1,2 @@ +local arr = [function(x) x] + std.makeArray(600, function(i) (function(x) arr[i](x + 1) tailstrict)); +arr[600](42) diff --git a/testdata/tailstrict2.golden b/testdata/tailstrict2.golden new file mode 100644 index 0000000..9445572 --- /dev/null +++ b/testdata/tailstrict2.golden @@ -0,0 +1,15 @@ +RUNTIME ERROR: xxx +------------------------------------------------- + testdata/tailstrict2:1:13-20 function + +local e(x)=(error x); + +------------------------------------------------- + testdata/tailstrict2:2:14-18 function + +(function(x) e(x))("xxx") tailstrict + +------------------------------------------------- + During evaluation + + diff --git a/testdata/tailstrict2.jsonnet b/testdata/tailstrict2.jsonnet new file mode 100644 index 0000000..230e8c7 --- /dev/null +++ b/testdata/tailstrict2.jsonnet @@ -0,0 +1,2 @@ +local e(x)=(error x); +(function(x) e(x))("xxx") tailstrict diff --git a/testdata/tailstrict3.golden b/testdata/tailstrict3.golden new file mode 100644 index 0000000..8d5630d --- /dev/null +++ b/testdata/tailstrict3.golden @@ -0,0 +1,15 @@ +RUNTIME ERROR: xxx +------------------------------------------------- + testdata/tailstrict3:1:16-26 function + +local foo(x, y=error "xxx")=x; + +------------------------------------------------- + testdata/tailstrict3:2:1-8 $ + +foo(42) tailstrict + +------------------------------------------------- + During evaluation + + diff --git a/testdata/tailstrict3.jsonnet b/testdata/tailstrict3.jsonnet new file mode 100644 index 0000000..906e04c --- /dev/null +++ b/testdata/tailstrict3.jsonnet @@ -0,0 +1,2 @@ +local foo(x, y=error "xxx")=x; +foo(42) tailstrict diff --git a/testdata/tailstrict4.golden b/testdata/tailstrict4.golden new file mode 100644 index 0000000..d81cc07 --- /dev/null +++ b/testdata/tailstrict4.golden @@ -0,0 +1 @@ +42 diff --git a/testdata/tailstrict4.jsonnet b/testdata/tailstrict4.jsonnet new file mode 100644 index 0000000..44bea40 --- /dev/null +++ b/testdata/tailstrict4.jsonnet @@ -0,0 +1,2 @@ +local foo(x, y=error "xxx")=x; +foo(42, y=5) tailstrict diff --git a/testdata/tailstrict5.golden b/testdata/tailstrict5.golden new file mode 100644 index 0000000..837e12b --- /dev/null +++ b/testdata/tailstrict5.golden @@ -0,0 +1 @@ +500500 diff --git a/testdata/tailstrict5.jsonnet b/testdata/tailstrict5.jsonnet new file mode 100644 index 0000000..2ce25d3 --- /dev/null +++ b/testdata/tailstrict5.jsonnet @@ -0,0 +1,7 @@ +local sum(x, v) = + if x <= 0 then + v + else + sum(x - 1, x + v) tailstrict; + +sum(1000, 0) diff --git a/thunks.go b/thunks.go index f2b0a56..8ab748a 100644 --- a/thunks.go +++ b/thunks.go @@ -70,7 +70,7 @@ func makeThunk(env environment, body ast.Node) *cachedThunk { } func (t *thunk) getValue(i *interpreter, trace *TraceElement) (value, error) { - return i.EvalInCleanEnv(trace, &t.env, t.body) + return i.EvalInCleanEnv(trace, &t.env, t.body, false) } // callThunk represents a concrete, but not yet evaluated call to a function @@ -205,6 +205,16 @@ type closure struct { params Parameters } +func forceThunks(e *evaluator, args bindingFrame) error { + for _, arg := range args { + _, err := e.evaluate(arg) + if err != nil { + return err + } + } + return nil +} + func (closure *closure) EvalCall(arguments callArguments, e *evaluator) (value, error) { argThunks := make(bindingFrame) parameters := closure.Parameters() @@ -234,11 +244,18 @@ func (closure *closure) EvalCall(arguments callArguments, e *evaluator) (value, } } + if arguments.tailstrict { + err := forceThunks(e, argThunks) + if err != nil { + return nil, err + } + } + calledEnvironment = makeEnvironment( addBindings(closure.env.upValues, argThunks), closure.env.sb, ) - return e.evalInCleanEnv(&calledEnvironment, closure.function.Body) + return e.evalInCleanEnv(&calledEnvironment, closure.function.Body, arguments.tailstrict) } func (closure *closure) Parameters() Parameters { diff --git a/value.go b/value.go index 0aa37b5..69874ce 100644 --- a/value.go +++ b/value.go @@ -328,6 +328,7 @@ type potentialValueInEnv interface { type callArguments struct { positional []potentialValue named []namedCallArgument + tailstrict bool } type namedCallArgument struct {