Fold compound expressions with literal arguments into literals (#5279)

* Refactor Expression.visitor => Expression.eachChild * Fix error message keys in ["step"] curves * Fold compound expressions with literal arguments into literals * Fixup UPDATE=1 behavior in expression test harness * Consolidate some is*Constant logic
mapbox · Sep 18, 2017 · 6ab53dc · 6ab53dc
1 parent 1cb1cff
commit 6ab53dc
Show file tree

Hide file tree

Showing 23 changed files with 223 additions and 120 deletions.
diff --git a/flow-typed/visitor.js b/flow-typed/visitor.js
diff --git a/src/style-spec/function/compile.js b/src/style-spec/function/compile.js
@@ -8,9 +8,12 @@ const {
     CompilationContext
 } = require('./expression');
 const parseExpression = require('./parse_expression');
-const { CompoundExpression } = require('./compound_expression');
 const definitions = require('./definitions');
 const evaluationContext = require('./evaluation_context');
+const {
+    isFeatureConstant,
+    isZoomConstant
+} = require('./is_constant');
 
 import type { Type } from './types.js';
 import type { Expression, ParsingError } from './expression.js';
@@ -78,34 +81,3 @@ function compileExpression(
     };
 }
 
-function isFeatureConstant(e: Expression) {
-    let result = true;
-    e.accept({
-        visit: (expression) => {
-            if (expression instanceof CompoundExpression) {
-                if (expression.name === 'get') {
-                    result = result && (expression.args.length > 1);
-                } else if (expression.name === 'has') {
-                    result = result && (expression.args.length > 1);
-                } else {
-                    result = result && !(
-                        expression.name === 'properties' ||
-                        expression.name === 'geometry-type' ||
-                        expression.name === 'id'
-                    );
-                }
-            }
-        }
-    });
-    return result;
-}
-
-function isZoomConstant(e: Expression) {
-    let result = true;
-    e.accept({
-        visit: (expression) => {
-            if (expression.name === 'zoom') result = false;
-        }
-    });
-    return result;
-}
diff --git a/src/style-spec/function/compound_expression.js b/src/style-spec/function/compound_expression.js
@@ -41,9 +41,8 @@ class CompoundExpression implements Expression {
         return [ name ].concat(args);
     }
 
