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 all 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: 1 addition & 1 deletion mapbox-gl-js
Submodule mapbox-gl-js updated 54 files
+1 −0 .eslintrc
+2 −0 .gitignore
+2 −4 docs/components/expression-metadata.js
+2 −2 docs/pages/api.js
+1 −1 docs/pages/style-spec.js
+1 −1 package.json
+6 −1 src/data/feature_index.js
+1 −1 src/shaders/index.js
+4 −0 src/style-spec/expression/compound_expression.js
+16 −0 src/style-spec/expression/definitions/array.js
+4 −0 src/style-spec/expression/definitions/assertion.js
+4 −0 src/style-spec/expression/definitions/at.js
+6 −0 src/style-spec/expression/definitions/case.js
+6 −0 src/style-spec/expression/definitions/coalesce.js
+6 −0 src/style-spec/expression/definitions/coercion.js
+8 −3 src/style-spec/expression/definitions/equals.js
+25 −0 src/style-spec/expression/definitions/interpolate.js
+6 −0 src/style-spec/expression/definitions/length.js
+10 −4 src/style-spec/expression/definitions/let.js
+16 −0 src/style-spec/expression/definitions/literal.js
+39 −0 src/style-spec/expression/definitions/match.js
+11 −0 src/style-spec/expression/definitions/step.js
+10 −4 src/style-spec/expression/definitions/var.js
+0 −15 src/style-spec/expression/evaluation_context.js
+4 −0 src/style-spec/expression/expression.js
+37 −19 src/style-spec/expression/parsing_context.js
+1 −1 test/build/browserify-test-fixture.js
+3 −4 test/build/min.test.js
+3 −0 test/build/style-spec.test.js
+58 −53 test/expression.test.js
+1 −0 test/integration/.eslintrc
+1 −1 test/integration/expression-tests/acos/basic/test.json
+1 −1 test/integration/expression-tests/asin/basic/test.json
+15 −0 test/integration/expression-tests/constant-folding/to-color-inferred/test.json
+21 −0 test/integration/expression-tests/constant-folding/var/test.json
+1 −2 test/integration/expression-tests/length/invalid/test.json
+6 −6 test/integration/expression-tests/let/basic/test.json
+4 −4 test/integration/expression-tests/let/nested/test.json
+15 −5 test/integration/expression-tests/let/shadow/test.json
+6 −6 test/integration/expression-tests/let/zoom/test.json
+3 −1 test/integration/expression-tests/object/implicit/test.json
+6 −3 test/integration/expression-tests/result_item.html.tmpl
+1 −3 test/integration/expression-tests/to-rgba/zero/test.json
+7 −5 test/integration/expression-tests/zoom/nested-let/test.json
+32 −31 test/integration/lib/expression.js
+24 −50 test/integration/query-tests/edge-cases/box-cutting-antimeridian-z0/expected.json
+2 −35 test/integration/query-tests/edge-cases/null-island/expected.json
+4 −0 test/integration/render-tests/regressions/mapbox-gl-native#10849/style.json
+3 −0 test/integration/results.html.tmpl
+1 −1 test/query.test.js
+1 −1 test/render.test.js
+1 −1 test/stub_loader.js
+1 −1 test/suite_implementation.js
+1 −1 test/unit/ui/control/geolocate.test.js
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