Clean up nullability annotations and ensure full NS_ASSUME_NONNULL coverage#2190
Open
lawrence-forooghian wants to merge 2 commits intomainfrom
Open
Clean up nullability annotations and ensure full NS_ASSUME_NONNULL coverage#2190lawrence-forooghian wants to merge 2 commits intomainfrom
NS_ASSUME_NONNULL coverage#2190lawrence-forooghian wants to merge 2 commits intomainfrom
Conversation
lawrence-forooghian
changed the title
NS_ASSUME_NONNULL coverage
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis PR systematically applies Objective-C nullability annotations (NS_ASSUME_NONNULL_BEGIN/END) across the codebase, updates method signatures to use consistent nullability syntax, removes explicit nonnull qualifiers where macro scopes cover them, and introduces a new public API for ARTCancellableFromCallback with corresponding factory methods in ARTStringifiable. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/2190/features
github-actions
bot
temporarily deployed
to
staging/pull/2190/jazzydoc
These files already have NS_ASSUME_NONNULL_BEGIN/END regions, so explicit nonnull, __nonnull, and _Nonnull annotations on declarations within those regions are redundant and can be removed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds NS_ASSUME_NONNULL_BEGIN/END to every header and .m @interface declaration that was missing it, improving Swift bridging (nonnull types bridge as non-optional) and making the nullability contract explicit for human readers. Some of the wrapped declarations don't currently contain pointer types, but are included for consistency and so that future additions to those interfaces will have coverage by default. Where adding coverage made existing explicit nonnull annotations redundant, those are removed. Where a return type or parameter is genuinely nullable (verified by reading the implementation), an explicit nullable annotation is added — notably on the dictionary lookup helpers in ARTNSDictionary+ARTDictionaryUtil.h and parseId in ARTPresenceMessage+Private.h. In ARTTypes.h, NS_ASSUME_NONNULL_BEGIN is moved earlier to cover the *ToStr() C functions that were previously annotated with inline _Nonnull. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lawrence-forooghian
force-pushed
the
2026-02-18-nullability-cleanup
branch
from
63e973f to
dd70742
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Source/ARTCrypto.m (1)
8-22:⚠️ Potential issue | 🟡 MinorAdd explicit
_Nullableannotation to theerrorparameter ofccAlgorithm:error:declaration.The private method declaration at line 12 resides within
NS_ASSUME_NONNULL, makingerror:(NSError **)errorimplicitlyerror:(NSError * _Nullable * _Nonnull)error. However, line 127 passesnildirectly:if (![cipherParams ccAlgorithm:&_algorithm error:nil]) {The implementation at line 95 defensively guards with
if (error)before dereferencing, confirming thatnilwas intentionally supported. Make the intent explicit by annotating the outer pointer as nullable:- (BOOL)ccAlgorithm:(CCAlgorithm *)algorithm error:(NSError * _Nullable * _Nullable)error;This clarifies the parameter contract and prevents potential warnings in stricter compiler configurations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/ARTCrypto.m` around lines 8 - 22, Update the private method declaration for ARTCipherParams's ccAlgorithm:error: to explicitly allow a nil error pointer by annotating the outer pointer nullable; change the signature inside the NS_ASSUME_NONNULL block to use NSError * _Nullable * _Nullable for the error parameter (i.e. the ccAlgorithm:error: declaration), so callers like the code in ARTCbcCipher that pass nil remain valid and stricter compilers won't warn; leave the implementation and its defensive if (error) guard unchanged.Source/ARTRealtimeChannel.m (1)
272-286:⚠️ Potential issue | 🟡 Minor
_pushChannelneeds_Nullable— it's lazily initialized, so it starts asnilThe
_pushChannelivar (line 278) is now insideNS_ASSUME_NONNULL_BEGINwithout an explicit_Nullable, making it implicitly nonnull. However, thepushgetter (lines 415–420) shows it is lazily initialized — the ivar starts asnilafterinitand is populated only on first access. This contradicts the nonnull annotation and can mislead the compiler and code readers about the ivar's lifecycle.🛠️ Proposed fix
`#if` TARGET_OS_IPHONE -ARTPushChannelInternal *_pushChannel; +ARTPushChannelInternal * _Nullable _pushChannel; `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/ARTRealtimeChannel.m` around lines 272 - 286, The _pushChannel ivar is declared inside NS_ASSUME_NONNULL_BEGIN but is lazily initialized in the push getter, so annotate it as nullable: change the declaration of ARTPushChannelInternal *_pushChannel; within the ARTRealtimeChannelInternal interface to ARTPushChannelInternal * _Nullable _pushChannel; (also check/use the same _Nullable annotation for any related ivar declarations if they are lazily set) so the compiler and readers know it may be nil until first access.
🧹 Nitpick comments (4)
Source/include/Ably/ARTChannels.h (1)
52-52: Block internals are not covered byNS_ASSUME_NONNULL.
NS_ASSUME_NONNULLapplies to themapperblock pointer itself (making it nonnull) but does not propagate into the block's own type signature. The block'sidreturn type andChannelTypeparameter remain unaudited. Since this is a:nodoc:internal method, the practical Swift bridging impact is zero, but for complete nullability coverage the block type could be explicitly annotated:✏️ Explicit nullability inside block signature
-/// :nodoc: -- (id<NSFastEnumeration>)copyIntoIteratorWithMapper:(id (^)(ChannelType))mapper; +/// :nodoc: +- (id<NSFastEnumeration>)copyIntoIteratorWithMapper:(id _Nonnull (^)(ChannelType _Nonnull))mapper;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/include/Ably/ARTChannels.h` at line 52, The block pointer in copyIntoIteratorWithMapper: is not fully nullability-annotated because NS_ASSUME_NONNULL does not apply inside the block signature; update the declaration of copyIntoIteratorWithMapper: to explicitly annotate the block's return and parameter nullability (e.g., make the ChannelType parameter _Nonnull and the block return either _Nullable or _Nonnull as appropriate for your API) so the block type is fully audited for nullability when bridged to Swift; refer to the method name copyIntoIteratorWithMapper: and the ChannelType type when making the change.Source/include/Ably/ARTStringifiable.h (1)
13-15: Preferinstancetypefor factory method return types.Using
instancetypeis the idiomatic Objective-C convention for class factory methods; it enables correct subclass return-type inference and aligns with Apple's SDK guidelines.♻️ Proposed refactor
-+ (ARTStringifiable*)withString:(NSString *)value; -+ (ARTStringifiable*)withNumber:(NSNumber *)value; -+ (ARTStringifiable*)withBool:(BOOL)value; ++ (instancetype)withString:(NSString *)value; ++ (instancetype)withNumber:(NSNumber *)value; ++ (instancetype)withBool:(BOOL)value;— The AI summary states these factory methods were added in this PR, but none of lines 13–15 are annotated with
~in the diff, indicating they are pre-existing declarations. Please verify whether these methods are genuinely new to this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/include/Ably/ARTStringifiable.h` around lines 13 - 15, Change the factory method return types from the concrete class pointer to instancetype so subclassing callers get correct types: update the declarations of withString:, withNumber:, and withBool: in ARTStringifiable (replace ARTStringifiable* return type with instancetype for the methods named withString:, withNumber:, and withBool:). Ensure any corresponding implementations and headers remain consistent with the instancetype return type.Source/ARTChannels.m (1)
9-18:__weakivar implicitly becomes_NonnullwithinNS_ASSUME_NONNULL— semantically inconsistent.
_delegateis a__weakreference, meaning it can be zeroed tonilat any point after the referenced object is deallocated. Marking it_Nonnull(implicitly viaNS_ASSUME_NONNULL) contradicts this: callers assume the reference is valid and won't nil-check before use. In practice this is safe in pure ObjC (messagingnilis a no-op), but it makes the annotation misleading.Consider explicitly annotating it as
_Nullableto convey the true contract:🔧 Suggested annotation
- __weak id<ARTChannelsDelegate> _delegate; // weak because delegates outlive their counterpart + __weak id<ARTChannelsDelegate> _Nullable _delegate; // weak because delegates outlive their counterpart🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/ARTChannels.m` around lines 9 - 18, The ivar _delegate in ARTChannels is declared __weak inside an NS_ASSUME_NONNULL region which incorrectly implies non-nullability; change the declaration of _delegate in the ARTChannels `@interface` to explicitly mark it nullable (e.g. __weak _Nullable id<ARTChannelsDelegate> _delegate) so the nullability contract matches the weak semantics and callers are aware it can be nil.Source/include/Ably/ARTTypes.h (1)
585-585: Nit:_Nonnullqualifiers onwrapperparameter are redundant insideNS_ASSUME_NONNULL.Both
_Nonnullannotations on thewrapperparameter are already implied by the surroundingNS_ASSUME_NONNULLregion. Since the PR's goal is to remove such redundancies, these could be dropped for consistency.Suggested change
-NSObject<ARTCancellable> * artCancellableFromCallback(ARTResultCallback callback, _Nonnull ARTResultCallback * _Nonnull wrapper); +NSObject<ARTCancellable> * artCancellableFromCallback(ARTResultCallback callback, ARTResultCallback * wrapper);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/include/Ably/ARTTypes.h` at line 585, Remove the redundant _Nonnull qualifiers on the wrapper parameter in the artCancellableFromCallback declaration: the surrounding NS_ASSUME_NONNULL already enforces non-null, so update the function signature by deleting the extra _Nonnull annotations for the parameter named wrapper in artCancellableFromCallback(ARTResultCallback callback, ARTResultCallback * wrapper).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Source/ARTCrypto.m`:
- Around line 8-22: Update the private method declaration for ARTCipherParams's
ccAlgorithm:error: to explicitly allow a nil error pointer by annotating the
outer pointer nullable; change the signature inside the NS_ASSUME_NONNULL block
to use NSError * _Nullable * _Nullable for the error parameter (i.e. the
ccAlgorithm:error: declaration), so callers like the code in ARTCbcCipher that
pass nil remain valid and stricter compilers won't warn; leave the
implementation and its defensive if (error) guard unchanged.
In `@Source/ARTRealtimeChannel.m`:
- Around line 272-286: The _pushChannel ivar is declared inside
NS_ASSUME_NONNULL_BEGIN but is lazily initialized in the push getter, so
annotate it as nullable: change the declaration of ARTPushChannelInternal
*_pushChannel; within the ARTRealtimeChannelInternal interface to
ARTPushChannelInternal * _Nullable _pushChannel; (also check/use the same
_Nullable annotation for any related ivar declarations if they are lazily set)
so the compiler and readers know it may be nil until first access.
---
Nitpick comments:
In `@Source/ARTChannels.m`:
- Around line 9-18: The ivar _delegate in ARTChannels is declared __weak inside
an NS_ASSUME_NONNULL region which incorrectly implies non-nullability; change
the declaration of _delegate in the ARTChannels `@interface` to explicitly mark it
nullable (e.g. __weak _Nullable id<ARTChannelsDelegate> _delegate) so the
nullability contract matches the weak semantics and callers are aware it can be
nil.
In `@Source/include/Ably/ARTChannels.h`:
- Line 52: The block pointer in copyIntoIteratorWithMapper: is not fully
nullability-annotated because NS_ASSUME_NONNULL does not apply inside the block
signature; update the declaration of copyIntoIteratorWithMapper: to explicitly
annotate the block's return and parameter nullability (e.g., make the
ChannelType parameter _Nonnull and the block return either _Nullable or _Nonnull
as appropriate for your API) so the block type is fully audited for nullability
when bridged to Swift; refer to the method name copyIntoIteratorWithMapper: and
the ChannelType type when making the change.
In `@Source/include/Ably/ARTStringifiable.h`:
- Around line 13-15: Change the factory method return types from the concrete
class pointer to instancetype so subclassing callers get correct types: update
the declarations of withString:, withNumber:, and withBool: in ARTStringifiable
(replace ARTStringifiable* return type with instancetype for the methods named
withString:, withNumber:, and withBool:). Ensure any corresponding
implementations and headers remain consistent with the instancetype return type.
In `@Source/include/Ably/ARTTypes.h`:
- Line 585: Remove the redundant _Nonnull qualifiers on the wrapper parameter in
the artCancellableFromCallback declaration: the surrounding NS_ASSUME_NONNULL
already enforces non-null, so update the function signature by deleting the
extra _Nonnull annotations for the parameter named wrapper in
artCancellableFromCallback(ARTResultCallback callback, ARTResultCallback *
wrapper).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nonnull/__nonnull/_Nonnullannotations from declarations already insideNS_ASSUME_NONNULLregions.NS_ASSUME_NONNULL_BEGIN/ENDto all public headers, private headers, and.m@interfacedeclarations that were missing it (excluding vendored SocketRocket files).nullableannotation.This improves Swift bridging (nonnull types bridge as non-optional) and makes the nullability contract explicit for human readers.
Test plan
swift buildpasses with no errors or warningsswift build --build-testscompiles all test targets🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Bug Fixes
New Features