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

Auto-coerce top-level results and concat arguments to strings #7280

Merged
merged 8 commits into from
Sep 18, 2018

Conversation

jfirebaugh
Copy link
Contributor

Fixes #6190, implementing auto-coercion for "concat" and string-valued properties. The latter change affects the behavior of coalesce as well, for instance "text-field": ["coalesce", ["get", "a"], ["get", "b"]] will now never produce a type error.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

Copy link
Contributor

@samanpwbb samanpwbb left a comment

Choose a reason for hiding this comment

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

Awesome, excited for this change – just had one piece of feedback about the expression migration script.

return get;
// By default, expressions for string-valued properties get coerced. To preserve
// legacy function semantics, insert an explicit assertion instead.
return propertySpec.type === 'string' ? ['string', get] : get;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 , updated.

// For string-valued properties, coerce to string at the top level rather than asserting.
const parsed = parser.parse(expression, undefined, undefined, undefined,
propertySpec.type === 'string' ? {typeAnnotation: 'coerce'} : undefined);

Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

No longer need to-string when using concat. However, it is still needed if the token string consists of a single token.
@jfirebaugh
Copy link
Contributor Author

The recently added format operator (#6994) adds a couple wrinkles here:

  • As written, this PR no longer actually applies to text-field, since its type changed from string to formatted. This should be okay though, because the default annotation behavior for the formatted type is to coerce.
  • We should probably extend the auto-coercion behavior to the format operator's arguments, as format will be used in very similar circumstances to concat.

const types = {
color: ColorType,
string: StringType,
number: NumberType,
enum: StringType,
boolean: BooleanType
boolean: BooleanType,
formatted: FormattedType
Copy link
Contributor

Choose a reason for hiding this comment

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

I think with this change, every place in the code that checks text instanceof Formatted has a dead branch. I can go clean those up in a later PR. There may also be some performance impact here because of the extra bookkeeping the "formatted" path uses, although I'd guess it's pretty minimal.

@ChrisLoer
Copy link
Contributor

No obvious change in the Layout benchmark, which should exercise the extra string->formatted coercion (although I've been getting pretty variable results between runs):

screenshot 2018-09-17 10 18 13

@ChrisLoer
Copy link
Contributor

I went ahead and made the changes to strip out the instanceof Formatted dead code, and as part of testing that realized that the implicit coercion strategy John used in 85fce01#diff-7341a1d97068bd8c5a153f89bc9e4b1aL117 wouldn't work on this relatively simple case:

[ "coalesce", [ "get", "name_en" ], [ "get", "name" ] ]

The reason was that the coalesce introduced a typeAnnotation of omit, triggering the assert at:

 assert(!options.typeAnnotation);
 parsed = new FormatExpression([{text: parsed, scale: null, font: null}]);

I'm not sure of the right answer here -- it seems like allowing omit might make sense in this case. But I think we have to support any type of annotation, because for instance if someone defines a text-field to be ['number', 0] that should generation an expression parsing error, not an assertion failure.

I tried going back to the approach of using Coercion, but added explicit format-supporting code to the Coercion so that it wouldn't try to serialize as to-formatted.

@mollymerp since I've started modifying this PR, can I ask you to step in as reviewer?

I'm going to try to add some test cases for the different "type annotation" cases.

…tion time.

 - Add coercion after call to 'getValueAndResolveTokens`, since the non-expression pathway here skips the coercion logic in parsing_context
 - Remove 'string | Formatted' typing and 'text instanceof Formatted' checks
 - Add Coercion support for 'Formatted', along with dedicated serialization
 - Use Coercion when parsing expected.kind === 'formatted' instead of directly creating a FormatExpression. This is necessary to accommodate expressions such as 'coalesce' that introduce a typeAnnotation.
Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

This looks good overall (to me, someone with limited expression parsing knowledge – thank you for the additional context in chat @ChrisLoer !)

"compiled": {
"result": "error",
"errors": [
{"key": "", "error": "Expected formatted but found number instead."}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a rather unhelpful error message, considering that most "text-field" values won't be actually using the "format" expression. Should we special case this type checking error for formatted or punt on this for a later error message improvement project?

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I think defer? The way I kind of think about it is: it's confusing because people expect to be able to return a 'string', and have it implicitly-coerce to 'formatted' without them having to even know 'formatted' exists. So you'd kind of like it to say something like Expected formatted or string but found number instead. -- but that's still actually confusing because (1) things other than 'formatted' or 'string' can still successfully coerce to 'formatted', (2) even 'number' can coerce to 'formatted' - the problem is that the assertion disables the implicit coercion.

if (expr === null || typeof expr === 'string' || typeof expr === 'boolean' || typeof expr === 'number') {
expr = ['literal', expr];
}

function annotate(parsed, type, typeAnnotation: 'assert' | 'coerce' | 'omit') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find typeAnnotation to be a very confusing name for this modifier (typeModifier 🤔 ) but we don't have to deal with that in this PR 😶

@ChrisLoer ChrisLoer merged commit a1233cb into master Sep 18, 2018
@ChrisLoer ChrisLoer deleted the concat-to-string branch September 18, 2018 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants