Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Fix style parsing bug for constant expressions #11606

Merged
merged 4 commits into from
Apr 9, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions include/mbgl/style/conversion/data_driven_property_value.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ struct Converter<DataDrivenPropertyValue<T>> {
} else if (!featureConstant && !zoomConstant) {
return DataDrivenPropertyValue<T>(CompositeFunction<T>(std::move(*expression)));
} else {
// If an expression is neither zoom- nor feature-dependent, it
// should have been reduced to a Literal when it was parsed.
auto literal = dynamic_cast<Literal*>(expression->get());
assert(literal);
optional<T> constant = fromExpressionValue<T>(literal->getValue());
Expand Down
3 changes: 3 additions & 0 deletions include/mbgl/style/expression/let.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ class Var : public Expression {

mbgl::Value serialize() const override;
std::string getOperator() const override { return "var"; }

const std::shared_ptr<Expression>& getBoundExpression() const { return value; }

private:
std::string name;
std::shared_ptr<Expression> value;
Expand Down
2 changes: 2 additions & 0 deletions platform/node/test/ignores.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"query-tests/circle-pitch-scale/viewport-inside-align-map": "https://github.com/mapbox/mapbox-gl-native/issues/10615",
"query-tests/circle-pitch-scale/viewport-inside-align-viewport": "https://github.com/mapbox/mapbox-gl-native/issues/10615",
"query-tests/edge-cases/box-cutting-antimeridian-z0": "https://github.com/mapbox/mapbox-gl-native/issues/11607",
"query-tests/edge-cases/null-island": "https://github.com/mapbox/mapbox-gl-native/issues/11607",
"query-tests/geometry/multilinestring": "needs investigation",
"query-tests/geometry/multipolygon": "needs investigation",
"query-tests/geometry/polygon": "needs investigation",
Expand Down
41 changes: 27 additions & 14 deletions src/mbgl/style/expression/parsing_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,23 +32,36 @@ namespace style {
namespace expression {

bool isConstant(const Expression& expression) {
if (dynamic_cast<const Var*>(&expression)) {
return false;
if (auto varExpression = dynamic_cast<const Var*>(&expression)) {
return isConstant(*varExpression->getBoundExpression());
}

if (auto compound = dynamic_cast<const CompoundExpressionBase*>(&expression)) {
if (compound->getName() == "error") {
return false;
}
}

bool isTypeAnnotation = dynamic_cast<const Coercion*>(&expression) ||
dynamic_cast<const Assertion*>(&expression) ||
dynamic_cast<const ArrayAssertion*>(&expression);

bool literalArgs = true;
bool childrenConstant = true;
expression.eachChild([&](const Expression& child) {
if (!dynamic_cast<const Literal*>(&child)) {
literalArgs = false;
// We can _almost_ assume that if `expressions` children are constant,
// they would already have been evaluated to Literal values when they
// were parsed. Type annotations are the exception, because they might
// have been inferred and added after a child was parsed.

// So we recurse into isConstant() for the children of type annotations,
// but otherwise simply check whether they are Literals.
if (isTypeAnnotation) {
childrenConstant = childrenConstant && isConstant(child);
} else {
childrenConstant = childrenConstant && dynamic_cast<const Literal*>(&child);
}
});
if (!literalArgs) {
if (!childrenConstant) {
return false;
}

Expand Down Expand Up @@ -141,13 +154,13 @@ ParseResult ParsingContext::parse(const Convertible& value, TypeAnnotationOption
return parsed;
}

if (expected) {
auto array = [&](std::unique_ptr<Expression> expression) {
std::vector<std::unique_ptr<Expression>> args;
args.push_back(std::move(expression));
return args;
};
auto array = [&](std::unique_ptr<Expression> expression) {
std::vector<std::unique_ptr<Expression>> args;
args.push_back(std::move(expression));
return args;
};

if (expected) {
const type::Type actual = (*parsed)->getType();
if ((*expected == type::String || *expected == type::Number || *expected == type::Boolean || *expected == type::Object) && actual == type::Value) {
if (typeAnnotationOption == includeTypeAnnotations) {
Expand All @@ -169,7 +182,7 @@ ParseResult ParsingContext::parse(const Convertible& value, TypeAnnotationOption
}
}

// If an expression's arguments are all literals, we can evaluate
// If an expression's arguments are all constant, we can evaluate
// it immediately and replace it with a literal value in the
// parsed result.
if (!dynamic_cast<Literal *>(parsed->get()) && isConstant(**parsed)) {
Expand Down