[ios] fixes #6248: properly handle all NSNumber types in filters (#6290)

4 files changed, 236 insertions, 7 deletions

diff --git a/platform/darwin/src/NSExpression+MGLAdditions.mm b/platform/darwin/src/NSExpression+MGLAdditions.mm
index 129c9a740c..52a0b9bd88 100644
--- a/platform/darwin/src/NSExpression+MGLAdditions.mm
+++ b/platform/darwin/src/NSExpression+MGLAdditions.mm
@@ -27,12 +27,33 @@
         return { std::string([(NSString *)value UTF8String]) };
     } else if ([value isKindOfClass:NSNumber.class]) {
         NSNumber *number = (NSNumber *)value;
-        if((strcmp([number objCType], @encode(int))) == 0) {
-            return { number.intValue };
-        } else if ((strcmp([number objCType], @encode(double))) == 0) {
-            return { number.doubleValue };
-        } else if ((strcmp([number objCType], @encode(char))) == 0) {
-            return { number.boolValue };
+        if ((strcmp([number objCType], @encode(char)) == 0) ||
+            (strcmp([number objCType], @encode(BOOL)) == 0)) {
+            // char: 32-bit boolean
+            // BOOL: 64-bit boolean
+            return { (bool)number.boolValue };
+        } else if (strcmp([number objCType], @encode(double)) == 0) {
+            // Double values on all platforms are interpreted precisely.
+            return { (double)number.doubleValue };
+        } else if (strcmp([number objCType], @encode(float)) == 0) {
+            // Float values when taken as double introduce precision problems,
+            // so warn the user to avoid them. This would require them to
+            // explicitly use -[NSNumber numberWithFloat:] arguments anyway.
+            // We still do this conversion in order to provide a valid value.
+            static dispatch_once_t onceToken;
+            dispatch_once(&onceToken, ^{
+                NSLog(@"One-time warning: Float values are converted to double and can introduce imprecision; please use double values explicitly in predicate arguments.");
+            });
+            return { (double)number.doubleValue };
+        } else if ([number compare:@(0)] == NSOrderedDescending ||
+                   [number compare:@(0)] == NSOrderedSame) {
+            // Positive integer or zero; use uint64_t per mbgl::Value definition.
+            // We use unsigned long long here to avoid any truncation.
+            return { (uint64_t)number.unsignedLongLongValue };
+        } else if ([number compare:@(0)] == NSOrderedAscending) {
+            // Negative integer; use int64_t per mbgl::Value definition.
+            // We use long long here to avoid any truncation.
+            return { (int64_t)number.longLongValue };
         }
     }
     [NSException raise:@"Value not handled"
diff --git a/platform/darwin/test/MGLExpressionTests.mm b/platform/darwin/test/MGLExpressionTests.mm
new file mode 100644
index 0000000000..971f7d0e54
--- /dev/null
+++ b/platform/darwin/test/MGLExpressionTests.mm
@@ -0,0 +1,200 @@
+#import <XCTest/XCTest.h>
+
+#import <string>
+
+#import "NSExpression+MGLAdditions.h"
+
+@interface MGLExpressionTests : XCTestCase
+
+@end
+
+@implementation MGLExpressionTests
+
+#pragma mark - Utility
+
+- (NSComparisonPredicate *)equalityComparisonPredicateWithRightConstantValue:(id)rightConstantValue
+{
+    NSComparisonPredicate *predicate = [NSComparisonPredicate
+        predicateWithLeftExpression:[NSExpression expressionForKeyPath:@"foo"]
+                    rightExpression:[NSExpression expressionForConstantValue:rightConstantValue]
+                           modifier:NSDirectPredicateModifier
+                               type:NSEqualToPredicateOperatorType
+                            options:0];
+    return predicate;
+}
+
+#pragma mark - String Tests
+
+- (void)testExpressionConversionString
+{
+    NSComparisonPredicate *predicate = [self equalityComparisonPredicateWithRightConstantValue:@"bar"];
+    mbgl::Value convertedValue = predicate.rightExpression.mgl_filterValue;
+    XCTAssert(convertedValue.is<std::string>() == true);
+    XCTAssertEqualObjects(@(convertedValue.get<std::string>().c_str()), @"bar");
+}
+
+#pragma mark - Boolean Tests
+
+- (void)testExpressionConversionBooleanTrue
+{
+    NSComparisonPredicate *predicate = [self equalityComparisonPredicateWithRightConstantValue:@YES];
+    mbgl::Value convertedValue = predicate.rightExpression.mgl_filterValue;
+    XCTAssert(convertedValue.is<bool>() == true);
+    XCTAssert(convertedValue.get<bool>() == true);
+}
+
+- (void)testExpressionConversionBooleanFalse
+{
+    NSComparisonPredicate *predicate = [self equalityComparisonPredicateWithRightConstantValue:@NO];
+    mbgl::Value convertedValue = predicate.rightExpression.mgl_filterValue;
+    XCTAssert(convertedValue.is<bool>() == true);
+    XCTAssert(convertedValue.get<bool>() == false);
+}
+
+#pragma mark - Floating Point Tests
+
+- (void)testExpressionConversionDouble
+{
+    NSComparisonPredicate *predicate;
+    mbgl::Value convertedValue;
+
+    predicate = [self equalityComparisonPredicateWithRightConstantValue:[NSNumber numberWithDouble:DBL_MIN]];
+    convertedValue = predicate.rightExpression.mgl_filterValue;
+    XCTAssertTrue(convertedValue.is<double>());
+    XCTAssertEqual(convertedValue.get<double>(), DBL_MIN);
+
+    predicate = [self equalityComparisonPredicateWithRightConstantValue:[NSNumber numberWithDouble:DBL_MAX]];
+    convertedValue = predicate.rightExpression.mgl_filterValue;
+    XCTAssertTrue(convertedValue.is<double>());
+    XCTAssertEqual(convertedValue.get<double>(), DBL_MAX);
+}
+
+- (void)testExpressionConversionFloat
+{
+    // Because we can't guarantee precision when using float, and because
+    // we warn the user to this effect in mgl_convertedValueWithValue:,
+    // we just check that things are in the ballpark here with integer values
+    // and some lower-precision checks.
+
+    NSComparisonPredicate *predicate;
+    mbgl::Value convertedValue;
+
+    predicate = [self equalityComparisonPredicateWithRightConstantValue:[NSNumber numberWithFloat:-1]];
+    convertedValue = predicate.rightExpression.mgl_filterValue;
+    XCTAssertTrue(convertedValue.is<double>());
+    XCTAssertEqual(convertedValue.get<double>(), -1);
+
+    predicate = [self equalityComparisonPredicateWithRightConstantValue:[NSNumber numberWithFloat:1]];
+    convertedValue = predicate.rightExpression.mgl_filterValue;
+    XCTAssertTrue(convertedValue.is<double>());
+    XCTAssertEqual(convertedValue.get<double>(), 1);
+
+    predicate = [self equalityComparisonPredicateWithRightConstantValue:[NSNumber numberWithFloat:-23.232342]];
+    convertedValue = predicate.rightExpression.mgl_filterValue;
+    XCTAssertTrue(convertedValue.is<double>());
+    XCTAssertLessThan(-23.24, convertedValue.get<double>());
+    XCTAssertGreaterThan(-23.23, convertedValue.get<double>());
+
+    predicate = [self equalityComparisonPredicateWithRightConstantValue:[NSNumber numberWithFloat:23.232342]];
+    convertedValue = predicate.rightExpression.mgl_filterValue;
+    XCTAssertTrue(convertedValue.is<double>());
+    XCTAssertLessThan(23.23, convertedValue.get<double>());
+    XCTAssertGreaterThan(23.24, convertedValue.get<double>());
+}
+
+#pragma mark - Integer Tests
+
+- (void)testExpressionNegativeIntegers
+{
+    NSComparisonPredicate *predicate;
+    mbgl::Value convertedValue;
+
+    NSArray<NSNumber *> *minValues = @[
+        [NSNumber numberWithShort:    SHRT_MIN],
+        [NSNumber numberWithInt:      INT_MIN],
+        [NSNumber numberWithLong:     LONG_MIN],
+        [NSNumber numberWithLongLong: LLONG_MIN],
+        [NSNumber numberWithInteger:  NSIntegerMin]
+    ];
+
+    NSArray<NSNumber *> *maxValues = @[
+        [NSNumber numberWithShort:    SHRT_MAX],
+        [NSNumber numberWithInt:      INT_MAX],
+        [NSNumber numberWithLong:     LONG_MAX],
+        [NSNumber numberWithLongLong: LLONG_MAX],
+        [NSNumber numberWithInteger:  NSIntegerMax]
+    ];
+
+    // Negative integers should always come back as int64_t per mbgl::Value definition.
+    // We use the long long value because it can store the highest number on both 32-
+    // and 64-bit and won't overflow.
+
+    for (NSNumber *min in minValues)
+    {
+        predicate = [self equalityComparisonPredicateWithRightConstantValue:min];
+        convertedValue = predicate.rightExpression.mgl_filterValue;
+        XCTAssertTrue(convertedValue.is<int64_t>());
+        XCTAssertEqual(convertedValue.get<int64_t>(), min.longLongValue);
+    }
+
+    // Positive integers should always come back as uint64_t per mbgl::Value definition.
+    // We use the unsigned long long value because it can store the highest number on
+    // both 32- and 64-bit and won't overflow.
+
+    for (NSNumber *max in maxValues)
+    {
+        predicate = [self equalityComparisonPredicateWithRightConstantValue:max];
+        convertedValue = predicate.rightExpression.mgl_filterValue;
+        XCTAssertTrue(convertedValue.is<uint64_t>());
+        XCTAssertEqual(convertedValue.get<uint64_t>(), max.unsignedLongLongValue);
+    }
+
+}
+
+- (void)testExpressionPositiveAndZeroIntegers
+{
+    NSComparisonPredicate *predicate;
+    mbgl::Value convertedValue;
+
+    NSArray<NSNumber *> *minValues = @[
+        [NSNumber numberWithUnsignedShort:    0],
+        [NSNumber numberWithUnsignedInt:      0],
+        [NSNumber numberWithUnsignedLong:     0],
+        [NSNumber numberWithUnsignedLongLong: 0],
+        [NSNumber numberWithUnsignedInteger:  0]
+    ];
+
+    NSArray<NSNumber *> *maxValues = @[
+        [NSNumber numberWithUnsignedShort:    USHRT_MAX],
+        [NSNumber numberWithUnsignedInt:      UINT_MAX],
+        [NSNumber numberWithUnsignedLong:     ULONG_MAX],
+        [NSNumber numberWithUnsignedLongLong: ULLONG_MAX],
+        [NSNumber numberWithUnsignedInteger:  NSUIntegerMax]
+    ];
+
+    // Zero-value integers should always come back as uint64_t per mbgl::Value definition
+    // (using the interpretation that zero is not negative). We use the unsigned long long
+    // value just for parity with the positive integer test.
+
+    for (NSNumber *min in minValues)
+    {
+        predicate = [self equalityComparisonPredicateWithRightConstantValue:min];
+        convertedValue = predicate.rightExpression.mgl_filterValue;
+        XCTAssertTrue(convertedValue.is<uint64_t>());
+        XCTAssertEqual(convertedValue.get<uint64_t>(), min.unsignedLongLongValue);
+    }
+
+    // Positive integers should always come back as uint64_t per mbgl::Value definition.
+    // We use the unsigned long long value because it can store the highest number on
+    // both 32- and 64-bit and won't overflow.
+
+    for (NSNumber *max in maxValues)
+    {
+        predicate = [self equalityComparisonPredicateWithRightConstantValue:max];
+        convertedValue = predicate.rightExpression.mgl_filterValue;
+        XCTAssertTrue(convertedValue.is<uint64_t>());
+        XCTAssertEqual(convertedValue.get<uint64_t>(), max.unsignedLongLongValue);
+    }
+}
+
+@end
diff --git a/platform/ios/ios.xcodeproj/project.pbxproj b/platform/ios/ios.xcodeproj/project.pbxproj
index 90e0f1cfbc..35a1075301 100644
--- a/platform/ios/ios.xcodeproj/project.pbxproj
+++ b/platform/ios/ios.xcodeproj/project.pbxproj
@@ -397,6 +397,7 @@
 		DAED38641D62D0FC00D7640F /* NSURL+MGLAdditions.h in Headers */ = {isa = PBXBuildFile; fileRef = DAED38611D62D0FC00D7640F /* NSURL+MGLAdditions.h */; };
 		DAED38651D62D0FC00D7640F /* NSURL+MGLAdditions.m in Sources */ = {isa = PBXBuildFile; fileRef = DAED38621D62D0FC00D7640F /* NSURL+MGLAdditions.m */; };
 		DAED38661D62D0FC00D7640F /* NSURL+MGLAdditions.m in Sources */ = {isa = PBXBuildFile; fileRef = DAED38621D62D0FC00D7640F /* NSURL+MGLAdditions.m */; };
+		DD58A4C61D822BD000E1F038 /* MGLExpressionTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = DD58A4C51D822BD000E1F038 /* MGLExpressionTests.mm */; };
 /* End PBXBuildFile section */
 
 /* Begin PBXContainerItemProxy section */
@@ -746,6 +747,7 @@
 		DAD165771CF4CDFF001FF4B9 /* MGLShapeCollection.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MGLShapeCollection.m; sourceTree = "<group>"; };
 		DAED38611D62D0FC00D7640F /* NSURL+MGLAdditions.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = "NSURL+MGLAdditions.h"; sourceTree = "<group>"; };
 		DAED38621D62D0FC00D7640F /* NSURL+MGLAdditions.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = "NSURL+MGLAdditions.m"; sourceTree = "<group>"; };
+		DD58A4C51D822BD000E1F038 /* MGLExpressionTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = MGLExpressionTests.mm; path = ../../darwin/test/MGLExpressionTests.mm; sourceTree = "<group>"; };
 /* End PBXFileReference section */
 
 /* Begin PBXFrameworksBuildPhase section */
@@ -882,7 +884,7 @@
 			children = (
 				3575798F1D513EF1000B822E /* Layers */,
 				35B8E08B1D6C8B5100E768D2 /* MGLFilterTests.mm */,
-                40CFA64E1D78754A008103BD /* Sources */,
+				40CFA64E1D78754A008103BD /* Sources */,
 			);
 			name = Styling;
 			sourceTree = "<group>";
@@ -1037,6 +1039,7 @@
 				DA2E885E1CC0382C00F24E7B /* MGLOfflineRegionTests.m */,
 				DA2E885F1CC0382C00F24E7B /* MGLOfflineStorageTests.m */,
 				DA2E88601CC0382C00F24E7B /* MGLStyleTests.mm */,
+				DD58A4C51D822BD000E1F038 /* MGLExpressionTests.mm */,
 				DA2E88551CC036F400F24E7B /* Info.plist */,
 			);
 			name = "SDK Tests";
