Revise approach to evaluation-time type errors #5263

anandthakker · 2017-09-08T21:03:24Z

Changes:

get now returns null for a missing property value, rather than throwing an error
coalesce now does NOT handle type errors -- it only coalesces null
Basic type assertions (string, number, boolean) now accept any number of arguments -- each one is tested in turn until the first value of the desired type is found. If the final value doesn't match, then an error is thrown.
to-number and to-color work the same way as the assertions, accepting any number of arguments and attempting to coerce each one in turn until one works

benchmark	master `6e2fbf4`	expressions-type-errors `6d78dbe`
map-load	181 ms	132 ms
style-load	150 ms	190 ms
buffer	1,176 ms	1,158 ms
tile_layout_dds	1,372 ms	1,381 ms
fps	60 fps	59 fps
frame-duration	7.1 ms, 1% > 16ms	7.3 ms, 2% > 16ms
query-point	1.00 ms	1.18 ms
query-box	80.64 ms	80.97 ms
geojson-setdata-small	10 ms	10 ms
geojson-setdata-large	191 ms	125 ms
filter	Done	Done

cc @jfirebaugh @mourner

mourner · 2017-09-12T15:41:03Z

src/style-spec/function/evaluation_context.js

+                if (c) return c;
+            } else if (Array.isArray(input)) {
+                error = this._validateRGBA(input[0], input[1], input[2], input[3]);
+                if (!error) return this.rgba(input[0], input[1], input[2], input[3]);


This looks redundant because this.rgba does the same validation check.

Oops, thanks.

mourner · 2017-09-12T15:41:36Z

src/style-spec/function/evaluation_context.js

-            if (!c)
-                throw new RuntimeError(`Could not parse color from value '${input}'`);
-            cached = this._parseColorCache[input] = new Color(...c);
+            cached = this._parseColorCache[input] = c ? new Color(...c) : null;


spread arguments with new alert

mourner · 2017-09-12T15:47:12Z

src/style-spec/function/evaluation_context.js

+            const num = Number(value);
+            if (isNaN(num)) continue;
+            return num;
+        }


Is there any way to avoid relying on arguments in a runtime-called method? Perhaps we could cache the possible values in an array like we do for stop outputs (and get rid of this method in the eval context)? Let's check and maybe benchmark to be sure it's faster.

👍, good call, will try that out.

@mourner btw, I was going off of the rule of thumb that using arguments is fine if you never use the object itself, only access arguments.length and arguments[i] -- what's your take on that?

@mourner fixed -- perf was equivalent, this way seems cleaner/better anyway

mourner · 2017-09-12T15:50:16Z

src/style-spec/function/index.js

+                        return defaultValue;
+                    }
+                    return val;
+                } catch (e) {


Is there any way we could avoid a full try/catch here? Maybe passing an errors array to the compiled function and accumulating errors there somehow?

Definitely possible, but I think it's outside the scope of this PR -- I experimented with a non-try catch approach recently, and it's definitely a nontrivial change. The naive approach (checking for errors before evaluating any expression) was slower than try/catch, and the non-naive approach will take some careful work.

@anandthakker I'm not sure how much exactly it affects performance (I guess it depends on browser engine/version), but as a rule of thumb, I avoid arguments keyword altogether in perf-sensitive code paths. At least it is easier to make sure a certain function is monomorphic.

anandthakker · 2017-09-13T00:17:45Z

cc @aparlato @nickidlugash

mourner · 2017-09-13T11:19:53Z

src/style-spec/function/evaluation_context.js

-        ensure(typeof a === 'undefined' ||
-            (a >= 0 && a <= 1), `Invalid rgba value [${[r, g, b, a || 1].join(', ')}]: 'a' must be between 0 and 1.`);
+        const error = this._validateRGBA(r, g, b, a);
+        if (error) throw new RuntimeError(error);
        return new Color(r / 255, g / 255, b / 255, a);
    },


Is this method still used anywhere?

yeah, it's used for evaluating the ["rgba", r, g, b, a] (and "rgb") expression

mourner · 2017-09-13T14:26:20Z

src/style-spec/function/evaluation_context.js

+                if (c) return c;
+            } else if (Array.isArray(input)) {
+                error = this._validateRGBA(input[0], input[1], input[2], input[3]);
+                if (!error) return new Color(input[0] / 255, input[1] / 255, input[2] / 255, input[3]);


Can you replace those 2 lines with a call to rgba?

No, because rgba throws a RuntimeError on failure.

Oh, nevermind — I just realized toColor now acts like coalesce through multiple args...

nickidlugash · 2017-09-13T15:52:42Z

@anandthakker for type assertions and coercions, is null an acceptable argument?

anandthakker · 2017-09-13T16:29:52Z

@nickidlugash for assertions, null is never accepted: ["string", null] will produce an error; ["string", null, "foo"] will fall back go "foo".

For coercions, it depends. ["to-boolean", null] => false, ["to-string", null] => "null", but to-number and to-color will both fail on null input value.

jfirebaugh · 2017-09-14T18:14:07Z

test/integration/expression-tests/to-string/basic/test.json

-        "error": "Expected a primitive value in [\"string\", ...], but found Object instead."
-      }
-    ]
+    "outputs": ["1", "false", "null", "[1,2]", "{\"y\":1}"]


