Address more review notes

5 files changed, 17 insertions, 33 deletions

diff --git a/include/mbgl/style/conversion/property_value.hpp b/include/mbgl/style/conversion/property_value.hpp
index e49861266b..d6c550ea03 100644
--- a/include/mbgl/style/conversion/property_value.hpp
+++ b/include/mbgl/style/conversion/property_value.hpp
@@ -8,6 +8,7 @@
 #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>
 
 namespace mbgl {
 namespace style {
@@ -24,6 +25,10 @@ struct Converter<PropertyValue<T>> {
                 return {};
             }
             if (isFeatureConstant(expression->get())) {
+                if (!isZoomConstant(expression->get()) && !findZoomCurve<typename CameraFunction<T>::ExpressionType>(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/src/mbgl/style/expression/at.cpp b/src/mbgl/style/expression/at.cpp
index 535ebee8a0..ffe7cae1c7 100644
--- a/src/mbgl/style/expression/at.cpp
+++ b/src/mbgl/style/expression/at.cpp
@@ -15,8 +15,8 @@ EvaluationResult At::evaluate(const EvaluationContext& params) const {
         return evaluatedInput.error();
     }
     
-    const auto i = *fromExpressionValue<double>(*evaluatedIndex);
-    const auto inputArray = *fromExpressionValue<std::vector<Value>>(*evaluatedInput);
+    const auto i = evaluatedIndex->get<double>();
+    const auto inputArray = evaluatedInput->get<std::vector<Value>>();
     
     if (i < 0 || i >= inputArray.size()) {
         return EvaluationError {
diff --git a/src/mbgl/style/expression/compound_expression.cpp b/src/mbgl/style/expression/compound_expression.cpp
index 65ece14328..c1e0639562 100644
--- a/src/mbgl/style/expression/compound_expression.cpp
+++ b/src/mbgl/style/expression/compound_expression.cpp
@@ -173,17 +173,6 @@ struct Signature<Lambda, std::enable_if_t<std::is_class<Lambda>::value>>
 using Definition = CompoundExpressionRegistry::Definition;
 
 template <typename T>
-Result<T> assertion(const Value& v) {
-    if (!v.is<T>()) {
-        return EvaluationError {
-            "Expected value to be of type " + toString(valueTypeToExpressionType<T>()) +
-            ", but found " + toString(typeOf(v)) + " instead."
-        };
-    }
-    return v.get<T>();
-}
-
-template <typename T>
 Result<bool> equal(const T& lhs, const T& rhs) { return lhs == rhs; }
 
 template <typename T>
@@ -205,10 +194,6 @@ std::unordered_map<std::string, CompoundExpressionRegistry::Definition> initiali
     define("ln2", []() -> Result<double> { return 0.6931471805599453; });
 
     define("typeof", [](const Value& v) -> Result<std::string> { return toString(typeOf(v)); });
-    define("number", assertion<double>);
-    define("string", assertion<std::string>);
-    define("boolean", assertion<bool>);
-    define("object", assertion<std::unordered_map<std::string, Value>>);
     
     define("to-string", [](const Value& value) -> Result<std::string> {
         return value.match(
diff --git a/src/mbgl/style/expression/step.cpp b/src/mbgl/style/expression/step.cpp
index 00bf9a4132..0ab67f8598 100644
--- a/src/mbgl/style/expression/step.cpp
+++ b/src/mbgl/style/expression/step.cpp
@@ -54,7 +54,7 @@ ParseResult Step::parse(const mbgl::style::conversion::Convertible& value, Parsi
         return ParseResult();
     }
     
-    // [step, input, first_output_value, 2 * (n pairs)...]
+    // [step, input, firstOutput_value, 2 * (n pairs)...]
     if ((length - 1) % 2 != 0) {
         ctx.error("Expected an even number of arguments.");
         return ParseResult();
@@ -75,20 +75,19 @@ ParseResult Step::parse(const mbgl::style::conversion::Convertible& value, Parsi
     
     // consume the first output value, which doesn't have a corresponding input value,
     // before proceeding into the "stops" loop below.
-    auto first_output = ctx.parse(arrayMember(value, 2), 2, outputType);
-    if (!first_output) {
+    auto firstOutput = ctx.parse(arrayMember(value, 2), 2, outputType);
+    if (!firstOutput) {
         return ParseResult();
     }
     if (!outputType) {
-        outputType = (*first_output)->getType();
+        outputType = (*firstOutput)->getType();
     }
-    stops.emplace(-std::numeric_limits<double>::infinity(), std::move(*first_output));
+    stops.emplace(-std::numeric_limits<double>::infinity(), std::move(*firstOutput));
     
     
     for (std::size_t i = 3; i + 1 < length; i += 2) {
         const optional<mbgl::Value> labelValue = toValue(arrayMember(value, i));
         optional<double> label;
-        optional<std::string> labelError;
         if (labelValue) {
             labelValue->match(
                 [&](uint64_t n) {
@@ -116,13 +115,11 @@ ParseResult Step::parse(const mbgl::style::conversion::Convertible& value, Parsi
             );
         }
         if (!label) {
-            ctx.error(labelError ? *labelError :
-                R"(Input/output pairs for "step" expressions must be defined using literal numeric values (not computed expressions) for the input values.)",
-                i);
+            ctx.error(R"(Input/output pairs for "step" expressions must be defined using literal numeric values (not computed expressions) for the input values.)", i);
             return ParseResult();
         }
         
-        if (*label < previous) {
+        if (*label <= previous) {
             ctx.error(
                 R"(Input/output pairs for "step" expressions must be arranged with input values in strictly ascending order.)",
                 i
diff --git a/src/mbgl/style/expression/value.cpp b/src/mbgl/style/expression/value.cpp
index 372d1db5e9..b75f471ce3 100644
--- a/src/mbgl/style/expression/value.cpp
+++ b/src/mbgl/style/expression/value.cpp
@@ -18,20 +18,17 @@ type::Type typeOf(const Value& value) {
             optional<type::Type> itemType;
             for (const auto& item : arr) {
                 const type::Type t = typeOf(item);
-                const std::string tname = type::toString(t);
                 if (!itemType) {
                     itemType = {t};
-                } else if (type::toString(*itemType) == tname) {
+                } else if (*itemType == t) {
                     continue;
                 } else {
                     itemType = {type::Value};
                     break;
                 }
             }
-            
-            if (!itemType) { itemType = {type::Value}; }
 
-            return type::Array(*itemType, arr.size());
+            return type::Array(itemType.value_or(type::Value), arr.size());
         }
     );
 }
@@ -56,7 +53,7 @@ void writeJSON(rapidjson::Writer<rapidjson::StringBuffer>& writer, const Value&
         [&] (const std::unordered_map<std::string, Value>& obj) {
             writer.StartObject();
             for(const auto& entry : obj) {
-                writer.String(entry.first);
+                writer.Key(entry.first.c_str());
                 writeJSON(writer, entry.second);
             }
             writer.EndObject();