@@ -1860,6 +1863,7 @@
 				40CFA6511D7875BB008103BD /* MGLGeoJSONSourceTests.mm in Sources */,
 				DA35A2C51CCA9F8300E826B2 /* MGLClockDirectionFormatterTests.m in Sources */,
 				35B8E08C1D6C8B5100E768D2 /* MGLFilterTests.mm in Sources */,
+				DD58A4C61D822BD000E1F038 /* MGLExpressionTests.mm in Sources */,
 				3575798B1D502B0C000B822E /* MGLBackgroundStyleLayerTests.m in Sources */,
 				DA2E88621CC0382C00F24E7B /* MGLOfflinePackTests.m in Sources */,
 				DA35A2AA1CCA058D00E826B2 /* MGLCoordinateFormatterTests.m in Sources */,
diff --git a/platform/macos/macos.xcodeproj/project.pbxproj b/platform/macos/macos.xcodeproj/project.pbxproj
index 728e15ea95..54d141e739 100644
--- a/platform/macos/macos.xcodeproj/project.pbxproj
+++ b/platform/macos/macos.xcodeproj/project.pbxproj
@@ -180,6 +180,7 @@
 		DAE6C3D61CC34C9900DB3429 /* MGLStyleTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = DAE6C3CC1CC34BD800DB3429 /* MGLStyleTests.mm */; };
 		DAED385F1D62CED700D7640F /* NSURL+MGLAdditions.h in Headers */ = {isa = PBXBuildFile; fileRef = DAED385D1D62CED700D7640F /* NSURL+MGLAdditions.h */; };
 		DAED38601D62CED700D7640F /* NSURL+MGLAdditions.m in Sources */ = {isa = PBXBuildFile; fileRef = DAED385E1D62CED700D7640F /* NSURL+MGLAdditions.m */; };
