From 2e107baa46ff0307cd41273ffa428aa1f1357b6c Mon Sep 17 00:00:00 2001
From: Chris Loer <chris.loer@gmail.com>
Date: Fri, 6 Apr 2018 16:07:15 -0700
Subject: [PATCH] Move 'collator' from three arguments to a single "options"
 argument. Support/test default behavior when case/diacriticSensitivity isn't
 specified. Remove 'collator','resolve-locale' from feature non-constant list.

---
 docs/components/expression-metadata.js        | 12 +-----
 .../expression/definitions/collator.js        | 39 ++++++++++++-------
 .../expression/definitions/index.js           |  1 -
 src/style-spec/expression/parsing_context.js  |  2 +-
 src/style-spec/reference/v8.json              |  2 +-
 .../collator/accent-equals-de/test.json       | 24 ++++++++++--
 .../collator/accent-lt-en/test.json           | 10 ++++-
 .../collator/accent-not-equals-en/test.json   | 10 ++++-
 .../collator/base-default-locale/test.json    |  4 +-
 .../collator/base-equals-en/test.json         | 10 ++++-
 .../collator/base-gt-en/test.json             | 10 ++++-
 .../collator/case-lteq-en/test.json           | 10 ++++-
 .../collator/case-not-equals-en/test.json     | 10 ++++-
 .../collator/case-omitted-en/test.json        | 32 +++++++++++++++
 .../comparison-number-error/test.json         |  7 +++-
 .../collator/diacritic-omitted-en/test.json   | 31 +++++++++++++++
 .../equals-non-string-error/test.json         |  7 +++-
 .../collator/non-object-error/test.json       | 11 ++++++
 .../collator/variant-equals-en/test.json      | 10 ++++-
 .../collator/variant-gteq-en/test.json        | 10 ++++-
 .../resolved-locale/basic/test.json           | 14 ++++---
 21 files changed, 208 insertions(+), 58 deletions(-)
 create mode 100644 test/integration/expression-tests/collator/case-omitted-en/test.json
 create mode 100644 test/integration/expression-tests/collator/diacritic-omitted-en/test.json
 create mode 100644 test/integration/expression-tests/collator/non-object-error/test.json

diff --git a/docs/components/expression-metadata.js b/docs/components/expression-metadata.js
index 55d78747521..8500ff8e74a 100644
--- a/docs/components/expression-metadata.js
+++ b/docs/components/expression-metadata.js
@@ -204,17 +204,7 @@ const types = {
     }],
     collator: [{
         type: 'collator',
-        parameters: [
-            'caseSensitive: boolean',
-            'diacriticSensitive: boolean',
-        ]
-    }, {
-        type: 'collator',
-        parameters: [
-            'caseSensitive: boolean',
-            'diacriticSensitive: boolean',
-            'locale: string'
-        ]
+        parameters: [ '{ "caseSensitive": boolean, "diacriticSensitive": boolean, "locale": string }' ]
     }]
 };
 
diff --git a/src/style-spec/expression/definitions/collator.js b/src/style-spec/expression/definitions/collator.js
index c3cdd82ccfd..c2628c2cfe6 100644
--- a/src/style-spec/expression/definitions/collator.js
+++ b/src/style-spec/expression/definitions/collator.js
@@ -61,13 +61,13 @@ export class CollatorInstantiation {
     }
 
     serialize() {
-        const serialized = ["collator"];
-        serialized.push(this.sensitivity === 'variant' || this.sensitivity === 'case');
-        serialized.push(this.sensitivity === 'variant' || this.sensitivity === 'accent');
+        const options = {};
+        options['caseSensitive'] = this.sensitivity === 'variant' || this.sensitivity === 'case';
+        options['diacriticSensitive'] = this.sensitivity === 'variant' || this.sensitivity === 'accent';
         if (this.locale) {
-            serialized.push(this.locale);
+            options['locale'] = this.locale;
         }
-        return serialized;
+        return ["collator", options];
     }
 }
 
