From 2e93a9ddcf1f97188250335d991fbd7a2cc283af Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Thu, 13 Sep 2018 15:03:19 -0700 Subject: [core] For string-valued properties, do coercion rather than assertion --- include/mbgl/style/expression/parsing_context.hpp | 33 ++++++++++++------ .../expression-string-coercion.json | 3 ++ src/mbgl/style/conversion/function.cpp | 4 +-- src/mbgl/style/expression/coalesce.cpp | 2 +- src/mbgl/style/expression/coercion.cpp | 33 +++++++++++++++--- src/mbgl/style/expression/compound_expression.cpp | 13 ------- src/mbgl/style/expression/dsl.cpp | 2 +- src/mbgl/style/expression/parsing_context.cpp | 40 ++++++++++++++-------- test/style/conversion/function.test.cpp | 10 +++--- 9 files changed, 89 insertions(+), 51 deletions(-) create mode 100644 scripts/changelog_staging/expression-string-coercion.json diff --git a/include/mbgl/style/expression/parsing_context.hpp b/include/mbgl/style/expression/parsing_context.hpp index 66014e33d4..dde907ec2c 100644 --- a/include/mbgl/style/expression/parsing_context.hpp +++ b/include/mbgl/style/expression/parsing_context.hpp @@ -52,6 +52,25 @@ public: } // namespace detail +/* + Controls the annotation behavior of the parser when encountering an expression + whose type is not a subtype of the expected type. The default behavior, used + when optional is a nullopt, is as follows: + + When we expect a number, string, boolean, or array but have a value, wrap it in an assertion. + When we expect a color or formatted string, but have a string or value, wrap it in a coercion. + Otherwise, we do static type-checking. + + These behaviors are overridable for: + * The "coalesce" operator, which needs to omit type annotations. + * String-valued properties (e.g. `text-field`), where coercion is more convenient than assertion. +*/ +enum class TypeAnnotationOption { + coerce, + assert, + omit +}; + class ParsingContext { public: ParsingContext() : errors(std::make_shared>()) {} @@ -70,24 +89,18 @@ public: const std::vector& getErrors() const { return *errors; } const std::string getCombinedErrors() const; - enum TypeAnnotationOption { - includeTypeAnnotations, - omitTypeAnnotations - }; - /* Parse the given style-spec JSON value as an expression. */ ParseResult parseExpression(const mbgl::style::conversion::Convertible& value, - TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations); + optional = {}); /* Parse the given style-spec JSON value as an expression intended to be used in a layout or paint property. This entails checking additional constraints that exist in that context but not, e.g., for filters. */ - ParseResult parseLayerPropertyExpression(const mbgl::style::conversion::Convertible& value, - TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations); + ParseResult parseLayerPropertyExpression(const mbgl::style::conversion::Convertible& value); /* Parse a child expression. For use by individual Expression::parse() methods. @@ -95,7 +108,7 @@ public: ParseResult parse(const mbgl::style::conversion::Convertible&, std::size_t, optional = {}, - TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations); + optional = {}); /* Parse a child expression. For use by individual Expression::parse() methods. @@ -156,7 +169,7 @@ private: appropriate ParseXxxx::parse(const V&, ParsingContext) method. */ ParseResult parse(const mbgl::style::conversion::Convertible& value, - TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations); + optional = {}); std::string key; optional expected; diff --git a/scripts/changelog_staging/expression-string-coercion.json b/scripts/changelog_staging/expression-string-coercion.json new file mode 100644 index 0000000000..118c89a51c --- /dev/null +++ b/scripts/changelog_staging/expression-string-coercion.json @@ -0,0 +1,3 @@ +{ + "core": "Arguments to the `concat` expression operator are now automatically converted to string values as if via `to-string`, as are values for the `text-field`, `icon-image`, and `*-pattern` properties. Previously, such values were required to be strings, and if they were not, an error occurred." +} diff --git a/src/mbgl/style/conversion/function.cpp b/src/mbgl/style/conversion/function.cpp index 7cc599be1d..6ee962fb77 100644 --- a/src/mbgl/style/conversion/function.cpp +++ b/src/mbgl/style/conversion/function.cpp @@ -52,7 +52,7 @@ std::unique_ptr convertTokenStringToExpression(const std::string& so if (pos != end) { for (brace++; brace != end && tokenReservedChars.find(*brace) == std::string::npos; brace++); if (brace != end && *brace == '}') { - inputs.push_back(toString(get(literal(std::string(pos + 1, brace))))); + inputs.push_back(get(literal(std::string(pos + 1, brace)))); pos = brace + 1; } else { inputs.push_back(literal(std::string(pos, brace))); @@ -65,7 +65,7 @@ std::unique_ptr convertTokenStringToExpression(const std::string& so case 0: return literal(source); case 1: - return std::move(inputs[0]); + return toString(std::move(inputs[0])); default: return concat(std::move(inputs)); } diff --git a/src/mbgl/style/expression/coalesce.cpp b/src/mbgl/style/expression/coalesce.cpp index cdbf452f7f..6b43e68ac6 100644 --- a/src/mbgl/style/expression/coalesce.cpp +++ b/src/mbgl/style/expression/coalesce.cpp @@ -57,7 +57,7 @@ ParseResult Coalesce::parse(const Convertible& value, ParsingContext& ctx) { Coalesce::Args args; args.reserve(length - 1); for (std::size_t i = 1; i < length; i++) { - auto parsed = ctx.parse(arrayMember(value, i), i, outputType, ParsingContext::omitTypeAnnotations); + auto parsed = ctx.parse(arrayMember(value, i), i, outputType, TypeAnnotationOption::omit); if (!parsed) { return parsed; } diff --git a/src/mbgl/style/expression/coercion.cpp b/src/mbgl/style/expression/coercion.cpp index 187cdd48a4..4a17895991 100644 --- a/src/mbgl/style/expression/coercion.cpp +++ b/src/mbgl/style/expression/coercion.cpp @@ -8,6 +8,16 @@ namespace mbgl { namespace style { namespace expression { +EvaluationResult toBoolean(const Value& v) { + return v.match( + [&] (double f) { return static_cast(f); }, + [&] (const std::string& s) { return s.length() > 0; }, + [&] (bool b) { return b; }, + [&] (const NullValue&) { return false; }, + [&] (const auto&) { return true; } + ); +} + EvaluationResult toNumber(const Value& v) { optional result = v.match( [](NullValue) -> optional { return 0.0; }, @@ -78,10 +88,14 @@ Coercion::Coercion(type::Type type_, std::vector> in { assert(!inputs.empty()); type::Type t = getType(); - if (t.is()) { - coerceSingleValue = toNumber; + if (t.is()) { + coerceSingleValue = toBoolean; } else if (t.is()) { coerceSingleValue = toColor; + } else if (t.is()) { + coerceSingleValue = toNumber; + } else if (t.is()) { + coerceSingleValue = [] (const Value& v) -> EvaluationResult { return toString(v); }; } else { assert(false); } @@ -89,16 +103,20 @@ Coercion::Coercion(type::Type type_, std::vector> in std::string Coercion::getOperator() const { return getType().match( - [](const type::NumberType&) { return "to-number"; }, + [](const type::BooleanType&) { return "to-boolean"; }, [](const type::ColorType&) { return "to-color"; }, + [](const type::NumberType&) { return "to-number"; }, + [](const type::StringType&) { return "to-string"; }, [](const auto&) { assert(false); return ""; }); } using namespace mbgl::style::conversion; ParseResult Coercion::parse(const Convertible& value, ParsingContext& ctx) { static std::unordered_map types { + {"to-boolean", type::Boolean}, + {"to-color", type::Color}, {"to-number", type::Number}, - {"to-color", type::Color} + {"to-string", type::String} }; std::size_t length = arrayLength(value); @@ -110,7 +128,12 @@ ParseResult Coercion::parse(const Convertible& value, ParsingContext& ctx) { auto it = types.find(*toString(arrayMember(value, 0))); assert(it != types.end()); - + + if ((it->second == type::Boolean || it->second == type::String) && length != 2) { + ctx.error("Expected one argument."); + return ParseResult(); + } + std::vector> parsed; parsed.reserve(length - 1); for (std::size_t i = 1; i < length; i++) { diff --git a/src/mbgl/style/expression/compound_expression.cpp b/src/mbgl/style/expression/compound_expression.cpp index f5cbd7030b..fc47b2d78e 100644 --- a/src/mbgl/style/expression/compound_expression.cpp +++ b/src/mbgl/style/expression/compound_expression.cpp @@ -309,19 +309,6 @@ std::unordered_map initiali define("typeof", [](const Value& v) -> Result { return toString(typeOf(v)); }); - define("to-string", [](const Value& value) -> Result { - return toString(value); - }); - - define("to-boolean", [](const Value& v) -> Result { - return v.match( - [&] (double f) { return static_cast(f); }, - [&] (const std::string& s) { return s.length() > 0; }, - [&] (bool b) { return b; }, - [&] (const NullValue&) { return false; }, - [&] (const auto&) { return true; } - ); - }); define("to-rgba", [](const Color& color) -> Result> { return color.toArray(); }); diff --git a/src/mbgl/style/expression/dsl.cpp b/src/mbgl/style/expression/dsl.cpp index cdada583a2..c6318fb637 100644 --- a/src/mbgl/style/expression/dsl.cpp +++ b/src/mbgl/style/expression/dsl.cpp @@ -74,7 +74,7 @@ std::unique_ptr toColor(std::unique_ptr value) { } std::unique_ptr toString(std::unique_ptr value) { - return compound("to-string", std::move(value)); + return std::make_unique(type::String, vec(std::move(value))); } std::unique_ptr get(const char* value) { diff --git a/src/mbgl/style/expression/parsing_context.cpp b/src/mbgl/style/expression/parsing_context.cpp index 29d04d96a2..0373b9721f 100644 --- a/src/mbgl/style/expression/parsing_context.cpp +++ b/src/mbgl/style/expression/parsing_context.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -76,7 +77,7 @@ using namespace mbgl::style::conversion; ParseResult ParsingContext::parse(const Convertible& value, std::size_t index_, optional expected_, - TypeAnnotationOption typeAnnotationOption) { + optional typeAnnotationOption) { ParsingContext child(key + "[" + util::toString(index_) + "]", errors, std::move(expected_), @@ -118,14 +119,16 @@ const ExpressionRegistry& getExpressionRegistry() { {"object", Assertion::parse}, {"step", Step::parse}, {"string", Assertion::parse}, + {"to-boolean", Coercion::parse}, {"to-color", Coercion::parse}, {"to-number", Coercion::parse}, + {"to-string", Coercion::parse}, {"var", Var::parse} }}; return registry; } -ParseResult ParsingContext::parse(const Convertible& value, TypeAnnotationOption typeAnnotationOption) { +ParseResult ParsingContext::parse(const Convertible& value, optional typeAnnotationOption) { ParseResult parsed; if (isArray(value)) { @@ -161,22 +164,27 @@ ParseResult ParsingContext::parse(const Convertible& value, TypeAnnotationOption return parsed; } - auto array = [&](std::unique_ptr expression) { - std::vector> args; - args.push_back(std::move(expression)); - return args; + auto annotate = [] (std::unique_ptr expression, type::Type type, TypeAnnotationOption typeAnnotation) -> std::unique_ptr { + switch (typeAnnotation) { + case TypeAnnotationOption::assert: + return std::make_unique(type, dsl::vec(std::move(expression))); + case TypeAnnotationOption::coerce: + return std::make_unique(type, dsl::vec(std::move(expression))); + case TypeAnnotationOption::omit: + return expression; + } + + // Not reachable, but placate GCC. + assert(false); + return expression; }; if (expected) { const type::Type actual = (*parsed)->getType(); if ((*expected == type::String || *expected == type::Number || *expected == type::Boolean || *expected == type::Object || expected->is()) && actual == type::Value) { - if (typeAnnotationOption == includeTypeAnnotations) { - parsed = { std::make_unique(*expected, array(std::move(*parsed))) }; - } + parsed = { annotate(std::move(*parsed), *expected, typeAnnotationOption.value_or(TypeAnnotationOption::assert)) }; } else if (*expected == type::Color && (actual == type::Value || actual == type::String)) { - if (typeAnnotationOption == includeTypeAnnotations) { - parsed = { std::make_unique(*expected, array(std::move(*parsed))) }; - } + parsed = { annotate(std::move(*parsed), *expected, typeAnnotationOption.value_or(TypeAnnotationOption::coerce)) }; } else { checkType((*parsed)->getType()); if (errors->size() > 0) { @@ -212,11 +220,15 @@ ParseResult ParsingContext::parse(const Convertible& value, TypeAnnotationOption return parsed; } -ParseResult ParsingContext::parseExpression(const Convertible& value, TypeAnnotationOption typeAnnotationOption) { +ParseResult ParsingContext::parseExpression(const Convertible& value, optional typeAnnotationOption) { return parse(value, typeAnnotationOption); } -ParseResult ParsingContext::parseLayerPropertyExpression(const Convertible& value, TypeAnnotationOption typeAnnotationOption) { +ParseResult ParsingContext::parseLayerPropertyExpression(const Convertible& value) { + optional typeAnnotationOption; + if (expected && *expected == type::String) { + typeAnnotationOption = TypeAnnotationOption::coerce; + } ParseResult parsed = parse(value, typeAnnotationOption); if (parsed && !isZoomConstant(**parsed)) { optional> zoomCurve = findZoomCurve(parsed->get()); diff --git a/test/style/conversion/function.test.cpp b/test/style/conversion/function.test.cpp index e63ad722cf..8ebdcdb9bb 100644 --- a/test/style/conversion/function.test.cpp +++ b/test/style/conversion/function.test.cpp @@ -86,9 +86,9 @@ TEST(StyleConversion, TokenStrings) { using namespace mbgl::style::expression::dsl; ASSERT_EQ(*convertTokenStringToExpression("{token}"), *toString(get(literal("token")))); - ASSERT_EQ(*convertTokenStringToExpression("token {token}"), *concat(vec(literal("token "), toString(get(literal("token")))))); - ASSERT_EQ(*convertTokenStringToExpression("{token} token"), *concat(vec(toString(get(literal("token"))), literal(" token")))); - ASSERT_EQ(*convertTokenStringToExpression("{token} {token}"), *concat(vec(toString(get(literal("token"))), literal(" "), toString(get(literal("token")))))); - ASSERT_EQ(*convertTokenStringToExpression("{token} {token"), *concat(vec(toString(get(literal("token"))), literal(" "), literal("{token")))); - ASSERT_EQ(*convertTokenStringToExpression("{token {token}"), *concat(vec(literal("{token "), toString(get(literal("token")))))); + ASSERT_EQ(*convertTokenStringToExpression("token {token}"), *concat(vec(literal("token "), get(literal("token"))))); + ASSERT_EQ(*convertTokenStringToExpression("{token} token"), *concat(vec(get(literal("token")), literal(" token")))); + ASSERT_EQ(*convertTokenStringToExpression("{token} {token}"), *concat(vec(get(literal("token")), literal(" "), get(literal("token"))))); + ASSERT_EQ(*convertTokenStringToExpression("{token} {token"), *concat(vec(get(literal("token")), literal(" "), literal("{token")))); + ASSERT_EQ(*convertTokenStringToExpression("{token {token}"), *concat(vec(literal("{token "), get(literal("token"))))); } -- cgit v1.2.1