[core] Omit inferred type annotations for 'coalesce' argumentsupstream/fix-10588

4 files changed, 60 insertions, 35 deletions

diff --git a/include/mbgl/style/expression/parsing_context.hpp b/include/mbgl/style/expression/parsing_context.hpp
index e5c3d3359d..f92a4c95ea 100644
--- a/include/mbgl/style/expression/parsing_context.hpp
+++ b/include/mbgl/style/expression/parsing_context.hpp
@@ -68,20 +68,27 @@ public:
     optional<type::Type> getExpected() const { return expected; }
     const std::vector<ParsingError>& getErrors() const { return *errors; }
 
+    enum TypeAnnotationOption {
+        includeTypeAnnotations,
+        omitTypeAnnotations
+    };
+
     /*
         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);
-    
+    ParseResult parse(const mbgl::style::conversion::Convertible& value,
+                      TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations);
+
     /*
         Parse a child expression.
     */
     ParseResult parse(const mbgl::style::conversion::Convertible&,
                       std::size_t,
-                      optional<type::Type> = {});
+                      optional<type::Type> = {},
+                      TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations);
     
     /*
         Parse a child expression.
diff --git a/platform/node/test/ignores.json b/platform/node/test/ignores.json
index 06c4d4885d..7cb3560f4d 100644
--- a/platform/node/test/ignores.json
+++ b/platform/node/test/ignores.json
@@ -1,5 +1,4 @@
 {
-  "expression-tests/coalesce/inference": "https://github.com/mapbox/mapbox-gl-native/issues/10588",
   "query-tests/circle-stroke-width/inside": "https://github.com/mapbox/mapbox-gl-native/issues/10307",
   "query-tests/geometry/multilinestring": "needs investigation",
   "query-tests/geometry/multipolygon": "needs investigation",
diff --git a/src/mbgl/style/expression/coalesce.cpp b/src/mbgl/style/expression/coalesce.cpp
index bfde3c7581..0373c9626c 100644
--- a/src/mbgl/style/expression/coalesce.cpp
+++ b/src/mbgl/style/expression/coalesce.cpp
@@ -1,4 +1,5 @@
 #include <mbgl/style/expression/coalesce.hpp>
+#include <mbgl/style/expression/check_subtype.hpp>
 
 namespace mbgl {
 namespace style {
@@ -36,14 +37,15 @@ ParseResult Coalesce::parse(const Convertible& value, ParsingContext& ctx) {
     }
  
     optional<type::Type> outputType;
-    if (ctx.getExpected() && *ctx.getExpected() != type::Value) {
-        outputType = ctx.getExpected();
+    optional<type::Type> expectedType = ctx.getExpected();
+    if (expectedType && *expectedType != type::Value) {
+        outputType = expectedType;
     }
 
     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);
+        auto parsed = ctx.parse(arrayMember(value, i), i, outputType, ParsingContext::omitTypeAnnotations);
         if (!parsed) {
             return parsed;
         }
@@ -52,9 +54,19 @@ ParseResult Coalesce::parse(const Convertible& value, ParsingContext& ctx) {
         }
         args.push_back(std::move(*parsed));
     }
- 
     assert(outputType);
-    return ParseResult(std::make_unique<Coalesce>(*outputType, std::move(args)));
+
+    // Above, we parse arguments without inferred type annotation so that
+    // they don't produce a runtime error for `null` input, which would
+    // preempt the desired null-coalescing behavior.
+    // Thus, if any of our arguments would have needed an annotation, we
+    // need to wrap the enclosing coalesce expression with it instead.
+    bool needsAnnotation = expectedType &&
+        std::any_of(args.begin(), args.end(), [&] (const auto& arg) {
+            return type::checkSubtype(*expectedType, arg->getType());
+        });
+
+    return ParseResult(std::make_unique<Coalesce>(needsAnnotation ? type::Value : *outputType, std::move(args)));
 }
 
 } // namespace expression
diff --git a/src/mbgl/style/expression/parsing_context.cpp b/src/mbgl/style/expression/parsing_context.cpp
index acf3e63a48..0215982209 100644
--- a/src/mbgl/style/expression/parsing_context.cpp
+++ b/src/mbgl/style/expression/parsing_context.cpp
@@ -57,12 +57,15 @@ bool isConstant(const Expression& expression) {
 
 using namespace mbgl::style::conversion;
 
-ParseResult ParsingContext::parse(const Convertible& value, std::size_t index_, optional<type::Type> expected_) {
+ParseResult ParsingContext::parse(const Convertible& value,
+                                  std::size_t index_,
+                                  optional<type::Type> expected_,
+                                  TypeAnnotationOption typeAnnotationOption) {
     ParsingContext child(key + "[" + util::toString(index_) + "]",
                          errors,
                          std::move(expected_),
                          scope);
-    return child.parse(value);
+    return child.parse(value, typeAnnotationOption);
 }
 
 ParseResult ParsingContext::parse(const Convertible& value, std::size_t index_, optional<type::Type> expected_,
@@ -100,8 +103,7 @@ const ExpressionRegistry& getExpressionRegistry() {
     return registry;
 }
 
-ParseResult ParsingContext::parse(const Convertible& value)
-{
+ParseResult ParsingContext::parse(const Convertible& value, TypeAnnotationOption typeAnnotationOption) {
     ParseResult parsed;
     
     if (isArray(value)) {
@@ -131,39 +133,44 @@ ParseResult ParsingContext::parse(const Convertible& value)
     } else {
         parsed = Literal::parse(value, *this);
     }
-    
+
     if (!parsed) {
         assert(errors->size() > 0);
-    } else if (expected) {
-        auto wrapForType = [&](const type::Type& target, std::unique_ptr<Expression> expression) -> std::unique_ptr<Expression> {
+        return parsed;
+    }
+
+    if (expected) {
+        auto array = [&](std::unique_ptr<Expression> expression) {
             std::vector<std::unique_ptr<Expression>> args;
             args.push_back(std::move(expression));
-            if (target == type::Color) {
-                return std::make_unique<Coercion>(target, std::move(args));
-            } else {
-                return std::make_unique<Assertion>(target, std::move(args));
-            }
+            return args;
         };
-        
+
         const type::Type actual = (*parsed)->getType();
-        if (*expected == type::Color && (actual == type::String || actual == type::Value)) {
-            parsed = wrapForType(type::Color, std::move(*parsed));
+        if ((*expected == type::String || *expected == type::Number || *expected == type::Boolean) && actual == type::Value) {
+            if (typeAnnotationOption == includeTypeAnnotations) {
+                parsed = { std::make_unique<Assertion>(*expected, array(std::move(*parsed))) };
+            }
         } else if (expected->is<type::Array>() && actual == type::Value) {
-            parsed = { std::make_unique<ArrayAssertion>(expected->get<type::Array>(), std::move(*parsed)) };
-        } else if ((*expected == type::String || *expected == type::Number || *expected == type::Boolean) && actual == type::Value) {
-            parsed = wrapForType(*expected, std::move(*parsed));
-        }
-    
-        checkType((*parsed)->getType());
-        if (errors->size() > 0) {
-            return ParseResult();
+            if (typeAnnotationOption == includeTypeAnnotations) {
+                parsed = { std::make_unique<ArrayAssertion>(expected->get<type::Array>(), std::move(*parsed)) };
+            }
+        } else if (*expected == type::Color && (actual == type::Value || actual == type::String)) {
+            if (typeAnnotationOption == includeTypeAnnotations) {
+                parsed = { std::make_unique<Coercion>(*expected, array(std::move(*parsed))) };
+            }
+        } else {
+            checkType((*parsed)->getType());
+            if (errors->size() > 0) {
+                return ParseResult();
+            }
         }
     }
-    
+
     // If an expression's arguments are all literals, we can evaluate
     // it immediately and replace it with a literal value in the
     // parsed result.
-    if (parsed && !dynamic_cast<Literal *>(parsed->get()) && isConstant(**parsed)) {
+    if (!dynamic_cast<Literal *>(parsed->get()) && isConstant(**parsed)) {
         EvaluationContext params(nullptr);
         EvaluationResult evaluated((*parsed)->evaluate(params));
         if (!evaluated) {
@@ -185,7 +192,7 @@ ParseResult ParsingContext::parse(const Convertible& value)
     }
 
     // if this is the root expression, enforce constraints on the use ["zoom"].
-    if (key.size() == 0 && parsed && !isZoomConstant(**parsed)) {
+    if (key.size() == 0 && !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.)");