From 4256281a16c74656414495b4aac56779c7cb6327 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Thu, 13 Sep 2018 13:23:40 -0700 Subject: [PATCH] For string-valued properties, do coercion rather than assertion --- docs/components/expression-metadata.js | 14 +++++-- .../expression/definitions/coalesce.js | 2 +- .../expression/definitions/coercion.js | 19 ++++++--- .../expression/definitions/index.js | 12 +----- src/style-spec/expression/index.js | 6 ++- src/style-spec/expression/parsing_context.js | 42 ++++++++++--------- src/style-spec/function/convert.js | 4 +- .../coalesce/inference/test.json | 8 ++-- .../to-boolean/2-ary/test.json | 10 +++++ .../to-string/2-ary/test.json | 10 +++++ .../to-string/implicit/test.json | 24 +++++++++++ 11 files changed, 106 insertions(+), 45 deletions(-) create mode 100644 test/integration/expression-tests/to-boolean/2-ary/test.json create mode 100644 test/integration/expression-tests/to-string/2-ary/test.json create mode 100644 test/integration/expression-tests/to-string/implicit/test.json diff --git a/docs/components/expression-metadata.js b/docs/components/expression-metadata.js index d96033e5a2f..b4fc300d788 100644 --- a/docs/components/expression-metadata.js +++ b/docs/components/expression-metadata.js @@ -65,14 +65,22 @@ const types = { type: 'object', parameters: ['value', { repeat: [ 'fallback: value' ] }] }], - 'to-number': [{ - type: 'number', - parameters: ['value', { repeat: [ 'fallback: value' ] }] + 'to-boolean': [{ + type: 'boolean', + parameters: ['value'] }], 'to-color': [{ type: 'color', parameters: ['value', { repeat: [ 'fallback: value' ] }] }], + 'to-number': [{ + type: 'number', + parameters: ['value', { repeat: [ 'fallback: value' ] }] + }], + 'to-string': [{ + type: 'string', + parameters: ['value'] + }], at: [{ type: 'ItemType', parameters: ['number', 'array'] diff --git a/src/style-spec/expression/definitions/coalesce.js b/src/style-spec/expression/definitions/coalesce.js index 3f8a8895542..26d879c0707 100644 --- a/src/style-spec/expression/definitions/coalesce.js +++ b/src/style-spec/expression/definitions/coalesce.js @@ -31,7 +31,7 @@ class Coalesce implements Expression { const parsedArgs = []; for (const arg of args.slice(1)) { - const parsed = context.parse(arg, 1 + parsedArgs.length, outputType, undefined, {omitTypeAnnotations: true}); + const parsed = context.parse(arg, 1 + parsedArgs.length, outputType, undefined, {typeAnnotation: 'omit'}); if (!parsed) return null; outputType = outputType || parsed.type; parsedArgs.push(parsed); diff --git a/src/style-spec/expression/definitions/coercion.js b/src/style-spec/expression/definitions/coercion.js index e48ea2192b4..58f062551b5 100644 --- a/src/style-spec/expression/definitions/coercion.js +++ b/src/style-spec/expression/definitions/coercion.js @@ -2,8 +2,8 @@ import assert from 'assert'; -import { ColorType, ValueType, NumberType } from '../types'; -import { Color, validateRGBA } from '../values'; +import {BooleanType, ColorType, NumberType, StringType, ValueType} from '../types'; +import {Color, toString as valueToString, validateRGBA} from '../values'; import RuntimeError from '../runtime_error'; import type { Expression } from '../expression'; @@ -14,8 +14,10 @@ import type { Type } from '../types'; import { Formatted, FormattedSection } from './formatted'; const types = { + 'to-boolean': BooleanType, + 'to-color': ColorType, 'to-number': NumberType, - 'to-color': ColorType + 'to-string': StringType }; /** @@ -41,6 +43,9 @@ class Coercion implements Expression { const name: string = (args[0]: any); assert(types[name], name); + if ((name === 'to-boolean' || name === 'to-string') && args.length !== 2) + return context.error(`Expected one argument.`); + const type = types[name]; const parsed = []; @@ -54,7 +59,9 @@ class Coercion implements Expression { } evaluate(ctx: EvaluationContext) { - if (this.type.kind === 'color') { + if (this.type.kind === 'boolean') { + return Boolean(this.args[0].evaluate(ctx)); + } else if (this.type.kind === 'color') { let input; let error; for (const arg of this.args) { @@ -86,7 +93,7 @@ class Coercion implements Expression { } } throw new RuntimeError(`Could not parse formatted text from value '${typeof input === 'string' ? input : JSON.stringify(input)}'`); - } else { + } else if (this.type.kind === 'number') { let value = null; for (const arg of this.args) { value = arg.evaluate(ctx); @@ -96,6 +103,8 @@ class Coercion implements Expression { return num; } throw new RuntimeError(`Could not convert ${JSON.stringify(value)} to number.`); + } else { + return valueToString(this.args[0].evaluate(ctx)); } } diff --git a/src/style-spec/expression/definitions/index.js b/src/style-spec/expression/definitions/index.js index 0ee11e55c60..107249f40ce 100644 --- a/src/style-spec/expression/definitions/index.js +++ b/src/style-spec/expression/definitions/index.js @@ -69,8 +69,10 @@ const expressions: ExpressionRegistry = { 'object': Assertion, 'step': Step, 'string': Assertion, + 'to-boolean': Coercion, 'to-color': Coercion, 'to-number': Coercion, + 'to-string': Coercion, 'var': Var }; @@ -121,16 +123,6 @@ CompoundExpression.register(expressions, { [ValueType], (ctx, [v]) => typeToString(typeOf(v.evaluate(ctx))) ], - 'to-string': [ - StringType, - [ValueType], - (ctx, [v]) => valueToString(v.evaluate(ctx)) - ], - 'to-boolean': [ - BooleanType, - [ValueType], - (ctx, [v]) => Boolean(v.evaluate(ctx)) - ], 'to-rgba': [ array(NumberType, 4), [ColorType], diff --git a/src/style-spec/expression/index.js b/src/style-spec/expression/index.js index d3ef57dd58d..69781b3d5f7 100644 --- a/src/style-spec/expression/index.js +++ b/src/style-spec/expression/index.js @@ -116,7 +116,11 @@ export function isExpression(expression: mixed) { */ export function createExpression(expression: mixed, propertySpec: StylePropertySpecification): Result> { const parser = new ParsingContext(definitions, [], getExpectedType(propertySpec)); - const parsed = parser.parse(expression); + + // For string-valued properties, coerce to string at the top level rather than asserting. + const parsed = parser.parse(expression, undefined, undefined, undefined, + propertySpec.type === 'string' ? {typeAnnotation: 'coerce'} : undefined); + if (!parsed) { assert(parser.errors.length > 0); return error(parser.errors); diff --git a/src/style-spec/expression/parsing_context.js b/src/style-spec/expression/parsing_context.js index 4a0c2d64ec5..d35777c77bd 100644 --- a/src/style-spec/expression/parsing_context.js +++ b/src/style-spec/expression/parsing_context.js @@ -61,7 +61,7 @@ class ParsingContext { index?: number, expectedType?: ?Type, bindings?: Array<[string, Expression]>, - options: {omitTypeAnnotations?: boolean} = {} + options: {typeAnnotation?: 'assert' | 'coerce' | 'omit'} = {} ): ?Expression { if (index) { return this.concat(index, expectedType, bindings)._parse(expr, options); @@ -69,12 +69,21 @@ class ParsingContext { return this._parse(expr, options); } - _parse(expr: mixed, options: {omitTypeAnnotations?: boolean}): ?Expression { - + _parse(expr: mixed, options: {typeAnnotation?: 'assert' | 'coerce' | 'omit'}): ?Expression { if (expr === null || typeof expr === 'string' || typeof expr === 'boolean' || typeof expr === 'number') { expr = ['literal', expr]; } + function annotate(parsed, type, typeAnnotation: 'assert' | 'coerce' | 'omit') { + if (typeAnnotation === 'assert') { + return new Assertion(type, [parsed]); + } else if (typeAnnotation === 'coerce') { + return new Coercion(type, [parsed]); + } else { + return parsed; + } + } + if (Array.isArray(expr)) { if (expr.length === 0) { return this.error(`Expected an array with at least one element. If you wanted a literal array, use ["literal", []].`); @@ -95,24 +104,19 @@ class ParsingContext { const expected = this.expectedType; const actual = parsed.type; - // When we expect a number, string, boolean, or array but - // have a Value, we can wrap it in a refining assertion. - // When we expect a Color but have a String or Value, we - // can wrap it in "to-color" coercion. + // When we expect a number, string, boolean, or array but have a value, wrap it in an assertion. + // When we expect a color or formatted string, but have a string or value, wrap it in a coercion. // Otherwise, we do static type-checking. + // + // These behaviors are overridable for: + // * The "coalesce" operator, which needs to omit type annotations. + // * String-valued properties (e.g. `text-field`), where coercion is more convenient than assertion. + // if ((expected.kind === 'string' || expected.kind === 'number' || expected.kind === 'boolean' || expected.kind === 'object' || expected.kind === 'array') && actual.kind === 'value') { - if (!options.omitTypeAnnotations) { - parsed = new Assertion(expected, [parsed]); - } - } else if (expected.kind === 'color' && (actual.kind === 'value' || actual.kind === 'string')) { - if (!options.omitTypeAnnotations) { - parsed = new Coercion(expected, [parsed]); - } - } else if (expected.kind === 'formatted' && (actual.kind === 'value' || actual.kind === 'string')) { - if (!options.omitTypeAnnotations) { - parsed = new Coercion(expected, [parsed]); - } - } else if (this.checkSubtype(this.expectedType, parsed.type)) { + parsed = annotate(parsed, expected, options.typeAnnotation || 'assert'); + } else if ((expected.kind === 'color' || expected.kind === 'formatted') && (actual.kind === 'value' || actual.kind === 'string')) { + parsed = annotate(parsed, expected, options.typeAnnotation || 'coerce'); + } else if (this.checkSubtype(expected, actual)) { return null; } } diff --git a/src/style-spec/function/convert.js b/src/style-spec/function/convert.js index a9de750e326..b200b40cc7b 100644 --- a/src/style-spec/function/convert.js +++ b/src/style-spec/function/convert.js @@ -36,7 +36,9 @@ function convertIdentityFunction(parameters, propertySpec): Array { const get = ['get', parameters.property]; if (parameters.default === undefined) { - return get; + // By default, expressions for string-valued properties get coerced. To preserve + // legacy function semantics, insert an explicit assertion instead. + return propertySpec.type === 'string' ? ['string', get] : get; } else if (propertySpec.type === 'enum') { return [ 'match', diff --git a/test/integration/expression-tests/coalesce/inference/test.json b/test/integration/expression-tests/coalesce/inference/test.json index b8cd7a1a346..8399bb10035 100644 --- a/test/integration/expression-tests/coalesce/inference/test.json +++ b/test/integration/expression-tests/coalesce/inference/test.json @@ -17,11 +17,9 @@ "outputs": [ "one", "two", - { - "error": "Expected value to be of type string, but found number instead." - }, - {"error": "Expected value to be of type string, but found null instead."} + "5", + "" ], - "serialized": ["string", ["coalesce", ["get", "a"], ["get", "b"]]] + "serialized": ["to-string", ["coalesce", ["get", "a"], ["get", "b"]]] } } diff --git a/test/integration/expression-tests/to-boolean/2-ary/test.json b/test/integration/expression-tests/to-boolean/2-ary/test.json new file mode 100644 index 00000000000..94de6a23d81 --- /dev/null +++ b/test/integration/expression-tests/to-boolean/2-ary/test.json @@ -0,0 +1,10 @@ +{ + "expression": ["to-boolean", ["get", "x"], ["get", "y"]], + "inputs": [[{}, {}]], + "expected": { + "compiled": { + "result": "error", + "errors": [{"key": "", "error": "Expected one argument."}] + } + } +} diff --git a/test/integration/expression-tests/to-string/2-ary/test.json b/test/integration/expression-tests/to-string/2-ary/test.json new file mode 100644 index 00000000000..4ee8bde793d --- /dev/null +++ b/test/integration/expression-tests/to-string/2-ary/test.json @@ -0,0 +1,10 @@ +{ + "expression": ["to-string", ["get", "x"], ["get", "y"]], + "inputs": [[{}, {}]], + "expected": { + "compiled": { + "result": "error", + "errors": [{"key": "", "error": "Expected one argument."}] + } + } +} diff --git a/test/integration/expression-tests/to-string/implicit/test.json b/test/integration/expression-tests/to-string/implicit/test.json new file mode 100644 index 00000000000..d13d2b807d0 --- /dev/null +++ b/test/integration/expression-tests/to-string/implicit/test.json @@ -0,0 +1,24 @@ +{ + "expression": ["get", "p"], + "propertySpec": { + "type": "string" + }, + "inputs": [ + [{}, {"properties": {}}], + [{}, {"properties": {"p": 0}}], + [{}, {"properties": {"p": "a"}}] + ], + "expected": { + "compiled": { + "result": "success", + "isFeatureConstant": false, + "isZoomConstant": true, + "type": "string" + }, + "outputs": ["", "0", "a"], + "serialized": [ + "to-string", + ["get", "p"] + ] + } +}