From cab3b5890d27745e5854c2b6a9a3c3ac0fb97330 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Fri, 4 Aug 2017 21:45:46 -0400 Subject: [PATCH] Simplify ParsingContext, add some internal docs --- .../function/compound_expression.js | 9 ++- src/style-spec/function/definitions/array.js | 2 +- src/style-spec/function/definitions/at.js | 4 +- src/style-spec/function/definitions/case.js | 6 +- .../function/definitions/coalesce.js | 2 +- src/style-spec/function/definitions/curve.js | 4 +- src/style-spec/function/definitions/let.js | 4 +- src/style-spec/function/definitions/match.js | 8 +-- src/style-spec/function/expression.js | 55 +++++++++++++------ 9 files changed, 60 insertions(+), 34 deletions(-) diff --git a/src/style-spec/function/compound_expression.js b/src/style-spec/function/compound_expression.js index 1cb670e45ba..4b9b13f2bf6 100644 --- 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 = []; 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, 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 index 8ac049016ef..8e3017b220b 100644 --- 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 index ba5112e0ba0..175075f7a74 100644 --- a/src/style-spec/function/definitions/at.js +++ b/src/style-spec/function/definitions/at.js @@ -27,8 +27,8 @@ class At implements Expression { 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(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 index 09e7cf87fba..49c1526230d 100644 --- a/src/style-spec/function/definitions/case.js +++ b/src/style-spec/function/definitions/case.js @@ -34,10 +34,10 @@ class Case implements Expression { 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]); @@ -45,7 +45,7 @@ class Case implements Expression { 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 index a7709211775..d4836dbb233 100644 --- a/src/style-spec/function/definitions/coalesce.js +++ b/src/style-spec/function/definitions/coalesce.js @@ -22,7 +22,7 @@ class Coalesce implements Expression { let outputType = expectedType; const parsedArgs = []; for (const arg of args) { - const argContext = context.concat(1 + parsedArgs.length, 'coalesce'); + const argContext = context.concat(1 + parsedArgs.length); const parsed = parseExpression(arg, argContext, outputType); if (!parsed) return null; outputType = outputType || parsed.type; diff --git a/src/style-spec/function/definitions/curve.js b/src/style-spec/function/definitions/curve.js index dea056e1d15..e64d0c39e64 100644 --- a/src/style-spec/function/definitions/curve.js +++ b/src/style-spec/function/definitions/curve.js @@ -73,7 +73,7 @@ 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 = []; @@ -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 index 60f95337f4d..5d6487b0d6c 100644 --- 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, bindings); const result = parseExpression(args[args.length - 1], resultContext); if (!result) return null; diff --git a/src/style-spec/function/definitions/match.js b/src/style-spec/function/definitions/match.js index fbacc1432a1..8ad2136fcf9 100644 --- a/src/style-spec/function/definitions/match.js +++ b/src/style-spec/function/definitions/match.js @@ -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 index 9e52383e05e..bc25231b7b9 100644 --- a/src/style-spec/function/expression.js +++ b/src/style-spec/function/expression.js @@ -41,6 +41,10 @@ class ParsingError extends Error { } } +/** + * Tracks `let` bindings during expression parsing. + * @private + */ class Scope { parent: ?Scope; bindings: {[string]: Expression}; @@ -68,35 +72,46 @@ class Scope { } } +/** + * State associated parsing at a given point in an expression tree. + * @private + */ class ParsingContext { - key: string; - path: Array; - ancestors: Array; definitions: {[string]: Class}; + path: Array; + key: string; scope: Scope; errors: Array; - constructor(definitions: *, path: Array = [], ancestors: * = [], scope: Scope = new Scope(), errors: Array = []) { + constructor(definitions: *, path: Array = [], scope: Scope = new Scope(), errors: Array = []) { this.definitions = definitions; this.path = path; this.key = path.map(part => `[${part}]`).join(''); - this.ancestors = ancestors; this.scope = scope; this.errors = errors; } - // 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, 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, 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) { const key = `${this.key}${keys.map(k => `[${k}]`).join('')}`; this.errors.push(new ParsingError(key, error)); @@ -104,6 +119,15 @@ class ParsingContext { } } +/** + * 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, expectedType?: ?Type) : ?Expression { if (expr === null || typeof expr === 'string' || typeof expr === 'boolean' || typeof expr === 'number') { expr = ['literal', expr]; @@ -144,9 +168,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 */