Assert duplicate objects and check object types on map lookup

Summary:
Followup to make sure that object type mismatches can't happen, even if identifiers collide (which is discouraged). Add assert when duplicates are detected.

Patched some unit tests while I'm in here.

Reviewed By: calimarkus

Differential Revision: D6439094

fbshipit-source-id: d669c01734e5ce9483e851051f548d9960b3087c

CHANGELOG.md

@@ -29,6 +29,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag
 
 - Duplicate view models in `IGListBindingSectionController` gets filtered out. [Weyert de Boer](https://github.com/weyert) [(#916)](https://github.com/Instagram/IGListKit/pull/916)
 
+- Check object type on lookup to prevent crossing types if different objects collide with their identifiers. [Ryan Nystrom](https://github.com/rnystrom) [(#tbd)](https://github.com/Instagram/IGListKit/pull/tbd)
+
 3.1.1
 -----
 

Source/Common/Internal/IGListArrayUtilsInternal.h

@@ -10,21 +10,24 @@
 #ifndef IGListArrayUtilsInternal_h
 #define IGListArrayUtilsInternal_h
 
+#import <IGListKit/IGListAssert.h>
+
 static NSArray *objectsWithDuplicateIdentifiersRemoved(NSArray<id<IGListDiffable>> *objects) {
     if (objects == nil) {
         return nil;
     }
     
-    NSMutableSet *identifiers = [NSMutableSet new];
+    NSMapTable *identifierMap = [NSMapTable strongToStrongObjectsMapTable];
     NSMutableArray *uniqueObjects = [NSMutableArray new];
     for (id<IGListDiffable> object in objects) {
         id diffIdentifier = [object diffIdentifier];
+        id previousObject = [identifierMap objectForKey:diffIdentifier];
         if (diffIdentifier != nil
-            && ![identifiers containsObject:diffIdentifier]) {
-            [identifiers addObject:diffIdentifier];
+            && previousObject == nil) {
+            [identifierMap setObject:object forKey:diffIdentifier];
             [uniqueObjects addObject:object];
         } else {
-            IGLKLog(@"WARNING: Diff identifier %@ for object %@ already appeared in objects array", diffIdentifier, object);
+            IGFailAssert(@"Duplicate identifier %@ for object %@ with object %@", diffIdentifier, object, previousObject);
         }
     }
     return uniqueObjects;

Source/IGListAdapterUpdater.m

@@ -384,9 +384,10 @@ void convertReloadToDeleteInsert(NSMutableIndexSet *reloads,
 #pragma mark - IGListUpdatingDelegate
 
 static BOOL IGListIsEqual(const void *a, const void *b, NSUInteger (*size)(const void *item)) {
-    const id<IGListDiffable> left = (__bridge id<IGListDiffable>)a;
-    const id<IGListDiffable> right = (__bridge id<IGListDiffable>)b;
-    return [[left diffIdentifier] isEqual:[right diffIdentifier]];
+    const id<IGListDiffable, NSObject> left = (__bridge id<IGListDiffable, NSObject>)a;
+    const id<IGListDiffable, NSObject> right = (__bridge id<IGListDiffable, NSObject>)b;
+    return [left class] == [right class]
+    && [[left diffIdentifier] isEqual:[right diffIdentifier]];
 }
 
 // since the diffing algo used in this updater keys items based on their -diffIdentifier, we must use a map table that

Source/Internal/IGListDebugger.h

@@ -20,6 +20,8 @@ IGLK_SUBCLASSING_RESTRICTED
 
 + (NSArray<NSString *> *)adapterDescriptions;
 
++ (void)clear;
+
 + (NSString *)dump;
 
 - (instancetype)init NS_UNAVAILABLE;

Source/Internal/IGListDebugger.m

@@ -33,6 +33,10 @@ static NSHashTable<IGListAdapter *> *livingAdaptersTable = nil;
     return descriptions;
 }
 
++ (void)clear {
+    [livingAdaptersTable removeAllObjects];
+}
+
 + (NSString *)dump {
     return [[self adapterDescriptions] componentsJoinedByString:@"\n"];
 }

Tests/IGListAdapterE2ETests.m

@@ -959,33 +959,6 @@
     [self waitForExpectationsWithTimeout:30 handler:nil];
 }
 
-- (void)test_whenBatchUpdating_withDuplicateIdentifiers_thatHaveDifferentValues_thatCollectionViewWorks {
-    [self setupWithObjects:@[
-                             // using string values IGTestDelegateController always returns 1 cell
-                             genTestObject(@1, @"a"),
-                             genTestObject(@2, @"a"),
-                             genTestObject(@3, @"a"),
-                             genTestObject(@4, @"b"), // problem item w/ key 4, value "b"
-                             ]];
-
-    self.dataSource.objects = @[
-                                genTestObject(@1, @"a"),
-                                genTestObject(@5, @"a"),
-                                genTestObject(@6, @"a"),
-                                genTestObject(@7, @"a"),
-                                genTestObject(@4, @"a"), // key 4 but value "a", so this needs reloaded
-                                genTestObject(@8, @"a"),
-                                genTestObject(@4, @"b"), // key 4 but value didn't change
-                                ];
-    XCTestExpectation *expectation = genExpectation;
-
-    [self.adapter performUpdatesAnimated:YES completion:^(BOOL finished) {
-        [expectation fulfill];
-    }];
-
-    [self waitForExpectationsWithTimeout:30 handler:nil];
-}
-
 - (void)test_whenPerformingUpdates_withWorkingRange_thatAccessingCellDoesntCrash {
     [self setupWithObjects:@[
                              genTestObject(@1, @1),

Tests/IGListAdapterStoryboardTests.m

@@ -65,7 +65,7 @@ static const CGRect kStackTestFrame = (CGRect){{0.0, 0.0}, {100.0, 100.0}};
     supplementarySource.collectionContext = self.adapter;
     supplementarySource.supportedElementKinds = @[UICollectionElementKindSectionHeader];
 
-    IGListSectionController *controller = [self.adapter sectionControllerForObject:@1];
+    IGListSectionController *controller = [self.adapter sectionControllerForObject:objects.firstObject];
     controller.supplementaryViewSource = supplementarySource;
     supplementarySource.sectionController = controller;
 
@@ -79,5 +79,4 @@ static const CGRect kStackTestFrame = (CGRect){{0.0, 0.0}, {100.0, 100.0}};
     XCTAssertNotNil([self.collectionView supplementaryViewForElementKind:UICollectionElementKindSectionHeader atIndexPath:[NSIndexPath indexPathForItem:0 inSection:0]]);
 }
 
-
 @end

Tests/IGListAdapterTests.m

@@ -1527,10 +1527,9 @@
     XCTAssertEqual(collectionView1.dataSource, adapter2);
 }
 
-- (void)test_whenPassingNonUniqueIdentifiers_adapterReloadShouldSkipDuplicates {
+- (void)test_whenPassingNonUniqueIdentifiers_adapterReloadShouldThrow {
     self.dataSource.objects = @[@0, @1, @2, @0];
-    [self.adapter reloadDataWithCompletion:nil];
-    XCTAssertEqual(self.adapter.objects.count, 3);
+    XCTAssertThrows([self.adapter reloadDataWithCompletion:nil]);
 }
 
 - (void)test_whenPrefetchingEnabled_thatSetterDisables {

Tests/IGListAdapterUpdaterTests.m

@@ -14,6 +14,7 @@
 
 #import "IGListAdapterUpdaterInternal.h"
 #import "IGListTestUICollectionViewDataSource.h"
+#import "IGTestObject.h"
 
 #define genExpectation [self expectationWithDescription:NSStringFromSelector(_cmd)]
 #define waitExpectation [self waitForExpectationsWithTimeout:30 handler:nil]
@@ -508,7 +509,7 @@
     [mockDelegate verify];
 }
 
-- (void)test_ {
+- (void)test_whenReloadBatchedWithUpdate_thatCompletionBlockStillExecuted {
     IGSectionObject *object = [IGSectionObject sectionWithObjects:@[@0, @1, @2]];
     self.dataSource.sections = @[object];
 
@@ -538,7 +539,7 @@
     [self waitForExpectationsWithTimeout:30 handler:nil];
 }
 
-- (void)test_2 {
+- (void)test_whenNotInViewHierarchy_thatUpdatesStillExecuteBlocks {
     [self.collectionView removeFromSuperview];
 
     IGSectionObject *object = [IGSectionObject sectionWithObjects:@[@0, @1, @2]];
@@ -579,4 +580,21 @@
     [self waitForExpectationsWithTimeout:30 handler:nil];
 }
 
+- (void)test_whenObjectIdentifiersCollide_withDifferentTypes_thatLookupReturnsNil {
+    id testObject = [[IGTestObject alloc] initWithKey:@"foo" value:@"bar"];
+    id collision = @"foo";
+    XCTAssertEqual(collision, [testObject diffIdentifier]);
+
+    IGListAdapterUpdater *updater = [IGListAdapterUpdater new];
+
+    // mimic internal map setup in IGListAdapter
+    NSPointerFunctions *keyFunctions = [updater objectLookupPointerFunctions];
+    NSPointerFunctions *valueFunctions = [NSPointerFunctions pointerFunctionsWithOptions:NSPointerFunctionsStrongMemory];
+    NSMapTable *table = [[NSMapTable alloc] initWithKeyPointerFunctions:keyFunctions valuePointerFunctions:valueFunctions capacity:0];
+
+    [table setObject:@1 forKey:testObject];
+    XCTAssertNotNil([table objectForKey:testObject]);
+    XCTAssertNil([table objectForKey:collision]);
+}
+
 @end

Tests/IGListBindingSectionControllerTests.m

@@ -109,7 +109,7 @@
     [self setupWithObjects:@[
                              [[IGTestDiffingObject alloc] initWithKey:@1 objects:@[@7, @"seven"]],
                              ]];
-    [self.adapter reloadObjects:@[[[IGTestDiffingObject alloc] initWithKey:@1 objects:@[@"four", @4, @"seven", @7, @"seven", @10]]]];
+    [self.adapter reloadObjects:@[[[IGTestDiffingObject alloc] initWithKey:@1 objects:@[@"four", @4, @"seven", @7, @10]]]];
 
     IGTestNumberBindableCell *cell00 = [self cellAtSection:0 item:0];
     IGTestStringBindableCell *cell01 = [self cellAtSection:0 item:1];

Tests/IGListDebuggerTests.m

@@ -24,6 +24,9 @@
 @implementation IGListDebuggerTests
 
 - (void)test_whenSearchingAdapterInstances_thatCorrectCountReturned {
+    // purge any leftover tracking
+    [IGListDebugger clear];
+
     UIViewController *controller = [UIViewController new];
     UICollectionView *collectionView = [[UICollectionView alloc] initWithFrame:CGRectZero collectionViewLayout:[UICollectionViewFlowLayout new]];
     IGListAdapterUpdater *updater = [IGListAdapterUpdater new];