-    accept(visitor: Visitor<Expression>) {
-        visitor.visit(this);
-        this.args.forEach(a => a.accept(visitor));
+    eachChild(fn: (Expression) => void) {
+        this.args.forEach(fn);
     }
 
     static parse(args: Array<mixed>, context: ParsingContext): ?Expression {

diff --git a/src/style-spec/function/definitions/array.js b/src/style-spec/function/definitions/array.js
@@ -78,9 +78,8 @@ class ArrayAssertion implements Expression {
         }
     }
 
-    accept(visitor: Visitor<Expression>) {
-        visitor.visit(this);
-        this.input.accept(visitor);
+    eachChild(fn: (Expression) => void) {
+        fn(this.input);
     }
 }
 

diff --git a/src/style-spec/function/definitions/assertion.js b/src/style-spec/function/definitions/assertion.js
@@ -23,12 +23,12 @@ const types = {
 class Assertion implements Expression {
     key: string;
     type: Type;
-    inputs: Array<Expression>;
+    args: Array<Expression>;
 
-    constructor(key: string, type: Type, inputs: Array<Expression>) {
+    constructor(key: string, type: Type, args: Array<Expression>) {
         this.key = key;
         this.type = type;
-        this.inputs = inputs;
+        this.args = args;
     }
 
     static parse(args: Array<mixed>, context: ParsingContext): ?Expression {
@@ -40,33 +40,32 @@ class Assertion implements Expression {
 
         const type = types[name];
 
-        const inputs = [];
+        const parsed = [];
         for (let i = 1; i < args.length; i++) {
             const input = parseExpression(args[i], context.concat(i, ValueType));
             if (!input) return null;
-            inputs.push(input);
+            parsed.push(input);
         }
 
-        return new Assertion(context.key, type, inputs);
+        return new Assertion(context.key, type, parsed);
     }
 
     compile(ctx: CompilationContext) {
         const jsType = JSON.stringify(this.type.kind.toLowerCase());
-        const inputs = [];
-        for (const input of this.inputs) {
-            inputs.push(ctx.addExpression(input.compile(ctx)));
+        const args = [];
+        for (const input of this.args) {
+            args.push(ctx.addExpression(input.compile(ctx)));
         }
-        const inputsVar = ctx.addVariable(`[${inputs.join(',')}]`);
+        const inputsVar = ctx.addVariable(`[${args.join(',')}]`);
         return `$this.asJSType(${jsType}, ${inputsVar})`;
     }
 
     serialize() {
-        return [ this.type.kind.toLowerCase() ].concat(this.inputs.map(i => i.serialize()));
+        return [ this.type.kind.toLowerCase() ].concat(this.args.map(i => i.serialize()));
     }
 
-    accept(visitor: Visitor<Expression>) {
-        visitor.visit(this);
-        this.inputs.forEach(input => input.accept(visitor));
+    eachChild(fn: (Expression) => void) {
+        this.args.forEach(fn);
     }
 }
 

diff --git a/src/style-spec/function/definitions/at.js b/src/style-spec/function/definitions/at.js
@@ -44,10 +44,9 @@ class At implements Expression {
         return [ 'at', this.index.serialize(), this.input.serialize() ];
     }
 
-    accept(visitor: Visitor<Expression>) {
-        visitor.visit(this);
-        this.index.accept(visitor);
-        this.input.accept(visitor);
+    eachChild(fn: (Expression) => void) {
+        fn(this.index);
+        fn(this.input);
     }
 }
 

diff --git a/src/style-spec/function/definitions/case.js b/src/style-spec/function/definitions/case.js
@@ -73,13 +73,12 @@ class Case implements Expression {
         return result;
     }
 
-    accept(visitor: Visitor<Expression>) {
-        visitor.visit(this);
+    eachChild(fn: (Expression) => void) {
         for (const [test, expression] of this.branches) {
-            test.accept(visitor);
-            expression.accept(visitor);
+            fn(test);
+            fn(expression);
         }
-        this.otherwise.accept(visitor);
+        fn(this.otherwise);
     }
 }
 

diff --git a/src/style-spec/function/definitions/coalesce.js b/src/style-spec/function/definitions/coalesce.js
@@ -50,11 +50,8 @@ class Coalesce implements Expression {
         return ['coalesce'].concat(this.args.map(a => a.serialize()));
     }
 
-    accept(visitor: Visitor<Expression>) {
-        visitor.visit(this);
-        for (const arg of this.args) {
-            arg.accept(visitor);
-        }
+    eachChild(fn: (Expression) => void) {
+        this.args.forEach(fn);
     }
 }
 

diff --git a/src/style-spec/function/definitions/coercion.js b/src/style-spec/function/definitions/coercion.js
@@ -26,12 +26,12 @@ const types = {
 class Coercion implements Expression {
     key: string;
     type: Type;
-    inputs: Array<Expression>;
+    args: Array<Expression>;
 
-    constructor(key: string, type: Type, inputs: Array<Expression>) {
+    constructor(key: string, type: Type, args: Array<Expression>) {
         this.key = key;
         this.type = type;
-        this.inputs = inputs;
+        this.args = args;
     }
 
     static parse(args: Array<mixed>, context: ParsingContext): ?Expression {
@@ -43,32 +43,31 @@ class Coercion implements Expression {
 
         const type = types[name];
 
-        const inputs = [];
+        const parsed = [];
         for (let i = 1; i < args.length; i++) {
             const input = parseExpression(args[i], context.concat(i, ValueType));
             if (!input) return null;
-            inputs.push(input);
+            parsed.push(input);
         }
 
-        return new Coercion(context.key, type, inputs);
+        return new Coercion(context.key, type, parsed);
     }
 
     compile(ctx: CompilationContext) {
-        const inputs = [];
-        for (const input of this.inputs) {
-            inputs.push(ctx.addExpression(input.compile(ctx)));
+        const args = [];
+        for (const input of this.args) {
+            args.push(ctx.addExpression(input.compile(ctx)));
         }
-        const inputsVar = ctx.addVariable(`[${inputs.join(',')}]`);
+        const inputsVar = ctx.addVariable(`[${args.join(',')}]`);
         return `$this.to${this.type.kind}(${inputsVar})`;
     }
 
     serialize() {
-        return [ `to-${this.type.kind.toLowerCase()}` ].concat(this.inputs.map(i => i.serialize()));
+        return [ `to-${this.type.kind.toLowerCase()}` ].concat(this.args.map(i => i.serialize()));
     }
 
-    accept(visitor: Visitor<Expression>) {
-        visitor.visit(this);
-        this.inputs.forEach(input => input.accept(visitor));
+    eachChild(fn: (Expression) => void) {
+        this.args.forEach(fn);
     }
 }
 

diff --git a/src/style-spec/function/definitions/contains.js b/src/style-spec/function/definitions/contains.js
@@ -50,10 +50,9 @@ class Contains implements Expression {
         return [ 'contains', this.value.serialize(), this.array.serialize() ];
     }
 
-    accept(visitor: Visitor<Expression>) {
-        visitor.visit(this);
-        this.array.accept(visitor);
-        this.value.accept(visitor);
+    eachChild(fn: (Expression) => void) {
+        fn(this.array);
+        fn(this.value);
     }
 }
 

diff --git a/src/style-spec/function/definitions/curve.js b/src/style-spec/function/definitions/curve.js
@@ -113,8 +113,8 @@ class Curve implements Expression {
             const label = rest[i];
             const value = rest[i + 1];
 
-            const labelKey = isStep ? i + 4 : i + 3;
-            const valueKey = isStep ? i + 5 : i + 4;
+            const labelKey = isStep ? i + 2 : i + 3;
+            const valueKey = isStep ? i + 3 : i + 4;
 
             if (typeof label !== 'number') {
                 return context.error('Input/output pairs for "curve" expressions must be defined using literal numeric values (not computed expressions) for the input values.', labelKey);
@@ -176,11 +176,10 @@ class Curve implements Expression {
         return result;
     }
 
-    accept(visitor: Visitor<Expression>) {
-        visitor.visit(this);
-        this.input.accept(visitor);
+    eachChild(fn: (Expression) => void) {
+        fn(this.input);
         for (const [ , expression] of this.stops) {
-            expression.accept(visitor);
+            fn(expression);
         }
     }
 }

diff --git a/src/style-spec/function/definitions/let.js b/src/style-spec/function/definitions/let.js
@@ -33,12 +33,11 @@ class Let implements Expression {
         return serialized;
     }
 
-    accept(visitor: Visitor<Expression>) {
-        visitor.visit(this);
+    eachChild(fn: (Expression) => void) {
         for (const binding of this.bindings) {
-            binding[1].accept(visitor);
+            fn(binding[1]);
         }
-        this.result.accept(visitor);
+        fn(this.result);
     }
 
     static parse(args: Array<mixed>, context: ParsingContext) {

diff --git a/src/style-spec/function/definitions/literal.js b/src/style-spec/function/definitions/literal.js
@@ -4,7 +4,7 @@ const { Color, isValue, typeOf } = require('../values');
 
 import type { Type } from '../types';
 import type { Value }  from '../values';
-import type { Expression, ParsingContext }  from '../expression';
+import type { Expression, ParsingContext, CompilationContext }  from '../expression';
 
 const u2028 = /\u2028/g;
 const u2029 = /\u2029/g;
@@ -45,9 +45,19 @@ class Literal implements Expression {
         return new Literal(context.key, type, value);
     }
 
-    compile() {
-        const value = Literal.compile(this.value);
-        return typeof this.value === 'object' ?  `(${value})` : value;
+    compile(ctx: CompilationContext) {
+        let value;
+        if (this.type.kind === 'Color') {
+            value = `(new $this.Color(${(this.value: any).join(', ')}))`;
+        } else {
+            value = Literal.compile(this.value);
+        }
+
+        if (typeof this.value === 'object' && this.value !== null) {
+            return ctx.addVariable(value);
+        } else {
+            return value;
+        }
     }
 
     static compile(value: Value) {
@@ -72,7 +82,7 @@ class Literal implements Expression {
         }
     }
 
-    accept(visitor: Visitor<Expression>) { visitor.visit(this); }
+    eachChild() {}
 }
 
 module.exports = Literal;
diff --git a/src/style-spec/function/definitions/match.js b/src/style-spec/function/definitions/match.js
@@ -135,13 +135,10 @@ class Match implements Expression {
         return result;
     }
 
-    accept(visitor: Visitor<Expression>) {
-        visitor.visit(this);
-        this.input.accept(visitor);
-        for (const output of this.outputs) {
-            output.accept(visitor);
-        }
-        this.otherwise.accept(visitor);
+    eachChild(fn: (Expression) => void) {
+        fn(this.input);
+        this.outputs.forEach(fn);
+        fn(this.otherwise);
     }
 }
 

diff --git a/src/style-spec/function/definitions/var.js b/src/style-spec/function/definitions/var.js
@@ -35,7 +35,7 @@ class Var implements Expression {
         return [this.name];
     }
 
-    accept(visitor: Visitor<Expression>) { visitor.visit(this); }
+    eachChild() {}
 }
 
 

diff --git a/src/style-spec/function/evaluation_context.js b/src/style-spec/function/evaluation_context.js
@@ -62,6 +62,7 @@ function ensure(condition: any, message: string) {
 module.exports = () => ({
     types: types,
 
+    Color: Color, // used for compiling color literals
     ensure: ensure,
     error: (msg: string) => ensure(false, msg),
 

diff --git a/src/style-spec/function/expression.js b/src/style-spec/function/expression.js
@@ -14,7 +14,7 @@ export interface Expression {
     compile(CompilationContext): string; // eslint-disable-line no-use-before-define
 
     serialize(): any;
-    accept(Visitor<Expression>): void;
+    eachChild(fn: Expression => void): void;
 }
 
 class ParsingError extends Error {
-Original file line number
+Diff line change
@@ Expand Up / @@ -35,7 +35,7 @@ class Var implements Expression { @@
             return [this.name];
         }
-        accept(visitor: Visitor<Expression>) { visitor.visit(this); }
+        eachChild() {}
     }
@@ Expand Down @@