From eeb1f6b2e760b54cc7763182c9b5b4e2d1f213d9 Mon Sep 17 00:00:00 2001
From: Vladimir Agafonkin <agafonkin@gmail.com>
Date: Tue, 3 Mar 2020 14:41:35 +0200
Subject: [PATCH] simplify expression outputs validation (#9355)

---
 src/style-spec/expression/compound_expression.js          | 4 ++--
 src/style-spec/expression/definitions/assertion.js        | 5 ++---
 src/style-spec/expression/definitions/at.js               | 4 ++--
 src/style-spec/expression/definitions/case.js             | 7 ++-----
 src/style-spec/expression/definitions/coalesce.js         | 5 ++---
 src/style-spec/expression/definitions/coercion.js         | 5 ++---
 src/style-spec/expression/definitions/collator.js         | 8 ++++----
 src/style-spec/expression/definitions/comparison.js       | 4 ++--
 src/style-spec/expression/definitions/format.js           | 4 ++--
 .../expression/definitions/format_section_override.js     | 5 ++---
 src/style-spec/expression/definitions/image.js            | 4 ++--
 src/style-spec/expression/definitions/in.js               | 4 ++--
 src/style-spec/expression/definitions/interpolate.js      | 5 ++---
 src/style-spec/expression/definitions/length.js           | 4 ++--
 src/style-spec/expression/definitions/let.js              | 4 ++--
 src/style-spec/expression/definitions/literal.js          | 4 ++--
 src/style-spec/expression/definitions/match.js            | 7 ++-----
 src/style-spec/expression/definitions/number_format.js    | 4 ++--
 src/style-spec/expression/definitions/step.js             | 5 ++---
 src/style-spec/expression/definitions/var.js              | 4 ++--
 src/style-spec/expression/expression.js                   | 6 ++----
 src/style-spec/validate/validate_expression.js            | 2 +-
 22 files changed, 45 insertions(+), 59 deletions(-)

diff --git a/src/style-spec/expression/compound_expression.js b/src/style-spec/expression/compound_expression.js
index af251d7299e..e948b0b7689 100644
--- a/src/style-spec/expression/compound_expression.js
+++ b/src/style-spec/expression/compound_expression.js
@@ -39,8 +39,8 @@ class CompoundExpression implements Expression {
         this.args.forEach(fn);
     }
 
-    possibleOutputs() {
-        return [undefined];
+    outputDefined() {
+        return false;
     }
 
     serialize(): Array<mixed> {
diff --git a/src/style-spec/expression/definitions/assertion.js b/src/style-spec/expression/definitions/assertion.js
index 6ce5d88cf70..cfadac6d5d5 100644
--- a/src/style-spec/expression/definitions/assertion.js
+++ b/src/style-spec/expression/definitions/assertion.js
@@ -18,7 +18,6 @@ import {typeOf} from '../values';
 import type {Expression} from '../expression';
 import type ParsingContext from '../parsing_context';
 import type EvaluationContext from '../evaluation_context';
-import type {Value} from '../values';
 import type {Type} from '../types';
 
 const types = {
@@ -105,8 +104,8 @@ class Assertion implements Expression {
         this.args.forEach(fn);
     }
 
-    possibleOutputs(): Array<Value | void> {
-        return [].concat(...this.args.map((arg) => arg.possibleOutputs()));
+    outputDefined(): boolean {
+        return this.args.every(arg => arg.outputDefined());
     }
 
     serialize(): Array<mixed> {
diff --git a/src/style-spec/expression/definitions/at.js b/src/style-spec/expression/definitions/at.js
index a6c3b93dd4a..e777f43355e 100644
--- a/src/style-spec/expression/definitions/at.js
+++ b/src/style-spec/expression/definitions/at.js
@@ -58,8 +58,8 @@ class At implements Expression {
         fn(this.input);
     }
 
-    possibleOutputs() {
-        return [undefined];
+    outputDefined() {
+        return false;
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/case.js b/src/style-spec/expression/definitions/case.js
index 005b92c6155..c750aed5ed7 100644
--- a/src/style-spec/expression/definitions/case.js
+++ b/src/style-spec/expression/definitions/case.js
@@ -7,7 +7,6 @@ import {BooleanType} from '../types';
 import type {Expression} from '../expression';
 import type ParsingContext from '../parsing_context';
 import type EvaluationContext from '../evaluation_context';
-import type {Value} from '../values';
 import type {Type} from '../types';
 
 type Branches = Array<[Expression, Expression]>;
@@ -72,10 +71,8 @@ class Case implements Expression {
         fn(this.otherwise);
     }
 
-    possibleOutputs(): Array<Value | void> {
-        return []
-            .concat(...this.branches.map(([_, out]) => out.possibleOutputs()))
-            .concat(this.otherwise.possibleOutputs());
+    outputDefined(): boolean {
+        return this.branches.every(([_, out]) => out.outputDefined()) && this.otherwise.outputDefined();
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/coalesce.js b/src/style-spec/expression/definitions/coalesce.js
index 82661c6acde..9cfce9e987d 100644
--- a/src/style-spec/expression/definitions/coalesce.js
+++ b/src/style-spec/expression/definitions/coalesce.js
@@ -8,7 +8,6 @@ import ResolvedImage from '../types/resolved_image';
 import type {Expression} from '../expression';
 import type ParsingContext from '../parsing_context';
 import type EvaluationContext from '../evaluation_context';
-import type {Value} from '../values';
 import type {Type} from '../types';
 
 class Coalesce implements Expression {
@@ -80,8 +79,8 @@ class Coalesce implements Expression {
         this.args.forEach(fn);
     }
 
-    possibleOutputs(): Array<Value | void> {
-        return [].concat(...this.args.map((arg) => arg.possibleOutputs()));
+    outputDefined(): boolean {
+        return this.args.every(arg => arg.outputDefined());
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/coercion.js b/src/style-spec/expression/definitions/coercion.js
index cd4cd82df4a..3eb46f77662 100644
--- a/src/style-spec/expression/definitions/coercion.js
+++ b/src/style-spec/expression/definitions/coercion.js
@@ -13,7 +13,6 @@ import ResolvedImage from '../types/resolved_image';
 import type {Expression} from '../expression';
 import type ParsingContext from '../parsing_context';
 import type EvaluationContext from '../evaluation_context';
-import type {Value} from '../values';
 import type {Type} from '../types';
 
 const types = {
@@ -112,8 +111,8 @@ class Coercion implements Expression {
         this.args.forEach(fn);
     }
 
-    possibleOutputs(): Array<Value | void> {
-        return [].concat(...this.args.map((arg) => arg.possibleOutputs()));
+    outputDefined(): boolean {
+        return this.args.every(arg => arg.outputDefined());
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/collator.js b/src/style-spec/expression/definitions/collator.js
index f97f4870506..379d5d6047e 100644
--- a/src/style-spec/expression/definitions/collator.js
+++ b/src/style-spec/expression/definitions/collator.js
@@ -58,12 +58,12 @@ export default class CollatorExpression implements Expression {
         }
     }
 
-    possibleOutputs() {
+    outputDefined() {
         // Technically the set of possible outputs is the combinatoric set of Collators produced
-        // by all possibleOutputs of locale/caseSensitive/diacriticSensitive
+        // by all possible outputs of locale/caseSensitive/diacriticSensitive
         // But for the primary use of Collators in comparison operators, we ignore the Collator's
-        // possibleOutputs anyway, so we can get away with leaving this undefined for now.
-        return [undefined];
+        // possible outputs anyway, so we can get away with leaving this false for now.
+        return false;
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/comparison.js b/src/style-spec/expression/definitions/comparison.js
index 275b8be3a40..210df55ac7a 100644
--- a/src/style-spec/expression/definitions/comparison.js
+++ b/src/style-spec/expression/definitions/comparison.js
@@ -164,8 +164,8 @@ function makeComparison(op: ComparisonOperator, compareBasic, compareWithCollato
             }
         }
 
-        possibleOutputs() {
-            return [true, false];
+        outputDefined(): boolean {
+            return true;
         }
 
         serialize() {
diff --git a/src/style-spec/expression/definitions/format.js b/src/style-spec/expression/definitions/format.js
index ff14dbca986..a376cacff02 100644
--- a/src/style-spec/expression/definitions/format.js
+++ b/src/style-spec/expression/definitions/format.js
@@ -117,10 +117,10 @@ export default class FormatExpression implements Expression {
         }
     }
 
-    possibleOutputs() {
+    outputDefined() {
         // Technically the combinatoric set of all children
         // Usually, this.text will be undefined anyway
-        return [undefined];
+        return false;
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/format_section_override.js b/src/style-spec/expression/definitions/format_section_override.js
index 58065e036e0..86952258263 100644
--- a/src/style-spec/expression/definitions/format_section_override.js
+++ b/src/style-spec/expression/definitions/format_section_override.js
@@ -3,7 +3,6 @@
 import assert from 'assert';
 import type {Expression} from '../expression';
 import type EvaluationContext from '../evaluation_context';
-import type {Value} from '../values';
 import type {Type} from '../types';
 import type {ZoomConstantExpression} from '../../expression';
 import {NullType} from '../types';
@@ -43,8 +42,8 @@ export default class FormatSectionOverride<T> implements Expression {
     }
 
     // Cannot be statically evaluated, as the output depends on the evaluation context.
-    possibleOutputs(): Array<Value | void> {
-        return [undefined];
+    outputDefined() {
+        return false;
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/image.js b/src/style-spec/expression/definitions/image.js
index 6eb119ddd90..78ddaedb080 100644
--- a/src/style-spec/expression/definitions/image.js
+++ b/src/style-spec/expression/definitions/image.js
@@ -43,9 +43,9 @@ export default class ImageExpression implements Expression {
         fn(this.input);
     }
 
-    possibleOutputs() {
+    outputDefined() {
         // The output of image is determined by the list of available images in the evaluation context
-        return [undefined];
+        return false;
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/in.js b/src/style-spec/expression/definitions/in.js
index d4cb0af51bd..6a885d36892 100644
--- a/src/style-spec/expression/definitions/in.js
+++ b/src/style-spec/expression/definitions/in.js
@@ -80,8 +80,8 @@ class In implements Expression {
         fn(this.haystack);
     }
 
-    possibleOutputs() {
-        return [true, false];
+    outputDefined() {
+        return true;
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/interpolate.js b/src/style-spec/expression/definitions/interpolate.js
index c2f9d5deaff..98a1b65e4f2 100644
--- a/src/style-spec/expression/definitions/interpolate.js
+++ b/src/style-spec/expression/definitions/interpolate.js
@@ -11,7 +11,6 @@ import type {Stops} from '../stops';
 import type {Expression} from '../expression';
 import type ParsingContext from '../parsing_context';
 import type EvaluationContext from '../evaluation_context';
-import type {Value} from '../values';
 import type {Type} from '../types';
 
 export type InterpolationType =
@@ -187,8 +186,8 @@ class Interpolate implements Expression {
         }
     }
 
-    possibleOutputs(): Array<Value | void> {
-        return [].concat(...this.outputs.map((output) => output.possibleOutputs()));
+    outputDefined(): boolean {
+        return this.outputs.every(out => out.outputDefined());
     }
 
     serialize(): Array<mixed> {
diff --git a/src/style-spec/expression/definitions/length.js b/src/style-spec/expression/definitions/length.js
index 1265822373f..abaaf0bc049 100644
--- a/src/style-spec/expression/definitions/length.js
+++ b/src/style-spec/expression/definitions/length.js
@@ -47,8 +47,8 @@ class Length implements Expression {
         fn(this.input);
     }
 
-    possibleOutputs() {
-        return [undefined];
+    outputDefined() {
+        return false;
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/let.js b/src/style-spec/expression/definitions/let.js
index 6b278de4ce3..cfa449d8a7e 100644
--- a/src/style-spec/expression/definitions/let.js
+++ b/src/style-spec/expression/definitions/let.js
@@ -55,8 +55,8 @@ class Let implements Expression {
         return new Let(bindings, result);
     }
 
-    possibleOutputs() {
-        return this.result.possibleOutputs();
+    outputDefined() {
+        return this.result.outputDefined();
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/literal.js b/src/style-spec/expression/definitions/literal.js
index 5bc9578aee5..cdb45dd7fdb 100644
--- a/src/style-spec/expression/definitions/literal.js
+++ b/src/style-spec/expression/definitions/literal.js
@@ -49,8 +49,8 @@ class Literal implements Expression {
 
     eachChild() {}
 
-    possibleOutputs() {
-        return [this.value];
+    outputDefined() {
+        return true;
     }
 
     serialize(): Array<mixed> {
diff --git a/src/style-spec/expression/definitions/match.js b/src/style-spec/expression/definitions/match.js
index a0fef3ff736..ec4d1c30fae 100644
--- a/src/style-spec/expression/definitions/match.js
+++ b/src/style-spec/expression/definitions/match.js
@@ -8,7 +8,6 @@ import {ValueType, type Type} from '../types';
 import type {Expression} from '../expression';
 import type ParsingContext from '../parsing_context';
 import type EvaluationContext from '../evaluation_context';
-import type {Value} from '../values';
 
 // Map input label values to output expression index
 type Cases = {[number | string]: number};
@@ -112,10 +111,8 @@ class Match implements Expression {
         fn(this.otherwise);
     }
 
-    possibleOutputs(): Array<Value | void> {
-        return []
-            .concat(...this.outputs.map((out) => out.possibleOutputs()))
-            .concat(this.otherwise.possibleOutputs());
+    outputDefined(): boolean {
+        return this.outputs.every(out => out.outputDefined()) && this.otherwise.outputDefined();
     }
 
     serialize(): Array<mixed> {
diff --git a/src/style-spec/expression/definitions/number_format.js b/src/style-spec/expression/definitions/number_format.js
index 0caf5550314..754503fb197 100644
--- a/src/style-spec/expression/definitions/number_format.js
+++ b/src/style-spec/expression/definitions/number_format.js
@@ -119,8 +119,8 @@ export default class NumberFormat implements Expression {
         }
     }
 
-    possibleOutputs() {
-        return [undefined];
+    outputDefined() {
+        return false;
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/step.js b/src/style-spec/expression/definitions/step.js
index c00bb9b99dd..3e2058290b5 100644
--- a/src/style-spec/expression/definitions/step.js
+++ b/src/style-spec/expression/definitions/step.js
@@ -8,7 +8,6 @@ import type {Stops} from '../stops';
 import type {Expression} from '../expression';
 import type ParsingContext from '../parsing_context';
 import type EvaluationContext from '../evaluation_context';
-import type {Value} from '../values';
 import type {Type} from '../types';
 
 class Step implements Expression {
@@ -102,8 +101,8 @@ class Step implements Expression {
         }
     }
 
-    possibleOutputs(): Array<Value | void> {
-        return [].concat(...this.outputs.map((output) => output.possibleOutputs()));
+    outputDefined(): boolean {
+        return this.outputs.every(out => out.outputDefined());
     }
 
     serialize() {
diff --git a/src/style-spec/expression/definitions/var.js b/src/style-spec/expression/definitions/var.js
index a06760631a6..aa97e6cc2e4 100644
--- a/src/style-spec/expression/definitions/var.js
+++ b/src/style-spec/expression/definitions/var.js
@@ -34,8 +34,8 @@ class Var implements Expression {
 
     eachChild() {}
 
-    possibleOutputs() {
-        return [undefined];
+    outputDefined() {
+        return false;
     }
 
     serialize() {
diff --git a/src/style-spec/expression/expression.js b/src/style-spec/expression/expression.js
index 73a1329f977..3404686ede6 100644
--- a/src/style-spec/expression/expression.js
+++ b/src/style-spec/expression/expression.js
@@ -1,7 +1,6 @@
 // @flow
 
 import type {Type} from './types';
-import type {Value} from './values';
 import type ParsingContext from './parsing_context';
 import type EvaluationContext from './evaluation_context';
 
@@ -16,10 +15,9 @@ export interface Expression {
 
     /**
      * Statically analyze the expression, attempting to enumerate possible outputs. Returns
-     * an array of values plus the sentinel value `undefined`, used to indicate that the
-     * complete set of outputs is statically undecidable.
+     * false if the complete set of outputs is statically undecidable, otherwise true.
      */
-    possibleOutputs(): Array<Value | void>;
+    outputDefined(): boolean;
 
     serialize(): SerializedExpression;
 }
diff --git a/src/style-spec/validate/validate_expression.js b/src/style-spec/validate/validate_expression.js
index 573a9ac3d02..63a73f441c1 100644
--- a/src/style-spec/validate/validate_expression.js
+++ b/src/style-spec/validate/validate_expression.js
@@ -17,7 +17,7 @@ export default function validateExpression(options: any): Array<ValidationError>
     const expressionObj = (expression.value: any).expression || (expression.value: any)._styleExpression.expression;
 
     if (options.expressionContext === 'property' && (options.propertyKey === 'text-font') &&
-        expressionObj.possibleOutputs().indexOf(undefined) !== -1) {
+        !expressionObj.outputDefined()) {
         return [new ValidationError(options.key, options.value, `Invalid data expression for "${options.propertyKey}". Output values must be contained as literals within the expression.`)];
     }