From d1b85505cde735cf40b37c994423380d6c0b02ac Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Fri, 7 Jul 2017 08:23:37 -0400 Subject: Factor out expression parsing - Also make error reporting consistent with GL JS --- cmake/core-files.cmake | 6 +- include/mbgl/style/conversion/expression.hpp | 43 ++----- include/mbgl/style/conversion/function.hpp | 1 - include/mbgl/style/expression/expression.hpp | 132 ++++++++++++++++++++++ include/mbgl/style/expression/parse.hpp | 65 +++++++++++ include/mbgl/style/expression/parsing_context.hpp | 40 +++++++ include/mbgl/style/function/expression.hpp | 101 ----------------- platform/node/src/node_expression.cpp | 63 +++++++---- platform/node/src/node_expression.hpp | 5 +- platform/node/test/expression.test.js | 12 +- src/mbgl/style/function/expression.cpp | 4 +- 11 files changed, 305 insertions(+), 167 deletions(-) create mode 100644 include/mbgl/style/expression/expression.hpp create mode 100644 include/mbgl/style/expression/parse.hpp create mode 100644 include/mbgl/style/expression/parsing_context.hpp delete mode 100644 include/mbgl/style/function/expression.hpp 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 -#include -#include +#include #include namespace mbgl { @@ -12,43 +11,15 @@ namespace conversion { using namespace mbgl::style::expression; template<> struct Converter> { - template - std::string getJSType(const V& value) const { - if (isUndefined(value)) { - return "undefined"; - } - if (isArray(value) || isObject(value)) { - return "object"; - } - optional v = toValue(value); - assert(v); - return v->match( - [&] (std::string) { return "string"; }, - [&] (bool) { return "boolean"; }, - [&] (auto) { return "number"; } - ); - } - template optional> 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& 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>()) { + return std::move(parsed.template get>()); } - return {std::make_unique("", 1.0f)}; - } + error = { parsed.template get().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 #include #include -#include #include #include #include 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 +#include +#include +#include +#include +#include +#include +#include +#include + +namespace mbgl { + +class GeometryTileFeature; + + +namespace style { +namespace expression { + +using OutputValue = variant< + float, + std::string, + mbgl::Color, + std::array, + std::array>; + +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 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 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>; +template +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 evaluate(float, const GeometryTileFeature&, EvaluationError&) const override { + return value; + } + + template + static ParseResult parse(const V& value, const ParsingContext& ctx) { + if (isUndefined(value)) + return std::make_unique(ctx.key()); + + if (isObject(value)) { + return CompileError {ctx.key(), "Unimplemented: object literals"}; + } + + if (isArray(value)) { + return CompileError {ctx.key(), "Unimplemented: array literals"}; + } + + optional v = toValue(value); + assert(v); + return v->match( + [&] (std::string s) { return std::make_unique(ctx.key(), s); }, + [&] (bool b) { return std::make_unique(ctx.key(), b); }, + [&] (auto f) { + auto number = numericValue(f); + assert(number); + return std::make_unique(ctx.key(), *number); + } + ); + } + +private: + optional value; +}; + +class LambdaExpression : public Expression { +public: + LambdaExpression(std::string key, + type::Lambda type, + std::string name_, + std::vector> args_) : + Expression(key, type), + name(name_), + args(std::move(args_)) + {} + +private: + std::string name; + std::vector> 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 +#include +#include +#include + +namespace mbgl { +namespace style { +namespace expression { + +using namespace mbgl::style; + +template +std::string getJSType(const V& value) { + using namespace mbgl::style::conversion; + if (isUndefined(value)) { + return "undefined"; + } + if (isArray(value) || isObject(value)) { + return "object"; + } + optional v = toValue(value); + assert(v); + return v->match( + [&] (std::string) { return "string"; }, + [&] (bool) { return "boolean"; }, + [&] (auto) { return "number"; } + ); +} + +using ParseResult = variant>; + +template +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& 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 +#include +#include + +namespace mbgl { +namespace style { +namespace expression { + +class ParsingContext { +public: + ParsingContext() {} + ParsingContext(ParsingContext previous, + optional index, + optional 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 path; + std::vector 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 -#include -#include -#include -#include -#include -#include - - -namespace mbgl { - -class GeometryTileFeature; - - -namespace style { -namespace expression { - -using OutputValue = variant< - float, - std::string, - mbgl::Color, - std::array, - std::array>; - -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 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 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 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> args_) : - Expression(key, type), - name(name_), - args(std::move(args_)) - {} - -private: - std::string name; - std::vector> 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 #include +#include #include #include @@ -23,15 +23,15 @@ void NodeExpression::Init(v8::Local 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& info) { - if (!info.IsConstructCall()) { - return Nan::ThrowTypeError("Use the new operator to create new Expression objects"); - } - +void NodeExpression::Parse(const Nan::FunctionCallbackInfo& info) { + v8::Local 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& info) { auto expr = info[0]; try { - conversion::Error error; - auto expression = conversion::convert>(expr, error); - if (!expression) { - return Nan::ThrowTypeError(error.message.c_str()); + auto parsed = parseExpression(expr, ParsingContext()); + if (parsed.template is>()) { + auto nodeExpr = new NodeExpression(std::move(parsed.template get>())); + const int argc = 0; + v8::Local argv[0] = {}; + auto wrapped = Nan::NewInstance(cons, argc, argv).ToLocalChecked(); + nodeExpr->Wrap(wrapped); + info.GetReturnValue().Set(wrapped); + } else { + const auto& error = parsed.template get(); + v8::Local err = Nan::New(); + 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 result = Nan::New(); + 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& 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& info) { NodeExpression* nodeExpr = ObjectWrap::Unwrap(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& info) Nan::ThrowTypeError(conversionError.message.c_str()); return; } - + try { mapbox::geojson::feature feature = geoJSON->get(); - 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&) {}, @@ -108,19 +129,23 @@ void NodeExpression::Evaluate(const Nan::FunctionCallbackInfo& info) void NodeExpression::GetType(const Nan::FunctionCallbackInfo& info) { NodeExpression* nodeExpr = ObjectWrap::Unwrap(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& info) { NodeExpression* nodeExpr = ObjectWrap::Unwrap(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& info) { NodeExpression* nodeExpr = ObjectWrap::Unwrap(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 #include -#include +#include +#include #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-parameter" @@ -10,6 +11,7 @@ #include #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&); + static void Parse(const Nan::FunctionCallbackInfo&); static void Evaluate(const Nan::FunctionCallbackInfo&); static void GetType(const Nan::FunctionCallbackInfo&); static void IsFeatureConstant(const Nan::FunctionCallbackInfo&); 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 +#include #include namespace mbgl { @@ -38,7 +38,7 @@ public: } }; -optional Expression::evaluate(float z, const Feature& feature, Error& error) const { +optional Expression::evaluate(float z, const Feature& feature, EvaluationError& error) const { std::unique_ptr f = std::make_unique(feature); return this->evaluate(z, *f, error); } -- cgit v1.2.1