From af9d3855f4cd6fdb118a0557296c0442cd26ead1 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 8 Jan 2018 14:46:44 -0800 Subject: [core] Omit inferred type annotations for 'coalesce' arguments --- include/mbgl/style/expression/parsing_context.hpp | 13 +++-- platform/node/test/ignores.json | 1 - src/mbgl/style/expression/coalesce.cpp | 22 +++++++-- src/mbgl/style/expression/parsing_context.cpp | 59 +++++++++++++---------- 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 getExpected() const { return expected; } const std::vector& 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 = {}); + optional = {}, + 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 +#include namespace mbgl { namespace style { @@ -36,14 +37,15 @@ ParseResult Coalesce::parse(const Convertible& value, ParsingContext& ctx) { } optional outputType; - if (ctx.getExpected() && *ctx.getExpected() != type::Value) { - outputType = ctx.getExpected(); + optional 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(*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(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 expected_) { +ParseResult ParsingContext::parse(const Convertible& value, + std::size_t index_, + optional 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 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) -> std::unique_ptr { + return parsed; + } + + if (expected) { + auto array = [&](std::unique_ptr expression) { std::vector> args; args.push_back(std::move(expression)); - if (target == type::Color) { - return std::make_unique(target, std::move(args)); - } else { - return std::make_unique(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(*expected, array(std::move(*parsed))) }; + } } else if (expected->is() && actual == type::Value) { - parsed = { std::make_unique(expected->get(), 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(expected->get(), std::move(*parsed)) }; + } + } else if (*expected == type::Color && (actual == type::Value || actual == type::String)) { + if (typeAnnotationOption == includeTypeAnnotations) { + parsed = { std::make_unique(*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(parsed->get()) && isConstant(**parsed)) { + if (!dynamic_cast(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> zoomCurve = findZoomCurve(parsed->get()); if (!zoomCurve) { error(R"("zoom" expression may only be used as input to a top-level "step" or "interpolate" expression.)"); -- cgit v1.2.1