Skip to content

Commit

Permalink
Avoid inferred type annotations for 'coalesce' arguments
Browse files Browse the repository at this point in the history
Fixes #5755
  • Loading branch information
Anand Thakker committed Nov 29, 2017
1 parent 797de07 commit 34c448d
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 24 deletions.
4 changes: 3 additions & 1 deletion src/style-spec/expression/definitions/array.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@ const types = {
class ArrayAssertion implements Expression {
type: ArrayType;
input: Expression;
_inferred: boolean;

constructor(type: ArrayType, input: Expression) {
constructor(type: ArrayType, input: Expression, inferred: boolean = false) {
this.type = type;
this.input = input;
this._inferred = inferred;
}

static parse(args: Array<mixed>, context: ParsingContext): ?Expression {
Expand Down
5 changes: 4 additions & 1 deletion src/style-spec/expression/definitions/assertion.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,12 @@ class Assertion implements Expression {
type: Type;
args: Array<Expression>;

constructor(type: Type, args: Array<Expression>) {
_inferred: boolean;

constructor(type: Type, args: Array<Expression>, inferred: boolean = false) {
this.type = type;
this.args = args;
this._inferred = inferred;
}

static parse(args: Array<mixed>, context: ParsingContext): ?Expression {
Expand Down
26 changes: 24 additions & 2 deletions src/style-spec/expression/definitions/coalesce.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
// @flow

const assert = require('assert');
const Assertion = require('./assertion');
const ArrayAssertion = require('./array');
const Coercion = require('./coercion');
const {ValueType} = require('../types');

import type { Expression } from '../expression';
import type ParsingContext from '../parsing_context';
Expand All @@ -25,14 +29,32 @@ class Coalesce implements Expression {
outputType = context.expectedType;
}
const parsedArgs = [];

let needsOuterAnnotation = false;

for (const arg of args.slice(1)) {
const parsed = context.parse(arg, 1 + parsedArgs.length, outputType);
let parsed = context.parse(arg, 1 + parsedArgs.length, outputType);
if (!parsed) return null;
outputType = outputType || parsed.type;

// strip off any inferred type assertions so that they don't
// produce a runtime error for `null` input, which would preempt
// the desired null-coalescing behavior
if ((parsed instanceof Assertion || parsed instanceof Coercion) &&
parsed._inferred) {
needsOuterAnnotation = true;
parsed = parsed.args[0];
} else if (parsed instanceof ArrayAssertion && parsed._inferred) {
needsOuterAnnotation = true;
parsed = parsed.input;
}

parsedArgs.push(parsed);
}
assert(outputType);
return new Coalesce((outputType: any), parsedArgs);
return needsOuterAnnotation ?
context.annotateType(outputType, new Coalesce(ValueType, parsedArgs)) :
new Coalesce((outputType: any), parsedArgs);
}

evaluate(ctx: EvaluationContext) {
Expand Down
5 changes: 4 additions & 1 deletion src/style-spec/expression/definitions/coercion.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,12 @@ class Coercion implements Expression {
type: Type;
args: Array<Expression>;

constructor(type: Type, args: Array<Expression>) {
_inferred: boolean;

constructor(type: Type, args: Array<Expression>, inferred: boolean = false) {
this.type = type;
this.args = args;
this._inferred = inferred;
}

static parse(args: Array<mixed>, context: ParsingContext): ?Expression {
Expand Down
53 changes: 34 additions & 19 deletions src/style-spec/expression/parsing_context.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,26 +68,10 @@ class ParsingContext {
if (Expr) {
let parsed = Expr.parse(expr, context);
if (!parsed) return null;
const expected = context.expectedType;
const actual = parsed.type;
if (expected) {
// When we expect a number, string, or boolean but have a
// Value, wrap it in a refining assertion, and when we expect
// a Color but have a String or Value, wrap it in "to-color"
// coercion.
const canAssert = expected.kind === 'string' ||
expected.kind === 'number' ||
expected.kind === 'boolean';

if (canAssert && actual.kind === 'value') {
parsed = new Assertion(expected, [parsed]);
} else if (expected.kind === 'array' && actual.kind === 'value') {
parsed = new ArrayAssertion(expected, parsed);
} else if (expected.kind === 'color' && (actual.kind === 'value' || actual.kind === 'string')) {
parsed = new Coercion(expected, [parsed]);
}
parsed = context.annotateType(context.expectedType, parsed);

if (context.checkSubtype(expected, parsed.type)) {
if (context.expectedType) {
if (context.checkSubtype(context.expectedType, parsed.type)) {
return null;
}
}
Expand Down Expand Up @@ -118,6 +102,37 @@ class ParsingContext {
}
}


/**
* Wrap the given expression in a type assertion if necessary.
* @private
*/
annotateType(expected: ?Type, expression: Expression) {
if (!expected) {
return expression;
}

const actual = expression.type;

// When we expect a number, string, or boolean but have a
// Value, wrap it in a refining assertion, and when we expect
// a Color but have a String or Value, wrap it in "to-color"
// coercion.
const canAssert = expected.kind === 'string' ||
expected.kind === 'number' ||
expected.kind === 'boolean';

if (canAssert && actual.kind === 'value') {
return new Assertion(expected, [expression], true);
} else if (expected.kind === 'array' && actual.kind === 'value') {
return new ArrayAssertion(expected, expression, true);
} else if (expected.kind === 'color' && (actual.kind === 'value' || actual.kind === 'string')) {
return new Coercion(expected, [expression], true);
}

return expression;
}

/**
* Returns a copy of this context suitable for parsing the subexpression at
* index `index`, optionally appending to 'let' binding map.
Expand Down

0 comments on commit 34c448d

Please sign in to comment.