@@ -85,17 +85,24 @@ export class CollatorExpression implements Expression {
     }
 
     static parse(args: Array<mixed>, context: ParsingContext): ?Expression {
-        if (args.length !== 3 && args.length !== 4)
-            return context.error(`Expected two or three arguments.`);
+        if (args.length !== 2)
+            return context.error(`Expected one argument.`);
 
-        const caseSensitive = context.parse(args[1], 1, BooleanType);
+        const options = (args[1]: any);
+        if (typeof options !== "object" || Array.isArray(options))
+            return context.error(`Collator options argument must be an object.`);
+
+        const caseSensitive = context.parse(
+            options['caseSensitive'] === undefined ? false : options['caseSensitive'], 1, BooleanType);
         if (!caseSensitive) return null;
-        const diacriticSensitive = context.parse(args[2], 2, BooleanType);
+
+        const diacriticSensitive = context.parse(
+            options['diacriticSensitive'] === undefined ? false : options['diacriticSensitive'], 2, BooleanType);
         if (!diacriticSensitive) return null;
 
         let locale = null;
-        if (args.length === 4) {
-            locale = context.parse(args[3], 3, StringType);
+        if (options['locale']) {
+            locale = context.parse(options['locale'], 3, StringType);
             if (!locale) return null;
         }
 
@@ -123,8 +130,12 @@ export class CollatorExpression implements Expression {
     }
 
     serialize() {
-        const serialized = ["collator"];
-        this.eachChild(child => { serialized.push(child.serialize()); });
-        return serialized;
+        const options = {};
+        options['caseSensitive'] = this.caseSensitive;
+        options['diacriticSensitive'] = this.diacriticSensitive;
+        if (this.locale) {
+            options['locale'] = this.locale;
+        }
+        return ["collator", options];
     }
 }
diff --git a/src/style-spec/expression/definitions/index.js b/src/style-spec/expression/definitions/index.js
index 8d2bea004d9..9bee081cbc8 100644
--- a/src/style-spec/expression/definitions/index.js
+++ b/src/style-spec/expression/definitions/index.js
@@ -526,7 +526,6 @@ CompoundExpression.register(expressions, {
         (ctx, args) => args.map(arg => arg.evaluate(ctx)).join('')
     ],
     'resolved-locale': [
-        // Must be added to non-featureConstant list in parsing_context.js
         StringType,
         [CollatorType],
         (ctx, [collator]) => collator.evaluate(ctx).resolvedLocale()
diff --git a/src/style-spec/expression/parsing_context.js b/src/style-spec/expression/parsing_context.js
index 43b0920906b..cb981ca6f5e 100644
--- a/src/style-spec/expression/parsing_context.js
+++ b/src/style-spec/expression/parsing_context.js
@@ -201,5 +201,5 @@ function isConstant(expression: Expression) {
     }
 
     return isFeatureConstant(expression) &&
-        isGlobalPropertyConstant(expression, ['zoom', 'heatmap-density', 'resolved-locale', 'collator']);
+        isGlobalPropertyConstant(expression, ['zoom', 'heatmap-density']);
 }
diff --git a/src/style-spec/reference/v8.json b/src/style-spec/reference/v8.json
index b664676b416..439dc4e6a67 100644
--- a/src/style-spec/reference/v8.json
+++ b/src/style-spec/reference/v8.json
@@ -2194,7 +2194,7 @@
         }
       },
       "collator": {
-        "doc": "Returns a `collator` for use in locale dependent comparison operations. The first two arguments control case and diacritic sensitivity, and the optional third argument specifies the IETF language tag of the locale to use. If no locale is provided, the default locale is used.",
+        "doc": "Returns a `collator` for use in locale dependent comparison operations. The `caseSensitive` and `diacriticSensitive` options default to `false`. The `locale` argument specifies the IETF language tag of the locale to use. If none is provided, the default locale is used. If the requested locale is not available, the `collator` will use a system-defined fallback locale. Use `resolved-locale` to test the results of locale fallback behavior.",
         "group": "Types",
         "sdk-support": {
           "basic functionality": {
diff --git a/test/integration/expression-tests/collator/accent-equals-de/test.json b/test/integration/expression-tests/collator/accent-equals-de/test.json
index a09a80e1efc..ba03ab38f89 100644
--- a/test/integration/expression-tests/collator/accent-equals-de/test.json
+++ b/test/integration/expression-tests/collator/accent-equals-de/test.json
@@ -1,12 +1,25 @@
 {
   "expression": [
     "case",
-    ["==", ["resolved-locale", ["collator", true, false, "de"]], "de"],
+    [
+      "==",
+      [
+        "resolved-locale",
+        [
+          "collator",
+          {"caseSensitive": true, "diacriticSensitive": false, "locale": "de"}
+        ]
+      ],
+      "de"
+    ],
     [
       "==",
       ["string", ["get", "lhs"]],
       ["get", "rhs"],
-      ["collator", true, false, "de"]
+      [
+        "collator",
+        {"caseSensitive": true, "diacriticSensitive": false, "locale": "de"}
+      ]
     ],
     ["case", ["==", ["get", "rhs"], "ue"], true, false]
   ],
@@ -24,12 +37,15 @@
     "outputs": [true, false],
     "serialized": [
       "case",
-      ["==", ["resolved-locale", ["collator", true, false, "de"]], "de"],
+      false,
       [
         "==",
         ["string", ["get", "lhs"]],
         ["get", "rhs"],
-        ["collator", true, false, "de"]
+        [
+          "collator",
+          {"caseSensitive": true, "diacriticSensitive": false, "locale": "de"}
+        ]
       ],
       ["case", ["==", ["get", "rhs"], "ue"], true, false]
     ]
diff --git a/test/integration/expression-tests/collator/accent-lt-en/test.json b/test/integration/expression-tests/collator/accent-lt-en/test.json
index d0ab166898a..339c06ce98a 100644
--- a/test/integration/expression-tests/collator/accent-lt-en/test.json
+++ b/test/integration/expression-tests/collator/accent-lt-en/test.json
@@ -3,7 +3,10 @@
     "<",
     ["string", ["get", "lhs"]],
     ["get", "rhs"],
-    ["collator", true, false, "en"]
+    [
+      "collator",
+      {"caseSensitive": true, "diacriticSensitive": false, "locale": "en"}
+    ]
   ],
   "inputs": [
     [{}, {"properties": {"lhs": "a", "rhs": "ä"}}],
@@ -22,7 +25,10 @@
       "<",
       ["string", ["get", "lhs"]],
       ["string", ["get", "rhs"]],
-      ["collator", true, false, "en"]
+      [
+        "collator",
+        {"caseSensitive": true, "diacriticSensitive": false, "locale": "en"}
+      ]
     ]
   }
 }
diff --git a/test/integration/expression-tests/collator/accent-not-equals-en/test.json b/test/integration/expression-tests/collator/accent-not-equals-en/test.json
index d46cb887c81..270a40c18e3 100644
--- a/test/integration/expression-tests/collator/accent-not-equals-en/test.json
+++ b/test/integration/expression-tests/collator/accent-not-equals-en/test.json
@@ -5,7 +5,10 @@
       "!=",
       ["string", ["get", "lhs"]],
       ["get", "rhs"],
-      ["collator", true, false, "en"]
+      [
+        "collator",
+        {"caseSensitive": true, "diacriticSensitive": false, "locale": "en"}
+      ]
     ]
   ],
   "inputs": [
@@ -27,7 +30,10 @@
         "!=",
         ["string", ["get", "lhs"]],
         ["get", "rhs"],
-        ["collator", true, false, "en"]
+        [
+          "collator",
+          {"caseSensitive": true, "diacriticSensitive": false, "locale": "en"}
+        ]
       ]
     ]
   }
