Add StyleParser tests for expression validation

(With related parsing fixups)

6 files changed, 107 insertions, 11 deletions

diff --git a/include/mbgl/style/conversion/data_driven_property_value.hpp b/include/mbgl/style/conversion/data_driven_property_value.hpp
index 79ea34979e..55e29c4641 100644
--- a/include/mbgl/style/conversion/data_driven_property_value.hpp
+++ b/include/mbgl/style/conversion/data_driven_property_value.hpp
@@ -29,16 +29,21 @@ struct Converter<DataDrivenPropertyValue<T>> {
             if (!expression) {
                 return {};
             }
-            if (isFeatureConstant(expression->get())) {
-                return DataDrivenPropertyValue<T>(CameraFunction<T>(std::move(*expression)));
-            } else if (isZoomConstant(expression->get())) {
-                return DataDrivenPropertyValue<T>(SourceFunction<T>(std::move(*expression)));
-            } else {
-                if (!CompositeFunction<T>::Curve::findZoomCurve(expression->get())) {
+            
+            bool zoomConstant = isZoomConstant(expression->get());
+            
+            if (!zoomConstant) {
+                if (!CameraFunction<T>::Curve::findZoomCurve(expression->get())) {
                     error = { R"("zoom" expression may only be used as input to a top-level "curve" expression.)" };
                     return {};
                 }
+            }
             
+            if (isFeatureConstant(expression->get())) {
+                return DataDrivenPropertyValue<T>(CameraFunction<T>(std::move(*expression)));
+            } else if (zoomConstant) {
+                return DataDrivenPropertyValue<T>(SourceFunction<T>(std::move(*expression)));
+            } else {
                 return DataDrivenPropertyValue<T>(CompositeFunction<T>(std::move(*expression)));
             }
         } else if (!isObject(value)) {
diff --git a/include/mbgl/style/conversion/expression.hpp b/include/mbgl/style/conversion/expression.hpp
index 31c5f629f0..287fcbbe36 100644
--- a/include/mbgl/style/conversion/expression.hpp
+++ b/include/mbgl/style/conversion/expression.hpp
@@ -23,7 +23,10 @@ template<> struct Converter<std::unique_ptr<Expression>> {
             if (combinedError.size() > 0) {
                 combinedError += "\n";
             }
-            combinedError += parsingError.key + ": " + parsingError.message;
+            if (parsingError.key.size() > 0) {
+                combinedError += parsingError.key + ": ";
+            }
+            combinedError += parsingError.message;
         }
         error = { combinedError };
         return {};
diff --git a/include/mbgl/style/conversion/property_value.hpp b/include/mbgl/style/conversion/property_value.hpp
index fd9b47b090..e49861266b 100644
--- a/include/mbgl/style/conversion/property_value.hpp
+++ b/include/mbgl/style/conversion/property_value.hpp
@@ -7,6 +7,7 @@
 #include <mbgl/style/conversion/expression.hpp>
 #include <mbgl/style/expression/value.hpp>
 #include <mbgl/style/expression/is_constant.hpp>
+#include <mbgl/style/expression/is_expression.hpp>
 
 namespace mbgl {
 namespace style {
@@ -17,15 +18,15 @@ struct Converter<PropertyValue<T>> {
     optional<PropertyValue<T>> operator()(const Convertible& value, Error& error) const {
         if (isUndefined(value)) {
             return PropertyValue<T>();
-        } else if (isObject(value) && objectMember(value, "expression")) {
-            optional<std::unique_ptr<Expression>> expression = convert<std::unique_ptr<Expression>>(*objectMember(value, "expression"), error, valueTypeToExpressionType<T>());
+        } else if (isExpression(value)) {
+            optional<std::unique_ptr<Expression>> expression = convert<std::unique_ptr<Expression>>(value, error, valueTypeToExpressionType<T>());
             if (!expression) {
                 return {};
             }
             if (isFeatureConstant(expression->get())) {
                 return { CameraFunction<T>(std::move(*expression)) };
             } else {
-                error = { "data-driven style property not supported " };
+                error = { "property expressions not supported" };
                 return {};
             }
         } else if (isObject(value)) {
diff --git a/src/mbgl/style/expression/curve.cpp b/src/mbgl/style/expression/curve.cpp
index 417f9cf5c7..c03250a50f 100644
--- a/src/mbgl/style/expression/curve.cpp
+++ b/src/mbgl/style/expression/curve.cpp
@@ -181,7 +181,8 @@ ParseResult parseCurve(const mbgl::style::conversion::Convertible& value, Parsin
         *outputType != type::Color &&
         !(
             outputType->is<type::Array>() &&
-            outputType->get<type::Array>().itemType == type::Number
+            outputType->get<type::Array>().itemType == type::Number &&
+            outputType->get<type::Array>().N
         )
     )
     {
diff --git a/test/fixtures/style_parser/expressions.info.json b/test/fixtures/style_parser/expressions.info.json
new file mode 100644
index 0000000000..49e0280d3f
--- /dev/null
+++ b/test/fixtures/style_parser/expressions.info.json
@@ -0,0 +1,12 @@
+{
+    "default": {
+        "log": [
+            [1, "WARNING", "ParseStyle", "Expected color but found number instead."],
+            [1, "WARNING", "ParseStyle", "[2]: Expected number but found string instead."],
+            [1, "WARNING", "ParseStyle", "\"zoom\" expression may only be used as input to a top-level \"curve\" expression."],
+            [1, "WARNING", "ParseStyle", "value must be a string"],
+            [1, "WARNING", "ParseStyle", "property expressions not supported"],
+            [1, "WARNING", "ParseStyle", "Type array<number> is not interpolatable, and thus cannot be used as a linear curve's output type."]
+        ]
+    }
+}
diff --git a/test/fixtures/style_parser/expressions.style.json b/test/fixtures/style_parser/expressions.style.json
new file mode 100644
index 0000000000..389cd923ea
--- /dev/null
+++ b/test/fixtures/style_parser/expressions.style.json
@@ -0,0 +1,74 @@
+{
+  "version": 8,
+  "sources": {
+    "source": {
+      "type": "vector",
+      "url": "mapbox://mapbox.mapbox-streets-v5"
+    }
+  },
+  "layers": [
+    {
+      "id": "valid expression",
+      "type": "fill",
+      "source": "source",
+      "source-layer": "layer",
+      "paint": {
+        "fill-color": ["rgba", 10, ["number", ["get", "x"]], 30, 1]
+      }
+    },
+    {
+      "id": "invalid expression type - color",
+      "type": "fill",
+      "source": "source",
+      "source-layer": "layer",
+      "paint": {
+        "fill-color": ["pi"]
+      }
+    },
+    {
+      "id": "invalid expression - fails type checking",
+      "type": "fill",
+      "source": "source",
+      "source-layer": "layer",
+      "paint": {
+        "fill-color": ["rgba", 1, "should be a number", 0, 1]
+      }
+    },
+    {
+      "id": "invalid expression - nested zoom expression",
+      "type": "fill",
+      "source": "source",
+      "source-layer": "layer",
+      "paint": {
+        "fill-opacity": ["+", 0.5, ["curve", ["linear"], ["zoom"], 0, 0, 1, 1]]
+      }
+    },
+    {
+      "id": "invalid expression - not allowed in visibility",
+      "type": "fill",
+      "source": "source",
+      "source-layer": "layer",
+      "layout": {
+        "visibility": ["literal", true]
+      }
+    },
+    {
+      "id": "invalid expression - not a DDS property",
+      "type": "fill-extrusion",
+      "source": "source",
+      "source-layer": "layer",
+      "paint": {
+        "fill-extrusion-opacity": ["get", "opacity"]
+      }
+    },
+    {
+      "id": "invalid expression - line-dasharray must use step interpolation",
+      "type": "line",
+      "source": "source",
+      "source-layer": "layer",
+      "paint": {
+        "line-dasharray": ["curve", ["linear"], ["zoom"], 0, ["literal", [1, 2]], 1, ["literal", [3, 4]]]
+      }
+    }
+  ]
+}