Factor out expression parsing

 - Also make error reporting consistent with GL JS

11 files changed, 305 insertions, 167 deletions

diff --git a/cmake/core-files.cmake b/cmake/core-files.cmake
index a93bedbf7a..d3eb02ac13 100644
--- a/cmake/core-files.cmake
+++ b/cmake/core-files.cmake
@@ -388,6 +388,11 @@ set(MBGL_CORE_FILES
     src/mbgl/style/conversion/json.hpp
     src/mbgl/style/conversion/stringify.hpp
 
+    # style/expression
+    include/mbgl/style/expression/expression.hpp
+    include/mbgl/style/expression/parse.hpp
+    include/mbgl/style/expression/parsing_context.hpp
+
     # style/function
     include/mbgl/style/function/camera_function.hpp
     include/mbgl/style/function/categorical_stops.hpp
@@ -396,7 +401,6 @@ set(MBGL_CORE_FILES
     include/mbgl/style/function/composite_function.hpp
     include/mbgl/style/function/composite_interval_stops.hpp
     include/mbgl/style/function/exponential_stops.hpp
-    include/mbgl/style/function/expression.hpp
     include/mbgl/style/function/identity_stops.hpp
     include/mbgl/style/function/interval_stops.hpp
     include/mbgl/style/function/source_function.hpp
diff --git a/include/mbgl/style/conversion/expression.hpp b/include/mbgl/style/conversion/expression.hpp
index d70e3e0b33..bce7c91396 100644
--- a/include/mbgl/style/conversion/expression.hpp
+++ b/include/mbgl/style/conversion/expression.hpp
@@ -1,8 +1,7 @@
 #pragma once
 
 #include <memory>
-#include <sstream>
-#include <mbgl/style/function/expression.hpp>
+#include <mbgl/style/expression/parse.hpp>
 #include <mbgl/style/conversion.hpp>
 
 namespace mbgl {
@@ -13,42 +12,14 @@ using namespace mbgl::style::expression;
 
 template<> struct Converter<std::unique_ptr<Expression>> {
     template <class V>
-    std::string getJSType(const V& value) const {
-        if (isUndefined(value)) {
-            return "undefined";
-        }
-        if (isArray(value) || isObject(value)) {
-            return "object";
-        }
-        optional<mbgl::Value> v = toValue(value);
-        assert(v);
-        return v->match(
-            [&] (std::string) { return "string"; },
-            [&] (bool) { return "boolean"; },
-            [&] (auto) { return "number"; }
-        );
-    }
-
-    template <class V>
     optional<std::unique_ptr<Expression>> operator()(const V& value, Error& error) const {
-        if (isArray(value)) {
-            if (arrayLength(value) == 0) {
-                error = { "Expected an array with at least one element. If you wanted a literal array, use [\"literal\", []]." };
-                return {};
-            }
-            
-            const optional<std::string>& op = toString(arrayMember(value, 0));
-            if (!op) {
-                std::ostringstream ss;
-                ss << "Expression name must be a string, but found "
-                    << getJSType(arrayMember(value, 0))
-                    << " instead. If you wanted a literal array, use [\"literal\", [...]].";
-                error = { ss.str() };
-                return {};
-            }
+        auto parsed = parseExpression(value, ParsingContext());
+        if (parsed.template is<std::unique_ptr<Expression>>()) {
+            return std::move(parsed.template get<std::unique_ptr<Expression>>());
         }
-        return {std::make_unique<LiteralExpression>("", 1.0f)};
-    }
+        error = { parsed.template get<CompileError>().message };
+        return {};
+    };
 };
 
 } // namespace conversion
diff --git a/include/mbgl/style/conversion/function.hpp b/include/mbgl/style/conversion/function.hpp
index d1d0bb709f..bf5b27a9a6 100644
--- a/include/mbgl/style/conversion/function.hpp
+++ b/include/mbgl/style/conversion/function.hpp
@@ -3,7 +3,6 @@
 #include <mbgl/style/function/camera_function.hpp>
 #include <mbgl/style/function/source_function.hpp>
 #include <mbgl/style/function/composite_function.hpp>
-#include <mbgl/style/function/expression.hpp>
 #include <mbgl/style/conversion.hpp>
 #include <mbgl/style/conversion/constant.hpp>
 #include <mbgl/util/ignore.hpp>
diff --git a/include/mbgl/style/expression/expression.hpp b/include/mbgl/style/expression/expression.hpp
new file mode 100644
index 0000000000..d7053fc1b8
--- /dev/null
+++ b/include/mbgl/style/expression/expression.hpp
@@ -0,0 +1,132 @@
+#pragma once
+
+#include <array>
+#include <vector>
+#include <memory>
+#include <mbgl/util/optional.hpp>
+#include <mbgl/util/color.hpp>
+#include <mbgl/style/function/type.hpp>
+#include <mbgl/util/feature.hpp>
+#include <mbgl/style/expression/parsing_context.hpp>
+#include <mbgl/style/conversion.hpp>
+
+namespace mbgl {
+
+class GeometryTileFeature;
+
+
+namespace style {
+namespace expression {
+
+using OutputValue = variant<
+    float,
+    std::string,
+    mbgl::Color,
+    std::array<float, 2>,
+    std::array<float, 4>>;
+
+struct EvaluationError {
+    std::string message;
+};
+
+struct CompileError {
+    std::string message;
+    std::string key;
+};
+
+class Expression {
+public:
+    Expression(std::string key_, type::Type type_) : key(key_), type(type_) {}
+    virtual ~Expression() {}
+    
+    virtual optional<OutputValue> evaluate(float, const GeometryTileFeature&, EvaluationError& e) const = 0;
+    
+    // Exposed for use with pure Feature objects (e.g. beyond the context of tiled map data).
+    optional<OutputValue> evaluate(float, const Feature&, EvaluationError&) const;
+    
+    type::Type getType() {
+        return type;
+    }
+    
+    bool isFeatureConstant() {
+        return true;
+    }
+    
+    bool isZoomConstant() {
+        return true;
+    }
+    
+private:
+    std::string key;
+    type::Type type;
+};
+
+using ParseResult = variant<CompileError, std::unique_ptr<Expression>>;
+template <class V>
+ParseResult parseExpression(const V& value, const ParsingContext& context);
+
+using namespace mbgl::style::conversion;
+
+class LiteralExpression : public Expression {
+public:
+    LiteralExpression(std::string key, float value_) : Expression(key, type::Primitive::Number), value(value_) {}
+    LiteralExpression(std::string key, const std::string& value_) : Expression(key, type::Primitive::String), value(value_) {}
+    LiteralExpression(std::string key, const mbgl::Color& value_) : Expression(key, type::Primitive::Color), value(value_) {}
+    LiteralExpression(std::string key) : Expression(key, type::Primitive::Null) {}
+    
+    optional<OutputValue> evaluate(float, const GeometryTileFeature&, EvaluationError&) const override {
+        return value;
+    }
+    
+    template <class V>
+    static ParseResult parse(const V& value, const ParsingContext& ctx) {
+        if (isUndefined(value))
+            return std::make_unique<LiteralExpression>(ctx.key());
+        
+        if (isObject(value)) {
+            return CompileError {ctx.key(), "Unimplemented: object literals"};
+        }
+        
+        if (isArray(value)) {
+            return CompileError {ctx.key(), "Unimplemented: array literals"};
+        }
+        
+        optional<mbgl::Value> v = toValue(value);
+        assert(v);
+        return v->match(
+            [&] (std::string s) { return std::make_unique<LiteralExpression>(ctx.key(), s); },
+            [&] (bool b) { return std::make_unique<LiteralExpression>(ctx.key(), b); },
+            [&] (auto f) {
+                auto number = numericValue<float>(f);
+                assert(number);
+                return std::make_unique<LiteralExpression>(ctx.key(), *number);
+            }
+        );
+    }
+    
+private:
+    optional<OutputValue> value;
+};
+
+class LambdaExpression : public Expression {
+public:
+    LambdaExpression(std::string key,
+                    type::Lambda type,
+                    std::string name_,
+                    std::vector<std::unique_ptr<Expression>> args_) :
+        Expression(key, type),
+        name(name_),
+        args(std::move(args_))
+    {}
+    
+private:
+    std::string name;
+    std::vector<std::unique_ptr<Expression>> args;
+};
+
+
+
+
+} // namespace expression
+} // namespace style
+} // namespace mbgl
diff --git a/include/mbgl/style/expression/parse.hpp b/include/mbgl/style/expression/parse.hpp
new file mode 100644
index 0000000000..3d7fad70b3
--- /dev/null
+++ b/include/mbgl/style/expression/parse.hpp
@@ -0,0 +1,65 @@
+#pragma once
+
+#include <memory>
+#include <mbgl/style/expression/parsing_context.hpp>
+#include <mbgl/style/expression/expression.hpp>
+#include <mbgl/style/conversion.hpp>
+
+namespace mbgl {
+namespace style {
+namespace expression {
+
+using namespace mbgl::style;
+
+template <class V>
+std::string getJSType(const V& value) {
+    using namespace mbgl::style::conversion;
+    if (isUndefined(value)) {
+        return "undefined";
+    }
+    if (isArray(value) || isObject(value)) {
+        return "object";
+    }
+    optional<mbgl::Value> v = toValue(value);
+    assert(v);
+    return v->match(
+        [&] (std::string) { return "string"; },
+        [&] (bool) { return "boolean"; },
+        [&] (auto) { return "number"; }
+    );
+}
+
+using ParseResult = variant<CompileError, std::unique_ptr<Expression>>;
+
+template <class V>
+ParseResult parseExpression(const V& value, const ParsingContext& context)
+{
+    using namespace mbgl::style::conversion;
+    
+    if (isArray(value)) {
+        if (arrayLength(value) == 0) {
+            CompileError error = {
+                "Expected an array with at least one element. If you wanted a literal array, use [\"literal\", []].",
+                context.key()
+            };
+            return error;
+        }
+        
+        const optional<std::string>& op = toString(arrayMember(value, 0));
+        if (!op) {
+            CompileError error = {
+                "Expression name must be a string, but found " + getJSType(arrayMember(value, 0)) +
+                    " instead. If you wanted a literal array, use [\"literal\", [...]].",
+                context.key(0)
+            };
+            return error;
+        }
+    }
+
+    return LiteralExpression::parse(value, context);
+}
+
+
+} // namespace expression
+} // namespace style
+} // namespace mbgl
diff --git a/include/mbgl/style/expression/parsing_context.hpp b/include/mbgl/style/expression/parsing_context.hpp
new file mode 100644
index 0000000000..7898da790d
--- /dev/null
+++ b/include/mbgl/style/expression/parsing_context.hpp
@@ -0,0 +1,40 @@
+#pragma once
+
+#include <string>
+#include <vector>
+#include <mbgl/util/optional.hpp>
+
+namespace mbgl {
+namespace style {
+namespace expression {
+
+class ParsingContext {
+public:
+    ParsingContext() {}
+    ParsingContext(ParsingContext previous,
+                   optional<size_t> index,
+                   optional<std::string> name) :
+                   path(previous.path),
+                   ancestors(previous.ancestors)
+    {
+        if (index) path.emplace_back(*index);
+        if (name) ancestors.emplace_back(*name);
+    }
+    
+    std::string key() const {
+        std::string result;
+        for(auto const& index : path) { result += "[" + std::to_string(index) + "]"; }
+        return result;
+    }
+    
+    std::string key(size_t lastIndex) const {
+        return key() + "[" + std::to_string(lastIndex) + "]";
+    }
+    
+    std::vector<size_t> path;
+    std::vector<std::string> ancestors;
+};
+
+} // namespace expression
+} // namespace style
+} // namespace mbgl
diff --git a/include/mbgl/style/function/expression.hpp b/include/mbgl/style/function/expression.hpp
deleted file mode 100644
index 6a25f006df..0000000000
--- a/include/mbgl/style/function/expression.hpp
+++ /dev/null
@@ -1,101 +0,0 @@
-#pragma once
-
-#include <array>
-#include <vector>
-#include <memory>
-#include <mbgl/util/optional.hpp>
-#include <mbgl/util/color.hpp>
-#include <mbgl/style/function/type.hpp>
-#include <mbgl/util/feature.hpp>
-
-
-namespace mbgl {
-
-class GeometryTileFeature;
-
-
-namespace style {
-namespace expression {
-
-using OutputValue = variant<
-    float,
-    std::string,
-    mbgl::Color,
-    std::array<float, 2>,
-    std::array<float, 4>>;
-
-class Error {
-public:
-    Error() {}
-    Error(std::string message_) : message(message_) {}
-    std::string message;
-};
-
-class Expression {
-public:
-    Expression(std::string key_, type::Type type_) : key(key_), type(type_) {}
-    virtual ~Expression() {}
-    
-    virtual optional<OutputValue> evaluate(float, const GeometryTileFeature&, Error& e) const = 0;
-    
-    // Exposed for use with pure Feature objects (e.g. beyond the context of tiled map data).
-    optional<OutputValue> evaluate(float, const Feature&, Error&) const;
-    
-    type::Type getType() {
-        return type;
-    }
-    
-    bool isFeatureConstant() {
-        return true;
-    }
-    
-    bool isZoomConstant() {
-        return true;
-    }
-    
-private:
-    std::string key;
-    type::Type type;
-};
-
-class LiteralExpression : public Expression {
-public:
-    LiteralExpression(std::string key, OutputValue value_) :
-        Expression(key, value_.match(
-            [&] (const float&) -> type::Type { return type::Primitive::Number; },
-            [&] (const std::string&) { return type::Primitive::String; },
-            [&] (const mbgl::Color) { return type::Primitive::Color; },
-            [&] (const auto&) { return type::Primitive::Null; } // TODO (remaining output types)
-        )),
-        value(value_)
-    {}
-    
-    optional<OutputValue> evaluate(float, const GeometryTileFeature&, Error&) const override {
-        return {value};
-    }
-    
-private:
-    OutputValue value;
-};
-
-class LambdaExpression : public Expression {
-public:
-    LambdaExpression(std::string key,
-                    type::Lambda type,
-                    std::string name_,
-                    std::vector<std::unique_ptr<Expression>> args_) :
-        Expression(key, type),
-        name(name_),
-        args(std::move(args_))
-    {}
-    
-private:
-    std::string name;
-    std::vector<std::unique_ptr<Expression>> args;
-};
-
-
-
-} // namespace expression
-} // namespace style
-} // namespace mbgl
diff --git a/platform/node/src/node_expression.cpp b/platform/node/src/node_expression.cpp
index 51a9fca5ef..dc9a50abd4 100644
--- a/platform/node/src/node_expression.cpp
+++ b/platform/node/src/node_expression.cpp
@@ -1,8 +1,8 @@
-#include "node_expression.hpp"
 #include "node_conversion.hpp"
+#include "node_expression.hpp"
 
-#include <mbgl/style/conversion/geojson.hpp>
 #include <mbgl/style/conversion/expression.hpp>
+#include <mbgl/style/conversion/geojson.hpp>
 #include <mbgl/util/geojson.hpp>
 #include <nan.h>
 
@@ -23,15 +23,15 @@ void NodeExpression::Init(v8::Local<v8::Object> target) {
     Nan::SetPrototypeMethod(tpl, "isFeatureConstant", IsFeatureConstant);
     Nan::SetPrototypeMethod(tpl, "isZoomConstant", IsZoomConstant);
 
+    Nan::SetMethod(tpl, "parse", Parse);
+
     constructor.Reset(tpl->GetFunction()); // what is this doing?
     Nan::Set(target, Nan::New("Expression").ToLocalChecked(), tpl->GetFunction());
 }
 
-void NodeExpression::New(const Nan::FunctionCallbackInfo<v8::Value>& info) {
-    if (!info.IsConstructCall()) {
-        return Nan::ThrowTypeError("Use the new operator to create new Expression objects");
-    }
-
+void NodeExpression::Parse(const Nan::FunctionCallbackInfo<v8::Value>& info) {
+    v8::Local<v8::Function> cons = Nan::New(constructor);
+    
     if (info.Length() < 1 || info[0]->IsUndefined()) {
         return Nan::ThrowTypeError("Requires a JSON style expression argument.");
     }
@@ -39,22 +39,43 @@ void NodeExpression::New(const Nan::FunctionCallbackInfo<v8::Value>& info) {
     auto expr = info[0];
 
     try {
-        conversion::Error error;
-        auto expression = conversion::convert<std::unique_ptr<Expression>>(expr, error);
-        if (!expression) {
-            return Nan::ThrowTypeError(error.message.c_str());
+        auto parsed = parseExpression(expr, ParsingContext());
+        if (parsed.template is<std::unique_ptr<Expression>>()) {
+            auto nodeExpr = new NodeExpression(std::move(parsed.template get<std::unique_ptr<Expression>>()));
+            const int argc = 0;
+            v8::Local<v8::Value> argv[0] = {};
+            auto wrapped = Nan::NewInstance(cons, argc, argv).ToLocalChecked();
+            nodeExpr->Wrap(wrapped);
+            info.GetReturnValue().Set(wrapped);
+        } else {
+            const auto& error = parsed.template get<CompileError>();
+            v8::Local<v8::Object> err = Nan::New<v8::Object>();
+            Nan::Set(err,
+                    Nan::New("key").ToLocalChecked(),
+                    Nan::New(error.key.c_str()).ToLocalChecked());
+            Nan::Set(err,
+                    Nan::New("error").ToLocalChecked(),
+                    Nan::New(error.message.c_str()).ToLocalChecked());
+            v8::Local<v8::Array> result = Nan::New<v8::Array>();
+            Nan::Set(result, Nan::New(0), err);
+            info.GetReturnValue().Set(result);
         }
-        auto nodeExpr = new NodeExpression(std::move(*expression));
-        nodeExpr->Wrap(info.This());
     } catch(std::exception &ex) {
         return Nan::ThrowError(ex.what());
     }
+}
+
+void NodeExpression::New(const Nan::FunctionCallbackInfo<v8::Value>& info) {
+    if (!info.IsConstructCall()) {
+        return Nan::ThrowTypeError("Use the new operator to create new Expression objects");
+    }
 
     info.GetReturnValue().Set(info.This());
 }
 
 void NodeExpression::Evaluate(const Nan::FunctionCallbackInfo<v8::Value>& info) {
     NodeExpression* nodeExpr = ObjectWrap::Unwrap<NodeExpression>(info.Holder());
+    const auto& expression = nodeExpr->expression;
 
     if (info.Length() < 2) {
         return Nan::ThrowTypeError("Requires arguments zoom and feature arguments.");
@@ -75,11 +96,11 @@ void NodeExpression::Evaluate(const Nan::FunctionCallbackInfo<v8::Value>& info)
         Nan::ThrowTypeError(conversionError.message.c_str());
         return;
     }
-    
+
     try {
         mapbox::geojson::feature feature = geoJSON->get<mapbox::geojson::feature>();
-        Error error;
-        auto result = nodeExpr->expression->evaluate(zoom, feature, error);
+        EvaluationError error;
+        auto result = expression->evaluate(zoom, feature, error);
         if (result) {
             result->match(
                 [&] (const std::array<float, 2>&) {},
@@ -108,19 +129,23 @@ void NodeExpression::Evaluate(const Nan::FunctionCallbackInfo<v8::Value>& info)
 
 void NodeExpression::GetType(const Nan::FunctionCallbackInfo<v8::Value>& info) {
     NodeExpression* nodeExpr = ObjectWrap::Unwrap<NodeExpression>(info.Holder());
-    const auto& type = nodeExpr->expression->getType();
+    const auto& expression = nodeExpr->expression;
+
+    const auto& type = expression->getType();
     const auto& name = type.match([&] (const auto& t) { return t.getName(); });
     info.GetReturnValue().Set(Nan::New(name.c_str()).ToLocalChecked());
 }
 
 void NodeExpression::IsFeatureConstant(const Nan::FunctionCallbackInfo<v8::Value>& info) {
     NodeExpression* nodeExpr = ObjectWrap::Unwrap<NodeExpression>(info.Holder());
-    info.GetReturnValue().Set(Nan::New(nodeExpr->expression->isFeatureConstant()));
+    const auto& expression = nodeExpr->expression;
+    info.GetReturnValue().Set(Nan::New(expression->isFeatureConstant()));
 }
 
 void NodeExpression::IsZoomConstant(const Nan::FunctionCallbackInfo<v8::Value>& info) {
     NodeExpression* nodeExpr = ObjectWrap::Unwrap<NodeExpression>(info.Holder());
-    info.GetReturnValue().Set(Nan::New(nodeExpr->expression->isZoomConstant()));
+    const auto& expression = nodeExpr->expression;
+    info.GetReturnValue().Set(Nan::New(expression->isZoomConstant()));
 }
 
 } // namespace node_mbgl
diff --git a/platform/node/src/node_expression.hpp b/platform/node/src/node_expression.hpp
index 8913bead1b..3797632236 100644
--- a/platform/node/src/node_expression.hpp
+++ b/platform/node/src/node_expression.hpp
@@ -2,7 +2,8 @@
 
 #include <exception>
 #include <memory>
-#include <mbgl/style/function/expression.hpp>
+#include <mbgl/style/conversion.hpp>
+#include <mbgl/style/expression/expression.hpp>
 
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wunused-parameter"
@@ -10,6 +11,7 @@
 #include <nan.h>
 #pragma GCC diagnostic pop
 
+using namespace mbgl::style;
 using namespace mbgl::style::expression;
 
 namespace node_mbgl {
@@ -24,6 +26,7 @@ private:
     {};
 
     static void New(const Nan::FunctionCallbackInfo<v8::Value>&);
+    static void Parse(const Nan::FunctionCallbackInfo<v8::Value>&);
     static void Evaluate(const Nan::FunctionCallbackInfo<v8::Value>&);
     static void GetType(const Nan::FunctionCallbackInfo<v8::Value>&);
     static void IsFeatureConstant(const Nan::FunctionCallbackInfo<v8::Value>&);
diff --git a/platform/node/test/expression.test.js b/platform/node/test/expression.test.js
index c243ba1e0e..842faa7350 100644
--- a/platform/node/test/expression.test.js
+++ b/platform/node/test/expression.test.js
@@ -15,13 +15,13 @@ suite.run('native', {tests: tests}, (fixture) => {
         compileResult
     };
 
-    try {
-        const expression = new mbgl.Expression(fixture.expression);
+    const expression = mbgl.Expression.parse(fixture.expression);
+
+    if (expression instanceof mbgl.Expression) {
         compileResult.result = 'success';
         compileResult.isFeatureConstant = expression.isFeatureConstant();
         compileResult.isZoomConstant = expression.isZoomConstant();
         compileResult.type = expression.getType();
-
         if (fixture.evaluate) {
             const evaluateResults = [];
             for (const input of fixture.evaluate) {
@@ -31,7 +31,7 @@ suite.run('native', {tests: tests}, (fixture) => {
                 const feature = Object.assign({
                     type: 'Feature',
                     properties: {},
-                    geometry: { type: 'Unknown', coordinates: [] }
+                    geometry: { type: 'Point', coordinates: [0, 0] }
                 }, input[1])
 
                 const output = expression.evaluate(zoom, feature);
@@ -42,9 +42,9 @@ suite.run('native', {tests: tests}, (fixture) => {
                 testResult.evaluateResults = evaluateResults;
             }
         }
-    } catch (e) {
+    } else {
         compileResult.result = 'error';
-        compileResult.errors = [e.message];
+        compileResult.errors = expression;
     }
 
     return testResult;
diff --git a/src/mbgl/style/function/expression.cpp b/src/mbgl/style/function/expression.cpp
index c620b0bb22..b609a6e576 100644
--- a/src/mbgl/style/function/expression.cpp
+++ b/src/mbgl/style/function/expression.cpp
@@ -1,4 +1,4 @@
-#include <mbgl/style/function/expression.hpp>
+#include <mbgl/style/expression/expression.hpp>
 #include <mbgl/tile/geometry_tile_data.hpp>
 
 namespace mbgl {
@@ -38,7 +38,7 @@ public:
     }
 };
 
-optional<OutputValue> Expression::evaluate(float z, const Feature& feature, Error& error) const {
+optional<OutputValue> Expression::evaluate(float z, const Feature& feature, EvaluationError& error) const {
     std::unique_ptr<const GeometryTileFeature> f = std::make_unique<const GeoJSONFeature>(feature);
     return this->evaluate(z, *f, error);
 }