From 63e64dc85cd058f6342572debde87f97792b43d3 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Mon, 9 Jul 2018 10:25:59 -0400 Subject: [PATCH 1/7] Update wording for == expression type error --- src/style-spec/expression/definitions/equals.js | 2 +- .../expression-tests/array/implicit-2/test.json | 4 +--- .../expression-tests/array/implicit-3/test.json | 4 +--- .../expression-tests/at/infer-array-type/test.json | 4 +--- test/integration/expression-tests/case/basic/test.json | 4 +--- .../expression-tests/case/infer-array-type/test.json | 4 +--- test/integration/expression-tests/ceil/basic/test.json | 2 +- .../expression-tests/coalesce/infer-array-type/test.json | 4 +--- .../expression-tests/collator/accent-equals-de/test.json | 6 +++++- .../constant-folding/evaluation-error/test.json | 4 +--- .../integration/expression-tests/equal/mismatch/test.json | 4 +++- test/integration/expression-tests/floor/basic/test.json | 2 +- .../expression-tests/heatmap-density/basic/test.json | 4 +--- .../interpolate/infer-array-type/test.json | 4 +--- .../expression-tests/interpolate/linear/test.json | 4 +--- .../literal/infer-empty-array-type/test.json | 4 +--- test/integration/expression-tests/match/basic/test.json | 8 +------- .../expression-tests/match/infer-array-type/test.json | 4 +--- .../expression-tests/not_equal/mismatch/test.json | 4 +++- test/integration/expression-tests/round/basic/test.json | 2 +- .../typecheck/array-invalid-item/test.json | 4 +--- .../typecheck/array-item-subtyping/test.json | 4 +--- .../typecheck/array-length-subtyping--no-length/test.json | 4 +--- .../typecheck/array-length-subtyping/test.json | 4 +--- .../typecheck/array-wrong-length/test.json | 4 +--- test/integration/lib/expression.js | 2 ++ 26 files changed, 35 insertions(+), 65 deletions(-) diff --git a/src/style-spec/expression/definitions/equals.js b/src/style-spec/expression/definitions/equals.js index 374d6f916c2..3135a9e6bf4 100644 --- a/src/style-spec/expression/definitions/equals.js +++ b/src/style-spec/expression/definitions/equals.js @@ -56,7 +56,7 @@ function makeComparison(op: string, negate: boolean) { } if (lhs.type.kind !== rhs.type.kind && lhs.type.kind !== 'value' && rhs.type.kind !== 'value') { - return context.error(`Cannot compare ${toString(lhs.type)} and ${toString(rhs.type)}.`); + return context.error(`Cannot compare types '${toString(lhs.type)}' and '${toString(rhs.type)}'.`); } let collator = null; diff --git a/test/integration/expression-tests/array/implicit-2/test.json b/test/integration/expression-tests/array/implicit-2/test.json index e8e753f5c84..8c6fe029c57 100644 --- a/test/integration/expression-tests/array/implicit-2/test.json +++ b/test/integration/expression-tests/array/implicit-2/test.json @@ -3,9 +3,7 @@ "type": "array", "value": "string", "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": ["get", "array"], "inputs": [ diff --git a/test/integration/expression-tests/array/implicit-3/test.json b/test/integration/expression-tests/array/implicit-3/test.json index 2461d871701..2b095e411b8 100644 --- a/test/integration/expression-tests/array/implicit-3/test.json +++ b/test/integration/expression-tests/array/implicit-3/test.json @@ -4,9 +4,7 @@ "value": "number", "length": 2, "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": ["get", "array"], "inputs": [ diff --git a/test/integration/expression-tests/at/infer-array-type/test.json b/test/integration/expression-tests/at/infer-array-type/test.json index 40214772e52..e3e6c2ac1cc 100644 --- a/test/integration/expression-tests/at/infer-array-type/test.json +++ b/test/integration/expression-tests/at/infer-array-type/test.json @@ -2,9 +2,7 @@ "propertySpec": { "type": "string", "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": ["at", 1, ["literal", [1, 2, 3]]], "inputs": [], diff --git a/test/integration/expression-tests/case/basic/test.json b/test/integration/expression-tests/case/basic/test.json index 2e4c8472982..510e4eb391a 100644 --- a/test/integration/expression-tests/case/basic/test.json +++ b/test/integration/expression-tests/case/basic/test.json @@ -2,9 +2,7 @@ "propertySpec": { "type": "string", "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": ["case", ["get", "x"], "x", ["get", "y"], "y", "otherwise"], "inputs": [ diff --git a/test/integration/expression-tests/case/infer-array-type/test.json b/test/integration/expression-tests/case/infer-array-type/test.json index 046ff1defb3..9c5e3ec25f8 100644 --- a/test/integration/expression-tests/case/infer-array-type/test.json +++ b/test/integration/expression-tests/case/infer-array-type/test.json @@ -3,9 +3,7 @@ "type": "array", "value": "string", "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": [ "case", diff --git a/test/integration/expression-tests/ceil/basic/test.json b/test/integration/expression-tests/ceil/basic/test.json index 3b29833ac71..8fbee7c586f 100644 --- a/test/integration/expression-tests/ceil/basic/test.json +++ b/test/integration/expression-tests/ceil/basic/test.json @@ -17,7 +17,7 @@ "isZoomConstant": true, "type": "number" }, - "outputs": [ -2, -2, -2, -2, 3, 3, 3, 2 ], + "outputs": [-2, -2, -2, -2, 3, 3, 3, 2], "serialized": ["ceil", ["number", ["get", "x"]]] } } diff --git a/test/integration/expression-tests/coalesce/infer-array-type/test.json b/test/integration/expression-tests/coalesce/infer-array-type/test.json index 1270a45ae38..8661cffed26 100644 --- a/test/integration/expression-tests/coalesce/infer-array-type/test.json +++ b/test/integration/expression-tests/coalesce/infer-array-type/test.json @@ -3,9 +3,7 @@ "type": "array", "value": "string", "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": [ "coalesce", diff --git a/test/integration/expression-tests/collator/accent-equals-de/test.json b/test/integration/expression-tests/collator/accent-equals-de/test.json index 0dd5c163db0..e69ebb8943b 100644 --- a/test/integration/expression-tests/collator/accent-equals-de/test.json +++ b/test/integration/expression-tests/collator/accent-equals-de/test.json @@ -43,7 +43,11 @@ "resolved-locale", [ "collator", - {"case-sensitive": true, "diacritic-sensitive": false, "locale": "de"} + { + "case-sensitive": true, + "diacritic-sensitive": false, + "locale": "de" + } ] ], "de" diff --git a/test/integration/expression-tests/constant-folding/evaluation-error/test.json b/test/integration/expression-tests/constant-folding/evaluation-error/test.json index c64ad651ccb..1cde4dc3584 100644 --- a/test/integration/expression-tests/constant-folding/evaluation-error/test.json +++ b/test/integration/expression-tests/constant-folding/evaluation-error/test.json @@ -2,9 +2,7 @@ "propertySpec": { "type": "color", "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": ["step", ["get", "x"], "black", 0, "invalid", 10, "blue"], "inputs": [ diff --git a/test/integration/expression-tests/equal/mismatch/test.json b/test/integration/expression-tests/equal/mismatch/test.json index 02a5ce690bb..a1b44024485 100644 --- a/test/integration/expression-tests/equal/mismatch/test.json +++ b/test/integration/expression-tests/equal/mismatch/test.json @@ -3,7 +3,9 @@ "expected": { "compiled": { "result": "error", - "errors": [{"key": "", "error": "Cannot compare string and number."}] + "errors": [ + {"key": "", "error": "Cannot compare types 'string' and 'number'."} + ] } } } diff --git a/test/integration/expression-tests/floor/basic/test.json b/test/integration/expression-tests/floor/basic/test.json index c0febcc36fd..ad28186cec9 100644 --- a/test/integration/expression-tests/floor/basic/test.json +++ b/test/integration/expression-tests/floor/basic/test.json @@ -17,7 +17,7 @@ "isZoomConstant": true, "type": "number" }, - "outputs": [ -3, -3, -3, -2, 2, 2, 2, 2 ], + "outputs": [-3, -3, -3, -2, 2, 2, 2, 2], "serialized": ["floor", ["number", ["get", "x"]]] } } diff --git a/test/integration/expression-tests/heatmap-density/basic/test.json b/test/integration/expression-tests/heatmap-density/basic/test.json index ab9885b8061..f5f9b76f4aa 100644 --- a/test/integration/expression-tests/heatmap-density/basic/test.json +++ b/test/integration/expression-tests/heatmap-density/basic/test.json @@ -12,9 +12,7 @@ ] }, "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": [ "interpolate", diff --git a/test/integration/expression-tests/interpolate/infer-array-type/test.json b/test/integration/expression-tests/interpolate/infer-array-type/test.json index 1cd7e29a781..fa2b0ad1445 100644 --- a/test/integration/expression-tests/interpolate/infer-array-type/test.json +++ b/test/integration/expression-tests/interpolate/infer-array-type/test.json @@ -3,9 +3,7 @@ "type": "array", "value": "string", "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": [ "step", diff --git a/test/integration/expression-tests/interpolate/linear/test.json b/test/integration/expression-tests/interpolate/linear/test.json index 85797017172..e23ff29bfa4 100644 --- a/test/integration/expression-tests/interpolate/linear/test.json +++ b/test/integration/expression-tests/interpolate/linear/test.json @@ -2,9 +2,7 @@ "propertySpec": { "type": "number", "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": ["interpolate", ["linear"], ["get", "x"], 0, 100, 10, 200], "inputs": [ diff --git a/test/integration/expression-tests/literal/infer-empty-array-type/test.json b/test/integration/expression-tests/literal/infer-empty-array-type/test.json index 09c9e3dcb36..8ad21ba861c 100644 --- a/test/integration/expression-tests/literal/infer-empty-array-type/test.json +++ b/test/integration/expression-tests/literal/infer-empty-array-type/test.json @@ -3,9 +3,7 @@ "type": "array", "value": "number", "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": ["literal", []], "inputs": [], diff --git a/test/integration/expression-tests/match/basic/test.json b/test/integration/expression-tests/match/basic/test.json index 1178d0e80a6..266eacdb3b2 100644 --- a/test/integration/expression-tests/match/basic/test.json +++ b/test/integration/expression-tests/match/basic/test.json @@ -14,13 +14,7 @@ "isZoomConstant": true, "type": "string" }, - "outputs": [ - "Apple", - "Banana", - "Kumquat", - "Kumquat", - "Kumquat" - ], + "outputs": ["Apple", "Banana", "Kumquat", "Kumquat", "Kumquat"], "serialized": [ "match", ["get", "x"], diff --git a/test/integration/expression-tests/match/infer-array-type/test.json b/test/integration/expression-tests/match/infer-array-type/test.json index d5dea55e8f2..2b1041fd36a 100644 --- a/test/integration/expression-tests/match/infer-array-type/test.json +++ b/test/integration/expression-tests/match/infer-array-type/test.json @@ -3,9 +3,7 @@ "type": "array", "value": "string", "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": [ "match", diff --git a/test/integration/expression-tests/not_equal/mismatch/test.json b/test/integration/expression-tests/not_equal/mismatch/test.json index 568aa6bc23c..8a455e01bee 100644 --- a/test/integration/expression-tests/not_equal/mismatch/test.json +++ b/test/integration/expression-tests/not_equal/mismatch/test.json @@ -3,7 +3,9 @@ "expected": { "compiled": { "result": "error", - "errors": [{"key": "", "error": "Cannot compare string and number."}] + "errors": [ + {"key": "", "error": "Cannot compare types 'string' and 'number'."} + ] } } } diff --git a/test/integration/expression-tests/round/basic/test.json b/test/integration/expression-tests/round/basic/test.json index 7ce0d025b7a..5c6f4a6f45b 100644 --- a/test/integration/expression-tests/round/basic/test.json +++ b/test/integration/expression-tests/round/basic/test.json @@ -17,7 +17,7 @@ "isZoomConstant": true, "type": "number" }, - "outputs": [ -3, -3, -2, -2, 3, 3, 2, 2 ], + "outputs": [-3, -3, -2, -2, 3, 3, 2, 2], "serialized": ["round", ["number", ["get", "x"]]] } } diff --git a/test/integration/expression-tests/typecheck/array-invalid-item/test.json b/test/integration/expression-tests/typecheck/array-invalid-item/test.json index 6eeb381cee2..07c9f8f1701 100644 --- a/test/integration/expression-tests/typecheck/array-invalid-item/test.json +++ b/test/integration/expression-tests/typecheck/array-invalid-item/test.json @@ -4,9 +4,7 @@ "value": "string", "length": 2, "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": ["array", "number", 2, ["get", "x"]], "inputs": [], diff --git a/test/integration/expression-tests/typecheck/array-item-subtyping/test.json b/test/integration/expression-tests/typecheck/array-item-subtyping/test.json index 89e365f56d8..a2859348cc5 100644 --- a/test/integration/expression-tests/typecheck/array-item-subtyping/test.json +++ b/test/integration/expression-tests/typecheck/array-item-subtyping/test.json @@ -2,9 +2,7 @@ "propertySpec": { "type": "array", "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": ["array", "number", 2, ["get", "x"]], "inputs": [], diff --git a/test/integration/expression-tests/typecheck/array-length-subtyping--no-length/test.json b/test/integration/expression-tests/typecheck/array-length-subtyping--no-length/test.json index 309312db483..d180e38bac3 100644 --- a/test/integration/expression-tests/typecheck/array-length-subtyping--no-length/test.json +++ b/test/integration/expression-tests/typecheck/array-length-subtyping--no-length/test.json @@ -4,9 +4,7 @@ "value": "number", "length": 3, "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": ["array", "number", ["get", "x"]], "inputs": [], diff --git a/test/integration/expression-tests/typecheck/array-length-subtyping/test.json b/test/integration/expression-tests/typecheck/array-length-subtyping/test.json index 28b9a050aff..2600dec7e2d 100644 --- a/test/integration/expression-tests/typecheck/array-length-subtyping/test.json +++ b/test/integration/expression-tests/typecheck/array-length-subtyping/test.json @@ -3,9 +3,7 @@ "type": "array", "value": "string", "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": ["array", "string", 2, ["get", "x"]], "inputs": [], diff --git a/test/integration/expression-tests/typecheck/array-wrong-length/test.json b/test/integration/expression-tests/typecheck/array-wrong-length/test.json index 4d09e80bf3f..a5e08bd02d6 100644 --- a/test/integration/expression-tests/typecheck/array-wrong-length/test.json +++ b/test/integration/expression-tests/typecheck/array-wrong-length/test.json @@ -4,9 +4,7 @@ "value": "number", "length": 3, "property-type": "data-driven", - "expression": { - "parameters": ["zoom", "feature"] - } + "expression": {"parameters": ["zoom", "feature"]} }, "expression": ["array", "number", 2, ["get", "x"]], "inputs": [], diff --git a/test/integration/lib/expression.js b/test/integration/lib/expression.js index 9e0f530ad9c..4dd0d55bc4b 100644 --- a/test/integration/lib/expression.js +++ b/test/integration/lib/expression.js @@ -101,6 +101,8 @@ exports.run = function (implementation, options, runExpressionTest) { serialized: result.serialized }; + delete fixture.metadata; + fs.writeFile(path.join(dir, 'test.json'), `${stringify(fixture, null, 2)}\n`, done); return; } From 6967e5bec41d329cf39c4f11f2cd8ca150a8169e Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Mon, 9 Jul 2018 10:41:41 -0400 Subject: [PATCH 2/7] Allow ['==', value, value] --- docs/components/expression-metadata.js | 6 ++++ .../expression/definitions/equals.js | 31 +++++++++++-------- .../expression-tests/equal/array/test.json | 4 +-- .../expression-tests/equal/color/test.json | 4 +-- .../expression-tests/equal/object/test.json | 4 +-- .../expression-tests/equal/value/test.json | 19 ++++++------ .../not_equal/value/test.json | 19 ++++++------ 7 files changed, 50 insertions(+), 37 deletions(-) diff --git a/docs/components/expression-metadata.js b/docs/components/expression-metadata.js index 0084ad6c257..8a71281f2d3 100644 --- a/docs/components/expression-metadata.js +++ b/docs/components/expression-metadata.js @@ -51,6 +51,9 @@ const types = { }, { type: 'boolean', parameters: ['value', 'null'] + }, { + type: 'boolean', + parameters: ['value', 'value'] }], '!=': [{ type: 'boolean', @@ -97,6 +100,9 @@ const types = { }, { type: 'boolean', parameters: ['value', 'null'] + }, { + type: 'boolean', + parameters: ['value', 'value'] }], string: [{ type: 'string', diff --git a/src/style-spec/expression/definitions/equals.js b/src/style-spec/expression/definitions/equals.js index 3135a9e6bf4..a3322ece809 100644 --- a/src/style-spec/expression/definitions/equals.js +++ b/src/style-spec/expression/definitions/equals.js @@ -11,20 +11,19 @@ function isComparableType(type: Type) { return type.kind === 'string' || type.kind === 'number' || type.kind === 'boolean' || - type.kind === 'null'; + type.kind === 'null' || + type.kind === 'value'; } /** - * Special form for ==, !=, implementing the following signatures: - * - (T1: Comparable, T2: Comparable) => boolean { T1 == T2 } - * - (Comparable, value) => boolean - * - (value, Comparable) => boolean - * - * Where Comparable = string | number | boolean | null. + * Special form for ==, !=, implementing the signatures: + * - (T, T) => boolean { T is 'value', 'string', 'number', 'boolean', or 'null' } + * - (T, value) => boolean { T is 'string', 'number', 'boolean', or 'null' } + * - (value, T) => boolean { T is 'string', 'number', 'boolean', or 'null' } * * Evaluation semantics for the value cases are equivalent to Javascript's - * strict equality (===/!==) -- i.e., when the value argument's type doesn't - * match that of the Comparable argument, == evaluates to false, != to true. + * strict equality (===/!==) -- i.e., when the arguments' types don't match, + * == evaluates to false, != to true. * * @private */ @@ -48,14 +47,20 @@ function makeComparison(op: string, negate: boolean) { const lhs = context.parse(args[1], 1, ValueType); if (!lhs) return null; + if (!isComparableType(lhs.type)) { + return context.concat(1).error(`Equality comparisons are not supported for type '${toString(lhs.type)}'.`); + } const rhs = context.parse(args[2], 2, ValueType); if (!rhs) return null; - - if (!isComparableType(lhs.type) && !isComparableType(rhs.type)) { - return context.error(`Expected at least one argument to be a string, number, boolean, or null, but found (${toString(lhs.type)}, ${toString(rhs.type)}) instead.`); + if (!isComparableType(rhs.type)) { + return context.concat(2).error(`Equality comparisons are not supported for type '${toString(rhs.type)}'.`); } - if (lhs.type.kind !== rhs.type.kind && lhs.type.kind !== 'value' && rhs.type.kind !== 'value') { + if (!( + lhs.type.kind === rhs.type.kind || + lhs.type.kind === 'value' || + rhs.type.kind === 'value' + )) { return context.error(`Cannot compare types '${toString(lhs.type)}' and '${toString(rhs.type)}'.`); } diff --git a/test/integration/expression-tests/equal/array/test.json b/test/integration/expression-tests/equal/array/test.json index 40b9285dd25..1a97a0f4226 100644 --- a/test/integration/expression-tests/equal/array/test.json +++ b/test/integration/expression-tests/equal/array/test.json @@ -5,8 +5,8 @@ "result": "error", "errors": [ { - "key": "", - "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, array) instead." + "key": "[2]", + "error": "Equality comparisons are not supported for type 'array'." } ] } diff --git a/test/integration/expression-tests/equal/color/test.json b/test/integration/expression-tests/equal/color/test.json index 37b3339adb1..9ad1e4ddfff 100644 --- a/test/integration/expression-tests/equal/color/test.json +++ b/test/integration/expression-tests/equal/color/test.json @@ -5,8 +5,8 @@ "result": "error", "errors": [ { - "key": "", - "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, color) instead." + "key": "[2]", + "error": "Equality comparisons are not supported for type 'color'." } ] } diff --git a/test/integration/expression-tests/equal/object/test.json b/test/integration/expression-tests/equal/object/test.json index f3eb3d67df6..addf90182b8 100644 --- a/test/integration/expression-tests/equal/object/test.json +++ b/test/integration/expression-tests/equal/object/test.json @@ -5,8 +5,8 @@ "result": "error", "errors": [ { - "key": "", - "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, object) instead." + "key": "[2]", + "error": "Equality comparisons are not supported for type 'object'." } ] } diff --git a/test/integration/expression-tests/equal/value/test.json b/test/integration/expression-tests/equal/value/test.json index 253e95b190e..52d65fd40e2 100644 --- a/test/integration/expression-tests/equal/value/test.json +++ b/test/integration/expression-tests/equal/value/test.json @@ -4,17 +4,18 @@ [{}, {"properties": {"x": 0, "y": 0}}], [{}, {"properties": {"x": "0", "y": "0"}}], [{}, {"properties": {"x": 0, "y": false}}], - [{}, {"properties": {"x": 0, "y": "0"}}] + [{}, {"properties": {"x": 0, "y": "0"}}], + [{}, {"properties": {"x": 0, "y": null}}], + [{}, {"properties": {"x": "0", "y": null}}] ], "expected": { "compiled": { - "result": "error", - "errors": [ - { - "key": "", - "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, value) instead." - } - ] - } + "result": "success", + "isFeatureConstant": false, + "isZoomConstant": true, + "type": "boolean" + }, + "outputs": [true, true, false, false, false, false], + "serialized": ["==", ["get", "x"], ["get", "y"]] } } diff --git a/test/integration/expression-tests/not_equal/value/test.json b/test/integration/expression-tests/not_equal/value/test.json index 0eb625b44d6..62951ff58d8 100644 --- a/test/integration/expression-tests/not_equal/value/test.json +++ b/test/integration/expression-tests/not_equal/value/test.json @@ -4,17 +4,18 @@ [{}, {"properties": {"x": 0, "y": 0}}], [{}, {"properties": {"x": "0", "y": "0"}}], [{}, {"properties": {"x": 0, "y": false}}], - [{}, {"properties": {"x": 0, "y": "0"}}] + [{}, {"properties": {"x": 0, "y": "0"}}], + [{}, {"properties": {"x": 0, "y": null}}], + [{}, {"properties": {"x": "0", "y": null}}] ], "expected": { "compiled": { - "result": "error", - "errors": [ - { - "key": "", - "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, value) instead." - } - ] - } + "result": "success", + "isFeatureConstant": false, + "isZoomConstant": true, + "type": "boolean" + }, + "outputs": [false, false, true, true, true, true], + "serialized": ["!=", ["get", "x"], ["get", "y"]] } } From 1cf1e6304f7874f9ecd4ff555aa59cd23b044f0c Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Mon, 16 Jul 2018 09:01:38 -0400 Subject: [PATCH 3/7] equals.js -> comparison.js --- .../expression/definitions/{equals.js => comparison.js} | 0 src/style-spec/expression/definitions/index.js | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename src/style-spec/expression/definitions/{equals.js => comparison.js} (100%) diff --git a/src/style-spec/expression/definitions/equals.js b/src/style-spec/expression/definitions/comparison.js similarity index 100% rename from src/style-spec/expression/definitions/equals.js rename to src/style-spec/expression/definitions/comparison.js diff --git a/src/style-spec/expression/definitions/index.js b/src/style-spec/expression/definitions/index.js index d357072c475..359335e1802 100644 --- a/src/style-spec/expression/definitions/index.js +++ b/src/style-spec/expression/definitions/index.js @@ -17,7 +17,7 @@ import Case from './case'; import Step from './step'; import Interpolate from './interpolate'; import Coalesce from './coalesce'; -import { Equals, NotEquals } from './equals'; +import { Equals, NotEquals } from './comparison'; import { CollatorExpression } from './collator'; import Length from './length'; From 232bc323ca74269854b23b269b0b47c6783c2d3d Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Fri, 13 Jul 2018 10:48:20 -0400 Subject: [PATCH 4/7] Relax typing for order comparison operators --- docs/components/expression-metadata.js | 183 ++++++++---------- .../expression/definitions/comparison.js | 107 +++++++--- .../expression/definitions/index.js | 55 ++---- .../comparison-number-error/test.json | 2 +- .../expression-tests/equal/array/test.json | 2 +- .../expression-tests/equal/color/test.json | 2 +- .../expression-tests/equal/object/test.json | 2 +- .../greater/boolean/test.json | 4 +- .../greater/mismatch/test.json | 5 +- .../expression-tests/greater/null/test.json | 4 +- .../greater/string-and-value/test.json | 26 +++ .../expression-tests/greater/value/test.json | 4 +- .../greater_or_equal/boolean/test.json | 4 +- .../greater_or_equal/mismatch/test.json | 5 +- .../greater_or_equal/null/test.json | 4 +- .../string-and-value/test.json | 26 +++ .../greater_or_equal/value/test.json | 4 +- .../expression-tests/less/boolean/test.json | 4 +- .../expression-tests/less/mismatch/test.json | 5 +- .../expression-tests/less/null/test.json | 4 +- .../less/string-and-value/test.json | 26 +++ .../expression-tests/less/value/test.json | 4 +- .../less_or_equal/boolean/test.json | 4 +- .../less_or_equal/mismatch/test.json | 5 +- .../less_or_equal/null/test.json | 4 +- .../less_or_equal/string-and-value/test.json | 26 +++ .../less_or_equal/value/test.json | 4 +- test/integration/lib/expression.js | 4 +- 28 files changed, 313 insertions(+), 216 deletions(-) create mode 100644 test/integration/expression-tests/greater/string-and-value/test.json create mode 100644 test/integration/expression-tests/greater_or_equal/string-and-value/test.json create mode 100644 test/integration/expression-tests/less/string-and-value/test.json create mode 100644 test/integration/expression-tests/less_or_equal/string-and-value/test.json diff --git a/docs/components/expression-metadata.js b/docs/components/expression-metadata.js index 8a71281f2d3..fd94061d019 100644 --- a/docs/components/expression-metadata.js +++ b/docs/components/expression-metadata.js @@ -5,105 +5,92 @@ import CompoundExpression from '../../src/style-spec/expression/compound_express // registers compound expressions import '../../src/style-spec/expression/definitions/index'; +const equalitySignatures = [{ + type: 'boolean', + parameters: ['string', 'string'] +}, { + type: 'boolean', + parameters: ['string', 'string', 'collator'] +}, { + type: 'boolean', + parameters: ['number', 'number'] +}, { + type: 'boolean', + parameters: ['boolean', 'boolean'] +}, { + type: 'boolean', + parameters: ['null', 'null'] +}, { + type: 'boolean', + parameters: ['string', 'value'] +}, { + type: 'boolean', + parameters: ['string', 'value', 'collator'] +}, { + type: 'boolean', + parameters: ['number', 'value'] +}, { + type: 'boolean', + parameters: ['boolean', 'value'] +}, { + type: 'boolean', + parameters: ['null', 'value'] +}, { + type: 'boolean', + parameters: ['value', 'string'] +}, { + type: 'boolean', + parameters: ['value', 'string', 'collator'] +}, { + type: 'boolean', + parameters: ['value', 'number'] +}, { + type: 'boolean', + parameters: ['value', 'boolean'] +}, { + type: 'boolean', + parameters: ['value', 'null'] +}, { + type: 'boolean', + parameters: ['value', 'value'] +}]; + +const inequalitySignatures = [{ + type: 'boolean', + parameters: ['string', 'string'] +}, { + type: 'boolean', + parameters: ['string', 'string', 'collator'] +}, { + type: 'boolean', + parameters: ['number', 'number'] +}, { + type: 'boolean', + parameters: ['string', 'value'] +}, { + type: 'boolean', + parameters: ['string', 'value', 'collator'] +}, { + type: 'boolean', + parameters: ['number', 'value'] +}, { + type: 'boolean', + parameters: ['value', 'string'] +}, { + type: 'boolean', + parameters: ['value', 'string', 'collator'] +}, { + type: 'boolean', + parameters: ['value', 'number'] +}]; + const types = { - '==': [{ - type: 'boolean', - parameters: ['string', 'string'] - }, { - type: 'boolean', - parameters: ['string', 'string', 'collator'] - }, { - type: 'boolean', - parameters: ['number', 'number'] - }, { - type: 'boolean', - parameters: ['boolean', 'boolean'] - }, { - type: 'boolean', - parameters: ['null', 'null'] - }, { - type: 'boolean', - parameters: ['string', 'value'] - }, { - type: 'boolean', - parameters: ['string', 'value', 'collator'] - }, { - type: 'boolean', - parameters: ['number', 'value'] - }, { - type: 'boolean', - parameters: ['boolean', 'value'] - }, { - type: 'boolean', - parameters: ['null', 'value'] - }, { - type: 'boolean', - parameters: ['value', 'string'] - }, { - type: 'boolean', - parameters: ['value', 'string', 'collator'] - }, { - type: 'boolean', - parameters: ['value', 'number'] - }, { - type: 'boolean', - parameters: ['value', 'boolean'] - }, { - type: 'boolean', - parameters: ['value', 'null'] - }, { - type: 'boolean', - parameters: ['value', 'value'] - }], - '!=': [{ - type: 'boolean', - parameters: ['string', 'string'] - }, { - type: 'boolean', - parameters: ['string', 'string', 'collator'] - }, { - type: 'boolean', - parameters: ['number', 'number'] - }, { - type: 'boolean', - parameters: ['boolean', 'boolean'] - }, { - type: 'boolean', - parameters: ['null', 'null'] - }, { - type: 'boolean', - parameters: ['string', 'value'] - }, { - type: 'boolean', - parameters: ['string', 'value', 'collator'] - }, { - type: 'boolean', - parameters: ['number', 'value'] - }, { - type: 'boolean', - parameters: ['boolean', 'value'] - }, { - type: 'boolean', - parameters: ['null', 'value'] - }, { - type: 'boolean', - parameters: ['value', 'string'] - }, { - type: 'boolean', - parameters: ['value', 'string', 'collator'] - }, { - type: 'boolean', - parameters: ['value', 'number'] - }, { - type: 'boolean', - parameters: ['value', 'boolean'] - }, { - type: 'boolean', - parameters: ['value', 'null'] - }, { - type: 'boolean', - parameters: ['value', 'value'] - }], + '==': [].concat(equalitySignatures), + '!=': [].concat(equalitySignatures), + '<': [].concat(inequalitySignatures), + '<=': [].concat(inequalitySignatures), + '>': [].concat(inequalitySignatures), + '>=': [].concat(inequalitySignatures), string: [{ type: 'string', parameters: ['value'] diff --git a/src/style-spec/expression/definitions/comparison.js b/src/style-spec/expression/definitions/comparison.js index a3322ece809..599763cb414 100644 --- a/src/style-spec/expression/definitions/comparison.js +++ b/src/style-spec/expression/definitions/comparison.js @@ -1,40 +1,71 @@ // @flow import { toString, ValueType, BooleanType, CollatorType } from '../types'; +import Assertion from './assertion'; import type { Expression } from '../expression'; import type EvaluationContext from '../evaluation_context'; import type ParsingContext from '../parsing_context'; import type { Type } from '../types'; -function isComparableType(type: Type) { - return type.kind === 'string' || - type.kind === 'number' || - type.kind === 'boolean' || - type.kind === 'null' || - type.kind === 'value'; +type ComparisonOperator = '==' | '!=' | '<' | '>' | '<=' | '>=' ; + +function isComparableType(op: ComparisonOperator, type: Type) { + if (op === '==' || op === '!=') { + // equality operator + return type.kind === 'boolean' || + type.kind === 'string' || + type.kind === 'number' || + type.kind === 'null' || + type.kind === 'value'; + } else { + // ordering operator + return type.kind === 'string' || + type.kind === 'number' || + type.kind === 'value'; + } } + +function eq(ctx, a, b) { return a.evaluate(ctx) === b.evaluate(ctx); } +function neq(ctx, a, b) { return a.evaluate(ctx) !== b.evaluate(ctx); } +function lt(ctx, a, b) { return a.evaluate(ctx) < b.evaluate(ctx); } +function gt(ctx, a, b) { return a.evaluate(ctx) > b.evaluate(ctx); } +function lteq(ctx, a, b) { return a.evaluate(ctx) <= b.evaluate(ctx); } +function gteq(ctx, a, b) { return a.evaluate(ctx) >= b.evaluate(ctx); } + +function eqCollate(ctx, a, b, c) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) === 0; } +function neqCollate(ctx, a, b, c) { return !eqCollate(ctx, a, b, c); } +function ltCollate(ctx, a, b, c) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) < 0; } +function gtCollate(ctx, a, b, c) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) > 0; } +function lteqCollate(ctx, a, b, c) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) <= 0; } +function gteqCollate(ctx, a, b, c) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) >= 0; } + /** - * Special form for ==, !=, implementing the signatures: - * - (T, T) => boolean { T is 'value', 'string', 'number', 'boolean', or 'null' } - * - (T, value) => boolean { T is 'string', 'number', 'boolean', or 'null' } - * - (value, T) => boolean { T is 'string', 'number', 'boolean', or 'null' } + * Special form for comparison operators, implementing the signatures: + * - (T, T, ?Collator) => boolean + * - (T, value, ?Collator) => boolean + * - (value, T, ?Collator) => boolean + * + * For inequalities, T must be either string or number. For ==/!=, it can also + * be boolean or null. + * + * Equality semantics are equivalent to Javascript's strict equality (===/!==) + * -- i.e., when the arguments' types don't match, == evaluates to false, != to + * true. * - * Evaluation semantics for the value cases are equivalent to Javascript's - * strict equality (===/!==) -- i.e., when the arguments' types don't match, - * == evaluates to false, != to true. + * When types don't match in an inequality, a runtime error is thrown. * * @private */ -function makeComparison(op: string, negate: boolean) { +function makeComparison(op: ComparisonOperator, compareBasic, compareWithCollator) { return class Comparison implements Expression { type: Type; lhs: Expression; rhs: Expression; - collator: Expression | null; + collator: ?Expression; - constructor(lhs: Expression, rhs: Expression, collator: Expression | null) { + constructor(lhs: Expression, rhs: Expression, collator: ?Expression) { this.type = BooleanType; this.lhs = lhs; this.rhs = rhs; @@ -45,15 +76,17 @@ function makeComparison(op: string, negate: boolean) { if (args.length !== 3 && args.length !== 4) return context.error(`Expected two or three arguments.`); - const lhs = context.parse(args[1], 1, ValueType); + const op: ComparisonOperator = (args[0]: any); + + let lhs = context.parse(args[1], 1, ValueType); if (!lhs) return null; - if (!isComparableType(lhs.type)) { - return context.concat(1).error(`Equality comparisons are not supported for type '${toString(lhs.type)}'.`); + if (!isComparableType(op, lhs.type)) { + return context.concat(1).error(`"${op}" comparisons are not supported for type '${toString(lhs.type)}'.`); } - const rhs = context.parse(args[2], 2, ValueType); + let rhs = context.parse(args[2], 2, ValueType); if (!rhs) return null; - if (!isComparableType(rhs.type)) { - return context.concat(2).error(`Equality comparisons are not supported for type '${toString(rhs.type)}'.`); + if (!isComparableType(op, rhs.type)) { + return context.concat(2).error(`"${op}" comparisons are not supported for type '${toString(rhs.type)}'.`); } if (!( @@ -64,6 +97,20 @@ function makeComparison(op: string, negate: boolean) { return context.error(`Cannot compare types '${toString(lhs.type)}' and '${toString(rhs.type)}'.`); } + if (op !== '==' && op !== '!=') { + // typing rules specific to less/greater than operators + if (lhs.type.kind === 'value' && rhs.type.kind !== 'value') { + // (value, T) + lhs = new Assertion(rhs.type, [lhs]); + } else if (lhs.type.kind !== 'value' && rhs.type.kind === 'value') { + // (T, value) + rhs = new Assertion(lhs.type, [rhs]); + } else if (lhs.type.kind === 'value' && rhs.type.kind === 'value') { + // (value, value) + return context.error(`Expected at least one argument to be a string, number, boolean, or null, but found (${toString(lhs.type)}, ${toString(rhs.type)}) instead.`); + } + } + let collator = null; if (args.length === 4) { if (lhs.type.kind !== 'string' && rhs.type.kind !== 'string') { @@ -77,11 +124,9 @@ function makeComparison(op: string, negate: boolean) { } evaluate(ctx: EvaluationContext) { - const equal = this.collator ? - this.collator.evaluate(ctx).compare(this.lhs.evaluate(ctx), this.rhs.evaluate(ctx)) === 0 : - this.lhs.evaluate(ctx) === this.rhs.evaluate(ctx); - - return negate ? !equal : equal; + return this.collator ? + compareWithCollator(ctx, this.lhs, this.rhs, this.collator) : + compareBasic(ctx, this.lhs, this.rhs); } eachChild(fn: (Expression) => void) { @@ -104,5 +149,9 @@ function makeComparison(op: string, negate: boolean) { }; } -export const Equals = makeComparison('==', false); -export const NotEquals = makeComparison('!=', true); +export const Equals = makeComparison('==', eq, eqCollate); +export const NotEquals = makeComparison('!=', neq, neqCollate); +export const LessThan = makeComparison('<', lt, ltCollate); +export const GreaterThan = makeComparison('>', gt, gtCollate); +export const LessThanOrEqual = makeComparison('<=', lteq, lteqCollate); +export const GreaterThanOrEqual = makeComparison('>=', gteq, gteqCollate); diff --git a/src/style-spec/expression/definitions/index.js b/src/style-spec/expression/definitions/index.js index 359335e1802..b0221082afc 100644 --- a/src/style-spec/expression/definitions/index.js +++ b/src/style-spec/expression/definitions/index.js @@ -17,7 +17,14 @@ import Case from './case'; import Step from './step'; import Interpolate from './interpolate'; import Coalesce from './coalesce'; -import { Equals, NotEquals } from './comparison'; +import { + Equals, + NotEquals, + LessThan, + GreaterThan, + LessThanOrEqual, + GreaterThanOrEqual +} from './comparison'; import { CollatorExpression } from './collator'; import Length from './length'; @@ -29,6 +36,10 @@ const expressions: ExpressionRegistry = { // special forms '==': Equals, '!=': NotEquals, + '>': GreaterThan, + '<': LessThan, + '>=': GreaterThanOrEqual, + '<=': LessThanOrEqual, 'array': ArrayAssertion, 'at': At, 'boolean': Assertion, @@ -68,16 +79,6 @@ function get(key, obj) { return typeof v === 'undefined' ? null : v; } -function lt(ctx, [a, b]) { return a.evaluate(ctx) < b.evaluate(ctx); } -function gt(ctx, [a, b]) { return a.evaluate(ctx) > b.evaluate(ctx); } -function lteq(ctx, [a, b]) { return a.evaluate(ctx) <= b.evaluate(ctx); } -function gteq(ctx, [a, b]) { return a.evaluate(ctx) >= b.evaluate(ctx); } - -function ltCollate(ctx, [a, b, c]) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) < 0; } -function gtCollate(ctx, [a, b, c]) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) > 0; } -function lteqCollate(ctx, [a, b, c]) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) <= 0; } -function gteqCollate(ctx, [a, b, c]) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) >= 0; } - function binarySearch(v, a, i, j) { while (i <= j) { const m = (i + j) >> 1; @@ -473,38 +474,6 @@ CompoundExpression.register(expressions, { // assumes v is a array literal with values sorted in ascending order and of a single type (ctx, [k, v]) => binarySearch(ctx.properties()[(k: any).value], (v: any).value, 0, (v: any).value.length - 1) ], - '>': { - type: BooleanType, - overloads: [ - [[NumberType, NumberType], gt], - [[StringType, StringType], gt], - [[StringType, StringType, CollatorType], gtCollate] - ] - }, - '<': { - type: BooleanType, - overloads: [ - [[NumberType, NumberType], lt], - [[StringType, StringType], lt], - [[StringType, StringType, CollatorType], ltCollate] - ] - }, - '>=': { - type: BooleanType, - overloads: [ - [[NumberType, NumberType], gteq], - [[StringType, StringType], gteq], - [[StringType, StringType, CollatorType], gteqCollate] - ] - }, - '<=': { - type: BooleanType, - overloads: [ - [[NumberType, NumberType], lteq], - [[StringType, StringType], lteq], - [[StringType, StringType, CollatorType], lteqCollate] - ] - }, 'all': { type: BooleanType, overloads: [ diff --git a/test/integration/expression-tests/collator/comparison-number-error/test.json b/test/integration/expression-tests/collator/comparison-number-error/test.json index cbc72182cbb..921caa41d19 100644 --- a/test/integration/expression-tests/collator/comparison-number-error/test.json +++ b/test/integration/expression-tests/collator/comparison-number-error/test.json @@ -9,7 +9,7 @@ "compiled": { "result": "error", "errors": [ - {"key": "[1]", "error": "Expected string but found number instead."} + {"key": "", "error": "Cannot use collator to compare non-string types."} ] } } diff --git a/test/integration/expression-tests/equal/array/test.json b/test/integration/expression-tests/equal/array/test.json index 1a97a0f4226..fe28d07984b 100644 --- a/test/integration/expression-tests/equal/array/test.json +++ b/test/integration/expression-tests/equal/array/test.json @@ -6,7 +6,7 @@ "errors": [ { "key": "[2]", - "error": "Equality comparisons are not supported for type 'array'." + "error": "\"==\" comparisons are not supported for type 'array'." } ] } diff --git a/test/integration/expression-tests/equal/color/test.json b/test/integration/expression-tests/equal/color/test.json index 9ad1e4ddfff..6e04c74104c 100644 --- a/test/integration/expression-tests/equal/color/test.json +++ b/test/integration/expression-tests/equal/color/test.json @@ -6,7 +6,7 @@ "errors": [ { "key": "[2]", - "error": "Equality comparisons are not supported for type 'color'." + "error": "\"==\" comparisons are not supported for type 'color'." } ] } diff --git a/test/integration/expression-tests/equal/object/test.json b/test/integration/expression-tests/equal/object/test.json index addf90182b8..97a51854e82 100644 --- a/test/integration/expression-tests/equal/object/test.json +++ b/test/integration/expression-tests/equal/object/test.json @@ -6,7 +6,7 @@ "errors": [ { "key": "[2]", - "error": "Equality comparisons are not supported for type 'object'." + "error": "\"==\" comparisons are not supported for type 'object'." } ] } diff --git a/test/integration/expression-tests/greater/boolean/test.json b/test/integration/expression-tests/greater/boolean/test.json index b65ca4be7de..0947dcf6eee 100644 --- a/test/integration/expression-tests/greater/boolean/test.json +++ b/test/integration/expression-tests/greater/boolean/test.json @@ -5,8 +5,8 @@ "result": "error", "errors": [ { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (boolean, boolean) instead." + "key": "[1]", + "error": "\">\" comparisons are not supported for type 'boolean'." } ] } diff --git a/test/integration/expression-tests/greater/mismatch/test.json b/test/integration/expression-tests/greater/mismatch/test.json index 6cf310c38bb..e073c4848a1 100644 --- a/test/integration/expression-tests/greater/mismatch/test.json +++ b/test/integration/expression-tests/greater/mismatch/test.json @@ -4,10 +4,7 @@ "compiled": { "result": "error", "errors": [ - { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (string, number) instead." - } + {"key": "", "error": "Cannot compare types 'string' and 'number'."} ] } } diff --git a/test/integration/expression-tests/greater/null/test.json b/test/integration/expression-tests/greater/null/test.json index e55826acf7f..44c8e864d72 100644 --- a/test/integration/expression-tests/greater/null/test.json +++ b/test/integration/expression-tests/greater/null/test.json @@ -5,8 +5,8 @@ "result": "error", "errors": [ { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (null, null) instead." + "key": "[1]", + "error": "\">\" comparisons are not supported for type 'null'." } ] } diff --git a/test/integration/expression-tests/greater/string-and-value/test.json b/test/integration/expression-tests/greater/string-and-value/test.json new file mode 100644 index 00000000000..e345c087acb --- /dev/null +++ b/test/integration/expression-tests/greater/string-and-value/test.json @@ -0,0 +1,26 @@ +{ + "expression": [">", ["string", ["get", "x"]], ["get", "y"]], + "inputs": [ + [{}, {"properties": {"x": "2", "y": "1"}}], + [{}, {"properties": {"x": "2", "y": 1}}], + [{}, {"properties": {"x": 2, "y": "1"}}] + ], + "expected": { + "compiled": { + "result": "success", + "isFeatureConstant": false, + "isZoomConstant": true, + "type": "boolean" + }, + "outputs": [ + true, + { + "error": "Expected value to be of type string, but found number instead." + }, + { + "error": "Expected value to be of type string, but found number instead." + } + ], + "serialized": [">", ["string", ["get", "x"]], ["string", ["get", "y"]]] + } +} diff --git a/test/integration/expression-tests/greater/value/test.json b/test/integration/expression-tests/greater/value/test.json index 0cf1194110c..50010a81e31 100644 --- a/test/integration/expression-tests/greater/value/test.json +++ b/test/integration/expression-tests/greater/value/test.json @@ -1,12 +1,12 @@ { - "expression": [">", ["string", ["get", "x"]], ["get", "y"]], + "expression": [">", ["get", "x"], ["get", "y"]], "expected": { "compiled": { "result": "error", "errors": [ { "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (string, value) instead." + "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, value) instead." } ] } diff --git a/test/integration/expression-tests/greater_or_equal/boolean/test.json b/test/integration/expression-tests/greater_or_equal/boolean/test.json index ec38d764258..5ec7d802104 100644 --- a/test/integration/expression-tests/greater_or_equal/boolean/test.json +++ b/test/integration/expression-tests/greater_or_equal/boolean/test.json @@ -5,8 +5,8 @@ "result": "error", "errors": [ { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (boolean, boolean) instead." + "key": "[1]", + "error": "\">=\" comparisons are not supported for type 'boolean'." } ] } diff --git a/test/integration/expression-tests/greater_or_equal/mismatch/test.json b/test/integration/expression-tests/greater_or_equal/mismatch/test.json index 6fe448d8974..9b2275f0f2c 100644 --- a/test/integration/expression-tests/greater_or_equal/mismatch/test.json +++ b/test/integration/expression-tests/greater_or_equal/mismatch/test.json @@ -4,10 +4,7 @@ "compiled": { "result": "error", "errors": [ - { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (string, number) instead." - } + {"key": "", "error": "Cannot compare types 'string' and 'number'."} ] } } diff --git a/test/integration/expression-tests/greater_or_equal/null/test.json b/test/integration/expression-tests/greater_or_equal/null/test.json index 7b776bbf811..4fbf753fcf8 100644 --- a/test/integration/expression-tests/greater_or_equal/null/test.json +++ b/test/integration/expression-tests/greater_or_equal/null/test.json @@ -5,8 +5,8 @@ "result": "error", "errors": [ { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (null, null) instead." + "key": "[1]", + "error": "\">=\" comparisons are not supported for type 'null'." } ] } diff --git a/test/integration/expression-tests/greater_or_equal/string-and-value/test.json b/test/integration/expression-tests/greater_or_equal/string-and-value/test.json new file mode 100644 index 00000000000..0cdd25d88a2 --- /dev/null +++ b/test/integration/expression-tests/greater_or_equal/string-and-value/test.json @@ -0,0 +1,26 @@ +{ + "expression": [">=", ["string", ["get", "x"]], ["get", "y"]], + "inputs": [ + [{}, {"properties": {"x": "1", "y": "1"}}], + [{}, {"properties": {"x": "1", "y": 1}}], + [{}, {"properties": {"x": 1, "y": "1"}}] + ], + "expected": { + "compiled": { + "result": "success", + "isFeatureConstant": false, + "isZoomConstant": true, + "type": "boolean" + }, + "outputs": [ + true, + { + "error": "Expected value to be of type string, but found number instead." + }, + { + "error": "Expected value to be of type string, but found number instead." + } + ], + "serialized": [">=", ["string", ["get", "x"]], ["string", ["get", "y"]]] + } +} diff --git a/test/integration/expression-tests/greater_or_equal/value/test.json b/test/integration/expression-tests/greater_or_equal/value/test.json index b37b9c96cdf..9c943823054 100644 --- a/test/integration/expression-tests/greater_or_equal/value/test.json +++ b/test/integration/expression-tests/greater_or_equal/value/test.json @@ -1,12 +1,12 @@ { - "expression": [">=", ["string", ["get", "x"]], ["get", "y"]], + "expression": [">=", ["get", "x"], ["get", "y"]], "expected": { "compiled": { "result": "error", "errors": [ { "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (string, value) instead." + "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, value) instead." } ] } diff --git a/test/integration/expression-tests/less/boolean/test.json b/test/integration/expression-tests/less/boolean/test.json index 0d0cb2a70a3..731d6dbb78c 100644 --- a/test/integration/expression-tests/less/boolean/test.json +++ b/test/integration/expression-tests/less/boolean/test.json @@ -5,8 +5,8 @@ "result": "error", "errors": [ { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (boolean, boolean) instead." + "key": "[1]", + "error": "\"<\" comparisons are not supported for type 'boolean'." } ] } diff --git a/test/integration/expression-tests/less/mismatch/test.json b/test/integration/expression-tests/less/mismatch/test.json index d59861c9579..c33f34feabd 100644 --- a/test/integration/expression-tests/less/mismatch/test.json +++ b/test/integration/expression-tests/less/mismatch/test.json @@ -4,10 +4,7 @@ "compiled": { "result": "error", "errors": [ - { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (string, number) instead." - } + {"key": "", "error": "Cannot compare types 'string' and 'number'."} ] } } diff --git a/test/integration/expression-tests/less/null/test.json b/test/integration/expression-tests/less/null/test.json index 0b5e5c2a7a4..ac9a426fdd1 100644 --- a/test/integration/expression-tests/less/null/test.json +++ b/test/integration/expression-tests/less/null/test.json @@ -5,8 +5,8 @@ "result": "error", "errors": [ { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (null, null) instead." + "key": "[1]", + "error": "\"<\" comparisons are not supported for type 'null'." } ] } diff --git a/test/integration/expression-tests/less/string-and-value/test.json b/test/integration/expression-tests/less/string-and-value/test.json new file mode 100644 index 00000000000..1674ece373f --- /dev/null +++ b/test/integration/expression-tests/less/string-and-value/test.json @@ -0,0 +1,26 @@ +{ + "expression": ["<", ["string", ["get", "x"]], ["get", "y"]], + "inputs": [ + [{}, {"properties": {"x": "1", "y": "2"}}], + [{}, {"properties": {"x": "1", "y": 2}}], + [{}, {"properties": {"x": 1, "y": "2"}}] + ], + "expected": { + "compiled": { + "result": "success", + "isFeatureConstant": false, + "isZoomConstant": true, + "type": "boolean" + }, + "outputs": [ + true, + { + "error": "Expected value to be of type string, but found number instead." + }, + { + "error": "Expected value to be of type string, but found number instead." + } + ], + "serialized": ["<", ["string", ["get", "x"]], ["string", ["get", "y"]]] + } +} diff --git a/test/integration/expression-tests/less/value/test.json b/test/integration/expression-tests/less/value/test.json index fef2cab5a9c..f879019e143 100644 --- a/test/integration/expression-tests/less/value/test.json +++ b/test/integration/expression-tests/less/value/test.json @@ -1,12 +1,12 @@ { - "expression": ["<", ["string", ["get", "x"]], ["get", "y"]], + "expression": ["<", ["get", "x"], ["get", "y"]], "expected": { "compiled": { "result": "error", "errors": [ { "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (string, value) instead." + "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, value) instead." } ] } diff --git a/test/integration/expression-tests/less_or_equal/boolean/test.json b/test/integration/expression-tests/less_or_equal/boolean/test.json index 19f03ce5a8c..1b657359ef6 100644 --- a/test/integration/expression-tests/less_or_equal/boolean/test.json +++ b/test/integration/expression-tests/less_or_equal/boolean/test.json @@ -5,8 +5,8 @@ "result": "error", "errors": [ { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (boolean, boolean) instead." + "key": "[1]", + "error": "\"<=\" comparisons are not supported for type 'boolean'." } ] } diff --git a/test/integration/expression-tests/less_or_equal/mismatch/test.json b/test/integration/expression-tests/less_or_equal/mismatch/test.json index 22bac383c11..40e257af409 100644 --- a/test/integration/expression-tests/less_or_equal/mismatch/test.json +++ b/test/integration/expression-tests/less_or_equal/mismatch/test.json @@ -4,10 +4,7 @@ "compiled": { "result": "error", "errors": [ - { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (string, number) instead." - } + {"key": "", "error": "Cannot compare types 'string' and 'number'."} ] } } diff --git a/test/integration/expression-tests/less_or_equal/null/test.json b/test/integration/expression-tests/less_or_equal/null/test.json index 343f73b7279..c16be2ca756 100644 --- a/test/integration/expression-tests/less_or_equal/null/test.json +++ b/test/integration/expression-tests/less_or_equal/null/test.json @@ -5,8 +5,8 @@ "result": "error", "errors": [ { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (null, null) instead." + "key": "[1]", + "error": "\"<=\" comparisons are not supported for type 'null'." } ] } diff --git a/test/integration/expression-tests/less_or_equal/string-and-value/test.json b/test/integration/expression-tests/less_or_equal/string-and-value/test.json new file mode 100644 index 00000000000..24b4fe682ce --- /dev/null +++ b/test/integration/expression-tests/less_or_equal/string-and-value/test.json @@ -0,0 +1,26 @@ +{ + "expression": ["<", ["string", ["get", "x"]], ["get", "y"]], + "inputs": [ + [{}, {"properties": {"x": "1", "y": "1"}}], + [{}, {"properties": {"x": "1", "y": 1}}], + [{}, {"properties": {"x": 1, "y": "1"}}] + ], + "expected": { + "compiled": { + "result": "success", + "isFeatureConstant": false, + "isZoomConstant": true, + "type": "boolean" + }, + "outputs": [ + false, + { + "error": "Expected value to be of type string, but found number instead." + }, + { + "error": "Expected value to be of type string, but found number instead." + } + ], + "serialized": ["<", ["string", ["get", "x"]], ["string", ["get", "y"]]] + } +} diff --git a/test/integration/expression-tests/less_or_equal/value/test.json b/test/integration/expression-tests/less_or_equal/value/test.json index 035d457e759..8bdee8e1c51 100644 --- a/test/integration/expression-tests/less_or_equal/value/test.json +++ b/test/integration/expression-tests/less_or_equal/value/test.json @@ -1,12 +1,12 @@ { - "expression": ["<=", ["string", ["get", "x"]], ["get", "y"]], + "expression": ["<=", ["get", "x"], ["get", "y"]], "expected": { "compiled": { "result": "error", "errors": [ { "key": "", - "error": "Expected arguments of type (number, number) | (string, string), but found (string, value) instead." + "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, value) instead." } ] } diff --git a/test/integration/lib/expression.js b/test/integration/lib/expression.js index 4dd0d55bc4b..f247f669a64 100644 --- a/test/integration/lib/expression.js +++ b/test/integration/lib/expression.js @@ -171,12 +171,12 @@ exports.run = function (implementation, options, runExpressionTest) { }; if (compileOk && !evalOk) { - const differences = diffOutputs(result.outputs); + const differences = `Original\n${diffOutputs(result.outputs)}\n`; diffOutput.text += differences; diffOutput.html += differences; } if (recompileOk && !roundTripOk) { - const differences = diffOutputs(result.roundTripOutputs); + const differences = `\nRoundtripped through serialize()\n${diffOutputs(result.roundTripOutputs)}\n`; diffOutput.text += differences; diffOutput.html += differences; } From 8009c7905801449d3bd33fb8e7f87e5813da9a2a Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Mon, 16 Jul 2018 08:51:03 -0400 Subject: [PATCH 5/7] Allow (value, value) for order comparisons --- .../expression/definitions/comparison.js | 51 ++++++++++++------- .../expression-tests/greater/value/test.json | 30 ++++++++--- .../greater_or_equal/value/test.json | 30 ++++++++--- .../expression-tests/less/value/test.json | 30 ++++++++--- .../less_or_equal/value/test.json | 30 ++++++++--- 5 files changed, 120 insertions(+), 51 deletions(-) diff --git a/src/style-spec/expression/definitions/comparison.js b/src/style-spec/expression/definitions/comparison.js index 599763cb414..1f0739c7424 100644 --- a/src/style-spec/expression/definitions/comparison.js +++ b/src/style-spec/expression/definitions/comparison.js @@ -2,6 +2,8 @@ import { toString, ValueType, BooleanType, CollatorType } from '../types'; import Assertion from './assertion'; +import { typeOf } from '../values'; +import RuntimeError from '../runtime_error'; import type { Expression } from '../expression'; import type EvaluationContext from '../evaluation_context'; @@ -27,19 +29,19 @@ function isComparableType(op: ComparisonOperator, type: Type) { } -function eq(ctx, a, b) { return a.evaluate(ctx) === b.evaluate(ctx); } -function neq(ctx, a, b) { return a.evaluate(ctx) !== b.evaluate(ctx); } -function lt(ctx, a, b) { return a.evaluate(ctx) < b.evaluate(ctx); } -function gt(ctx, a, b) { return a.evaluate(ctx) > b.evaluate(ctx); } -function lteq(ctx, a, b) { return a.evaluate(ctx) <= b.evaluate(ctx); } -function gteq(ctx, a, b) { return a.evaluate(ctx) >= b.evaluate(ctx); } +function eq(ctx, a, b) { return a === b; } +function neq(ctx, a, b) { return a !== b; } +function lt(ctx, a, b) { return a < b; } +function gt(ctx, a, b) { return a > b; } +function lteq(ctx, a, b) { return a <= b; } +function gteq(ctx, a, b) { return a >= b; } -function eqCollate(ctx, a, b, c) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) === 0; } +function eqCollate(ctx, a, b, c) { return c.compare(a, b) === 0; } function neqCollate(ctx, a, b, c) { return !eqCollate(ctx, a, b, c); } -function ltCollate(ctx, a, b, c) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) < 0; } -function gtCollate(ctx, a, b, c) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) > 0; } -function lteqCollate(ctx, a, b, c) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) <= 0; } -function gteqCollate(ctx, a, b, c) { return c.evaluate(ctx).compare(a.evaluate(ctx), b.evaluate(ctx)) >= 0; } +function ltCollate(ctx, a, b, c) { return c.compare(a, b) < 0; } +function gtCollate(ctx, a, b, c) { return c.compare(a, b) > 0; } +function lteqCollate(ctx, a, b, c) { return c.compare(a, b) <= 0; } +function gteqCollate(ctx, a, b, c) { return c.compare(a, b) >= 0; } /** * Special form for comparison operators, implementing the signatures: @@ -47,14 +49,14 @@ function gteqCollate(ctx, a, b, c) { return c.evaluate(ctx).compare(a.evaluate(c * - (T, value, ?Collator) => boolean * - (value, T, ?Collator) => boolean * - * For inequalities, T must be either string or number. For ==/!=, it can also - * be boolean or null. + * For inequalities, T must be either value, string, or number. For ==/!=, it + * can also be boolean or null. * * Equality semantics are equivalent to Javascript's strict equality (===/!==) * -- i.e., when the arguments' types don't match, == evaluates to false, != to * true. * - * When types don't match in an inequality, a runtime error is thrown. + * When types don't match in an ordering comparison, a runtime error is thrown. * * @private */ @@ -64,12 +66,15 @@ function makeComparison(op: ComparisonOperator, compareBasic, compareWithCollato lhs: Expression; rhs: Expression; collator: ?Expression; + needsRuntimeTypeCheck: boolean; constructor(lhs: Expression, rhs: Expression, collator: ?Expression) { this.type = BooleanType; this.lhs = lhs; this.rhs = rhs; this.collator = collator; + this.needsRuntimeTypeCheck = op !== '==' && op !== '!=' && + lhs.type.kind === 'value' && rhs.type.kind === 'value'; } static parse(args: Array, context: ParsingContext): ?Expression { @@ -105,9 +110,6 @@ function makeComparison(op: ComparisonOperator, compareBasic, compareWithCollato } else if (lhs.type.kind !== 'value' && rhs.type.kind === 'value') { // (T, value) rhs = new Assertion(lhs.type, [rhs]); - } else if (lhs.type.kind === 'value' && rhs.type.kind === 'value') { - // (value, value) - return context.error(`Expected at least one argument to be a string, number, boolean, or null, but found (${toString(lhs.type)}, ${toString(rhs.type)}) instead.`); } } @@ -124,9 +126,20 @@ function makeComparison(op: ComparisonOperator, compareBasic, compareWithCollato } evaluate(ctx: EvaluationContext) { + const lhs = this.lhs.evaluate(ctx); + const rhs = this.rhs.evaluate(ctx); + if (this.needsRuntimeTypeCheck) { + const lt = typeOf(lhs); + const rt = typeOf(rhs); + // check that type is string or number, and equal + if (lt.kind !== rt.kind || !(lt.kind === 'string' || lt.kind === 'number')) { + throw new RuntimeError(`Expected arguments for "${op}" to be (string, string) or (number, number), but found (${lt.kind}, ${rt.kind}) instead.`); + } + } + return this.collator ? - compareWithCollator(ctx, this.lhs, this.rhs, this.collator) : - compareBasic(ctx, this.lhs, this.rhs); + compareWithCollator(ctx, lhs, rhs, this.collator.evaluate(ctx)) : + compareBasic(ctx, lhs, rhs); } eachChild(fn: (Expression) => void) { diff --git a/test/integration/expression-tests/greater/value/test.json b/test/integration/expression-tests/greater/value/test.json index 50010a81e31..75c314eeccd 100644 --- a/test/integration/expression-tests/greater/value/test.json +++ b/test/integration/expression-tests/greater/value/test.json @@ -1,14 +1,28 @@ { "expression": [">", ["get", "x"], ["get", "y"]], + "inputs": [ + [{}, {"properties": {"x": "2", "y": "10"}}], + [{}, {"properties": {"x": 10, "y": 1}}], + [{}, {"properties": {"x": "1", "y": 1}}], + [{}, {"properties": {"x": 1, "y": "1"}}] + ], "expected": { "compiled": { - "result": "error", - "errors": [ - { - "key": "", - "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, value) instead." - } - ] - } + "result": "success", + "isFeatureConstant": false, + "isZoomConstant": true, + "type": "boolean" + }, + "outputs": [ + true, + true, + { + "error": "Expected arguments for \">\" to be (string, string) or (number, number), but found (string, number) instead." + }, + { + "error": "Expected arguments for \">\" to be (string, string) or (number, number), but found (number, string) instead." + } + ], + "serialized": [">", ["get", "x"], ["get", "y"]] } } diff --git a/test/integration/expression-tests/greater_or_equal/value/test.json b/test/integration/expression-tests/greater_or_equal/value/test.json index 9c943823054..e92a5684bba 100644 --- a/test/integration/expression-tests/greater_or_equal/value/test.json +++ b/test/integration/expression-tests/greater_or_equal/value/test.json @@ -1,14 +1,28 @@ { "expression": [">=", ["get", "x"], ["get", "y"]], + "inputs": [ + [{}, {"properties": {"x": "2", "y": "10"}}], + [{}, {"properties": {"x": 10, "y": 1}}], + [{}, {"properties": {"x": "1", "y": 1}}], + [{}, {"properties": {"x": 1, "y": "1"}}] + ], "expected": { "compiled": { - "result": "error", - "errors": [ - { - "key": "", - "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, value) instead." - } - ] - } + "result": "success", + "isFeatureConstant": false, + "isZoomConstant": true, + "type": "boolean" + }, + "outputs": [ + true, + true, + { + "error": "Expected arguments for \">=\" to be (string, string) or (number, number), but found (string, number) instead." + }, + { + "error": "Expected arguments for \">=\" to be (string, string) or (number, number), but found (number, string) instead." + } + ], + "serialized": [">=", ["get", "x"], ["get", "y"]] } } diff --git a/test/integration/expression-tests/less/value/test.json b/test/integration/expression-tests/less/value/test.json index f879019e143..1d12eb582c8 100644 --- a/test/integration/expression-tests/less/value/test.json +++ b/test/integration/expression-tests/less/value/test.json @@ -1,14 +1,28 @@ { "expression": ["<", ["get", "x"], ["get", "y"]], + "inputs": [ + [{}, {"properties": {"x": "10", "y": "2"}}], + [{}, {"properties": {"x": 1, "y": 10}}], + [{}, {"properties": {"x": "1", "y": 1}}], + [{}, {"properties": {"x": 1, "y": "1"}}] + ], "expected": { "compiled": { - "result": "error", - "errors": [ - { - "key": "", - "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, value) instead." - } - ] - } + "result": "success", + "isFeatureConstant": false, + "isZoomConstant": true, + "type": "boolean" + }, + "outputs": [ + true, + true, + { + "error": "Expected arguments for \"<\" to be (string, string) or (number, number), but found (string, number) instead." + }, + { + "error": "Expected arguments for \"<\" to be (string, string) or (number, number), but found (number, string) instead." + } + ], + "serialized": ["<", ["get", "x"], ["get", "y"]] } } diff --git a/test/integration/expression-tests/less_or_equal/value/test.json b/test/integration/expression-tests/less_or_equal/value/test.json index 8bdee8e1c51..71b3fe81095 100644 --- a/test/integration/expression-tests/less_or_equal/value/test.json +++ b/test/integration/expression-tests/less_or_equal/value/test.json @@ -1,14 +1,28 @@ { "expression": ["<=", ["get", "x"], ["get", "y"]], + "inputs": [ + [{}, {"properties": {"x": "10", "y": "2"}}], + [{}, {"properties": {"x": 1, "y": 10}}], + [{}, {"properties": {"x": "1", "y": 1}}], + [{}, {"properties": {"x": 1, "y": "1"}}] + ], "expected": { "compiled": { - "result": "error", - "errors": [ - { - "key": "", - "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, value) instead." - } - ] - } + "result": "success", + "isFeatureConstant": false, + "isZoomConstant": true, + "type": "boolean" + }, + "outputs": [ + true, + true, + { + "error": "Expected arguments for \"<=\" to be (string, string) or (number, number), but found (string, number) instead." + }, + { + "error": "Expected arguments for \"<=\" to be (string, string) or (number, number), but found (number, string) instead." + } + ], + "serialized": ["<=", ["get", "x"], ["get", "y"]] } } From afc7d29ece779470cd1d88019d1b4e2fc370c01e Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Mon, 23 Jul 2018 17:00:01 -0400 Subject: [PATCH 6/7] Allow [==, value, value, collator] Use collator.compare() if both arguments evaluate to strings, and === otherwise --- .../expression/definitions/comparison.js | 37 +++++++++++++------ .../equal/collator-value/test.json | 31 ++++++++++++++++ 2 files changed, 57 insertions(+), 11 deletions(-) create mode 100644 test/integration/expression-tests/equal/collator-value/test.json diff --git a/src/style-spec/expression/definitions/comparison.js b/src/style-spec/expression/definitions/comparison.js index 1f0739c7424..d643d6c2cc5 100644 --- a/src/style-spec/expression/definitions/comparison.js +++ b/src/style-spec/expression/definitions/comparison.js @@ -61,20 +61,21 @@ function gteqCollate(ctx, a, b, c) { return c.compare(a, b) >= 0; } * @private */ function makeComparison(op: ComparisonOperator, compareBasic, compareWithCollator) { + const isOrderComparison = op !== '==' && op !== '!='; + return class Comparison implements Expression { type: Type; lhs: Expression; rhs: Expression; collator: ?Expression; - needsRuntimeTypeCheck: boolean; + hasUntypedArgument: boolean; constructor(lhs: Expression, rhs: Expression, collator: ?Expression) { this.type = BooleanType; this.lhs = lhs; this.rhs = rhs; this.collator = collator; - this.needsRuntimeTypeCheck = op !== '==' && op !== '!=' && - lhs.type.kind === 'value' && rhs.type.kind === 'value'; + this.hasUntypedArgument = lhs.type.kind === 'value' || rhs.type.kind === 'value'; } static parse(args: Array, context: ParsingContext): ?Expression { @@ -94,15 +95,15 @@ function makeComparison(op: ComparisonOperator, compareBasic, compareWithCollato return context.concat(2).error(`"${op}" comparisons are not supported for type '${toString(rhs.type)}'.`); } - if (!( - lhs.type.kind === rhs.type.kind || - lhs.type.kind === 'value' || - rhs.type.kind === 'value' - )) { + if ( + lhs.type.kind !== rhs.type.kind && + lhs.type.kind !== 'value' && + rhs.type.kind !== 'value' + ) { return context.error(`Cannot compare types '${toString(lhs.type)}' and '${toString(rhs.type)}'.`); } - if (op !== '==' && op !== '!=') { + if (isOrderComparison) { // typing rules specific to less/greater than operators if (lhs.type.kind === 'value' && rhs.type.kind !== 'value') { // (value, T) @@ -115,7 +116,12 @@ function makeComparison(op: ComparisonOperator, compareBasic, compareWithCollato let collator = null; if (args.length === 4) { - if (lhs.type.kind !== 'string' && rhs.type.kind !== 'string') { + if ( + lhs.type.kind !== 'string' && + rhs.type.kind !== 'string' && + lhs.type.kind !== 'value' && + rhs.type.kind !== 'value' + ) { return context.error(`Cannot use collator to compare non-string types.`); } collator = context.parse(args[3], 3, CollatorType); @@ -128,7 +134,8 @@ function makeComparison(op: ComparisonOperator, compareBasic, compareWithCollato evaluate(ctx: EvaluationContext) { const lhs = this.lhs.evaluate(ctx); const rhs = this.rhs.evaluate(ctx); - if (this.needsRuntimeTypeCheck) { + + if (isOrderComparison && this.hasUntypedArgument) { const lt = typeOf(lhs); const rt = typeOf(rhs); // check that type is string or number, and equal @@ -137,6 +144,14 @@ function makeComparison(op: ComparisonOperator, compareBasic, compareWithCollato } } + if (this.collator && !isOrderComparison && this.hasUntypedArgument) { + const lt = typeOf(lhs); + const rt = typeOf(rhs); + if (lt.kind !== 'string' || rt.kind !== 'string') { + return compareBasic(ctx, lhs, rhs); + } + } + return this.collator ? compareWithCollator(ctx, lhs, rhs, this.collator.evaluate(ctx)) : compareBasic(ctx, lhs, rhs); diff --git a/test/integration/expression-tests/equal/collator-value/test.json b/test/integration/expression-tests/equal/collator-value/test.json new file mode 100644 index 00000000000..4d0db1e8dc0 --- /dev/null +++ b/test/integration/expression-tests/equal/collator-value/test.json @@ -0,0 +1,31 @@ +{ + "expression": [ + "==", + ["get", "x"], + ["get", "y"], + ["collator", {"case-sensitive": false, "diacritic-sensitive": false}] + ], + "inputs": [ + [{}, {"properties": {"x": "a", "y": "A"}}], + [{}, {"properties": {"x": "1", "y": "2"}}], + [{}, {"properties": {"x": "1", "y": null}}], + [{}, {"properties": {"x": null, "y": null}}], + [{}, {"properties": {"x": 1, "y": "1"}}], + [{}, {"properties": {"x": 1, "y": 1}}] + ], + "expected": { + "compiled": { + "result": "success", + "isFeatureConstant": false, + "isZoomConstant": true, + "type": "boolean" + }, + "outputs": [true, false, false, true, false, true], + "serialized": [ + "==", + ["get", "x"], + ["get", "y"], + ["collator", {"case-sensitive": false, "diacritic-sensitive": false}] + ] + } +} From a964dcc7e44f570e68faac705fe4a2cf9602362f Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Tue, 24 Jul 2018 08:54:53 -0400 Subject: [PATCH 7/7] Simplify documentation for comparison operators --- docs/components/expression-metadata.js | 87 +++----------------------- src/style-spec/reference/v8.json | 12 ++-- 2 files changed, 14 insertions(+), 85 deletions(-) diff --git a/docs/components/expression-metadata.js b/docs/components/expression-metadata.js index fd94061d019..40928cf52b7 100644 --- a/docs/components/expression-metadata.js +++ b/docs/components/expression-metadata.js @@ -5,92 +5,21 @@ import CompoundExpression from '../../src/style-spec/expression/compound_express // registers compound expressions import '../../src/style-spec/expression/definitions/index'; -const equalitySignatures = [{ - type: 'boolean', - parameters: ['string', 'string'] -}, { - type: 'boolean', - parameters: ['string', 'string', 'collator'] -}, { - type: 'boolean', - parameters: ['number', 'number'] -}, { - type: 'boolean', - parameters: ['boolean', 'boolean'] -}, { - type: 'boolean', - parameters: ['null', 'null'] -}, { - type: 'boolean', - parameters: ['string', 'value'] -}, { - type: 'boolean', - parameters: ['string', 'value', 'collator'] -}, { - type: 'boolean', - parameters: ['number', 'value'] -}, { - type: 'boolean', - parameters: ['boolean', 'value'] -}, { - type: 'boolean', - parameters: ['null', 'value'] -}, { - type: 'boolean', - parameters: ['value', 'string'] -}, { - type: 'boolean', - parameters: ['value', 'string', 'collator'] -}, { - type: 'boolean', - parameters: ['value', 'number'] -}, { - type: 'boolean', - parameters: ['value', 'boolean'] -}, { - type: 'boolean', - parameters: ['value', 'null'] -}, { +const comparisonSignatures = [{ type: 'boolean', parameters: ['value', 'value'] -}]; - -const inequalitySignatures = [{ - type: 'boolean', - parameters: ['string', 'string'] -}, { - type: 'boolean', - parameters: ['string', 'string', 'collator'] -}, { - type: 'boolean', - parameters: ['number', 'number'] -}, { - type: 'boolean', - parameters: ['string', 'value'] -}, { - type: 'boolean', - parameters: ['string', 'value', 'collator'] -}, { - type: 'boolean', - parameters: ['number', 'value'] -}, { - type: 'boolean', - parameters: ['value', 'string'] -}, { - type: 'boolean', - parameters: ['value', 'string', 'collator'] }, { type: 'boolean', - parameters: ['value', 'number'] + parameters: ['value', 'value', 'collator'] }]; const types = { - '==': [].concat(equalitySignatures), - '!=': [].concat(equalitySignatures), - '<': [].concat(inequalitySignatures), - '<=': [].concat(inequalitySignatures), - '>': [].concat(inequalitySignatures), - '>=': [].concat(inequalitySignatures), + '==': comparisonSignatures, + '!=': comparisonSignatures, + '<': comparisonSignatures, + '<=': comparisonSignatures, + '>': comparisonSignatures, + '>=': comparisonSignatures, string: [{ type: 'string', parameters: ['value'] diff --git a/src/style-spec/reference/v8.json b/src/style-spec/reference/v8.json index 31073ac0e26..fd4c24b768c 100644 --- a/src/style-spec/reference/v8.json +++ b/src/style-spec/reference/v8.json @@ -2963,7 +2963,7 @@ } }, "==": { - "doc": "Returns `true` if the input values are equal, `false` otherwise. Equality is strictly typed: values of different types are always considered not equal. Accepts an optional `collator` argument to control locale-dependent string comparisons.", + "doc": "Returns `true` if the input values are equal, `false` otherwise. The comparison is strictly typed: values of different runtime types are always considered unequal. Cases where the types are known to be different at parse time are considered invalid and will produce a parse error. Accepts an optional `collator` argument to control locale-dependent string comparisons.", "group": "Decision", "sdk-support": { "basic functionality": { @@ -2975,7 +2975,7 @@ } }, "!=": { - "doc": "Returns `true` if the input values are not equal, `false` otherwise. Equality is strictly typed: values of different types are always considered not equal. Accepts an optional `collator` argument to control locale-dependent string comparisons.", + "doc": "Returns `true` if the input values are not equal, `false` otherwise. The comparison is strictly typed: values of different runtime types are always considered unequal. Cases where the types are known to be different at parse time are considered invalid and will produce a parse error. Accepts an optional `collator` argument to control locale-dependent string comparisons.", "group": "Decision", "sdk-support": { "basic functionality": { @@ -2987,7 +2987,7 @@ } }, ">": { - "doc": "Returns `true` if the first input is strictly greater than the second, `false` otherwise. The inputs must be numbers or strings, and both of the same type. Accepts an optional `collator` argument to control locale-dependent string comparisons.", + "doc": "Returns `true` if the first input is strictly greater than the second, `false` otherwise. The arguments are required to be either both strings or both numbers; if during evaluation they are not, expression evaluation produces an error. Cases where this constraint is known not to hold at parse time are considered in valid and will produce a parse error. Accepts an optional `collator` argument to control locale-dependent string comparisons.", "group": "Decision", "sdk-support": { "basic functionality": { @@ -2999,7 +2999,7 @@ } }, "<": { - "doc": "Returns `true` if the first input is strictly less than the second, `false` otherwise. The inputs must be numbers or strings, and both of the same type. Accepts an optional `collator` argument to control locale-dependent string comparisons.", + "doc": "Returns `true` if the first input is strictly less than the second, `false` otherwise. The arguments are required to be either both strings or both numbers; if during evaluation they are not, expression evaluation produces an error. Cases where this constraint is known not to hold at parse time are considered in valid and will produce a parse error. Accepts an optional `collator` argument to control locale-dependent string comparisons.", "group": "Decision", "sdk-support": { "basic functionality": { @@ -3011,7 +3011,7 @@ } }, ">=": { - "doc": "Returns `true` if the first input is greater than or equal to the second, `false` otherwise. The inputs must be numbers or strings, and both of the same type. Accepts an optional `collator` argument to control locale-dependent string comparisons.", + "doc": "Returns `true` if the first input is greater than or equal to the second, `false` otherwise. The arguments are required to be either both strings or both numbers; if during evaluation they are not, expression evaluation produces an error. Cases where this constraint is known not to hold at parse time are considered in valid and will produce a parse error. Accepts an optional `collator` argument to control locale-dependent string comparisons.", "group": "Decision", "sdk-support": { "basic functionality": { @@ -3023,7 +3023,7 @@ } }, "<=": { - "doc": "Returns `true` if the first input is less than or equal to the second, `false` otherwise. The inputs must be numbers or strings, and both of the same type. Accepts an optional `collator` argument to control locale-dependent string comparisons.", + "doc": "Returns `true` if the first input is less than or equal to the second, `false` otherwise. The arguments are required to be either both strings or both numbers; if during evaluation they are not, expression evaluation produces an error. Cases where this constraint is known not to hold at parse time are considered in valid and will produce a parse error. Accepts an optional `collator` argument to control locale-dependent string comparisons.", "group": "Decision", "sdk-support": { "basic functionality": {