Separate manifestation from serialization to string

This is necessary for example for native functions
(which take json as arguments).

Standard "encoding/json" representation is used, but I have
mixed feeling about it. Not sure if treating json values as interface{}
is the right trade-off in our case.

builtins.go

@@ -17,7 +17,6 @@ limitations under the License.
 package jsonnet
 
 import (
-	"bytes"
 	"crypto/md5"
 	"encoding/hex"
 	"fmt"
@@ -241,12 +240,11 @@ func builtinToString(e *evaluator, xp potentialValue) (value, error) {
 	case *valueString:
 		return x, nil
 	}
-	var buf bytes.Buffer
+	s, err := e.i.manifestAndSerializeJSON(e.trace, x, false, "")
-	err = e.i.manifestJSON(e.trace, x, false, "", &buf)
 	if err != nil {
 		return nil, err
 	}
-	return makeValueString(buf.String()), nil
+	return makeValueString(s), nil
 }
 
 func builtinMakeArray(e *evaluator, szp potentialValue, funcp potentialValue) (value, error) {

interpreter.go

@@ -523,15 +523,83 @@ func unparseNumber(v float64) string {
 	return fmt.Sprintf("%.17g", v)
 }
 
-// TODO(sbarzowski) Perhaps it should be a builtin?
+// manifestJSON converts to standard JSON representation as in "encoding/json" package
-// TODO(sbarzowski) Perhaps we should separate recursive evaluation from serialization?
+func (i *interpreter) manifestJSON(trace *TraceElement, v value) (interface{}, error) {
-// 					Strictly evaluating something may be useful by itself.
-func (i *interpreter) manifestJSON(trace *TraceElement, v value, multiline bool, indent string, buf *bytes.Buffer) error {
-	// TODO(dcunnin): All the other types...
 	e := &evaluator{i: i, trace: trace}
 	switch v := v.(type) {
+
+	case *valueBoolean:
+		return v.value, nil
+
+	case *valueFunction:
+		return nil, makeRuntimeError("Couldn't manifest function in JSON output.", i.getCurrentStackTrace(trace))
+
+	case *valueNumber:
+		return v.value, nil
+
+	case *valueString:
+		return v.getString(), nil
+
+	case *valueNull:
+		return nil, nil
+
 	case *valueArray:
-		if len(v.elements) == 0 {
+		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
+			if err != nil {
+				return nil, err
+			}
+			elem, err := i.manifestJSON(trace, elVal)
+			if err != nil {
+				return nil, err
+			}
+			result = append(result, elem)
+		}
+		return result, nil
+
+	case valueObject:
+		fieldNames := objectFields(v, withoutHidden)
+		sort.Strings(fieldNames)
+
+		err := checkAssertions(e, v)
+		if err != nil {
+			return nil, err
+		}
+
+		result := make(map[string]interface{})
+
+		for _, fieldName := range fieldNames {
+			fieldVal, err := v.index(e, fieldName)
+			if err != nil {
+				return nil, err
+			}
+
+			field, err := i.manifestJSON(trace, fieldVal)
+			if err != nil {
+				return nil, err
+			}
+			result[fieldName] = field
+		}
+
+		return result, nil
+
+	default:
+		return nil, makeRuntimeError(
+			fmt.Sprintf("Manifesting this value not implemented yet: %s", reflect.TypeOf(v)),
+			i.getCurrentStackTrace(trace),
+		)
+
+	}
+}
+
+func serializeJSON(v interface{}, multiline bool, indent string, buf *bytes.Buffer) {
+	switch v := v.(type) {
+	case nil:
+		buf.WriteString("null")
+
+	case []interface{}:
+		if len(v) == 0 {
 			buf.WriteString("[ ]")
 		} else {
 			var prefix string
@@ -543,20 +611,10 @@ func (i *interpreter) manifestJSON(trace *TraceElement, v value, multiline bool,
 				prefix = "["
 				indent2 = indent
 			}
-			for _, th := range v.elements {
+			for _, elem := range v {
-				// if th.body != nil {
-				// 	tloc = th.body.Loc()
-				// }
-				elVal, err := th.getValue(i, trace) // TODO(sbarzowski) perhaps manifestJSON should just take potentialValue
-				if err != nil {
-					return err
-				}
 				buf.WriteString(prefix)
 				buf.WriteString(indent2)
-				err = i.manifestJSON(trace, elVal, multiline, indent2, buf)
+				serializeJSON(elem, multiline, indent2, buf)
-				if err != nil {
-					return err
-				}
 				if multiline {
 					prefix = ",\n"
 				} else {
@@ -570,30 +628,22 @@ func (i *interpreter) manifestJSON(trace *TraceElement, v value, multiline bool,
 			buf.WriteString("]")
 		}
 
-	case *valueBoolean:
+	case bool:
-		if v.value {
+		if v {
 			buf.WriteString("true")
 		} else {
 			buf.WriteString("false")
 		}
 
-	case *valueFunction:
+	case float64:
-		return makeRuntimeError("Couldn't manifest function in JSON output.", i.getCurrentStackTrace(trace))
+		buf.WriteString(unparseNumber(v))
 
-	case *valueNumber:
+	case map[string]interface{}:
-		buf.WriteString(unparseNumber(v.value))
+		fieldNames := make([]string, 0, len(v))
-
+		for name := range v {
-	case *valueNull:
+			fieldNames = append(fieldNames, name)
-		buf.WriteString("null")
-
-	case valueObject:
-		fieldNames := objectFields(v, withoutHidden)
-		sort.Strings(fieldNames)
-
-		err := checkAssertions(e, v)
-		if err != nil {
-			return err
 		}
+		sort.Strings(fieldNames)
 
 		if len(fieldNames) == 0 {
 			buf.WriteString("{ }")
@@ -608,10 +658,7 @@ func (i *interpreter) manifestJSON(trace *TraceElement, v value, multiline bool,
 				indent2 = indent
 			}
 			for _, fieldName := range fieldNames {
-				fieldVal, err := v.index(e, fieldName)
+				fieldVal := v[fieldName]
-				if err != nil {
-					return err
-				}
 
 				buf.WriteString(prefix)
 				buf.WriteString(indent2)
@@ -619,11 +666,7 @@ func (i *interpreter) manifestJSON(trace *TraceElement, v value, multiline bool,
 				buf.WriteString(unparseString(fieldName))
 				buf.WriteString(": ")
 
-				// TODO(sbarzowski) body.Loc()
+				serializeJSON(fieldVal, multiline, indent2, buf)
-				err = i.manifestJSON(trace, fieldVal, multiline, indent2, buf)
-				if err != nil {
-					return err
-				}
 
 				if multiline {
 					prefix = ",\n"
@@ -639,17 +682,26 @@ func (i *interpreter) manifestJSON(trace *TraceElement, v value, multiline bool,
 			buf.WriteString("}")
 		}
 
-	case *valueString:
+	case string:
-		buf.WriteString(unparseString(v.getString()))
+		buf.WriteString(unparseString(v))
 
 	default:
-		return makeRuntimeError(
+		panic(fmt.Sprintf("Unsupported value for serialization %#+v", v))
-			fmt.Sprintf("Manifesting this value not implemented yet: %s", reflect.TypeOf(v)),
-			i.getCurrentStackTrace(trace),
-		)
-
 	}
-	return nil
+}
+
+// TODO(sbarzowski) Perhaps it should be a builtin?
+// TODO(sbarzowski) Perhaps we should separate recursive evaluation from serialization?
+// 					Strictly evaluating something may be useful by itself.
+//					For example may help with error reporting from custom serialization functions.
+func (i *interpreter) manifestAndSerializeJSON(trace *TraceElement, v value, multiline bool, indent string) (string, error) {
+	var buf bytes.Buffer
+	manifested, err := i.manifestJSON(trace, v)
+	if err != nil {
+		return "", err
+	}
+	serializeJSON(manifested, multiline, indent, &buf)
+	return buf.String(), nil
 }
 
 func (i *interpreter) EvalInCleanEnv(fromWhere *TraceElement, newContext *TraceContext,
@@ -739,15 +791,6 @@ func buildInterpreter(ext vmExtMap, maxStack int, importer Importer) (*interpret
 	return &i, nil
 }
 
-func manifest(e *evaluator, v value) (string, error) {
-	var buffer bytes.Buffer
-	err := e.i.manifestJSON(e.trace, v, true, "", &buffer)
-	if err != nil {
-		return "", err
-	}
-	return buffer.String(), nil
-}
-
 func evaluate(node ast.Node, ext vmExtMap, maxStack int, importer Importer) (string, error) {
 	i, err := buildInterpreter(ext, maxStack, importer)
 	if err != nil {
@@ -766,9 +809,9 @@ func evaluate(node ast.Node, ext vmExtMap, maxStack int, importer Importer) (str
 	manifestationTrace := &TraceElement{
 		loc: &manifestationLoc,
 	}
-	e := &evaluator{
+	s, err := i.manifestAndSerializeJSON(manifestationTrace, result, true, "")
-		i:     i,
+	if err != nil {
-		trace: manifestationTrace,
+		return "", err
 	}
-	return manifest(e, result)
+	return s, nil
 }