From 9f80495288428ecfe46ce21402d0928eb544e9dc Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Tue, 28 Nov 2017 23:10:20 -0500 Subject: [PATCH] Avoid inferred type annotations for 'coalesce' arguments Fixes #5755 --- .../expression/definitions/array.js | 4 +- .../expression/definitions/assertion.js | 5 +- .../expression/definitions/coalesce.js | 26 ++++++++- .../expression/definitions/coercion.js | 5 +- src/style-spec/expression/parsing_context.js | 53 ++++++++++++------- 5 files changed, 69 insertions(+), 24 deletions(-) diff --git a/src/style-spec/expression/definitions/array.js b/src/style-spec/expression/definitions/array.js index 85b7ff683da..2441fa32bf7 100644 --- a/src/style-spec/expression/definitions/array.js +++ b/src/style-spec/expression/definitions/array.js @@ -27,10 +27,12 @@ const types = { class ArrayAssertion implements Expression { type: ArrayType; input: Expression; + _inferred: boolean; - constructor(type: ArrayType, input: Expression) { + constructor(type: ArrayType, input: Expression, inferred: boolean = false) { this.type = type; this.input = input; + this._inferred = inferred; } static parse(args: Array, context: ParsingContext): ?Expression { diff --git a/src/style-spec/expression/definitions/assertion.js b/src/style-spec/expression/definitions/assertion.js index 0ed23b13604..a40fc5bd736 100644 --- a/src/style-spec/expression/definitions/assertion.js +++ b/src/style-spec/expression/definitions/assertion.js @@ -29,9 +29,12 @@ class Assertion implements Expression { type: Type; args: Array; - constructor(type: Type, args: Array) { + _inferred: boolean; + + constructor(type: Type, args: Array, inferred: boolean = false) { this.type = type; this.args = args; + this._inferred = inferred; } static parse(args: Array, context: ParsingContext): ?Expression { diff --git a/src/style-spec/expression/definitions/coalesce.js b/src/style-spec/expression/definitions/coalesce.js index 5d21091175c..7fc7c7626ba 100644 --- a/src/style-spec/expression/definitions/coalesce.js +++ b/src/style-spec/expression/definitions/coalesce.js @@ -1,6 +1,10 @@ // @flow const assert = require('assert'); +const Assertion = require('./assertion'); +const ArrayAssertion = require('./array'); +const Coercion = require('./coercion'); +const {ValueType} = require('../types'); import type { Expression } from '../expression'; import type ParsingContext from '../parsing_context'; @@ -25,14 +29,32 @@ class Coalesce implements Expression { outputType = context.expectedType; } const parsedArgs = []; + + let needsOuterAnnotation = false; + for (const arg of args.slice(1)) { - const parsed = context.parse(arg, 1 + parsedArgs.length, outputType); + let parsed = context.parse(arg, 1 + parsedArgs.length, outputType); if (!parsed) return null; outputType = outputType || parsed.type; + + // strip off any inferred type assertions so that they don't + // produce a runtime error for `null` input, which would preempt + // the desired null-coalescing behavior + if ((parsed instanceof Assertion || parsed instanceof Coercion) && + parsed._inferred) { + needsOuterAnnotation = true; + parsed = parsed.args[0]; + } else if (parsed instanceof ArrayAssertion && parsed._inferred) { + needsOuterAnnotation = true; + parsed = parsed.input; + } + parsedArgs.push(parsed); } assert(outputType); - return new Coalesce((outputType: any), parsedArgs); + return needsOuterAnnotation ? + context.annotateType(outputType, new Coalesce(ValueType, parsedArgs)) : + new Coalesce((outputType: any), parsedArgs); } evaluate(ctx: EvaluationContext) { diff --git a/src/style-spec/expression/definitions/coercion.js b/src/style-spec/expression/definitions/coercion.js index d1d072f2458..3ff5f928a29 100644 --- a/src/style-spec/expression/definitions/coercion.js +++ b/src/style-spec/expression/definitions/coercion.js @@ -31,9 +31,12 @@ class Coercion implements Expression { type: Type; args: Array; - constructor(type: Type, args: Array) { + _inferred: boolean; + + constructor(type: Type, args: Array, inferred: boolean = false) { this.type = type; this.args = args; + this._inferred = inferred; } static parse(args: Array, context: ParsingContext): ?Expression { diff --git a/src/style-spec/expression/parsing_context.js b/src/style-spec/expression/parsing_context.js index 25e42df7ea2..34747685d26 100644 --- a/src/style-spec/expression/parsing_context.js +++ b/src/style-spec/expression/parsing_context.js @@ -68,26 +68,10 @@ 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]); - } else if (expected.kind === 'array' && actual.kind === 'value') { - parsed = new ArrayAssertion(expected, parsed); - } else if (expected.kind === 'color' && (actual.kind === 'value' || actual.kind === 'string')) { - parsed = new Coercion(expected, [parsed]); - } + parsed = context.annotateType(context.expectedType, parsed); - if (context.checkSubtype(expected, parsed.type)) { + if (context.expectedType) { + if (context.checkSubtype(context.expectedType, parsed.type)) { return null; } } @@ -118,6 +102,37 @@ class ParsingContext { } } + + /** + * Wrap the given expression in a type assertion if necessary. + * @private + */ + annotateType(expected: ?Type, expression: Expression) { + if (!expected) { + return expression; + } + + const actual = expression.type; + + // 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') { + return new Assertion(expected, [expression], true); + } else if (expected.kind === 'array' && actual.kind === 'value') { + return new ArrayAssertion(expected, expression, true); + } else if (expected.kind === 'color' && (actual.kind === 'value' || actual.kind === 'string')) { + return new Coercion(expected, [expression], true); + } + + return expression; + } + /** * Returns a copy of this context suitable for parsing the subexpression at * index `index`, optionally appending to 'let' binding map.