From a6526ce097fe38de85459cd6a34d948ec8440db7 Mon Sep 17 00:00:00 2001 From: Nate Stedman Date: Thu, 10 Sep 2020 12:28:54 -0700 Subject: [PATCH] Expose ListGenericSectionController.object and ListSectionController.collectionContext as implicitly-unwrapped optionals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: These methods are currently correctly annotated as `nullable`, because they can return `nil`. However, this is not practical for writing actual Swift code that uses the APIs. Overrides of `cellForItem(at:)` //must// return a `UICollectionViewCell`. Without a collection context, there's no good way to get one, so callsites must either: - Force-unwrap the `collectionContext`. - `guard let`/`else fatalError` the `collectionContext`. - Return a default `UICollectionViewCell`, which just hides the bug. None of these are good: we don't want to encourage things force-unwraps and `fatalError` in idiomatic product code, because people will see them and get the idea that these patterns are okay to use elsewhere, and they are not. This also happens with `IGListGenericSectionController`'s `object` value: to use it when creating a cell, it must be explicitly unwrapped. We could make these `nonnull`, since there's no enforcement of the annotations in Objective-C, that feels a bit too far, and it would break existing code that uses the optional API, because you can't force-unwrap (etc.) a non-optional value. Instead, we can use `null_resettable` explicitly. [While it's not covered by name in the Apple docs](https://developer.apple.com/documentation/swift/objective-c_and_c_code_customization/designating_nullability_in_objective-c_apis): > Without a nullability annotation or with a null_resettable annotation—Imported as implicitly unwrapped optionals ...it bridges the property to Swift as an implicitly-unwrapped optional. I suppose it's implied as the equivalent of "without a nullability annotation". This works even for `readonly` properties that can't be reset, and it solves both issues: it feels like less of a "100% `nonnull`" guarantee, and it's backwards-compatible with code using optionals. To demonstrate that this is backwards-compatible with existing Swift code, this test code, which runs through various optional/non-optional APIs, compiles: ``` final class TestSectionController: ListSectionController { override func cellForItem(at index: Int) -> UICollectionViewCell { let cell = collectionContext.dequeueReusableCell(for: self, at: index) let optional = collectionContext?.dequeueReusableCell(for: self, at: index) if let _ = collectionContext {} return optional ?? cell } } final class TestGenericSectionController: ListGenericSectionController { override func cellForItem(at index: Int) -> UICollectionViewCell { let label = UILabel() // imagine it's from a cell label.text = object as String label.text = (object as String) + "Suffix" // wouldn't work with optional label.text = object.map { $0 as String } if let object = self.object { label.text = object as String } return collectionContext.dequeueReusableCell(for: self, at: index) } } ``` Reviewed By: patters, lorixx Differential Revision: D23603716 fbshipit-source-id: e4750dcfe0072d482951dbf2e9efb1ee3de46884 --- CHANGELOG.md | 4 ++++ Source/IGListKit/IGListGenericSectionController.h | 5 +++-- Source/IGListKit/IGListSectionController.h | 7 ++++++- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6f143abd..a23b27f35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,10 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag - Removed unneeded diffing functions `IGListDiffExperiment(...)` and `IGListDiffPathsExperiment(...)`. [Maxime Ollivier](https://github.com/maxolls) (tbd) +- `ListSectionController.collectionContext` and `ListGenericSectionController.object` are now implicitly-unwrapped optionals in Swift. [Nate Stedman](https://github.com/natestedman) (tbd) + +- The argument of `IGListGenericSectionController`'s `-didUpdateToObject:` is now generic, not `id`. [Nate Stedman](https://github.com/natestedman) (tbd) + ### Enhancements - Introduce `IGListSwiftKit`, with Swift refinements for `dequeueReusableCellOfClass` methods. [Koen Punt](https://github.com/koenpunt) [(#1388)](https://github.com/Instagram/IGListKit/pull/1388). diff --git a/Source/IGListKit/IGListGenericSectionController.h b/Source/IGListKit/IGListGenericSectionController.h index e0cf25e13..25f84e3b7 100644 --- a/Source/IGListKit/IGListGenericSectionController.h +++ b/Source/IGListKit/IGListGenericSectionController.h @@ -22,9 +22,10 @@ NS_SWIFT_NAME(ListGenericSectionController) returned. @note This object is briefly `nil` between initialization and the first call to `didUpdateToObject:`. After that, it is - safe to assume that this is non-`nil`. + safe to assume that this is non-`nil`. For this reason, we bridge it to Swift as an implicitly-unwrapped Optional, so + that idiomatic IGListKit code is not forced to handle nullability with explicit `as!` or `fatalError`. */ -@property (nonatomic, strong, nullable, readonly) ObjectType object; +@property (nonatomic, strong, null_unspecified, readonly) ObjectType object; /** Updates the section controller to a new object. diff --git a/Source/IGListKit/IGListSectionController.h b/Source/IGListKit/IGListSectionController.h index 68a455f4b..446346ad4 100644 --- a/Source/IGListKit/IGListSectionController.h +++ b/Source/IGListKit/IGListSectionController.h @@ -147,8 +147,13 @@ NS_SWIFT_NAME(ListSectionController) A context object for interacting with the collection. Use this property for accessing the collection size, dequeuing cells, reloading, inserting, deleting, etc. + + @note When created outside of `-listAdapter:sectionControllerForObject:`, this object is temporarily `nil` + after initialization. We bridge it to Swift as an implicitly-unwrapped Optional, so that idiomatic IGListKit + code is not forced to handle nullability with explicit `as!` or `fatalError`, as using a non-`nil` instance + of this object is essential for dequeueing cells. */ -@property (nonatomic, weak, nullable, readonly) id collectionContext; +@property (nonatomic, weak, null_unspecified, readonly) id collectionContext; /** Returns the section within the list for this section controller.