Expression evaluate issue

[slaviczavik]

Describe the bug
Valid expression syntax does not select all objects and throws a warning.

To Reproduce
This simple example shows two layers:

first layer with red circles has applied this filter with deprecated syntax:

[
  "any",
  [">", "level", 0],
  ["!has", "level"]
]

and second and the last layer with black circles has applied this filter:

[
  "any",
  [">", ["get", "level"], 0],
  ["!", ["has", "level"]]
]

These filters should be evaluated equally and should select same objects, but as you can see, the second filter selects just few objects and it throws this warning: Expected value to be of type number, but found null instead.

Expected behavior
The expression syntax should select same objects as the deprecated syntax.

Screenshots

Red circles are those objects with not defined level feature key and should be selected via the ["!", ["has", "level"]] expression.

Desktop

• OS: macOS 14.2.1
• Browser: Chrome
• Version: 120.0.6099.216

Additional context
For some reason the ! is not applied and only objects with defined level feature key are selected.

Important: if the negation expression is placed before the > operator it works as expected. But I remember that this order hadn't worked few months ago, but I cannot remember what version it was.

[HarelM]

Thanks for taking the time to report this issue!
Any chance you can simplify the example and have it all "inline" in the same page (i.e. not link to an external style URL)?
It will make things much easier to reason with.
Maybe even create two layer with different color and alpha to show the overlap of the filter?

[slaviczavik]

Thanks for taking the time to report this issue! Any chance you can simplify the example and have it all "inline" in the same page (i.e. not link to an external style URL)? It will make things much easier to reason with. Maybe even create two layer with different color and alpha to show the overlap of the filter?

I have updated the example – style is now included inside and it is possible to play with it. I have also tried to apply some style to see how it is overlapped, it should be clearer that black circles have red circles beneath.

[HarelM]

If I change the order I think I get the same results, can you verify it?

I'm not saying it's not a bug, I'm just surprised about the results...

[HarelM]

If you'd like to look into it, there are elaborated expression tests in this repo and you should be able to create some tests for this scenario and test it to see if the behavior, once a test is in place I think it might be simple to fix it.

[slaviczavik]

If I change the order I think I get the same results, can you verify it?

I'm not saying it's not a bug, I'm just surprised about the results...

This order indeed works as expected, but it hadn't worked few months ago.

[neodescis]

My initial guess is that the "deprecated" expression parser is simply being less strict about type checking. I think in both examples it would be preferable to swap the order to check if the value exists first.

[slaviczavik]

My initial guess is that the "deprecated" expression parser is simply being less strict about type checking. I think in both examples it would be preferable to swap the order to check if the value exists first.

Is there a specific reason why it would be better to swap order if any operator is used?

[slaviczavik]

If you'd like to look into it, there are elaborated expression tests in this repo and you should be able to create some tests for this scenario and test it to see if the behavior, once a test is in place I think it might be simple to fix it.

I have just tested the expression with following test and it pass as expected (same result for flipped order):

{
  "expression": [
    "any",
    [">", ["get", "level"], 0],
    ["!", ["has", "level"]]
  ],
  "inputs": [
    [{}, {"properties": {"level": 5}}],
    [{}, {"properties": {"level": 0}}],
    [{}, {"properties": {"level": -5}}],
    [{}, {"properties": {"level": null}}]
  ],
  "expected": {
    "compiled": {
      "result": "success",
      "isFeatureConstant": false,
      "isZoomConstant": true,
      "type": "boolean"
    },
    "outputs": [
      true,
      false,
      false,
      {"error":"Expected value to be of type number, but found null instead."}
    ]
  }
}

but I'm not sure what should be the fourth output, for the > operator it make sense to yell the warning, but not for the ! operator.

[neodescis]

My initial guess is that the "deprecated" expression parser is simply being less strict about type checking. I think in both examples it would be preferable to swap the order to check if the value exists first.

Is there a specific reason why it would be better to swap order if any operator is used?

I haven't verified this, but I would expect "any" to use short-circuit evaluation. Basically, no need to keep running through all the checks if only one needs to pass.

[slaviczavik]

My initial guess is that the "deprecated" expression parser is simply being less strict about type checking. I think in both examples it would be preferable to swap the order to check if the value exists first.

Is there a specific reason why it would be better to swap order if any operator is used?

I haven't verified this, but I would expect "any" to use short-circuit evaluation. Basically, no need to keep running through all the checks if only one needs to pass.

That's true, but first expression from the example with > is evaluated to false (because object doesn't contain the attribute) so it continue to the expression with ! and it should be evaluated to true.

But yes, it makes sense to put ! operator first, but validator should accept it even if the negation is not first.

[neodescis]

My initial guess is that the "deprecated" expression parser is simply being less strict about type checking. I think in both examples it would be preferable to swap the order to check if the value exists first.

Is there a specific reason why it would be better to swap order if any operator is used?

I haven't verified this, but I would expect "any" to use short-circuit evaluation. Basically, no need to keep running through all the checks if only one needs to pass.

That's true, but first expression from the example with > is evaluated to false (because object doesn't contain the attribute) so it continue to the expression with ! and it should be evaluated to true.

But yes, it makes sense to put ! operator first, but validator should accept it even if the negation is not first.

If the value is not there, then it is (apparently) comparing whether null > 0. I think it's perfectly acceptable for the validator to throw an error on this check. It is also documented as such: https://maplibre.org/maplibre-style-spec/expressions/#%3E

So, I think the bug is really that the deprecated syntax isn't throwing a validation error when it should be.