groupByLayout erroneously groups two layers with different layout expressions

[jfirebaugh]

These two layers will be placed into the same group, even though they have different icon-size expressions:

  "layers": [{
    "id": "a",
    "type": "symbol",
    "source": "geojson",
    "layout": {
      "icon-image": "dot.sdf",
      "icon-allow-overlap": true,
      "icon-size": ["get", "sizeA"]
    },
    "paint": {
      "icon-color": "blue"
    }
  }, {
    "id": "b",
    "type": "symbol",
    "source": "geojson",
    "layout": {
      "icon-image": "dot.sdf",
      "icon-allow-overlap": true,
      "icon-size": ["get", "sizeB"]
    },
    "paint": {
      "icon-color": "red"
    }
  }]

This is because groupByLayout stringifies layout values, and the stringification code hasn't been updated for expressions. I noticed it while working on #11247.

We can switch over to stringifying the expression, which is possibly the result of converting an old-style function. However, this leaves an edge case: we don't include the default value in the conversion logic (it's handled during evaluation instead). So two old-style functions that were alike in everything except default would be treated as if they were identical for the purposes of grouping layers.

cc @anandthakker

[anandthakker]

@jfirebaugh this is a bit hacky, but we could handle the edge case by wrapping the converted function expression with ["coalesce", converted, defaultValue] -- though one problem with that would be that we might well at some point statically optimize coalesce by truncating it down to the last non-Value-typed output, and then we'd be back where we started.

Another option is to translate old-style functions that include an explicit default value as ["case", ["has", property], converted, defaultValue]

[jfirebaugh]

There are four cases where default is used, enumerated in the documentation:

• In categorical functions, when the feature value does not match any of the stop domain values. For source categorical functions, this case can be handled with the existing "match" syntax. For composite categorical functions, it can't be, because the default value needs to be the result value for the entire composite expression, not just the inner "match" expression, and we can't wrap the whole thing with additional expressions because of the limitations on where "zoom" can appear.
• In property and zoom-and-property functions, when a feature does not contain a value for the specified property. Same issue as the first case.
• In identity functions, when the feature value is not valid for the style property (for example, if the function is being used for a circle-color property but the feature property value is not a string or not a valid color). This case can be handled by assertion fallbacks.
• In interval or exponential property and zoom-and-property functions, when the feature value is not numeric. Same issue as the first case.

The complexity of converting default to expression syntax is why we opted to handle it during evaluation in the first place.

[anandthakker]

Oy, yeah I forgot about the "zoom" constraint -- we could still do the wrapping at the nested property-expression level if we really wanted to. (I wasn't meaning to propose that we try modeling the entire old-style function behavior in the expression--as you say, that's exactly what we bailed on before--only that we sneak the default value somewhere into the converted expression so that it would be present in the serialized value.)

[jfirebaugh]

#11452 simply ignores the edge case. It seems unlikely to matter in practice.