diff --git a/test/integration/expression-tests/collator/base-default-locale/test.json b/test/integration/expression-tests/collator/base-default-locale/test.json
index 18b7907488e..98f80ca8d40 100644
--- a/test/integration/expression-tests/collator/base-default-locale/test.json
+++ b/test/integration/expression-tests/collator/base-default-locale/test.json
@@ -3,7 +3,7 @@
     "==",
     ["string", ["get", "lhs"]],
     ["get", "rhs"],
-    ["collator", false, false]
+    ["collator", {"caseSensitive": false, "diacriticSensitive": false}]
   ],
   "inputs": [
     [{}, {"properties": {"lhs": "a", "rhs": "a"}}],
@@ -22,7 +22,7 @@
       "==",
       ["string", ["get", "lhs"]],
       ["get", "rhs"],
-      ["collator", false, false]
+      ["collator", {"caseSensitive": false, "diacriticSensitive": false}]
     ]
   }
 }
diff --git a/test/integration/expression-tests/collator/base-equals-en/test.json b/test/integration/expression-tests/collator/base-equals-en/test.json
index 6a6bac95067..a594d22e01d 100644
--- a/test/integration/expression-tests/collator/base-equals-en/test.json
+++ b/test/integration/expression-tests/collator/base-equals-en/test.json
@@ -3,7 +3,10 @@
     "==",
     ["string", ["get", "lhs"]],
     ["get", "rhs"],
