Omit inferred type annotations for 'coalesce' arguments

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) {

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,33 @@ class ParsingContext {
             if (Expr) {
                 let parsed = Expr.parse(expr, context);
                 if (!parsed) return null;
-                const expected = context.expectedType;
-                const actual = parsed.type;
-                if (expected) {
+
+                if (context.expectedType) {
+                    const expected = context.expectedType;
+                    const actual = parsed.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') {
-                        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]);
+                    const canAnnotate =
+                        (actual.kind === 'value' && (canAssert || expected.kind === 'array')) ||
+                        expected.kind === 'color' && (actual.kind === 'value' || actual.kind === 'string');
+
+                    if (canAnnotate && !options.omitTypeAnnotations) {
+                        if (canAssert) {
+                            parsed = new Assertion(expected, [parsed]);
+                        } else if (expected.kind === 'array') {
+                            parsed = new ArrayAssertion(expected, parsed);
+                        } else if (expected.kind === 'color') {
+                            parsed = new Coercion(expected, [parsed]);
+                        }
                     }
 
-                    if (context.checkSubtype(expected, parsed.type)) {
+                    if (!canAnnotate && context.checkSubtype(context.expectedType, parsed.type)) {
                         return null;
                     }
                 }

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."
+      }]
+    }
+  }
+}

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."}
+    ]
+  }
+}