Port stricter zoom expression validation

Refs https://github.com/mapbox/mapbox-gl-js/pull/5611

10 files changed, 84 insertions, 48 deletions

diff --git a/include/mbgl/style/conversion/data_driven_property_value.hpp b/include/mbgl/style/conversion/data_driven_property_value.hpp
index 13e196a772..8880d28fb1 100644
--- a/include/mbgl/style/conversion/data_driven_property_value.hpp
+++ b/include/mbgl/style/conversion/data_driven_property_value.hpp
@@ -32,16 +32,9 @@ struct Converter<DataDrivenPropertyValue<T>> {
                 return {};
             }
             
-            bool zoomConstant = isZoomConstant(**expression);
-            
-            if (!zoomConstant && !findZoomCurve(expression->get())) {
-                error = { R"("zoom" expression may only be used as input to a top-level "step" or "interpolate" expression.)" };
-                return {};
-            }
-            
             if (isFeatureConstant(**expression)) {
                 return DataDrivenPropertyValue<T>(CameraFunction<T>(std::move(*expression)));
-            } else if (zoomConstant) {
+            } else if (isZoomConstant(**expression)) {
                 return DataDrivenPropertyValue<T>(SourceFunction<T>(std::move(*expression)));
             } else {
                 return DataDrivenPropertyValue<T>(CompositeFunction<T>(std::move(*expression)));
diff --git a/include/mbgl/style/conversion/property_value.hpp b/include/mbgl/style/conversion/property_value.hpp
index e65c110c71..97117de2ec 100644
--- a/include/mbgl/style/conversion/property_value.hpp
+++ b/include/mbgl/style/conversion/property_value.hpp
@@ -25,10 +25,6 @@ struct Converter<PropertyValue<T>> {
                 return {};
             }
             if (isFeatureConstant(**expression)) {
-                if (!isZoomConstant(**expression) && !findZoomCurve(expression->get())) {
-                    error = { R"("zoom" expression may only be used as input to a top-level "step" or "interpolate" expression.)" };
-                    return {};
-                }
                 return { CameraFunction<T>(std::move(*expression)) };
             } else {
                 error = { "property expressions not supported" };
diff --git a/include/mbgl/style/expression/find_zoom_curve.hpp b/include/mbgl/style/expression/find_zoom_curve.hpp
index e7dbfa1040..6301938033 100644
--- a/include/mbgl/style/expression/find_zoom_curve.hpp
+++ b/include/mbgl/style/expression/find_zoom_curve.hpp
@@ -1,5 +1,6 @@
 #pragma once
 
+#include <mbgl/style/expression/parsing_context.hpp>
 #include <mbgl/style/expression/interpolate.hpp>
 #include <mbgl/style/expression/step.hpp>
 
@@ -10,7 +11,9 @@ namespace mbgl {
 namespace style {
 namespace expression {
 
-optional<variant<InterpolateBase*, Step*>> findZoomCurve(expression::Expression* e);
+optional<variant<const InterpolateBase*, const Step*, ParsingError>> findZoomCurve(const expression::Expression* e);
+
+variant<const InterpolateBase*, const Step*> findZoomCurveChecked(const expression::Expression* e);
 
 } // namespace expression
 } // namespace style
diff --git a/include/mbgl/style/expression/interpolate.hpp b/include/mbgl/style/expression/interpolate.hpp
index b000633f5d..cb3bfe8b4a 100644
--- a/include/mbgl/style/expression/interpolate.hpp
+++ b/include/mbgl/style/expression/interpolate.hpp
@@ -73,8 +73,8 @@ public:
 
     void eachChild(const std::function<void(const Expression&)>& visit) const override {
         visit(*input);
-        for (auto it = stops.begin(); it != stops.end(); it++) {
-            visit(*(it->second));
+        for (const std::pair<const double, const std::unique_ptr<Expression>&>& stop : stops) {
+            visit(*stop.second);
         }
     }
     
diff --git a/include/mbgl/style/expression/parsing_context.hpp b/include/mbgl/style/expression/parsing_context.hpp
index 5127a476b1..65c5ebe188 100644
--- a/include/mbgl/style/expression/parsing_context.hpp
+++ b/include/mbgl/style/expression/parsing_context.hpp
@@ -18,6 +18,7 @@ class Expression;
 struct ParsingError {
     std::string message;
     std::string key;
+    bool operator==(const ParsingError& rhs) const { return message == rhs.message && key == rhs.key; }
 };
 
 using ParseResult = optional<std::unique_ptr<Expression>>;
diff --git a/include/mbgl/style/function/camera_function.hpp b/include/mbgl/style/function/camera_function.hpp
index 47b062558c..25b38e3616 100644
--- a/include/mbgl/style/function/camera_function.hpp
+++ b/include/mbgl/style/function/camera_function.hpp
@@ -28,8 +28,8 @@ public:
             IntervalStops<T>>>;
     
     CameraFunction(std::unique_ptr<expression::Expression> expression_)
-        : expression(std::move(expression_)),
-          zoomCurve(*expression::findZoomCurve(expression.get()))
+         : expression(std::move(expression_)),
+           zoomCurve(expression::findZoomCurveChecked(expression.get()))
     {
         assert(!expression::isZoomConstant(*expression));
         assert(expression::isFeatureConstant(*expression));
@@ -40,7 +40,7 @@ public:
           expression(stops.match([&] (const auto& s) {
             return expression::Convert::toExpression(s);
           })),
-          zoomCurve(*expression::findZoomCurve(expression.get()))
+          zoomCurve(expression::findZoomCurveChecked(expression.get()))
     {}
 
     T evaluate(float zoom) const {
@@ -54,10 +54,10 @@ public:
     
     float interpolationFactor(const Range<float>& inputLevels, const float inputValue) const {
         return zoomCurve.match(
-            [&](expression::InterpolateBase* z) {
+            [&](const expression::InterpolateBase* z) {
                 return z->interpolationFactor(Range<double> { inputLevels.min, inputLevels.max }, inputValue);
             },
-            [&](expression::Step*) { return 0.0f; }
+            [&](const expression::Step*) { return 0.0f; }
         );
     }
     
@@ -79,7 +79,7 @@ public:
 
 private:
     std::shared_ptr<expression::Expression> expression;
-    const variant<expression::InterpolateBase*, expression::Step*> zoomCurve;
+    const variant<const expression::InterpolateBase*, const expression::Step*> zoomCurve;
 };
 
 } // namespace style
diff --git a/include/mbgl/style/function/composite_function.hpp b/include/mbgl/style/function/composite_function.hpp
index cde1598e7d..b44bf8e6fe 100644
--- a/include/mbgl/style/function/composite_function.hpp
+++ b/include/mbgl/style/function/composite_function.hpp
@@ -52,7 +52,7 @@ public:
 
     CompositeFunction(std::unique_ptr<expression::Expression> expression_)
     :   expression(std::move(expression_)),
-        zoomCurve(*expression::findZoomCurve(expression.get()))
+        zoomCurve(expression::findZoomCurveChecked(expression.get()))
     {
         assert(!expression::isZoomConstant(*expression));
         assert(!expression::isFeatureConstant(*expression));
@@ -65,7 +65,7 @@ public:
         expression(stops.match([&] (const auto& s) {
             return expression::Convert::toExpression(property, s);
         })),
-        zoomCurve(*expression::findZoomCurve(expression.get()))
+        zoomCurve(expression::findZoomCurveChecked(expression.get()))
     {}
 
     // Return the range obtained by evaluating the function at each of the zoom levels in zoomRange
@@ -89,10 +89,10 @@ public:
     
     float interpolationFactor(const Range<float>& inputLevels, const float inputValue) const {
         return zoomCurve.match(
-            [&](expression::InterpolateBase* z) {
+            [&](const expression::InterpolateBase* z) {
                 return z->interpolationFactor(Range<double> { inputLevels.min, inputLevels.max }, inputValue);
             },
-            [&](expression::Step*) { return 0.0f; }
+            [&](const expression::Step*) { return 0.0f; }
         );
     }
     
@@ -114,7 +114,7 @@ public:
     
 private:
     std::shared_ptr<expression::Expression> expression;
-    const variant<expression::InterpolateBase*, expression::Step*> zoomCurve;
+    const variant<const expression::InterpolateBase*, const expression::Step*> zoomCurve;
 };
 
 } // namespace style
diff --git a/mapbox-gl-js b/mapbox-gl-js
-Subproject d8ad175f5e721bce705c6c0681047edaceb4bb4
+Subproject ff47116f248c323f6fa6f1b517c1c840a1b5ab6
diff --git a/src/mbgl/style/expression/find_zoom_curve.cpp b/src/mbgl/style/expression/find_zoom_curve.cpp
index 9ee8257777..5d39e0791e 100644
--- a/src/mbgl/style/expression/find_zoom_curve.cpp
+++ b/src/mbgl/style/expression/find_zoom_curve.cpp
@@ -10,36 +10,65 @@ namespace mbgl {
 namespace style {
 namespace expression {
 
-optional<variant<InterpolateBase*, Step*>> findZoomCurve(expression::Expression* e) {
-    if (auto curve = dynamic_cast<InterpolateBase*>(e)) {
+optional<variant<const InterpolateBase*, const Step*, ParsingError>> findZoomCurve(const expression::Expression* e) {
+    optional<variant<const InterpolateBase*, const Step*, ParsingError>> result;
+    
+    if (auto let = dynamic_cast<const Let*>(e)) {
+        result = findZoomCurve(let->getResult());
+    } else if (auto coalesce = dynamic_cast<const Coalesce*>(e)) {
+        std::size_t length = coalesce->getLength();
+        for (std::size_t i = 0; i < length; i++) {
+            result = findZoomCurve(coalesce->getChild(i));
+            if (result) {
+                break;
+            }
+        }
+    } else if (auto curve = dynamic_cast<const InterpolateBase*>(e)) {
         auto z = dynamic_cast<CompoundExpressionBase*>(curve->getInput().get());
         if (z && z->getName() == "zoom") {
-            return {curve};
-        } else {
-            return optional<variant<InterpolateBase*, Step*>>();
+            result = {curve};
         }
-    } else if (auto step = dynamic_cast<Step*>(e)) {
+    } else if (auto step = dynamic_cast<const Step*>(e)) {
         auto z = dynamic_cast<CompoundExpressionBase*>(step->getInput().get());
         if (z && z->getName() == "zoom") {
-            return {step};
-        } else {
-            return optional<variant<InterpolateBase*, Step*>>();
+            result = {step};
         }
-    } else if (auto let = dynamic_cast<Let*>(e)) {
-        return findZoomCurve(let->getResult());
-    } else if (auto coalesce = dynamic_cast<Coalesce*>(e)) {
-        std::size_t length = coalesce->getLength();
-        for (std::size_t i = 0; i < length; i++) {
-            optional<variant<InterpolateBase*, Step*>> childInterpolate = findZoomCurve(coalesce->getChild(i));
-            if (!childInterpolate) {
-                continue;
-            } else {
-                return childInterpolate;
+    }
+    
+    if (result && result->is<ParsingError>()) {
+        return result;
+    }
+    
+    e->eachChild([&](const Expression& child) {
+        optional<variant<const InterpolateBase*, const Step*, ParsingError>> childResult(findZoomCurve(&child));
+        if (childResult) {
+            if (childResult->is<ParsingError>()) {
+                result = childResult;
+            } else if (!result && childResult) {
+                result = {ParsingError {
+                    R"("zoom" expression may only be used as input to a top-level "step" or "interpolate" expression.)", ""
+                }};
+            } else if (result && childResult && result != childResult) {
+                result = {ParsingError {
+                    R"(Only one zoom-based "step" or "interpolate" subexpression may be used in an expression.)", ""
+                }};
             }
         }
-    }
+    });
 
-    return optional<variant<InterpolateBase*, Step*>>();
+    return result;
+}
+
+variant<const InterpolateBase*, const Step*> findZoomCurveChecked(const expression::Expression* e) {
+    return findZoomCurve(e)->match(
+        [](const ParsingError&) -> variant<const InterpolateBase*, const Step*> {
+            assert(false);
+            return {};
+        },
+        [](auto zoomCurve) -> variant<const InterpolateBase*, const Step*> {
+            return {std::move(zoomCurve)};
+        }
+    );
 }
 
 } // namespace expression
diff --git a/src/mbgl/style/expression/parsing_context.cpp b/src/mbgl/style/expression/parsing_context.cpp
index 4928edc92a..81cbdede59 100644
--- a/src/mbgl/style/expression/parsing_context.cpp
+++ b/src/mbgl/style/expression/parsing_context.cpp
@@ -19,6 +19,8 @@
 #include <mbgl/style/expression/match.hpp>
 #include <mbgl/style/expression/step.hpp>
 
+#include <mbgl/style/expression/find_zoom_curve.hpp>
+
 #include <mbgl/style/conversion/get_json_type.hpp>
 
 namespace mbgl {
@@ -174,7 +176,19 @@ ParseResult ParsingContext::parse(const Convertible& value)
             return ParseResult(std::make_unique<Literal>(*evaluated));
         }
     }
-    
+
+    // if this is the root expression, enforce constraints on the use ["zoom"].
+    if (key.size() == 0 && 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.)");
+            return ParseResult();
+        } else if (zoomCurve->is<ParsingError>()) {
+            error(zoomCurve->get<ParsingError>().message);
+            return ParseResult();
+        }
+    }
+
     return parsed;
 }