-    ["collator", false, false, "en"]
+    [
+      "collator",
+      {"caseSensitive": false, "diacriticSensitive": false, "locale": "en"}
+    ]
   ],
   "inputs": [
     [{}, {"properties": {"lhs": "a", "rhs": "ä"}}],
@@ -22,7 +25,10 @@
       "==",
       ["string", ["get", "lhs"]],
       ["get", "rhs"],
-      ["collator", false, false, "en"]
+      [
+        "collator",
+        {"caseSensitive": false, "diacriticSensitive": false, "locale": "en"}
+      ]
     ]
   }
 }
diff --git a/test/integration/expression-tests/collator/base-gt-en/test.json b/test/integration/expression-tests/collator/base-gt-en/test.json
index 6c1a75a3448..93e860f6912 100644
--- a/test/integration/expression-tests/collator/base-gt-en/test.json
+++ b/test/integration/expression-tests/collator/base-gt-en/test.json
@@ -3,7 +3,10 @@
     ">",
     ["string", ["get", "lhs"]],
     ["get", "rhs"],
-    ["collator", false, false, "en"]
+    [
+      "collator",
+      {"caseSensitive": false, "diacriticSensitive": false, "locale": "en"}
+    ]
   ],
   "inputs": [
     [{}, {"properties": {"lhs": "a", "rhs": "ä"}}],
@@ -22,7 +25,10 @@
       ">",
       ["string", ["get", "lhs"]],
       ["string", ["get", "rhs"]],
-      ["collator", false, false, "en"]
+      [
+        "collator",
+        {"caseSensitive": false, "diacriticSensitive": false, "locale": "en"}
+      ]
     ]
   }
 }
diff --git a/test/integration/expression-tests/collator/case-lteq-en/test.json b/test/integration/expression-tests/collator/case-lteq-en/test.json
index 7d159668644..a014f39f139 100644
--- a/test/integration/expression-tests/collator/case-lteq-en/test.json
+++ b/test/integration/expression-tests/collator/case-lteq-en/test.json
@@ -3,7 +3,10 @@
     "<=",
     ["string", ["get", "lhs"]],
     ["get", "rhs"],
-    ["collator", false, true, "en"]
+    [
+      "collator",
+      {"caseSensitive": false, "diacriticSensitive": true, "locale": "en"}
+    ]
   ],
   "inputs": [
     [{}, {"properties": {"lhs": "ä", "rhs": "a"}}],
@@ -23,7 +26,10 @@
       "<=",
       ["string", ["get", "lhs"]],
       ["string", ["get", "rhs"]],
-      ["collator", false, true, "en"]
+      [
+        "collator",
+        {"caseSensitive": false, "diacriticSensitive": true, "locale": "en"}
+      ]
     ]
   }
 }
diff --git a/test/integration/expression-tests/collator/case-not-equals-en/test.json b/test/integration/expression-tests/collator/case-not-equals-en/test.json
index bd830c5efa5..1be44168675 100644
--- a/test/integration/expression-tests/collator/case-not-equals-en/test.json
+++ b/test/integration/expression-tests/collator/case-not-equals-en/test.json
@@ -5,7 +5,10 @@
       "!=",
       ["string", ["get", "lhs"]],
       ["get", "rhs"],