+		DD58A4C91D822C6700E1F038 /* MGLExpressionTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = DD58A4C71D822C6200E1F038 /* MGLExpressionTests.mm */; };
 /* End PBXBuildFile section */
 
 /* Begin PBXContainerItemProxy section */
@@ -412,6 +413,7 @@
 		DAE6C3CC1CC34BD800DB3429 /* MGLStyleTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = MGLStyleTests.mm; path = ../../darwin/test/MGLStyleTests.mm; sourceTree = "<group>"; };
 		DAED385D1D62CED700D7640F /* NSURL+MGLAdditions.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = "NSURL+MGLAdditions.h"; sourceTree = "<group>"; };
 		DAED385E1D62CED700D7640F /* NSURL+MGLAdditions.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = "NSURL+MGLAdditions.m"; sourceTree = "<group>"; };
+		DD58A4C71D822C6200E1F038 /* MGLExpressionTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = MGLExpressionTests.mm; path = ../../darwin/test/MGLExpressionTests.mm; sourceTree = "<group>"; };
 /* End PBXFileReference section */
 
 /* Begin PBXFrameworksBuildPhase section */
@@ -773,6 +775,7 @@
 				DAE6C3CA1CC34BD800DB3429 /* MGLOfflineRegionTests.m */,
 				DAE6C3CB1CC34BD800DB3429 /* MGLOfflineStorageTests.m */,
 				DAE6C3CC1CC34BD800DB3429 /* MGLStyleTests.mm */,
+				DD58A4C71D822C6200E1F038 /* MGLExpressionTests.mm */,
 				DAE6C33A1CC30DB200DB3429 /* Info.plist */,
 			);
 			name = "SDK Tests";
@@ -1169,6 +1172,7 @@
 				DA35A2B61CCA14D700E826B2 /* MGLCompassDirectionFormatterTests.m in Sources */,
 				DAE6C3D21CC34C9900DB3429 /* MGLGeometryTests.mm in Sources */,
 				DAE6C3D51CC34C9900DB3429 /* MGLOfflineStorageTests.m in Sources */,
+				DD58A4C91D822C6700E1F038 /* MGLExpressionTests.mm in Sources */,
 				DAE6C3D31CC34C9900DB3429 /* MGLOfflinePackTests.m in Sources */,
 				DA35A2A81CC9F41600E826B2 /* MGLCoordinateFormatterTests.m in Sources */,
 				DA0CD58E1CF56F5800A5F5A5 /* MGLFeatureTests.mm in Sources */,