[core] For string-valued properties, do coercion rather than assertion

9 files changed, 89 insertions, 51 deletions

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<TypeAnnotationOption> 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<std::vector<ParsingError>>()) {}
@@ -70,24 +89,18 @@ public:
     const std::vector<ParsingError>& 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<TypeAnnotationOption> = {});
 
     /*
         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<type::Type> = {},
-                      TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations);
+                      optional<TypeAnnotationOption> = {});
     
     /*
         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<TypeAnnotationOption> = {});
     
     std::string key;
     optional<type::Type> 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<Expression> 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<Expression> 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<bool>(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<double> result = v.match(
         [](NullValue) -> optional<double> { return 0.0; },
@@ -78,10 +88,14 @@ Coercion::Coercion(type::Type type_, std::vector<std::unique_ptr<Expression>> in
 {
     assert(!inputs.empty());
     type::Type t = getType();
-    if (t.is<type::NumberType>()) {
-        coerceSingleValue = toNumber;
+    if (t.is<type::BooleanType>()) {
+        coerceSingleValue = toBoolean;
     } else if (t.is<type::ColorType>()) {
         coerceSingleValue = toColor;
+    } else if (t.is<type::NumberType>()) {
+        coerceSingleValue = toNumber;
+    } else if (t.is<type::StringType>()) {
+        coerceSingleValue = [] (const Value& v) -> EvaluationResult { return toString(v); };
     } else {
         assert(false);
     }
@@ -89,16 +103,20 @@ Coercion::Coercion(type::Type type_, std::vector<std::unique_ptr<Expression>> 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<std::string, type::Type> 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<std::unique_ptr<Expression>> 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<std::string, CompoundExpressionRegistry::Definition> initiali
 
     define("typeof", [](const Value& v) -> Result<std::string> { return toString(typeOf(v)); });
 
-    define("to-string", [](const Value& value) -> Result<std::string> {
-        return toString(value);
-    });
-
-    define("to-boolean", [](const Value& v) -> Result<bool> {
-        return v.match(
-            [&] (double f) { return static_cast<bool>(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<std::array<double, 4>> {
         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<Expression> toColor(std::unique_ptr<Expression> value) {
 }
 
 std::unique_ptr<Expression> toString(std::unique_ptr<Expression> value) {
-    return compound("to-string", std::move(value));
+    return std::make_unique<Coercion>(type::String, vec(std::move(value)));
 }
 
 std::unique_ptr<Expression> 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 <mbgl/style/expression/step.hpp>
 
 #include <mbgl/style/expression/find_zoom_curve.hpp>
+#include <mbgl/style/expression/dsl.hpp>
 
 #include <mbgl/style/conversion/get_json_type.hpp>
 #include <mbgl/style/conversion_impl.hpp>
@@ -76,7 +77,7 @@ using namespace mbgl::style::conversion;
 ParseResult ParsingContext::parse(const Convertible& value,
                                   std::size_t index_,
                                   optional<type::Type> expected_,
-                                  TypeAnnotationOption typeAnnotationOption) {
+                                  optional<TypeAnnotationOption> 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> 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> expression) {
-        std::vector<std::unique_ptr<Expression>> args;
-        args.push_back(std::move(expression));
-        return args;
+    auto annotate = [] (std::unique_ptr<Expression> expression, type::Type type, TypeAnnotationOption typeAnnotation) -> std::unique_ptr<Expression> {
+        switch (typeAnnotation) {
+        case TypeAnnotationOption::assert:
+            return std::make_unique<Assertion>(type, dsl::vec(std::move(expression)));
+        case TypeAnnotationOption::coerce:
+            return std::make_unique<Coercion>(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<type::Array>()) && actual == type::Value) {
-            if (typeAnnotationOption == includeTypeAnnotations) {
-                parsed = { std::make_unique<Assertion>(*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<Coercion>(*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> typeAnnotationOption) {
     return parse(value, typeAnnotationOption);
 }
 
-ParseResult ParsingContext::parseLayerPropertyExpression(const Convertible& value, TypeAnnotationOption typeAnnotationOption) {
+ParseResult ParsingContext::parseLayerPropertyExpression(const Convertible& value) {
+    optional<TypeAnnotationOption> typeAnnotationOption;
+    if (expected && *expected == type::String) {
+        typeAnnotationOption = TypeAnnotationOption::coerce;
+    }
     ParseResult parsed = parse(value, typeAnnotationOption);
     if (parsed && !isZoomConstant(**parsed)) {
         optional<variant<const Interpolate*, const Step*, ParsingError>> 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")))));
 }