Fix style parsing bug for constant expressions #11606
Conversation
| } else { | ||
| // we didn't manage to fold to a literal during parsing, so evaluate it now | ||
| EvaluationContext params(nullptr); | ||
| EvaluationResult evaluated((*expression)->evaluate(params)); |
There was a problem hiding this comment.
Does this need to check for heatmap-density or other computed properties?
There was a problem hiding this comment.
Ah, good question -- heatmap-density is used only in heatmap-color, which isn't data-driven and so isn't parsed as a DataDrivenPropertyValue. Rather, it actually has its own wrapper HeatmapColorPropertyValue
There was a problem hiding this comment.
Wouldn't it be better to fix this by eliminating the edge cases that cause ParsingContext::parse not to evaluate to a literal when it could, rather than adding another place where constant expression evaluation can happen?
I don't really understand the "to-color" example in #10849 (comment). Isn't adding the coercion and then checking isConstant the correct order?
For the let/var example, it seems like isConstant should follow vars.
Yeah, definitely - I figured I'd submit this narrow fix (since it's very straightforward) and then follow up with the "right" one here and in GL JS.
Yes, this is the correct order. The bug is that we incorrectly assume an expression is constant only if its immediate children are all literals. In spirit, this assumption makes sense, since any constant child expression should have been collapsed to a constant when it was parsed. But in the case where an inferred type assertion/coercion has been inserted, this assumption fails to be true, and so we don't fold it to a literal. |
|
The more complete fix @jfirebaugh suggested wasn't as involved as I'd feared -- pushed it in 986d659 but now need to update some expression tests that no longer test the intended expression (because it gets folded away 😂 ) and add tests for the edge cases that this covers. |
Closes #10849