diff --git a/src/style-spec/expression/definitions/coalesce.js b/src/style-spec/expression/definitions/coalesce.js index 5d21091175c..d3d49526f76 100644 --- a/src/style-spec/expression/definitions/coalesce.js +++ b/src/style-spec/expression/definitions/coalesce.js @@ -1,6 +1,7 @@ // @flow const assert = require('assert'); +const {checkSubtype, ValueType} = require('../types'); import type { Expression } from '../expression'; import type ParsingContext from '../parsing_context'; @@ -21,18 +22,31 @@ class Coalesce implements Expression { return context.error("Expectected at least one argument."); } let outputType: Type = (null: any); - if (context.expectedType && context.expectedType.kind !== 'value') { - outputType = context.expectedType; + const expectedType = context.expectedType; + if (expectedType && expectedType.kind !== 'value') { + outputType = expectedType; } const parsedArgs = []; + for (const arg of args.slice(1)) { - const parsed = context.parse(arg, 1 + parsedArgs.length, outputType); + const parsed = context.parse(arg, 1 + parsedArgs.length, outputType, undefined, {omitTypeAnnotations: true}); if (!parsed) return null; outputType = outputType || parsed.type; parsedArgs.push(parsed); } assert(outputType); - return new Coalesce((outputType: any), parsedArgs); + + // Above, we parse arguments without inferred type annotation so that + // they don't produce a runtime error for `null` input, which would + // preempt the desired null-coalescing behavior. + // Thus, if any of our arguments would have needed an annotation, we + // need to wrap the enclosing coalesce expression with it instead. + const needsAnnotation = expectedType && + parsedArgs.some(arg => checkSubtype(expectedType, arg.type)); + + return needsAnnotation ? + new Coalesce(ValueType, parsedArgs) : + new Coalesce((outputType: any), parsedArgs); } evaluate(ctx: EvaluationContext) { diff --git a/src/style-spec/expression/parsing_context.js b/src/style-spec/expression/parsing_context.js index 25e42df7ea2..7dac65e7873 100644 --- a/src/style-spec/expression/parsing_context.js +++ b/src/style-spec/expression/parsing_context.js @@ -43,7 +43,20 @@ class ParsingContext { this.expectedType = expectedType; } - parse(expr: mixed, index?: number, expectedType?: ?Type, bindings?: Array<[string, Expression]>): ?Expression { + /** + * @param expr the JSON expression to parse + * @param index the optional argument index if this expression is an argument of a parent expression that's being parsed + * @param options + * @param options.omitTypeAnnotations set true to omit inferred type annotations. Caller beware: with this option set, the parsed expression's type will NOT satisfy `expectedType` if it would normally be wrapped in an inferred annotation. + * @private + */ + parse( + expr: mixed, + index?: number, + expectedType?: ?Type, + bindings?: Array<[string, Expression]>, + options: {omitTypeAnnotations?: boolean} = {} + ): ?Expression { let context = this; if (index) { context = context.concat(index, expectedType, bindings); @@ -68,26 +81,29 @@ class ParsingContext { if (Expr) { let parsed = Expr.parse(expr, context); if (!parsed) return null; - const expected = context.expectedType; - const actual = parsed.type; - if (expected) { - // When we expect a number, string, or boolean but have a - // Value, wrap it in a refining assertion, and when we expect - // a Color but have a String or Value, wrap it in "to-color" - // coercion. - const canAssert = expected.kind === 'string' || - expected.kind === 'number' || - expected.kind === 'boolean'; - - if (canAssert && actual.kind === 'value') { - parsed = new Assertion(expected, [parsed]); + + if (context.expectedType) { + const expected = context.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. + // Otherwise, we do static type-checking. + if ((expected.kind === 'string' || expected.kind === 'number' || expected.kind === 'boolean') && actual.kind === 'value') { + if (!options.omitTypeAnnotations) { + parsed = new Assertion(expected, [parsed]); + } } else if (expected.kind === 'array' && actual.kind === 'value') { - parsed = new ArrayAssertion(expected, parsed); + if (!options.omitTypeAnnotations) { + parsed = new ArrayAssertion(expected, parsed); + } } else if (expected.kind === 'color' && (actual.kind === 'value' || actual.kind === 'string')) { - parsed = new Coercion(expected, [parsed]); - } - - if (context.checkSubtype(expected, parsed.type)) { + if (!options.omitTypeAnnotations) { + parsed = new Coercion(expected, [parsed]); + } + } else if (context.checkSubtype(context.expectedType, parsed.type)) { return null; } } diff --git a/test/integration/expression-tests/coalesce/argument-type-mismatch/test.json b/test/integration/expression-tests/coalesce/argument-type-mismatch/test.json new file mode 100644 index 00000000000..0383fe078c4 --- /dev/null +++ b/test/integration/expression-tests/coalesce/argument-type-mismatch/test.json @@ -0,0 +1,17 @@ +{ + "propertySpec": {"type": "string"}, + "expression": [ + "coalesce", + ["get", "a"], + 5 + ], + "expected": { + "compiled": { + "result": "error", + "errors": [{ + "key": "[2]", + "error": "Expected string but found number instead." + }] + } + } +} diff --git a/test/integration/expression-tests/coalesce/inference/test.json b/test/integration/expression-tests/coalesce/inference/test.json new file mode 100644 index 00000000000..4fc57795369 --- /dev/null +++ b/test/integration/expression-tests/coalesce/inference/test.json @@ -0,0 +1,28 @@ +{ + "propertySpec": {"type": "string"}, + "expression": [ + "coalesce", + ["get", "a"], + ["get", "b"] + ], + "inputs": [ + [{}, {"properties": {"a": "one"}}], + [{}, {"properties": {"b": "two"}}], + [{}, {"properties": {"b": 5}}], + [{}, {"properties": {}}] + ], + "expected": { + "compiled": { + "isFeatureConstant": false, + "isZoomConstant": true, + "result": "success", + "type": "string" + }, + "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."} + ] + } +}