I think at one point we decided we didn't want to commit to using a JSON.stringify representation for to-string of arrays and objects for portability reasons. Are you thinking differently now?

I am, yeah. It feels to me like to-string should always work, and since, with the exception of Color, all the values in our system can be represented in JSON just fine, I think it seems fine to commit to JSON.stringify. (Once Colors can be contained within objects and arrays, we can use the replacer parameter of JSON.stringify to deal with those.)

I don't think we want to commit to having a string representation of every type we ever add to the expression type system. What, for example, would be the result of ["to-string", ["image", "foo"]] (#5261)?

Eh, I think it's okay to come up with some (obviously lossy) way to stringify Image values -- {"image": "foo"} or something -- but even if not, I'd still be in favor of being as liberal as possible. If nothing else, I could imagine it being super convenient while debugging to set a symbol's text-field property to ["to-string", ["get", "offset"]] (when we finally have support for array values in feature properties)

jfirebaugh · 2017-09-14T18:55:27Z

src/style-spec/function/evaluation_context.js

+            return String(value);
+        } else if (value instanceof Color) {
+            const [r, g, b, a] = value.value;
+            return `rgba(${r * 255}, ${g * 255}, ${b * 255}, ${a})`;


Is there a test for this?

Added in bae93ca

jfirebaugh · 2017-09-14T18:59:27Z

src/style-spec/function/definitions/coalesce.js

-                var result = ${ctx.compileAndCache(this.args[i])};
-                if (result !== null) return result;
-            } catch (e) {}`);
+        for (let i = 0; i < this.args.length; i++) {


for arg of this.args

jfirebaugh · 2017-09-14T19:15:35Z

src/style-spec/function/evaluation_context.js

+    },
+
+    coalesce(args: Array<Function>) {
+        for (let i = 0; i < args.length - 1; i++) {


Could be written as:

let result = null; for (const arg of args) { result = arg(); if (result !== null) break; } return result;

Which handles the empty array case. (coalesce requires one argument currently, but we could consider relaxing this.)

jfirebaugh · 2017-09-14T19:41:48Z

src/style-spec/function/evaluation_context.js

@@ -208,6 +235,15 @@ module.exports = () => ({
        }

        return interpolate[resultType](outputLower, outputUpper, t);
+    },
+
+    coalesce(args: Array<Function>) {


Function → ()=>Value

jfirebaugh · 2017-09-14T19:47:42Z

src/style-spec/function/definitions/index.js

-    'to-boolean': [ BooleanType, [ValueType], ([v]) => `Boolean(${v})` ],
-    'to-rgba': [ array(NumberType, 4), [ColorType], ([v]) => `${v}.value` ],
-    'to-color': [ ColorType, [ValueType], fromContext('toColor') ],
+    'to-number': [ NumberType, varargs(ValueType), (ctx, args) => {


to-number and to-color now do not necessarily evaluate all their arguments -- i.e. they are special forms. Let's write them as such. I think this will also allow you to avoid all the other ctx and cache changes to this file.

Another reason these ctx / cache changes may be useful is that they'll let us experiment with inlining arguments (rather than always caching each one as a function eN() { ... })

Moved to-number and to-color to a special form, Coercion. If/when it becomes possible for to-string to fail, we can move it there, too.

jfirebaugh · 2017-09-14T21:49:03Z

src/style-spec/function/parse_expression.js

 function wrapForType(expected: Type, expression: Expression, context: ParsingContext) {
-    const wrapper = typeWrappers[expected.kind];
-    if (!wrapper) {
+    if (expected.kind === 'Color') {


These conditionals largely repeat the conditionals at the wrapForType callsite. Inline?

👍 did so.

jfirebaugh · 2017-09-14T21:50:16Z

src/style-spec/function/parse_expression.js

+        expected.kind === 'String' ||
+        expected.kind === 'Boolean'
+    ) {
+        // workaround for circular dependency


Can directly require('./definitions/assertion') now, right? Same for to-color if it's a made a special form.

jfirebaugh · 2017-09-14T21:52:22Z

src/style-spec/function/evaluation_context.js

-        return num;
+    toNumber: function(args: Array<()=>Value>) {
+        let value;
+        for (let i = 0; i < args.length; i++) {


jfirebaugh · 2017-09-14T21:55:13Z

test/integration/expression-tests/to-color/basic/test.json

@@ -22,7 +22,9 @@
      [0, 1, 0, 1],
      [0, 1, 0, 1],
      [0, 1, 0, 1],
-      {"error": "Could not parse color from value '[0,255]'"}
+      {
+        "error": "Invalid rgba value [0, 255, ]: 'r', 'g', and 'b' must be between 0 and 255."


This error is misleading -- the issue is the array length, not its contents.

jfirebaugh · 2017-09-14T21:57:24Z

src/style-spec/function/evaluation_context.js

-        } else {
-            type = typeOf(value);
-            typeError = checkSubtype(expectedType, type);
+    asJSType: function (expectedType: string, values: Array<Function>) {


Function → () => Value

jfirebaugh · 2017-09-14T22:22:17Z

src/style-spec/function/definitions/match.js

-        const inputType = this.inputType.kind !== 'Array' ?
-            `$this.types.${this.inputType.kind}` : JSON.stringify(this.inputType);
-
-        return `(${lookupObject}[$this.as(${input}, ${inputType})] || ${ctx.addExpression(this.otherwise.compile(ctx))})();`;


What was the purpose of using $this.as here before?

I believe it was always redundant, since type checking should already ensure that the input value is the right type.

jfirebaugh · 2017-09-14T22:24:59Z

src/style-spec/function/definitions/match.js

@@ -107,15 +107,12 @@ class Match implements Expression {
        for (let i = 0; i < labels.length; i++) {
            if (i > 0) lookup += ', ';
            const label = labels[i];
-            lookup += `${String(label)}: ${outputs[this.cases[label]]}`;
+            lookup += `${JSON.stringify(label)}: ${outputs[this.cases[label]]}`;


Why JSON.stringify rather than String? The input is going to implicitly go through String in ${lookupObject}[${input}], so if they behave differently that would seem to be an issue.

The JSON.stringify() here is to get a JS string literal for the key in the JS object literal we're building. If not, then label === "foo-bar" would result in invalid code { foo-bar: output }

Looks like JSON.stringify(number) is equivalent to String(number), so that should be fine. For strings, U+2028 and U+2029 need special handling.

Oy. We'll need to handle those in Literal, too, then.

Less magic, more explicit

anandthakker force-pushed the expressions-type-errors branch from ee39cb1 to 8c949ca Compare September 12, 2017 03:25

mourner reviewed Sep 12, 2017

View reviewed changes

anandthakker changed the title ~~WIP - Revise approach to evaluation-time type errors~~ Revise approach to evaluation-time type errors Sep 13, 2017

anandthakker force-pushed the expressions-type-errors branch 2 times, most recently from ee88053 to 953015b Compare September 13, 2017 01:31

anandthakker requested review from jfirebaugh and nickidlugash September 13, 2017 01:38

anandthakker mentioned this pull request Sep 13, 2017

Fold compound expressions with literal arguments into literals #5279

Merged

anandthakker requested a review from mourner September 13, 2017 11:08

mourner reviewed Sep 13, 2017

View reviewed changes

mourner approved these changes Sep 13, 2017

View reviewed changes

mourner reviewed Sep 13, 2017

View reviewed changes

jfirebaugh suggested changes Sep 14, 2017

View reviewed changes

anandthakker force-pushed the expressions-type-errors branch from 9826552 to e7c60bd Compare September 14, 2017 20:23

jfirebaugh reviewed Sep 14, 2017

View reviewed changes

jfirebaugh approved these changes Sep 15, 2017

View reviewed changes

Anand Thakker added 11 commits September 15, 2017 08:32

Remove expressions.md

1a2a092

'coalesce' only handles null, 'get' returns null for missing values

67d61bf

Accept fallback values in type assertions

9519515

Accept fallbacks for to-number and to-color

f9aa620

Fixup (most) stop-function conversions

b0c0604

Refactor compound expression definitions

8752560

Less magic, more explicit

Update docs to match spec changes

2e3d980

Add test for [to-string, COLOR]

9fc5887

Simplify coalesce loops

4565c28

Add special form for to-{number,color}

93aa55b

Format test fixture JSON

1df40b9

Anand Thakker added 2 commits September 15, 2017 08:32

Fixup loops, clarify error message

56030f4

Handle valid JSON strings that are not valid JS string literals

0308e41

anandthakker force-pushed the expressions-type-errors branch from 33b5444 to 0308e41 Compare September 15, 2017 12:32

anandthakker merged commit 2200752 into master Sep 15, 2017

anandthakker deleted the expressions-type-errors branch September 15, 2017 12:50

Revise approach to evaluation-time type errors #5263

Revise approach to evaluation-time type errors #5263

Conversation

anandthakker commented Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickidlugash commented Sep 13, 2017

anandthakker commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker commented Sep 8, 2017 •

edited

Loading