Simplify parse(), ParsingContext, add some internal docs

mapbox · Aug 5, 2017 · db7c15c · db7c15c
1 parent 0da019a
commit db7c15c
Show file tree

Hide file tree

Showing 11 changed files with 100 additions and 58 deletions.
diff --git a/src/style-spec/function/compile.js b/src/style-spec/function/compile.js
@@ -54,8 +54,8 @@ function compileExpression(
     expr: mixed,
     expectedType?: Type
 ): CompiledExpression | CompileErrors {
-    const context = new ParsingContext(definitions);
-    const parsed = parseExpression(expr, context, expectedType);
+    const context = new ParsingContext(definitions, [], expectedType || null);
+    const parsed = parseExpression(expr, context);
     if (!parsed) {
         assert(context.errors.length > 0);
         return {

diff --git a/src/style-spec/function/compound_expression.js b/src/style-spec/function/compound_expression.js
@@ -63,7 +63,7 @@ class CompoundExpression implements Expression {
         // First parse all the args
         const parsedArgs: Array<Expression> = [];
         for (const arg of args.slice(1)) {
-            const parsed = parseExpression(arg, context.concat(1 + parsedArgs.length, op));
+            const parsed = parseExpression(arg, context.concat(1 + parsedArgs.length));
             if (!parsed) return null;
             parsedArgs.push(parsed);
         }
@@ -78,7 +78,10 @@ class CompoundExpression implements Expression {
         let signatureContext: ParsingContext = (null: any);
 
         for (const [params, compileFromArgs] of overloads) {
-            signatureContext = new ParsingContext(context.definitions, context.path, context.ancestors, context.scope);
+            // Use a fresh context for each attempted signature so that, if
+            // we eventually succeed, we haven't polluted `context.errors`.
+            signatureContext = new ParsingContext(context.definitions, context.path, null, context.scope);
+
             if (Array.isArray(params)) {
                 if (params.length !== parsedArgs.length) {
                     signatureContext.error(`Expected ${params.length} arguments, but found ${parsedArgs.length} instead.`);
@@ -89,7 +92,7 @@ class CompoundExpression implements Expression {
             for (let i = 0; i < parsedArgs.length; i++) {
                 const expected = Array.isArray(params) ? params[i] : params.type;
                 const arg = parsedArgs[i];
-                match(expected, arg.type, signatureContext.concat(i + 1, op));
+                match(expected, arg.type, signatureContext.concat(i + 1));
             }
 
             if (signatureContext.errors.length === 0) {

diff --git a/src/style-spec/function/definitions/array.js b/src/style-spec/function/definitions/array.js
@@ -57,7 +57,7 @@ class ArrayAssertion implements Expression {
 
         const type = array(itemType, N);
 
-        const input = parseExpression(args[args.length - 1], context.concat(args.length - 1, 'array'), ValueType);
+        const input = parseExpression(args[args.length - 1], context.concat(args.length - 1, ValueType));
         if (!input) return null;
 
         return new ArrayAssertion(context.key, type, input);

diff --git a/src/style-spec/function/definitions/at.js b/src/style-spec/function/definitions/at.js
@@ -23,12 +23,12 @@ class At implements Expression {
         this.input = input;
     }
 
-    static parse(args: Array<mixed>, context: ParsingContext, expectedType?: Type) {
+    static parse(args: Array<mixed>, context: ParsingContext) {
         if (args.length !== 3)
             return context.error(`Expected 2 arguments, but found ${args.length - 1} instead.`);
 
-        const index = parseExpression(args[1], context.concat(1, 'at'), NumberType);
-        const input = parseExpression(args[2], context.concat(2, 'at'), array(expectedType || ValueType));
+        const index = parseExpression(args[1], context.concat(1, NumberType));
+        const input = parseExpression(args[2], context.concat(2, array(context.expectedType || ValueType)));
 
         if (!index || !input) return null;
 

diff --git a/src/style-spec/function/definitions/case.js b/src/style-spec/function/definitions/case.js
@@ -23,29 +23,29 @@ class Case implements Expression {
         this.otherwise = otherwise;
     }
 
-    static parse(args: Array<mixed>, context: ParsingContext, expectedType?: Type) {
+    static parse(args: Array<mixed>, context: ParsingContext) {
         args = args.slice(1);
         if (args.length < 3)
             return context.error(`Expected at least 3 arguments, but found only ${args.length}.`);
         if (args.length % 2 === 0)
             return context.error(`Expected an odd number of arguments.`);
 
-        let outputType: ?Type = expectedType;
+        let outputType: ?Type = context.expectedType;
 
         const branches = [];
         for (let i = 0; i < args.length - 1; i += 2) {
-            const test = parseExpression(args[i], context.concat(i, 'case'), BooleanType);
+            const test = parseExpression(args[i], context.concat(i, BooleanType));
             if (!test) return null;
 
-            const result = parseExpression(args[i + 1], context.concat(i + 1, 'case'), outputType);
+            const result = parseExpression(args[i + 1], context.concat(i + 1, outputType));
             if (!result) return null;
 
             branches.push([test, result]);
 
             outputType = outputType || result.type;
         }
 
-        const otherwise = parseExpression(args[args.length - 1], context.concat(args.length, 'case'), outputType);
+        const otherwise = parseExpression(args[args.length - 1], context.concat(args.length, outputType));
         if (!otherwise) return null;
 
         assert(outputType);

diff --git a/src/style-spec/function/definitions/coalesce.js b/src/style-spec/function/definitions/coalesce.js
@@ -17,13 +17,13 @@ class Coalesce implements Expression {
         this.args = args;
     }
 
-    static parse(args: Array<mixed>, context: ParsingContext, expectedType?: Type) {
+    static parse(args: Array<mixed>, context: ParsingContext) {
         args = args.slice(1);
-        let outputType = expectedType;
+        let outputType = context.expectedType;
         const parsedArgs = [];
         for (const arg of args) {
-            const argContext = context.concat(1 + parsedArgs.length, 'coalesce');
-            const parsed = parseExpression(arg, argContext, outputType);
+            const argContext = context.concat(1 + parsedArgs.length, outputType);
+            const parsed = parseExpression(arg, argContext);
             if (!parsed) return null;
             outputType = outputType || parsed.type;
             parsedArgs.push(parsed);

diff --git a/src/style-spec/function/definitions/curve.js b/src/style-spec/function/definitions/curve.js
@@ -33,7 +33,7 @@ class Curve implements Expression {
         this.stops = stops;
     }
 
-    static parse(args: Array<mixed>, context: ParsingContext, expectedType?: Type) {
+    static parse(args: Array<mixed>, context: ParsingContext) {
         args = args.slice(1);
         if (args.length < 4)
             return context.error(`Expected at least 2 arguments, but found only ${args.length}.`);
@@ -73,12 +73,12 @@ class Curve implements Expression {
             return context.error(`Unknown interpolation type ${String(interpolation[0])}`, 1, 0);
         }
 
-        input = parseExpression(input, context.concat(2, 'curve'), NumberType);
+        input = parseExpression(input, context.concat(2, NumberType));
         if (!input) return null;
 
         const stops: Stops = [];
 
-        let outputType: Type = (expectedType: any);
+        let outputType: Type = (context.expectedType: any);
         for (let i = 0; i < rest.length; i += 2) {
             const label = rest[i];
             const value = rest[i + 1];
@@ -91,7 +91,7 @@ class Curve implements Expression {
                 return context.error('Input/output pairs for "curve" expressions must be arranged with input values in strictly ascending order.', i + 3);
             }
 
-            const parsed = parseExpression(value, context.concat(i + 4, 'curve'), outputType);
+            const parsed = parseExpression(value, context.concat(i + 4, outputType));
             if (!parsed) return null;
             outputType = outputType || parsed.type;
             stops.push([label, parsed]);

diff --git a/src/style-spec/function/definitions/let.js b/src/style-spec/function/definitions/let.js
@@ -69,13 +69,13 @@ class Let implements Expression {
             if (context.definitions[name])
                 return context.error(`"${name}" is reserved, so it cannot not be used as a "let" binding.`, i + 1);
 
-            const value = parseExpression(args[i + 1], context.concat(i + 2, 'let.binding'));
+            const value = parseExpression(args[i + 1], context.concat(i + 2));
             if (!value) return null;
 
             bindings.push([name, value]);
         }
 
-        const resultContext = context.concat(args.length, 'let.result', bindings);
+        const resultContext = context.concat(args.length, undefined, bindings);
         const result = parseExpression(args[args.length - 1], resultContext);
         if (!result) return null;
 

diff --git a/src/style-spec/function/definitions/literal.js b/src/style-spec/function/definitions/literal.js
@@ -17,7 +17,7 @@ class Literal implements Expression {
         this.value = value;
     }
 
-    static parse(args: Array<mixed>, context: ParsingContext, expectedType?: ?Type) {
+    static parse(args: Array<mixed>, context: ParsingContext) {
         if (args.length !== 2)
             return context.error(`'literal' expression requires exactly one argument, but found ${args.length - 1} instead.`);
 
@@ -28,14 +28,15 @@ class Literal implements Expression {
         let type = typeOf(value);
 
         // special case: infer the item type if possible for zero-length arrays
+        const expected = context.expectedType;
         if (
             type.kind === 'array' &&
             type.N === 0 &&
-            expectedType &&
-            expectedType.kind === 'array' &&
-            (typeof expectedType.N !== 'number' || expectedType.N === 0)
+            expected &&
+            expected.kind === 'array' &&
+            (typeof expected.N !== 'number' || expected.N === 0)
         ) {
-            type = expectedType;
+            type = expected;
         }
 
         return new Literal(context.key, type, value);

diff --git a/src/style-spec/function/definitions/match.js b/src/style-spec/function/definitions/match.js
@@ -27,15 +27,15 @@ class Match implements Expression {
         this.otherwise = otherwise;
     }
 
-    static parse(args: Array<mixed>, context: ParsingContext, expectedType?: Type) {
+    static parse(args: Array<mixed>, context: ParsingContext) {
         args = args.slice(1);
         if (args.length < 2)
             return context.error(`Expected at least 2 arguments, but found only ${args.length}.`);
         if (args.length % 2 !== 0)
             return context.error(`Expected an even number of arguments.`);
 
         let inputType;
-        let outputType = expectedType;
+        let outputType = context.expectedType;
         const branches = [];
         for (let i = 1; i < args.length - 1; i += 2) {
             let labels = args[i];
@@ -45,7 +45,7 @@ class Match implements Expression {
                 labels = [labels];
             }
 
-            const labelContext = context.concat(i + 1, 'match');
+            const labelContext = context.concat(i + 1);
             if (labels.length === 0) {
                 return labelContext.error('Expected at least one branch label.');
             }
@@ -60,17 +60,17 @@ class Match implements Expression {
                 }
             }
 
-            const result = parseExpression(value, context.concat(i + 1, 'match'), outputType);
+            const result = parseExpression(value, context.concat(i + 1, outputType));
             if (!result) return null;
             outputType = outputType || result.type;
 
             branches.push([(labels: any), result]);
         }
 
-        const input = parseExpression(args[0], context.concat(1, 'match'), inputType);
+        const input = parseExpression(args[0], context.concat(1, inputType));
         if (!input) return null;
 
-        const otherwise = parseExpression(args[args.length - 1], context.concat(args.length, 'match'), outputType);
+        const otherwise = parseExpression(args[args.length - 1], context.concat(args.length, outputType));
         if (!otherwise) return null;
 
         assert(inputType && outputType);

diff --git a/src/style-spec/function/expression.js b/src/style-spec/function/expression.js
@@ -19,10 +19,7 @@ export interface Expression {
     key: string;
     +type: Type;
 
-    // @param args The JSON representation of the expression to be parsed
-    // @param context
-    // @param [expectedType] The expected type of this expression. Provided only to allow parse() implementations to infer argument types: parse() need not check that the output type of the parsed expression matches `expectedType`.
-    static parse(args: Array<mixed>, context: ParsingContext, expectedType?: ?Type): ?Expression;
+    static parse(args: Array<mixed>, context: ParsingContext): ?Expression;
 
     compile(): string;
 
@@ -41,6 +38,10 @@ class ParsingError extends Error {
     }
 }
 
+/**
+ * Tracks `let` bindings during expression parsing.
+ * @private
+ */
 class Scope {
     parent: ?Scope;
     bindings: {[string]: Expression};
@@ -68,43 +69,81 @@ class Scope {
     }
 }
 
+/**
+ * State associated parsing at a given point in an expression tree.
+ * @private
+ */
 class ParsingContext {
-    key: string;
-    path: Array<number>;
-    ancestors: Array<string>;
     definitions: {[string]: Class<Expression>};
+    path: Array<number>;
+    key: string;
     scope: Scope;
     errors: Array<ParsingError>;
 
-    constructor(definitions: *, path: Array<number> = [], ancestors: * = [], scope: Scope = new Scope(), errors: Array<ParsingError> = []) {
+    // The expected type of this expression. Provided only to allow parse()
+    // implementations to infer argument types: parse() need not check that the
+    // output type of the parsed expression matches `expectedType`.
+    expectedType: ?Type;
+
+    constructor(
+        definitions: *,
+        path: Array<number> = [],
+        expectedType: ?Type,
+        scope: Scope = new Scope(),
+        errors: Array<ParsingError> = []
+    ) {
         this.definitions = definitions;
         this.path = path;
         this.key = path.map(part => `[${part}]`).join('');
-        this.ancestors = ancestors;
         this.scope = scope;
         this.errors = errors;
+        this.expectedType = expectedType;
     }
 
-    // Returns a copy of this context suitable for parsing the subexpression at
-    // index `index`, optionally appending to 'let' binding map.
-    //
-    // Note that `errors` property, intended for collecting errors while
-    // parsing, is copied by reference rather than cloned.
-    concat(index: number, expressionName: string, bindings?: Array<[string, Expression]>) {
+    /**
+     * Returns a copy of this context suitable for parsing the subexpression at
+     * index `index`, optionally appending to 'let' binding map.
+     *
+     * Note that `errors` property, intended for collecting errors while
+     * parsing, is copied by reference rather than cloned.
+     * @private
+     */
+    concat(index: number, expectedType?: ?Type, bindings?: Array<[string, Expression]>) {
         const path = typeof index === 'number' ? this.path.concat(index) : this.path;
-        const ancestors = expressionName ? this.ancestors.concat(expressionName) : this.ancestors;
         const scope = bindings ? this.scope.concat(bindings) : this.scope;
-        return new ParsingContext(this.definitions, path, ancestors, scope, this.errors);
+        return new ParsingContext(
+            this.definitions,
+            path,
+            expectedType || null,
+            scope,
+            this.errors
+        );
     }
 
+    /**
+     * Push a parsing (or type checking) error into the `this.errors`
+     * @param error The message
+     * @param keys Optionally specify the source of the error at a child
+     * of the current expression at `this.key`.
+     * @private
+     */
     error(error: string, ...keys: Array<number>) {
         const key = `${this.key}${keys.map(k => `[${k}]`).join('')}`;
         this.errors.push(new ParsingError(key, error));
         return null;
     }
 }
 
-function parseExpression(expr: mixed, context: ParsingContext, expectedType?: ?Type) : ?Expression {
+/**
+ * Parse the given JSON expression.
+ *
+ * @param expectedType If provided, the parsed expression will be checked
+ * against this type.  Additionally, `expectedType` will be pssed to
+ * Expression#parse(), wherein it may be used to infer child expression types
+ *
+ * @private
+ */
+function parseExpression(expr: mixed, context: ParsingContext) : ?Expression {
     if (expr === null || typeof expr === 'string' || typeof expr === 'boolean' || typeof expr === 'number') {
         expr = ['literal', expr];
     }
@@ -122,9 +161,9 @@ function parseExpression(expr: mixed, context: ParsingContext, expectedType?: ?T
 
         const Expr = context.definitions[op];
         if (Expr) {
-            const parsed = Expr.parse(expr, context, expectedType);
+            const parsed = Expr.parse(expr, context);
             if (!parsed) return null;
-            if (expectedType && match(expectedType, parsed.type, context)) {
+            if (context.expectedType && match(context.expectedType, parsed.type, context)) {
                 return null;
             } else {
                 return parsed;
@@ -144,9 +183,8 @@ function parseExpression(expr: mixed, context: ParsingContext, expectedType?: ?T
 /**
  * Returns null if the type matches, or an error message if not.
  *
- * Also populate the given typenames context when a generic type is successfully
- * matched against a concrete one, with `scope` controlling whether type names
- * from `expected` or `t` are to be bound.
+ * If `context` is provided, then also push the error to it via
+ * `context.error()`
  *
  * @private
  */