Don't enforce ["zoom"] constraints for filters (#11672)

* Don't enforce ["zoom"] constraints for filters

Fixes #11594

* Add a couple of comments

* Fix ambiguous constructor call

* ParsingContext(optional<Type>) => ParsingContext(Type)

11 files changed, 88 insertions, 70 deletions

diff --git a/cmake/core-files.cmake b/cmake/core-files.cmake
index dd39395e5a..0853097630 100644
--- a/cmake/core-files.cmake
+++ b/cmake/core-files.cmake
@@ -410,7 +410,6 @@ set(MBGL_CORE_FILES
     include/mbgl/style/conversion/coordinate.hpp
     include/mbgl/style/conversion/custom_geometry_source_options.hpp
     include/mbgl/style/conversion/data_driven_property_value.hpp
-    include/mbgl/style/conversion/expression.hpp
     include/mbgl/style/conversion/filter.hpp
     include/mbgl/style/conversion/function.hpp
     include/mbgl/style/conversion/geojson.hpp
diff --git a/include/mbgl/style/conversion/data_driven_property_value.hpp b/include/mbgl/style/conversion/data_driven_property_value.hpp
index b4297816df..07ed201c99 100644
--- a/include/mbgl/style/conversion/data_driven_property_value.hpp
+++ b/include/mbgl/style/conversion/data_driven_property_value.hpp
@@ -4,12 +4,12 @@
 #include <mbgl/style/conversion.hpp>
 #include <mbgl/style/conversion/constant.hpp>
 #include <mbgl/style/conversion/function.hpp>
-#include <mbgl/style/conversion/expression.hpp>
 #include <mbgl/style/expression/is_expression.hpp>
 #include <mbgl/style/expression/is_constant.hpp>
 #include <mbgl/style/expression/find_zoom_curve.hpp>
 #include <mbgl/style/expression/literal.hpp>
 #include <mbgl/style/expression/value.hpp>
+#include <mbgl/style/expression/parsing_context.hpp>
 
 #include <unordered_set>
 
@@ -22,15 +22,15 @@ template <class T>
 struct Converter<DataDrivenPropertyValue<T>> {
 
     optional<DataDrivenPropertyValue<T>> operator()(const Convertible& value, Error& error) const {
+        using namespace mbgl::style::expression;
+        
         if (isUndefined(value)) {
             return DataDrivenPropertyValue<T>();
-        } else if (expression::isExpression(value)) {
-            optional<std::unique_ptr<Expression>> expression = convert<std::unique_ptr<Expression>>(
-                value,
-                error,
-                valueTypeToExpressionType<T>());
-            
+        } else if (isExpression(value)) {
+            ParsingContext ctx(valueTypeToExpressionType<T>());
+            ParseResult expression = ctx.parseLayerPropertyExpression(value);
             if (!expression) {
+                error = { ctx.getCombinedErrors() };
                 return {};
             }
             
diff --git a/include/mbgl/style/conversion/expression.hpp b/include/mbgl/style/conversion/expression.hpp
deleted file mode 100644
index c5fcf906a7..0000000000
--- a/include/mbgl/style/conversion/expression.hpp
+++ /dev/null
@@ -1,39 +0,0 @@
-#pragma once
-
-#include <mbgl/style/expression/parsing_context.hpp>
-#include <mbgl/style/expression/type.hpp>
-#include <mbgl/style/conversion.hpp>
-
-#include <memory>
-
-namespace mbgl {
-namespace style {
-namespace conversion {
-
-using namespace mbgl::style::expression;
-
-template<> struct Converter<std::unique_ptr<Expression>> {
-    optional<std::unique_ptr<Expression>> operator()(const Convertible& value, Error& error, type::Type expected) const {
-        ParsingContext ctx(optional<type::Type> {expected});
-        ParseResult parsed = ctx.parse(value);
-        if (parsed) {
-            return std::move(*parsed);
-        }
-        std::string combinedError;
-        for (const ParsingError& parsingError : ctx.getErrors()) {
-            if (combinedError.size() > 0) {
-                combinedError += "\n";
-            }
-            if (parsingError.key.size() > 0) {
-                combinedError += parsingError.key + ": ";
-            }
-            combinedError += parsingError.message;
-        }
-        error = { combinedError };
-        return {};
-    };
-};
-
-} // namespace conversion
-} // namespace style
-} // namespace mbgl
diff --git a/include/mbgl/style/conversion/heatmap_color_property_value.hpp b/include/mbgl/style/conversion/heatmap_color_property_value.hpp
index e3689c524c..a4710792d2 100644
--- a/include/mbgl/style/conversion/heatmap_color_property_value.hpp
+++ b/include/mbgl/style/conversion/heatmap_color_property_value.hpp
@@ -4,11 +4,11 @@
 #include <mbgl/style/conversion.hpp>
 #include <mbgl/style/conversion/constant.hpp>
 #include <mbgl/style/conversion/function.hpp>
-#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>
 #include <mbgl/style/expression/find_zoom_curve.hpp>
+#include <mbgl/style/expression/parsing_context.hpp>
 
 namespace mbgl {
 namespace style {
@@ -17,11 +17,14 @@ namespace conversion {
 template <>
 struct Converter<HeatmapColorPropertyValue> {
     optional<HeatmapColorPropertyValue> operator()(const Convertible& value, Error& error) const {
+        using namespace mbgl::style::expression;
         if (isUndefined(value)) {
             return HeatmapColorPropertyValue();
         } else if (isExpression(value)) {
-            optional<std::unique_ptr<Expression>> expression = convert<std::unique_ptr<Expression>>(value, error, expression::type::Color);
+            ParsingContext ctx(type::Color);
+            ParseResult expression = ctx.parseLayerPropertyExpression(value);
             if (!expression) {
+                error = { ctx.getCombinedErrors() };
                 return {};
             }
             assert(*expression);
diff --git a/include/mbgl/style/conversion/property_value.hpp b/include/mbgl/style/conversion/property_value.hpp
index 97117de2ec..3130661f61 100644
--- a/include/mbgl/style/conversion/property_value.hpp
+++ b/include/mbgl/style/conversion/property_value.hpp
@@ -4,11 +4,11 @@
 #include <mbgl/style/conversion.hpp>
 #include <mbgl/style/conversion/constant.hpp>
 #include <mbgl/style/conversion/function.hpp>
-#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>
 #include <mbgl/style/expression/find_zoom_curve.hpp>
+#include <mbgl/style/expression/parsing_context.hpp>
 
 namespace mbgl {
 namespace style {
@@ -17,13 +17,18 @@ namespace conversion {
 template <class T>
 struct Converter<PropertyValue<T>> {
     optional<PropertyValue<T>> operator()(const Convertible& value, Error& error) const {
+        using namespace mbgl::style::expression;
+
         if (isUndefined(value)) {
             return PropertyValue<T>();
         } else if (isExpression(value)) {
-            optional<std::unique_ptr<Expression>> expression = convert<std::unique_ptr<Expression>>(value, error, valueTypeToExpressionType<T>());
+            ParsingContext ctx(valueTypeToExpressionType<T>());
+            ParseResult expression = ctx.parseLayerPropertyExpression(value);
             if (!expression) {
+                error = { ctx.getCombinedErrors() };
                 return {};
             }
+
             if (isFeatureConstant(**expression)) {
                 return { CameraFunction<T>(std::move(*expression)) };
             } else {
diff --git a/include/mbgl/style/expression/parsing_context.hpp b/include/mbgl/style/expression/parsing_context.hpp
index f92a4c95ea..c19974a4f7 100644
--- a/include/mbgl/style/expression/parsing_context.hpp
+++ b/include/mbgl/style/expression/parsing_context.hpp
@@ -55,7 +55,7 @@ class ParsingContext {
 public:
     ParsingContext() : errors(std::make_shared<std::vector<ParsingError>>()) {}
     ParsingContext(std::string key_) : key(std::move(key_)), errors(std::make_shared<std::vector<ParsingError>>()) {}
-    explicit ParsingContext(optional<type::Type> expected_)
+    explicit ParsingContext(type::Type expected_)
         : expected(std::move(expected_)),
           errors(std::make_shared<std::vector<ParsingError>>())
     {}
@@ -67,6 +67,7 @@ public:
     std::string getKey() const { return key; }
     optional<type::Type> getExpected() const { return expected; }
     const std::vector<ParsingError>& getErrors() const { return *errors; }
+    const std::string getCombinedErrors() const;
 
     enum TypeAnnotationOption {
         includeTypeAnnotations,
@@ -74,16 +75,21 @@ public:
     };
 
     /*
-        Parse the given style-spec JSON value into an Expression object.
-        Specifically, this function is responsible for determining the expression
-        type (either Literal, or the one named in value[0]) and dispatching to the
-        appropriate ParseXxxx::parse(const V&, ParsingContext) method.
+        Parse the given style-spec JSON value as an expression.
     */
-    ParseResult parse(const mbgl::style::conversion::Convertible& value,
-                      TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations);
+    ParseResult parseExpression(const mbgl::style::conversion::Convertible& value,
+                                TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations);
 
     /*
-        Parse a child expression.
+        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);
+
+    /*
+        Parse a child expression. For use by individual Expression::parse() methods.
     */
     ParseResult parse(const mbgl::style::conversion::Convertible&,
                       std::size_t,
@@ -91,7 +97,7 @@ public:
                       TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations);
     
     /*
-        Parse a child expression.
+        Parse a child expression.  For use by individual Expression::parse() methods.
     */
     ParseResult parse(const mbgl::style::conversion::Convertible&,
                       std::size_t index,
@@ -141,6 +147,16 @@ private:
         errors(std::move(errors_))
     {}
     
+    
+    /*
+        Parse the given style-spec JSON value into an Expression object.
+        Specifically, this function is responsible for determining the expression
+        type (either Literal, or the one named in value[0]) and dispatching to the
+        appropriate ParseXxxx::parse(const V&, ParsingContext) method.
+    */
+    ParseResult parse(const mbgl::style::conversion::Convertible& value,
+                      TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations);
+    
     std::string key;
     optional<type::Type> expected;
     std::shared_ptr<detail::Scope> scope;
diff --git a/platform/node/src/node_expression.cpp b/platform/node/src/node_expression.cpp
index 84515060a3..27866ccbed 100644
--- a/platform/node/src/node_expression.cpp
+++ b/platform/node/src/node_expression.cpp
@@ -75,8 +75,8 @@ void NodeExpression::Parse(const Nan::FunctionCallbackInfo<v8::Value>& info) {
     auto expr = info[0];
 
     try {
-        ParsingContext ctx(expected);
-        ParseResult parsed = ctx.parse(mbgl::style::conversion::Convertible(expr));
+        ParsingContext ctx = expected ? ParsingContext(*expected) : ParsingContext();
+        ParseResult parsed = ctx.parseLayerPropertyExpression(mbgl::style::conversion::Convertible(expr));
         if (parsed) {
             assert(ctx.getErrors().size() == 0);
             auto nodeExpr = new NodeExpression(std::move(*parsed));
diff --git a/src/mbgl/style/conversion/filter.cpp b/src/mbgl/style/conversion/filter.cpp
index fba149da12..3c941945fd 100644
--- a/src/mbgl/style/conversion/filter.cpp
+++ b/src/mbgl/style/conversion/filter.cpp
@@ -2,7 +2,7 @@
 #include <mbgl/util/geometry.hpp>
 #include <mbgl/style/expression/expression.hpp>
 #include <mbgl/style/expression/type.hpp>
-#include <mbgl/style/conversion/expression.hpp>
+#include <mbgl/style/expression/parsing_context.hpp>
 
 namespace mbgl {
 namespace style {
@@ -236,10 +236,13 @@ optional<Filter> convertCompoundFilter(const Convertible& value, Error& error) {
 }
     
 optional<Filter> convertExpressionFilter(const Convertible& value, Error& error) {
-    optional<std::unique_ptr<Expression>> expression = convert<std::unique_ptr<Expression>>(value, error, expression::type::Boolean);
+    expression::ParsingContext ctx(expression::type::Boolean);
+    expression::ParseResult expression = ctx.parseExpression(value);
     if (!expression) {
+        error = { ctx.getCombinedErrors() };
         return {};
     }
+
     return { ExpressionFilter { std::move(*expression) } };
 }
 
diff --git a/src/mbgl/style/expression/parsing_context.cpp b/src/mbgl/style/expression/parsing_context.cpp
index 713c0b5d27..b522aeff9a 100644
--- a/src/mbgl/style/expression/parsing_context.cpp
+++ b/src/mbgl/style/expression/parsing_context.cpp
@@ -206,8 +206,16 @@ ParseResult ParsingContext::parse(const Convertible& value, TypeAnnotationOption
         }
     }
 
-    // if this is the root expression, enforce constraints on the use ["zoom"].
-    if (key.size() == 0 && !isZoomConstant(**parsed)) {
+    return parsed;
+}
+
+ParseResult ParsingContext::parseExpression(const Convertible& value, TypeAnnotationOption typeAnnotationOption) {
+    return parse(value, typeAnnotationOption);
+}
+
+ParseResult ParsingContext::parseLayerPropertyExpression(const Convertible& value, TypeAnnotationOption typeAnnotationOption) {
+    ParseResult parsed = parse(value, typeAnnotationOption);
+    if (parsed && !isZoomConstant(**parsed)) {
         optional<variant<const InterpolateBase*, const Step*, ParsingError>> zoomCurve = findZoomCurve(parsed->get());
         if (!zoomCurve) {
             error(R"("zoom" expression may only be used as input to a top-level "step" or "interpolate" expression.)");
@@ -217,10 +225,23 @@ ParseResult ParsingContext::parse(const Convertible& value, TypeAnnotationOption
             return ParseResult();
         }
     }
-
     return parsed;
 }
 
+const std::string ParsingContext::getCombinedErrors() const {
+    std::string combinedError;
+    for (const ParsingError& parsingError : *errors) {
+        if (combinedError.size() > 0) {
+            combinedError += "\n";
+        }
+        if (parsingError.key.size() > 0) {
+            combinedError += parsingError.key + ": ";
+        }
+        combinedError += parsingError.message;
+    }
+    return combinedError;
+}
+
 optional<std::string> ParsingContext::checkType(const type::Type& t) {
     assert(expected);
     optional<std::string> err = type::checkSubtype(*expected, t);
diff --git a/test/style/expression/expression.test.cpp b/test/style/expression/expression.test.cpp
index 694569695c..fe5c261be1 100644
--- a/test/style/expression/expression.test.cpp
+++ b/test/style/expression/expression.test.cpp
@@ -49,7 +49,7 @@ TEST_P(ExpressionEqualityTest, ExpressionEquality) {
         assert(!document.HasParseError());
         const JSValue* expression = &document;
         expression::ParsingContext ctx;
-        expression::ParseResult parsed = ctx.parse(conversion::Convertible(expression));
+        expression::ParseResult parsed = ctx.parseExpression(conversion::Convertible(expression));
         if (!parsed) {
             error_ = ctx.getErrors().size() > 0 ? ctx.getErrors()[0].message : "failed to parse";
         };
diff --git a/test/style/filter.test.cpp b/test/style/filter.test.cpp
index 6f261c43ec..49edcaef45 100644
--- a/test/style/filter.test.cpp
+++ b/test/style/filter.test.cpp
@@ -11,14 +11,19 @@
 using namespace mbgl;
 using namespace mbgl::style;
 
-bool filter(const char * json, const PropertyMap& featureProperties = {{}}, optional<FeatureIdentifier> featureId = {}, FeatureType featureType = FeatureType::Point, GeometryCollection featureGeometry = {}) {
+bool filter(const char * json,
+            const PropertyMap& featureProperties = {{}},
+            optional<FeatureIdentifier> featureId = {},
+            FeatureType featureType = FeatureType::Point,
+            GeometryCollection featureGeometry = {},
+            float zoom = 0.0f) {
     conversion::Error error;
     optional<Filter> filter = conversion::convertJSON<Filter>(json, error);
     EXPECT_TRUE(bool(filter));
     EXPECT_EQ(error.message, "");
     
     StubGeometryTileFeature feature { featureId, featureType, featureGeometry, featureProperties };
-    expression::EvaluationContext context = { &feature };
+    expression::EvaluationContext context = { zoom, &feature };
     
     return (*filter)(context);
 }
@@ -109,3 +114,8 @@ TEST(Filter, PropertyExpression) {
     ASSERT_TRUE(filter("[\"==\", [\"get\", \"two\"], 2]", {{"two", int64_t(2)}}));
     ASSERT_FALSE(filter("[\"==\", [\"get\", \"two\"], 4]", {{"two", int64_t(2)}}));
 }
+
+TEST(Filter, ZoomExpressionNested) {
+    ASSERT_TRUE(filter(R"(["==", ["get", "two"], ["zoom"]])", {{"two", int64_t(2)}}, {}, FeatureType::Point, {}, 2.0f));
+    ASSERT_FALSE(filter(R"(["==", ["get", "two"], ["+", ["zoom"], 1]])", {{"two", int64_t(2)}}, {}, FeatureType::Point, {}, 2.0f));
+}