Avoid using 'coalesce' for filter comparison operators

Changes treatment of 'undefined' property values: we now treat them as equivalent to `null` -- i.e. `[==, 'foo', null]` now returns true for `{foo: undefined}`

src/style-spec/feature_filter/index.js

@@ -2,6 +2,7 @@
 
 const compileExpression = require('../function/compile');
 const {BooleanType} = require('../function/types');
+const {typeOf} = require('../function/values');
 
 import type {Feature} from '../function';
 export type FeatureFilter = (globalProperties: {+zoom?: number}, feature: VectorTileFeature) => boolean;
@@ -19,23 +20,36 @@ module.exports = createFilter;
  */
 function createFilter(filter: any): FeatureFilter {
     if (!filter) {
-        return (globalProperties, _: VectorTileFeature) => true;
+        return () => true;
     }
 
     let expression = Array.isArray(filter) ? convertFilter(filter) : filter.expression;
-    if (Array.isArray(expression) && expression[0] !== 'coalesce') {
-        expression = ['coalesce', expression, false];
+    let fallback = false;
+
+    // unwrap 'coalesce' if it's the outermost expression
+    if (
+        Array.isArray(expression) &&
+        expression.length === 3 &&
+        expression[0] === 'coalesce' &&
+        typeof expression[2] === 'boolean'
+    ) {
+        fallback = expression[2];
+        expression = expression[1];
     }
+
     const compiled = compileExpression(expression, BooleanType);
 
     if (compiled.result === 'success') {
         return (globalProperties, feature: VectorTileFeature) => {
-            const expressionFeature: Feature = {
-                properties: feature.properties || {},
-                type: feature.type,
-                id: typeof feature.id !== 'undefined' ? feature.id : null
-            };
-            return compiled.function(globalProperties, expressionFeature);
+            try {
+                const result = compiled.function(globalProperties, {
+                    properties: feature.properties || {},
+                    type: feature.type,
+                    id: typeof feature.id !== 'undefined' ? feature.id : null
+                });
+                if (result !== null) return result;
+            } catch (e) { return fallback; }
+            return fallback;
         };
     } else {
         throw new Error(compiled.errors.map(err => `${err.key}: ${err.message}`).join(', '));
@@ -53,9 +67,9 @@ function convertFilter(filter: ?Array<any>): mixed {
         op === '>' ||
         op === '<=' ||
         op === '>=' ? compileComparisonOp(filter[1], filter[2], op) :
-        op === 'any' ? compileLogicalOp(filter.slice(1), '||') :
-        op === 'all' ? compileLogicalOp(filter.slice(1), '&&') :
-        op === 'none' ? compileNegation(compileLogicalOp(filter.slice(1), '||')) :
+        op === 'any' ? compileDisjunctionOp(filter.slice(1)) :
+        op === 'all' ? ['&&'].concat(filter.slice(1).map(convertFilter)) :
+        op === 'none' ? ['&&'].concat(filter.slice(1).map(convertFilter).map(compileNegation)) :
         op === 'in' ? compileInOp(filter[1], filter.slice(2)) :
         op === '!in' ? compileNegation(compileInOp(filter[1], filter.slice(2))) :
         op === 'has' ? compileHasOp(filter[1]) :
@@ -71,20 +85,30 @@ function compilePropertyReference(property: string, type?: ?string) {
 }
 
 function compileComparisonOp(property: string, value: any, op: string) {
-    const fallback = op === '!=';
+    let compare;
     if (value === null) {
+        compare = [op, ['typeof', compilePropertyReference(property)], 'Null'];
+    } else {
+        const ref = compilePropertyReference(property, typeof value);
+        compare = [op, ref, value];
+    }
+
+    if (op === '!=') {
+        const ref = compilePropertyReference(property);
+        const type = typeOf(value).kind;
         return [
-            'coalesce',
-            [op, ['typeof', compilePropertyReference(property)], 'Null'],
-            fallback
+            '||',
+            ['!', compileHasOp(property)],
+            ['!=', ['typeof', ref], type],
+            compare
         ];
+    } else {
+        return compare;
     }
-    const ref = compilePropertyReference(property, typeof value);
-    return ['coalesce', [op, ref, value], fallback];
 }
 
-function compileLogicalOp(expressions: Array<Array<any>>, op: string) {
-    return [op].concat(expressions.map(convertFilter));
+function compileDisjunctionOp(filters: Array<Array<any>>) {
+    return ['||'].concat(filters.map(convertFilter).map(compiled => ['coalesce', compiled, false]));
 }
 
 function compileInOp(property: string, values: Array<any>) {
@@ -93,17 +117,17 @@ function compileInOp(property: string, values: Array<any>) {
     }
 
     const input = compilePropertyReference(property);
-    return ["coalesce", ["contains", input, ["array", ["literal", values]]], false];
+    return ["contains", input, ["array", ["literal", values]]];
 }
 
 function compileHasOp(property: string) {
-    const has = property === '$id' ?
-        ['!=', ['typeof', ['id']], 'Null'] :
+    const has = property === '$id' ?  ['!=', ['typeof', ['id']], 'Null'] :
+        property === '$type' ? true :
         ['has', property];
     return has;
 }
 
-function compileNegation(filter: boolean | Array<any>) {
-    return ['!', filter];
+function compileNegation(filter: mixed) {
+    return ['coalesce', ['!', filter], true];
 }
 

src/style-spec/function/evaluation_context.js

@@ -94,7 +94,7 @@ module.exports = () => ({
     },
 
     typeOf: function (x: Value): string {
-        return toString(typeOf(x));
+        return toString(typeOf(typeof x === 'undefined' ? null : x));
     },
 
     as: function (value: Value, expectedType: Type, name?: string) {

test/unit/style-spec/feature_filter.test.js

@@ -86,7 +86,7 @@ test('==, null', (t) => {
     t.equal(f({zoom: 0}, {properties: {foo: true}}), false);
     t.equal(f({zoom: 0}, {properties: {foo: false}}), false);
     t.equal(f({zoom: 0}, {properties: {foo: null}}), true);
-    t.equal(f({zoom: 0}, {properties: {foo: undefined}}), false);
+    t.equal(f({zoom: 0}, {properties: {foo: undefined}}), true);
     t.equal(f({zoom: 0}, {properties: {}}), false);
     t.end();
 });
@@ -136,7 +136,7 @@ test('!=, null', (t) => {
     t.equal(f({zoom: 0}, {properties: {foo: true}}), true);
     t.equal(f({zoom: 0}, {properties: {foo: false}}), true);
     t.equal(f({zoom: 0}, {properties: {foo: null}}), false);
-    t.equal(f({zoom: 0}, {properties: {foo: undefined}}), true);
+    t.equal(f({zoom: 0}, {properties: {foo: undefined}}), false);
     t.equal(f({zoom: 0}, {properties: {}}), true);
     t.end();
 });