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

Style parsing fails for constant expression values #10849

Closed
jfirebaugh opened this issue Jan 5, 2018 · 3 comments
Closed

Style parsing fails for constant expression values #10849

jfirebaugh opened this issue Jan 5, 2018 · 3 comments
Labels
Core The cross-platform C++ core, aka mbgl

Comments

@jfirebaugh
Copy link
Contributor

jfirebaugh commented Jan 5, 2018

Triggered by the following style:

{
        "version": 8,
        "layers": [{
            "id": "symbol",
            "type": "symbol",
            "source": "vector",
            "layout": {
              "text-font": ["array", "string", ["literal", ["Helvetica"]]]
            }
        }]
}

cc @anandthakker

@jfirebaugh jfirebaugh added this to the ios-v4.0.0 milestone Jan 6, 2018
@anandthakker anandthakker changed the title Assertion failed: (false), function operator(), src/mbgl/style/expression/find_zoom_curve.cpp, line 65. Style parsing fails for constant expression values Feb 22, 2018
@anandthakker
Copy link
Contributor

The issue here is that in Converter<DataDrivenPropertyValue<T>>, we're assuming that any expression value must be a camera, source, or composite expression.

anandthakker pushed a commit to mapbox/mapbox-gl-js that referenced this issue Feb 22, 2018
anandthakker added a commit that referenced this issue Feb 22, 2018
* Correctly parse constant expressions in dds style properties

Closes #10849

* Clarify
anandthakker pushed a commit to mapbox/mapbox-gl-js that referenced this issue Feb 22, 2018
anandthakker added a commit to mapbox/mapbox-gl-js that referenced this issue Feb 22, 2018
@kkaefer kkaefer added the Core The cross-platform C++ core, aka mbgl label Feb 23, 2018
@anandthakker
Copy link
Contributor

anandthakker commented Apr 5, 2018

#11282 did not fully fix this issue. The assertion in

if (featureConstant && !zoomConstant) {
return DataDrivenPropertyValue<T>(CameraFunction<T>(std::move(*expression)));
} else if (!featureConstant && zoomConstant) {
return DataDrivenPropertyValue<T>(SourceFunction<T>(std::move(*expression)));
} 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());

fails in the following cases:

  • ["to-color", ["get", "x", ["literal", {"x": "red"}]] -- because we add the to-color annotation before checking isConstant() (and isConstant assumes that all of its argument's children have already been folded to literals if possible)
  • ["let", "x", 1, ["var", "x"]] -- because we conservatively just bail out of isConstant when we see a var expression

The quick fix would be to replace the last three lines above with:

optional<T> constant;
if (auto literal = dynamic_cast<Literal*>(expression->get()) {
    // cool, it's pre-folded to a literal
    constant = fromExpressionValue<T>(literal->getValue());
} else {
    // we didn't manage to fold to a literal during parsing, so evaluate it now
    EvaluationContext params(nullptr);
    EvaluationResult evaluated((*parsed)->evaluate(params));
    assert(evaluated); // this assert really should work, since we've already checked above that the expression is zoom- & feature-constant.
    constant = fromExpressionValue<T>(*evaluated);
}

The fuller fix would be to fix constant folding during parsing to work during every case.

@anandthakker anandthakker reopened this Apr 5, 2018
anandthakker pushed a commit to mapbox/mapbox-gl-js that referenced this issue Apr 5, 2018
anandthakker pushed a commit that referenced this issue Apr 5, 2018
anandthakker pushed a commit to mapbox/mapbox-gl-js that referenced this issue Apr 6, 2018
anandthakker pushed a commit to mapbox/mapbox-gl-js that referenced this issue Apr 6, 2018
anandthakker added a commit to mapbox/mapbox-gl-js that referenced this issue Apr 9, 2018
* Add variations to constant expression test

Refs mapbox/mapbox-gl-native#10849

* Clean up ParsingContext#parse()

* Resolve "var" binding at parse, not runtime

This is simpler, is how native does it, and is helpful for constant
folding as it allows evaluating a descendant of "let" without needing a
context.

* Fix constant folding for var and inferred type annotations

* Fix let expression tests, add constant folding tests
anandthakker added a commit that referenced this issue Apr 9, 2018
* Fix style parsing bug for constant expressions

Closes #10849

* Ignore tests for unported GL JS change

Refs mapbox/mapbox-gl-js#6429

* Fuller fix

* Update mapbox-gl-js
@anandthakker
Copy link
Contributor

Fixed in #11606

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

No branches or pull requests

3 participants