From 980cceeca868bed8c164d4e103fbe1214240d625 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Mon, 11 Dec 2017 09:51:24 -0500 Subject: [PATCH 1/3] Improve typing for ==, != expressions Closes #5761 Closes #5835 The overloads of == and != are now effectively: - `(T1: Comparable, T2: Comparable) => boolean { T1 == T2 }` - `(Comparable, value) => boolean` - `(value, Comparable) => boolean` Where `Comparable = string | number | boolean | null`. --- .../expression/definitions/equals.js | 76 +++++++++++++++++++ .../expression/definitions/index.js | 46 ++++------- test/expression.test.js | 2 +- .../expression-tests/equal/array/test.json | 14 ++++ .../expression-tests/equal/color/test.json | 14 ++++ .../expression-tests/equal/mismatch/test.json | 7 +- .../expression-tests/equal/null-lhs/test.json | 13 ++++ .../expression-tests/equal/null-rhs/test.json | 13 ++++ .../expression-tests/equal/number/test.json | 7 +- .../expression-tests/equal/object/test.json | 14 ++++ .../expression-tests/equal/string/test.json | 13 ++-- .../expression-tests/equal/untagged/test.json | 14 ---- .../expression-tests/equal/value/test.json | 20 +++++ .../not_equal/mismatch/test.json | 7 +- .../not_equal/number/test.json | 5 +- .../not_equal/string/test.json | 5 +- .../not_equal/untagged/test.json | 14 ---- .../not_equal/value/test.json | 20 +++++ 18 files changed, 216 insertions(+), 88 deletions(-) create mode 100644 src/style-spec/expression/definitions/equals.js create mode 100644 test/integration/expression-tests/equal/array/test.json create mode 100644 test/integration/expression-tests/equal/color/test.json create mode 100644 test/integration/expression-tests/equal/null-lhs/test.json create mode 100644 test/integration/expression-tests/equal/null-rhs/test.json create mode 100644 test/integration/expression-tests/equal/object/test.json delete mode 100644 test/integration/expression-tests/equal/untagged/test.json create mode 100644 test/integration/expression-tests/equal/value/test.json delete mode 100644 test/integration/expression-tests/not_equal/untagged/test.json create mode 100644 test/integration/expression-tests/not_equal/value/test.json diff --git a/src/style-spec/expression/definitions/equals.js b/src/style-spec/expression/definitions/equals.js new file mode 100644 index 00000000000..05be7369444 --- /dev/null +++ b/src/style-spec/expression/definitions/equals.js @@ -0,0 +1,76 @@ +// @flow + +const { + ValueType, + BooleanType, +} = require('../types'); +const {toString, checkSubtype} = require('../types'); + +import type { Expression } from '../expression'; +import type EvaluationContext from '../evaluation_context'; +import type ParsingContext from '../parsing_context'; +import type { Type } from '../types'; + +function eq(ctx) { return this.lhs.evaluate(ctx) === this.rhs.evaluate(ctx); } +function ne(ctx) { return this.lhs.evaluate(ctx) !== this.rhs.evaluate(ctx); } + +function isComparableType(type: Type) { + return type.kind === 'string' || + type.kind === 'number' || + type.kind === 'boolean' || + type.kind === 'null'; +} + +/** + * Special form for error-coalescing coercion expressions "to-number", + * "to-color". Since these coercions can fail at runtime, they accept multiple + * arguments, only evaluating one at a time until one succeeds. + * + * @private + */ +class Equals implements Expression { + type: Type; + lhs: Expression; + rhs: Expression; + evaluate: (EvaluationContext) => any; + + constructor(op: '==' | '!=', lhs: Expression, rhs: Expression) { + this.type = BooleanType; + this.lhs = lhs; + this.rhs = rhs; + this.evaluate = op === '==' ? eq : ne; + } + + static parse(args: Array, context: ParsingContext): ?Expression { + if (args.length !== 3) + return context.error(`Expected two arguments.`); + + const op: '==' | '!=' = (args[0]: any); + + const lhs = context.parse(args[1], 1, ValueType); + if (!lhs) return null; + 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 (checkSubtype(lhs.type, rhs.type) && checkSubtype(rhs.type, lhs.type)) { + return context.error(`Cannot compare ${toString(lhs.type)} and ${toString(rhs.type)}.`); + } + + return new Equals(op, lhs, rhs); + } + + eachChild(fn: (Expression) => void) { + fn(this.lhs); + fn(this.rhs); + } + + possibleOutputs() { + return this.lhs.possibleOutputs().concat(this.rhs.possibleOutputs()); + } +} + +module.exports = Equals; diff --git a/src/style-spec/expression/definitions/index.js b/src/style-spec/expression/definitions/index.js index 324e94d0b18..e398d409de6 100644 --- a/src/style-spec/expression/definitions/index.js +++ b/src/style-spec/expression/definitions/index.js @@ -1,7 +1,6 @@ // @flow const { - NullType, NumberType, StringType, BooleanType, @@ -28,27 +27,30 @@ const Case = require('./case'); const Step = require('./step'); const Interpolate = require('./interpolate'); const Coalesce = require('./coalesce'); +const Equals = require('./equals'); import type { Expression } from '../expression'; const expressions: { [string]: Class } = { // special forms - 'let': Let, - 'var': Var, - 'literal': Literal, - 'string': Assertion, - 'number': Assertion, - 'boolean': Assertion, - 'object': Assertion, + '!=': Equals, + '==': Equals, 'array': ArrayAssertion, - 'to-number': Coercion, - 'to-color': Coercion, 'at': At, + 'boolean': Assertion, 'case': Case, - 'match': Match, 'coalesce': Coalesce, + 'interpolate': Interpolate, + 'let': Let, + 'literal': Literal, + 'match': Match, + 'number': Assertion, + 'object': Assertion, 'step': Step, - 'interpolate': Interpolate + 'string': Assertion, + 'to-color': Coercion, + 'to-number': Coercion, + 'var': Var }; function rgba(ctx, [r, g, b, a]) { @@ -74,8 +76,6 @@ function length(ctx, [v]) { return v.evaluate(ctx).length; } -function eq(ctx, [a, b]) { return a.evaluate(ctx) === b.evaluate(ctx); } -function ne(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); } @@ -315,24 +315,6 @@ CompoundExpression.register(expressions, { varargs(NumberType), (ctx, args) => Math.max(...args.map(arg => arg.evaluate(ctx))) ], - '==': { - type: BooleanType, - overloads: [ - [[NumberType, NumberType], eq], - [[StringType, StringType], eq], - [[BooleanType, BooleanType], eq], - [[NullType, NullType], eq] - ] - }, - '!=': { - type: BooleanType, - overloads: [ - [[NumberType, NumberType], ne], - [[StringType, StringType], ne], - [[BooleanType, BooleanType], ne], - [[NullType, NullType], ne] - ] - }, '>': { type: BooleanType, overloads: [ diff --git a/test/expression.test.js b/test/expression.test.js index 2fdb9e706ab..2bec0845786 100644 --- a/test/expression.test.js +++ b/test/expression.test.js @@ -45,7 +45,7 @@ expressionSuite.run('js', { ignores, tests }, (fixture) => { } }; - for (const input of fixture.inputs) { + for (const input of fixture.inputs || []) { try { const feature = { properties: input[1].properties || {} }; if ('id' in input[1]) { diff --git a/test/integration/expression-tests/equal/array/test.json b/test/integration/expression-tests/equal/array/test.json new file mode 100644 index 00000000000..40b9285dd25 --- /dev/null +++ b/test/integration/expression-tests/equal/array/test.json @@ -0,0 +1,14 @@ +{ + "expression": ["==", ["get", "x"], ["literal", [1]]], + "expected": { + "compiled": { + "result": "error", + "errors": [ + { + "key": "", + "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, array) instead." + } + ] + } + } +} diff --git a/test/integration/expression-tests/equal/color/test.json b/test/integration/expression-tests/equal/color/test.json new file mode 100644 index 00000000000..37b3339adb1 --- /dev/null +++ b/test/integration/expression-tests/equal/color/test.json @@ -0,0 +1,14 @@ +{ + "expression": ["==", ["get", "x"], ["to-color", "red"]], + "expected": { + "compiled": { + "result": "error", + "errors": [ + { + "key": "", + "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, color) instead." + } + ] + } + } +} diff --git a/test/integration/expression-tests/equal/mismatch/test.json b/test/integration/expression-tests/equal/mismatch/test.json index 150c74dfb2f..02a5ce690bb 100644 --- a/test/integration/expression-tests/equal/mismatch/test.json +++ b/test/integration/expression-tests/equal/mismatch/test.json @@ -3,12 +3,7 @@ "expected": { "compiled": { "result": "error", - "errors": [ - { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string) | (boolean, boolean) | (null, null), but found (string, number) instead." - } - ] + "errors": [{"key": "", "error": "Cannot compare string and number."}] } } } diff --git a/test/integration/expression-tests/equal/null-lhs/test.json b/test/integration/expression-tests/equal/null-lhs/test.json new file mode 100644 index 00000000000..dfc092a478c --- /dev/null +++ b/test/integration/expression-tests/equal/null-lhs/test.json @@ -0,0 +1,13 @@ +{ + "expression": ["==", null, ["get", "x"]], + "inputs": [[{}, {"properties": {}}], [{}, {"properties": {"x": 1}}]], + "expected": { + "outputs": [true, false], + "compiled": { + "result": "success", + "isZoomConstant": true, + "isFeatureConstant": false, + "type": "boolean" + } + } +} diff --git a/test/integration/expression-tests/equal/null-rhs/test.json b/test/integration/expression-tests/equal/null-rhs/test.json new file mode 100644 index 00000000000..dfc092a478c --- /dev/null +++ b/test/integration/expression-tests/equal/null-rhs/test.json @@ -0,0 +1,13 @@ +{ + "expression": ["==", null, ["get", "x"]], + "inputs": [[{}, {"properties": {}}], [{}, {"properties": {"x": 1}}]], + "expected": { + "outputs": [true, false], + "compiled": { + "result": "success", + "isZoomConstant": true, + "isFeatureConstant": false, + "type": "boolean" + } + } +} diff --git a/test/integration/expression-tests/equal/number/test.json b/test/integration/expression-tests/equal/number/test.json index f6e8674fe2d..cffb5e260ab 100644 --- a/test/integration/expression-tests/equal/number/test.json +++ b/test/integration/expression-tests/equal/number/test.json @@ -1,11 +1,12 @@ { - "expression": ["==", ["number", ["get", "x"]], ["number", ["get", "y"]]], + "expression": ["==", ["number", ["get", "x"]], ["get", "y"]], "inputs": [ [{}, {"properties": {"x": 1, "y": 1}}], - [{}, {"properties": {"x": 1, "y": 2}}] + [{}, {"properties": {"x": 1, "y": 2}}], + [{}, {"properties": {"x": 1, "y": "1"}}] ], "expected": { - "outputs": [true, false], + "outputs": [true, false, false], "compiled": { "result": "success", "isZoomConstant": true, diff --git a/test/integration/expression-tests/equal/object/test.json b/test/integration/expression-tests/equal/object/test.json new file mode 100644 index 00000000000..f3eb3d67df6 --- /dev/null +++ b/test/integration/expression-tests/equal/object/test.json @@ -0,0 +1,14 @@ +{ + "expression": ["==", ["get", "x"], ["literal", {}]], + "expected": { + "compiled": { + "result": "error", + "errors": [ + { + "key": "", + "error": "Expected at least one argument to be a string, number, boolean, or null, but found (value, object) instead." + } + ] + } + } +} diff --git a/test/integration/expression-tests/equal/string/test.json b/test/integration/expression-tests/equal/string/test.json index 22a4f8d06c7..3e412bc9f43 100644 --- a/test/integration/expression-tests/equal/string/test.json +++ b/test/integration/expression-tests/equal/string/test.json @@ -1,15 +1,12 @@ { - "expression": [ - "==", - ["to-string", ["get", "x"]], - ["to-string", ["get", "y"]] - ], + "expression": ["==", ["string", ["get", "x"]], ["get", "y"]], "inputs": [ - [{}, {"properties": {"x": 1, "y": 1}}], - [{}, {"properties": {"x": 1, "y": 2}}] + [{}, {"properties": {"x": "1", "y": "1"}}], + [{}, {"properties": {"x": "1", "y": 2}}], + [{}, {"properties": {"x": "1", "y": 1}}] ], "expected": { - "outputs": [true, false], + "outputs": [true, false, false], "compiled": { "result": "success", "isZoomConstant": true, diff --git a/test/integration/expression-tests/equal/untagged/test.json b/test/integration/expression-tests/equal/untagged/test.json deleted file mode 100644 index 0d8565fd977..00000000000 --- a/test/integration/expression-tests/equal/untagged/test.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "expression": ["==", ["string", ["get", "x"]], ["get", "y"]], - "expected": { - "compiled": { - "result": "error", - "errors": [ - { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string) | (boolean, boolean) | (null, null), but found (string, value) instead." - } - ] - } - } -} diff --git a/test/integration/expression-tests/equal/value/test.json b/test/integration/expression-tests/equal/value/test.json new file mode 100644 index 00000000000..253e95b190e --- /dev/null +++ b/test/integration/expression-tests/equal/value/test.json @@ -0,0 +1,20 @@ +{ + "expression": ["==", ["get", "x"], ["get", "y"]], + "inputs": [ + [{}, {"properties": {"x": 0, "y": 0}}], + [{}, {"properties": {"x": "0", "y": "0"}}], + [{}, {"properties": {"x": 0, "y": false}}], + [{}, {"properties": {"x": 0, "y": "0"}}] + ], + "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." + } + ] + } + } +} diff --git a/test/integration/expression-tests/not_equal/mismatch/test.json b/test/integration/expression-tests/not_equal/mismatch/test.json index 180f3e68b98..568aa6bc23c 100644 --- a/test/integration/expression-tests/not_equal/mismatch/test.json +++ b/test/integration/expression-tests/not_equal/mismatch/test.json @@ -3,12 +3,7 @@ "expected": { "compiled": { "result": "error", - "errors": [ - { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string) | (boolean, boolean) | (null, null), but found (string, number) instead." - } - ] + "errors": [{"key": "", "error": "Cannot compare string and number."}] } } } diff --git a/test/integration/expression-tests/not_equal/number/test.json b/test/integration/expression-tests/not_equal/number/test.json index 19a51b0cb9c..e3f75d74272 100644 --- a/test/integration/expression-tests/not_equal/number/test.json +++ b/test/integration/expression-tests/not_equal/number/test.json @@ -1,11 +1,12 @@ { - "expression": ["!=", ["number", ["get", "x"]], ["number", ["get", "y"]]], + "expression": ["!=", ["number", ["get", "x"]], ["get", "y"]], "inputs": [ [{}, {"properties": {"x": 1, "y": 1}}], + [{}, {"properties": {"x": 1, "y": "1"}}], [{}, {"properties": {"x": 1, "y": 2}}] ], "expected": { - "outputs": [false, true], + "outputs": [false, true, true], "compiled": { "result": "success", "isZoomConstant": true, diff --git a/test/integration/expression-tests/not_equal/string/test.json b/test/integration/expression-tests/not_equal/string/test.json index 33107f7448c..ab15fa49f15 100644 --- a/test/integration/expression-tests/not_equal/string/test.json +++ b/test/integration/expression-tests/not_equal/string/test.json @@ -1,11 +1,12 @@ { - "expression": ["!=", ["string", ["get", "x"]], ["string", ["get", "y"]]], + "expression": ["!=", ["string", ["get", "x"]], ["get", "y"]], "inputs": [ [{}, {"properties": {"x": "1", "y": "1"}}], + [{}, {"properties": {"x": "1", "y": 1}}], [{}, {"properties": {"x": "1", "y": "2"}}] ], "expected": { - "outputs": [false, true], + "outputs": [false, true, true], "compiled": { "result": "success", "isZoomConstant": true, diff --git a/test/integration/expression-tests/not_equal/untagged/test.json b/test/integration/expression-tests/not_equal/untagged/test.json deleted file mode 100644 index a66a9f90a28..00000000000 --- a/test/integration/expression-tests/not_equal/untagged/test.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "expression": ["!=", ["string", ["get", "x"]], ["get", "y"]], - "expected": { - "compiled": { - "result": "error", - "errors": [ - { - "key": "", - "error": "Expected arguments of type (number, number) | (string, string) | (boolean, boolean) | (null, null), but found (string, value) instead." - } - ] - } - } -} diff --git a/test/integration/expression-tests/not_equal/value/test.json b/test/integration/expression-tests/not_equal/value/test.json new file mode 100644 index 00000000000..0eb625b44d6 --- /dev/null +++ b/test/integration/expression-tests/not_equal/value/test.json @@ -0,0 +1,20 @@ +{ + "expression": ["!=", ["get", "x"], ["get", "y"]], + "inputs": [ + [{}, {"properties": {"x": 0, "y": 0}}], + [{}, {"properties": {"x": "0", "y": "0"}}], + [{}, {"properties": {"x": 0, "y": false}}], + [{}, {"properties": {"x": 0, "y": "0"}}] + ], + "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." + } + ] + } + } +} From e81519b2c6708ac78d90f7fe52456b16dab9f504 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Mon, 11 Dec 2017 10:51:02 -0500 Subject: [PATCH 2/3] Simplify comparability check --- src/style-spec/expression/definitions/equals.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/style-spec/expression/definitions/equals.js b/src/style-spec/expression/definitions/equals.js index 05be7369444..90421f8e9d4 100644 --- a/src/style-spec/expression/definitions/equals.js +++ b/src/style-spec/expression/definitions/equals.js @@ -4,7 +4,7 @@ const { ValueType, BooleanType, } = require('../types'); -const {toString, checkSubtype} = require('../types'); +const {toString} = require('../types'); import type { Expression } from '../expression'; import type EvaluationContext from '../evaluation_context'; @@ -56,7 +56,7 @@ class Equals implements Expression { 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 (checkSubtype(lhs.type, rhs.type) && checkSubtype(rhs.type, lhs.type)) { + 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)}.`); } From 2e619746bebaa03cf46602c9965952dd6c7b0248 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Mon, 11 Dec 2017 20:46:19 -0500 Subject: [PATCH 3/3] Add internal docs, fix possibleValues --- src/style-spec/expression/definitions/equals.js | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/style-spec/expression/definitions/equals.js b/src/style-spec/expression/definitions/equals.js index 90421f8e9d4..5151daa395c 100644 --- a/src/style-spec/expression/definitions/equals.js +++ b/src/style-spec/expression/definitions/equals.js @@ -22,9 +22,16 @@ function isComparableType(type: Type) { } /** - * Special form for error-coalescing coercion expressions "to-number", - * "to-color". Since these coercions can fail at runtime, they accept multiple - * arguments, only evaluating one at a time until one succeeds. + * 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. + * + * 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. * * @private */ @@ -69,7 +76,7 @@ class Equals implements Expression { } possibleOutputs() { - return this.lhs.possibleOutputs().concat(this.rhs.possibleOutputs()); + return [true, false]; } }