Inconsistent behaviours of “coalesce” with “image”

[frodrigo]

According to the documentation, #8684 and #8052. “coalesce” should return the first existing “image”.

With last v1.9.1. Same behaviours on Firefox and Chrome.
https://js.do/code/432516

The style have icon for start_11 and oneway, but not for not_existing. Nevertheless, the flowing code return no icon.

    "icon-image": [
      "coalesce",
      ["image", "not_existing"],
      ["image", "star_11"]
    ],

The same code in text return not_existing:

    "text-field": [
      "string",
      [
        "coalesce",
        ["image", "not_existing"],
        ["image", "star_11"]
      ]
    ]

Playing more with coalesce give some “random” results

    "icon-image": [
      "coalesce",
      ["image", "not_existing"]
    ],
    "text-field": [
      "string",
      [
        "coalesce",
        ["image", "not_existing"]
      ]
    ]

Give unexpected not_existing (expected null)

    "icon-image": [
      "coalesce",
      ["image", "star_11"]
    ],
    "text-field": [
      "string",
      [
        "coalesce",
        ["image", "star_11"]
      ]
    ]

Give the star_11.

      "coalesce",
      ["image", "not_existing"],
      ["image", "star_11"],

Give unexpected not_existing (expected star_11)

      "coalesce",
      ["image", "star_11"],
      ["image", "not_existing"]

Give the star_11.

      "coalesce",
      ["image", "not_existing"],
      "star_11"

Give the star_11.

      "coalesce",
      ["image", "not_existing"],
      ["image", "star_11"],
      ["image", "oneway"]

Give unexpected not_existing (expected star_11)

      "coalesce",
      ["image", "not_existing"],
      ["image", "star_11"],
      "oneway"

Give unexpected oneway (expected star_11)

[arindam1993]

Capturing internal convo,

@ryanhamley found the issue. It's a regression introduced with https://github.com/mapbox/mapbox-gl-js/pull/9352/files#diff-0856eb2171400e111b6a65dde5e0c789R76

since the canonical param was introduced in the middle of the function signature, it seems like certain places that call it place the data in the canonical param instead of availableImages.

@zmiao would you please be able to look at this? 🙏

[ryanhamley]

@zmiao More specifically, this is where the bug reported in this demo is being caused. Changing this line to possiblyEvaluate(parameters, null, availableImages) fixes the immediate issue.

mapbox-gl-js/src/style/properties.js

Line 323 in cd7d14b

result._values[property] = this._values[property].possiblyEvaluate(parameters, availableImages);

This line looks like it has the same issue but I'm not exactly sure what conditions may trigger a bug here:

mapbox-gl-js/src/style/properties.js

Line 391 in cd7d14b

result._values[property] = this._values[property].possiblyEvaluate(parameters, availableImages);

I also see a similar signature here but again I'm not sure if/what bugs this can cause:

mapbox-gl-js/src/style/style_layer.js

Lines 201 to 205 in cd7d14b

	if (this._unevaluatedLayout) {
	(this: any).layout = this._unevaluatedLayout.possiblyEvaluate(parameters, availableImages);
	}
	(this: any).paint = this._transitioningPaint.possiblyEvaluate(parameters, availableImages);

I think basically anywhere that evaluate or possiblyEvaluate is called without the canonical parameter before availableImages, this could be an issue.

I'd guess this could also break formatted usage since it also comes after the canonical arg but I haven't searched for instances of that.

#9198 probably also is broken. See this function:

mapbox-gl-js/src/data/feature_index.js

Lines 292 to 297 in cd7d14b

	function evaluateProperties(serializedProperties, styleLayerProperties, feature, featureState, availableImages) {
	return mapObject(serializedProperties, (property, key) => {
	const prop = styleLayerProperties instanceof PossiblyEvaluated ? styleLayerProperties.get(key) : null;
	return prop && prop.evaluate ? prop.evaluate(feature, featureState, availableImages) : prop;
	});
	}

I'm not sure if it's better to just update a handful of signatures like this or if we should refactor canonical maybe to the end of the signature. This should have been caught in code review, but it was a large PR. I think maybe more unfortunate is that none of our tests caught this. The reason for that is that our expressions are tested in isolation rather than in a holistic manner within the map. This makes sense, but this bug shows that we have a blindspot in our testing.

[karimnaaji]

I'm not sure if it's better to just update a handful of signatures like this or if we should refactor canonical maybe to the end of the signature.

@ryanhamley Is there any way we could detect the code paths that require availableImages to be set? For example unwrap the parameters and assert if they are not complete in some of the above conditions?

I would also think that if these code paths worked before the regression without canonical we should be able to refactor and optionally add it at the end, availableImages does not seem to be optional. These are the cases that would be affected:

$ git grep "possiblyEvaluate(parameters,.*availableImages"
src/style/properties.js:        const finalValue = this.value.possiblyEvaluate(parameters, canonical, availableImages);
src/style/properties.js:            return prior.possiblyEvaluate(parameters, canonical, availableImages);
src/style/properties.js:            return this.property.interpolate(prior.possiblyEvaluate(parameters, canonical, availableImages), finalValue, easeCubicInOut(t));
src/style/properties.js:            result._values[property] = this._values[property].possiblyEvaluate(parameters, availableImages);
src/style/properties.js:            result._values[property] = this._values[property].possiblyEvaluate(parameters, availableImages);
src/style/style_layer.js:            (this: any).layout = this._unevaluatedLayout.possiblyEvaluate(parameters, availableImages);
src/style/style_layer.js:        (this: any).paint = this._transitioningPaint.possiblyEvaluate(parameters, availableImages);

[chloekraw]

If our current theory about the root cause is correct, the regression was introduced in 1.9.0, so this should be patched into 1.9.x as well. I've created a milestone to help track that.

[chloekraw]

Oh, nvm, an issue can't be on two milestones at once. 😛

[ryanhamley]

We have a fix for this issue in #9668. Just working on a test and fixing Flow. This issue also helped uncover a second bug which is also fixed by #9668!

[ryanhamley]

Closed by #9668