Add migration to expressions into gl-style-migrate

[anandthakker]

Closes #6927

@mapbox/studio @mapbox/maps-design.

Updates the style-spec's old gl-style-migrate script to include conversion of legacy functions and filters to their expression equivalents. Note that because of some subtle differences in how legacy filters and expressions handle type mismatches, the converted expressions for complex filters (particularly "any" or "none" filters) may end up being overly complicated and under-performant. For styles that have those, it would be worth manually inspecting and revising the auto-generated expressions.

cd src/style-spec
npm run build
bin/gl-style-migrate YOUR_STYLE.json > OUTPUT.json

• briefly describe the changes in this PR
• write tests for all new functionality

[anandthakker]

The expressions system does not treat undefined as a distinct value, and so it has no way to mimic this aspect of GL JS's legacy filter implementation. Since this isn't even something that can be meaningfully represented in the GeoJSON spec, I'm inclined to just drop these assertions and consider filtering on properties like {foo: undefined} to be (ahem) undefined behavior.

[nickidlugash]

Excited to see this update!

Updates the style-spec's old gl-style-migrate script to include conversion of legacy functions and filters to their expression equivalents.

@anandthakker @jfirebaugh What is the official status now of tokens ({data-field}) for text-field and icon-image? Are they also considered legacy syntax, and if so, should they also get migrated to expressions?

Note that because of some subtle differences in how legacy filters and expressions handle type mismatches, the converted expressions for complex filters (particularly "any" or "none" filters) may end up being overly complicated and under-performant. For styles that have those, it would be worth manually inspecting and revising the auto-generated expressions.

Luckily, for our core styles we've almost completely avoided these, due to nesting restrictions in Studio's UI representation of filters 👍 I did a quick check of current core styles and can only find one example of an any filter – present in Mapbox Light and Mapbox Dark – but it could actually be rewritten as a single filter.

(This doesn't actually affect our future core style updates since we've manually migrated these to use expressions. However, I think this is relevant for the many customers who have created custom styles based on our core styles over the last few years and may be interested in auto-migrating those custom styles.)

[jfirebaugh]

The most difficult case for full-fidelity conversion is the default property for legacy functions. We couldn't think of a straightforward way to translate the semantics of default to expressions, and in native, we resorted to supporting it via a special case in the evaluator. Do you have any new ideas on that front?

[jfirebaugh]

Can you add a few sentences outlining the motivation for this particular approach as opposed to alternatives? I'm assuming it's to minimize the cases where typechecking is needed (only when "any" is used; other cases are fine to let error out [Are they? Will this produce annoying error messages?]).

[anandthakker]

I'm assuming it's to minimize the cases where typechecking is needed (only when "any" is used; other cases are fine to let error out [Are they? Will this produce annoying error messages?])

Yep, that was the reason. Other cases are fine to let error out, in the sense that they'll produce the correct behavior. It's true that they'll produce annoying warnings in that case. But this (and the other issues that have come up re: evaluation error warnings) makes me wonder whether it might make sense to simply disable these warnings (or some subset of them?) by default, only showing them if a map.expressionDebug property is set to true...

[anandthakker]

simply disable these warnings (or some subset of them?)

Another possibility: we currently only log each distinct evaluation error once, with that uniqueness scoped to each expression. If we expanded the uniqueness scope to the whole map, that would dramatically reduce the number of these warnings, while still giving developers a clue if they're trying to figure out why their style isn't doing what they want.

[jfirebaugh]

What is the official status now of tokens ({data-field}) for text-field and icon-image?

Tokens will continue to work, but they're about the same level of deprecated as legacy functions. @anandthakker, want to add conversion for literal string values? (Token strings in legacy function stops are already handled.)

[anandthakker]

We couldn't think of a straightforward way to translate the semantics of default to expressions, and in native, we resorted to supporting it via a special case in the evaluator. Do you have any new ideas on that front?

Ah, good point @jfirebaugh -- I just relied on the existing conversion implementation in https://github.com/mapbox/mapbox-gl-js/blob/855b850e62196c46f0987ab73600fffddc3e22bc/src/style-spec/function/convert.js, but that implementation doesn't fully handle legacy functions' default semantics. (Specifically, the expressions it produces don't fall back to the user-provided default when there's a type mismatch on the input property.) No new ideas as such--I'd say we could just introduce a similar approach as for filters, including type checks explicitly in the expression when they're needed.

[jfirebaugh]

The tricky case for defaults turns out to be arrays. For e.g. {"type": "identity", "property": "p", "default": [-1]} we want to convert to something like ["array", "number", ["get", "p"], ["literal", [-1]]. This poses two problems:

• Unlike other assertion operators, "array" doesn't currently accept fallbacks.
• It's not obvious how to add them due to how it is overloaded. With three arguments, the second argument is an expected length, but in this case we don't want to make assertions about the length. text-font doesn't have a fixed length, and legacy functions don't use the default value for an incorrect length on other array-valued properties. Supporting a null placeholder, i.e. ["array", "number", null, ["get", "p"], ["literal", [-1]], is an option, but I'm not fond of it.

The other missing piece is "interpolate-lab" and "interpolate-hcl" expression operators, so that colorSpace can be converted. (#5326)

[jfirebaugh]

Supporting a null placeholder, i.e. ["array", "number", null, ["get", "p"], ["literal", [-1]], is an option...

I ended up just doing this.

This is now ready for review.

[ChrisLoer]

I spent some time reading through this PR, enough I think to have a rough understanding, but it's too big and with parts I'm too unfamiliar with for me to give much detailed feedback. Are we OK relying on all the review @jfirebaugh and @anandthakker already did together?

The "use 'null' to indicate a fallback argument for 'array'" hack seems... awkward... but I guess reasonable for handling the "default value" case. It doesn't look like we've documented it: is our hope that users will never even discover it?

[jfirebaugh]

Yeah, that's kind of my hope. If I was redesigning type assertions I'd do it like ["assert", {"type": "array", "element-type": "number"}, input, fallbacks...] or something like that, but it's too late to change now.

[jfirebaugh]

FYI @mapbox/studio @mapbox/maps-design (who got de-tagged ☝️in the description due to Anand's departure) -- this merged. Please try it out and let us know if you notice any issues with the migration.