-      ["collator", false, true, "en"]
+      [
+        "collator",
+        {"caseSensitive": false, "diacriticSensitive": true, "locale": "en"}
+      ]
     ]
   ],
   "inputs": [
@@ -27,7 +30,10 @@
         "!=",
         ["string", ["get", "lhs"]],
         ["get", "rhs"],
-        ["collator", false, true, "en"]
+        [
+          "collator",
+          {"caseSensitive": false, "diacriticSensitive": true, "locale": "en"}
+        ]
       ]
     ]
   }
diff --git a/test/integration/expression-tests/collator/case-omitted-en/test.json b/test/integration/expression-tests/collator/case-omitted-en/test.json
new file mode 100644
index 00000000000..80ae5dc4d47
--- /dev/null
+++ b/test/integration/expression-tests/collator/case-omitted-en/test.json
@@ -0,0 +1,32 @@
+{
+  "expression": [
+    "==",
+    ["string", ["get", "lhs"]],
+    ["get", "rhs"],
+    ["collator", {"diacriticSensitive": true, "locale": "en"}]
+  ],
+  "inputs": [
+    [{}, {"properties": {"lhs": "ä", "rhs": "a"}}],
+    [{}, {"properties": {"lhs": "A", "rhs": "a"}}],
+    [{}, {"properties": {"lhs": "a", "rhs": "a"}}],
+    [{}, {"properties": {"lhs": "ä", "rhs": "b"}}]
+  ],
+  "expected": {
+    "compiled": {
+      "result": "success",
+      "isFeatureConstant": false,
+      "isZoomConstant": true,
+      "type": "boolean"
+    },
+    "outputs": [false, true, true, false],
+    "serialized": [
+      "==",
+      ["string", ["get", "lhs"]],
+      ["get", "rhs"],
+      [
+        "collator",
+        {"caseSensitive": false, "diacriticSensitive": true, "locale": "en"}
+      ]
+    ]
+  }
+}
diff --git a/test/integration/expression-tests/collator/comparison-number-error/test.json b/test/integration/expression-tests/collator/comparison-number-error/test.json
index 9eed27bb59d..31acf3136df 100644
--- a/test/integration/expression-tests/collator/comparison-number-error/test.json
+++ b/test/integration/expression-tests/collator/comparison-number-error/test.json
@@ -1,5 +1,10 @@
 {
-  "expression": ["<", 1, 2, ["collator", false, false]],
+  "expression": [
+    "<",
+    1,
+    2,
+    ["collator", {"caseSensitive": false, "diacriticSensitive": false}]
+  ],
   "expected": {
     "compiled": {
       "result": "error",
diff --git a/test/integration/expression-tests/collator/diacritic-omitted-en/test.json b/test/integration/expression-tests/collator/diacritic-omitted-en/test.json
new file mode 100644
index 00000000000..bdfaf53c80f
--- /dev/null
+++ b/test/integration/expression-tests/collator/diacritic-omitted-en/test.json
@@ -0,0 +1,31 @@
+{
+  "expression": [
+    "<",
+    ["string", ["get", "lhs"]],
+    ["get", "rhs"],
+    ["collator", {"caseSensitive": ["==", 1, 1], "locale": "en"}]
+  ],
+  "inputs": [
+    [{}, {"properties": {"lhs": "a", "rhs": "ä"}}],
+    [{}, {"properties": {"lhs": "a", "rhs": "A"}}],
+    [{}, {"properties": {"lhs": "ä", "rhs": "b"}}]
+  ],
+  "expected": {
+    "compiled": {
+      "result": "success",
+      "isFeatureConstant": false,
+      "isZoomConstant": true,
+      "type": "boolean"
+    },
+    "outputs": [false, true, true],
+    "serialized": [
+      "<",
+      ["string", ["get", "lhs"]],
+      ["string", ["get", "rhs"]],
+      [
+        "collator",
+        {"caseSensitive": true, "diacriticSensitive": false, "locale": "en"}
+      ]
+    ]
+  }
+}
diff --git a/test/integration/expression-tests/collator/equals-non-string-error/test.json b/test/integration/expression-tests/collator/equals-non-string-error/test.json
index 6f51af5ab25..ed8551d16eb 100644
--- a/test/integration/expression-tests/collator/equals-non-string-error/test.json
+++ b/test/integration/expression-tests/collator/equals-non-string-error/test.json
@@ -1,5 +1,10 @@
 {
-  "expression": ["==", 1, 2, ["collator", false, false]],
+  "expression": [
+    "==",
+    1,
+    2,
+    ["collator", {"caseSensitive": false, "diacriticSensitive": false}]
+  ],
   "expected": {
     "compiled": {
       "result": "error",
diff --git a/test/integration/expression-tests/collator/non-object-error/test.json b/test/integration/expression-tests/collator/non-object-error/test.json
new file mode 100644
index 00000000000..148c9f1b5f9
--- /dev/null
+++ b/test/integration/expression-tests/collator/non-object-error/test.json
@@ -0,0 +1,11 @@
+{
+  "expression": ["==", "foo", "bar", ["collator", ["subexpression"]]],
+  "expected": {
+    "compiled": {
+      "result": "error",
+      "errors": [
+        {"key": "[3]", "error": "Collator options argument must be an object."}
+      ]
+    }
+  }
+}
diff --git a/test/integration/expression-tests/collator/variant-equals-en/test.json b/test/integration/expression-tests/collator/variant-equals-en/test.json
index 6b858abbf43..8e33b73b598 100644
--- a/test/integration/expression-tests/collator/variant-equals-en/test.json
+++ b/test/integration/expression-tests/collator/variant-equals-en/test.json
@@ -3,7 +3,10 @@
     "==",
     ["string", ["get", "lhs"]],
     ["get", "rhs"],
-    ["collator", true, true, "en"]
+    [
+      "collator",
+      {"caseSensitive": true, "diacriticSensitive": true, "locale": "en"}
+    ]
   ],
   "inputs": [
     [{}, {"properties": {"lhs": "a", "rhs": "ä"}}],
@@ -22,7 +25,10 @@
       "==",
       ["string", ["get", "lhs"]],
       ["get", "rhs"],
-      ["collator", true, true, "en"]
+      [
+        "collator",
+        {"caseSensitive": true, "diacriticSensitive": true, "locale": "en"}
+      ]
     ]
   }
 }
diff --git a/test/integration/expression-tests/collator/variant-gteq-en/test.json b/test/integration/expression-tests/collator/variant-gteq-en/test.json
index 8bfcd08eab4..22a9ad3ed17 100644
--- a/test/integration/expression-tests/collator/variant-gteq-en/test.json
+++ b/test/integration/expression-tests/collator/variant-gteq-en/test.json
@@ -3,7 +3,10 @@
     ">=",
     ["string", ["get", "lhs"]],
     ["get", "rhs"],
-    ["collator", true, true, "en"]
+    [
+      "collator",
+      {"caseSensitive": true, "diacriticSensitive": true, "locale": "en"}
+    ]
   ],
   "inputs": [
     [{}, {"properties": {"lhs": "a", "rhs": "ä"}}],
@@ -23,7 +26,10 @@
       ">=",
       ["string", ["get", "lhs"]],
       ["string", ["get", "rhs"]],
-      ["collator", true, true, "en"]
+      [
+        "collator",
+        {"caseSensitive": true, "diacriticSensitive": true, "locale": "en"}
+      ]
     ]
   }
 }
diff --git a/test/integration/expression-tests/resolved-locale/basic/test.json b/test/integration/expression-tests/resolved-locale/basic/test.json
index 6a65a9b81d5..097e10573b4 100644
--- a/test/integration/expression-tests/resolved-locale/basic/test.json
+++ b/test/integration/expression-tests/resolved-locale/basic/test.json
@@ -1,7 +1,13 @@
 {
   "expression": [
     "==",
-    ["resolved-locale", ["collator", true, true, "en"]],
+    [
+      "resolved-locale",
+      [
+        "collator",
+        {"caseSensitive": true, "diacriticSensitive": true, "locale": "en"}
+      ]
+    ],
     "en"
   ],
   "inputs": [[{}, {}]],
@@ -13,10 +19,6 @@
       "type": "boolean"
     },
     "outputs": [true],
-    "serialized": [
-      "==",
-      ["resolved-locale", ["collator", true, true, "en"]],
-      "en"
-    ]
+    "serialized": true
   }
 }