Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS arbex implementation: parse, typecheck, compile expressions #4841

Merged
merged 4 commits into from
Jul 3, 2017

Conversation

anandthakker
Copy link
Contributor

@anandthakker anandthakker commented Jun 15, 2017

Todo:

  • Implement match expressions (blocked by syntax design Project Arbex (Arbitrary Expressions) #4777 (comment))
  • Implement curve expressions
  • Implement color expressions
  • Add unit tests for each function type
  • Revise get, has to be more concise per draft of expressions examples doc #4836 (comment)
  • Add constructors for Vector<T> and Array<T, N>
  • Implement runtime error handling
  • Add coalesce
  • Write stop function => expression function conversion, and use it to replace current function implementation
  • Add constraint on use of zoom expression, and add stop/interpolation metadata to compiled result
  • OO-ify
  • Implement let
  • Type checking a number->number against number->variant<number|string> should yield an expression of type number->number. (punt on this for now)
  • Update style spec to accept {expression: [...]} as a function.
  • Refactor style function call sites to pass (globalProperties, feature) rather than (globalProperties, featureProperties) (needed for geometry_type and id)
  • Include expression type checking in spec validation (e.g. in setPaintProperty, etc.) -- but see JS arbex implementation: parse, typecheck, compile expressions #4841 (comment); we should look into what native does in this case.
  • Remove => (and any other es6 stuff) from generated code, since it won't be transpiled.
  • Update expressions.md to accurately reflect the implementation here.
  • Implement cubic-bezier interpolation type for curve expressions (doing this in a followup)

@anandthakker anandthakker force-pushed the compile-expressions branch 2 times, most recently from e72044a to ea2521b Compare June 15, 2017 20:14
@anandthakker anandthakker force-pushed the compile-expressions branch 5 times, most recently from 72237fa to 38f0058 Compare June 17, 2017 02:47
@anandthakker
Copy link
Contributor Author

@jfirebaugh @ansis plenty of work still left to do here, but a base implementation is in place in case you want to take an early look.

@anandthakker anandthakker force-pushed the compile-expressions branch 4 times, most recently from 637f4af to 0a272d4 Compare June 19, 2017 20:37
name: 'object',
type: lambda(ObjectType, ValueType),
compile: fromContext('asObject')
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

json_array and object are assertions (yielding an error if the input isn't the right type), whereas number, string, boolean are coercions. Should we also have assertions for number/string/boolean?

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool stuff! I didn't thoroughly review everything, but wanted to submit what I noticed so far.

};
}

function defineBinaryMathOp(name, isAssociative) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the current uses of defineBinaryMathOp pass a second argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, whoops! This was intended for allowing +, &&, etc. to take > 2 args as a convenience.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2b91368

};
}

function defineComparisonOp(name, isAssociative) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto defineComparisonOp.

}
*/

const expressions: { [string]: Definition } = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ExpressionName]: Definition

Copy link
Contributor Author

@anandthakker anandthakker Jun 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack. Seems like this isn't compatible with accessing this map using values pulled from a mixed array. There's no good way to take myRawJSONExpression[0] and refine it so that flow knows that it's an ExpressionName.

},

typeOf: function (x) {
if (Array.isArray(x)) return 'Vector<Value>';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only place generics syntax is exposed publicly? I wonder if Array is a better public name for this type, for familiarity. Internally we could rename Vector<T>Array<T> and Array<T, N>FixedArray<T, N>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not quite the only place it's exposed publicly: type checking error messages will need to refer to types by name, and probably some runtime error messages will, too. That said, I'm also with you in that I've been uneasy/unsatisfied with having Vector<Value> as a possible output of typeof (because of both the generics syntax and the Value alias).

Would it be confusing for [typeof, [get, 'foo']] to equal Array, but then to have [number, [at, [get, 'foo'], 0]] to produce a runtime error message like Expected Vector<number> but properties.foo was an object instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could always print names like:

  • Value: JSONValue
  • Vector: T[]
  • Array<T, N>: T[N]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, if we think T[] is too concise and easy to misread, we could do:

  • Vector: Array of T's
  • Array<T, N>: Array of N T's

const result = [];
while (args.length > 1) {
const c = args.splice(0, 2);
result.push(`${c[0].js} ? ${c[1].js}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parentheses are needed around subexpressions, either in individual compile definitions or inserted globally by the compiler. Else you'll get the wrong precedence when compiling things like ["&&", a, ["case", b, c, d]] -- desired precedence is a && (b ? c : d), but without parentheses a && b ? c : d parses as (a && b) ? c : d.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 yep -- it's done globally at the end of compile(), 04dbb97#diff-442324b3174c25e635f18536d7732df2R147, but this reminds me, we should include a test that targets this since it's be very easy to regress

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 013a59e

*
* @private
*/
function compileExpression(expr: any) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's possible to narrow any to mixed here without too much trouble.

isFeatureConstant: boolean,
isZoomConstant: boolean,
expression: TypedExpression,
function?: any
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This any can be Function.

@anandthakker anandthakker force-pushed the compile-expressions branch 4 times, most recently from 2d49f8e to 69ca97a Compare June 20, 2017 12:47
@jfirebaugh
Copy link
Contributor

For consistency with get/has, we should also reorder at's arguments

@@ -134,7 +134,7 @@ Every expression evaluates to a value of one of the following types.
###Decision:
- `["case", cond1: Boolean, result1: T, cond2: Boolean, result2: T, ..., cond_m, result_m: T, result_otherwise: T] -> T`
- `["match", x: T, a_1: T, y_1: U, a_2: T, y_2: U, ..., a_m: T, y_m: U, y_else: U]` - `a_1`, `a_2`, ... must be _literal_ values of type `T`.
- `["is_error", expr: T]` - `true` if `expr` is an `Error` value, `false` otherwise
- `["coalesce", e1: T, e2: T, e3: T, ...] -> T` - evaluates each expression in turn until the first non-error value is obtained, and returns that value.
Copy link
Contributor Author

@anandthakker anandthakker Jun 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be "non-error and non-null"

@anandthakker anandthakker force-pushed the compile-expressions branch 4 times, most recently from 8d1e797 to d2692a0 Compare June 22, 2017 15:49
@anandthakker
Copy link
Contributor Author

If a user specifies a property function with, say, numeric stop outputs, for a style property whose spec definition specifies a string, we accept the mistyped function and just yield the default. IMO, this should be a validation error instead.

@anandthakker anandthakker changed the title WIP - JS arbex implementation: parse, typecheck, compile expressions JS arbex implementation: parse, typecheck, compile expressions Jun 22, 2017
@jfirebaugh
Copy link
Contributor

When I look at real world examples like in #4836, I want to be able to extract array and object literals as constants, assigned in "let" expressions, and then write the guts of the expression with a data-driven dispatch into the constant. So, yes, if we make them possible/convenient, I think there will be use cases for arrays and objects outside of (a) and (b).

If/when we do support this for both objects and arrays, they should be parallel in syntax. Here are some possibilities:

  • ["array", [...]] and ["object", {...}], with no evaluation "inside" the argument.
  • ["array", value, value...], and ["object", string, value, string, value...], with evaluation of arguments.
  • ["quote", [...]] and ["quote", {...}], with no evaluation "inside" the argument. A lispish single special form; provides the option to have "quasiquote" form in the future for partial inner evaluation. I also like this form because it leaves the names "array" and "object" free for assertion/coercion. We could choose less lispish names if you prefer, for example "quote" ⇢ "literal", "quasiquote" ⇢ "template", "unquote" ⇢ "substitute".

@jfirebaugh
Copy link
Contributor

For the primitive types, I think coercion will be used much more commonly than assertion, which makes me want coercion to be as concise as possible

Funny, I have the opposite instinct. I would think assertion/annotation to be the more common (especially if we expect Studio to auto-insert annotations based on tilestats), and coercion to be the more dangerous, and hence better suited to the less concise name.

'string': {
name: 'string',
type: lambda(StringType, ValueType),
compile: args => ({ js: `String(${args[0].js})` })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it wise to inherit all of JS's coercion quirks?

  • ["string", ["^", 2, 70]] ⇢ "1.1805916207174113e+21"
  • ["string", ["properties"]] ⇢ "[object Object]"
  • ["string", ["array", 1, 2]] ⇢ "1,2"
  • ["string, ["get", "weird"]] + {"properties": {"weird": {"toString": "asdf"}} ⇢ TypeError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it seems reasonable to lean on the platform's number->string conversion... as for objects and arrays... would it be reasonable to just punt, requiring primitive types only?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

@anandthakker
Copy link
Contributor Author

Funny, I have the opposite instinct. I would think assertion/annotation to be the more common (especially if we expect Studio to auto-insert annotations based on tilestats)

Good point -- I was thinking more that coercion would be more common in manually-authored expressions, which is why I was going for conciseness...

coercion to be the more dangerous, and hence better suited to the less concise name.

... but this is convincing. 👍 for making coercion less concise / more explicit.

@1ec5
Copy link
Contributor

1ec5 commented Jun 27, 2017

Good point -- I was thinking more that coercion would be more common in manually-authored expressions, which is why I was going for conciseness...

This was my thought as well. Runtime styling doesn’t have the benefit of auto-annotation via tilestats. Per mapbox/mapbox-gl-native#8074 (comment), I don’t think developers on iOS will mind a little danger if they can avoid mandatory type annotations (which would have major usability issues).

@anandthakker anandthakker force-pushed the compile-expressions branch 2 times, most recently from 074b108 to 8f9f2df Compare June 30, 2017 01:38
@anandthakker anandthakker force-pushed the compile-expressions branch 2 times, most recently from cc10399 to ab4c1fb Compare June 30, 2017 16:25
}
type = (!itemtype || itemtype === 'Value') ?
'Array' : `Array<${titlecase(itemtype)}>`;
const t = ArrayLiteral.inferArrayType(items);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfirebaugh This change causes the runtime typing of [get, foo] when foo is an array to be as specific as possible, based on its actual contents:

  • the array is homogenous of type number, string, or boolean, then => Array<typeof item, properties.foo.length>
  • otherwise => Array<Value, properties.foo.length>


return array(itemType || ValueType, value.length);
}
if (!isValue(args[0]))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of now, there's actually no way to express a color literal -- rgba and color are functions -- but this would allow ['literal', new Color(...)].

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I figured that would be okay -- since we're not likely to export Color publicly, there no way to construct such an expression. And even if there were, it'd be harmless to do so.

@@ -145,12 +145,12 @@ function parseExpression(expr: mixed, context: ParsingContext) : Expression {
return new LiteralExpression(key, NumberType, expr);
} else if (Array.isArray(expr)) {
if (expr.length === 0) {
throw new ParsingError(key, `Expected an array with at least one element.`);
throw new ParsingError(key, `Expected an array with at least one element. If you wanted a literal array, use ["literal", []].`);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to this and the others like it.

@jfirebaugh
Copy link
Contributor

Now that we have an AST made up of Expression instances, we should define things like isFeatureConstant and isZoomConstant as independent recursive functions rather than lumping them in with the compilation step.

Expression#compile itself should be typed () => string | Array<CompileError>.

@anandthakker
Copy link
Contributor Author

Now that we have an AST made up of Expression instances, we should define things like isFeatureConstant and isZoomConstant as independent recursive functions rather than lumping them in with the compilation step.
Expression#compile itself should be typed () => string | Array.

💯 I'm doing this as part of implementation of let.

@anandthakker
Copy link
Contributor Author

Few notes/questions about let:

  • Uses let (as opposed to let* or letrec) semantics.
  • We could make Reference part of LiteralExpression and LetExpression a subclass of LambdaExpression. I kept the former separate for semantic reasons, and the latter also because it requires specialized type checking.
  • To keep things simple for now, a [zoom] curve is not allowed in a let binding; it can only appear in the result of a let, even though we theoretically could accept, e.g. [let, 'a', [curve, [linear], [zoom], 0, 0, 10, 10], ['a']]
  • Currently, a let binding that produces an error will cause the entire let expression to be an error, even if it would have been unused. E.g. [let, 'a', [number, [get, x]], [case, false, [a], 0]]. What's more natural: the present behavior, or only having an error propagated if the erroneous binding is actually referenced?

constructor(key: string, name: string, type: Type) {
super(key, type);
if (!/^[a-zA-Z_]+[a-zA-Z_0-9]*$/.test(name))
throw new ParsingError(key, `Invalid identifier ${name}.`);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check should be moved to LetExpression.parse(), and it should also either check against JS reserved words or else 'escape' them (e.g. replace function with var_function or something)

@anandthakker
Copy link
Contributor Author

Squashed this down, FF merging into feature/expressions branch

@anandthakker anandthakker merged commit 6ba399f into feature/expressions Jul 3, 2017
@jfirebaugh jfirebaugh deleted the compile-expressions branch September 21, 2017 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants