Investigate whether we can get the compiler to do more nullability checking

Background

An earlier version of #1187 had some code like this:

NSString *deviceId = [storage objectForKey:ARTDeviceIdKey];
NSString *deviceSecret = [storage secretForDevice:deviceId];

This caused a crash at runtime when deviceId was nil. We wondered why the compiler didn't catch this, since on the face of it the two methods involved have nullability annotations that should have prevented this:

NS_ASSUME_NONNULL_BEGIN

@protocol ARTDeviceStorage <NSObject>
- (nullable id)objectForKey:(NSString *)key;
- (nullable NSString *)secretForDevice:(ARTDeviceId *)deviceId;
@end 

NS_ASSUME_NONNULL_END

We'd like to know whether the compiler can do more work for us, and catch this sort of thing in the future.

Some investigation into nullability in Objective-C

A first reading around nullability in Obj-C left me with the impression that the nullability attributes were added primarily for the benefit of Swift, to provide richer interfaces for imported Obj-C code. But a bit of experimentation shows that there is at least some checking in the Obj-C compiler, too.

For example, passing a nil literal to a nonnull argument results in a compiler warning: NSString *deviceSecret = [storage secretForDevice:nil] gives Null passed to a callee that requires a non-null argument.

But this checking seems to go out of the window as soon as you introduce an intermediate variable:

NSString *deviceId = nil;
NSString *deviceSecret = [storage secretForDevice:deviceId]; // this doesn’t generate a warning

There are then two situations in which, with our current config, the compiler will not emit a warning, but in which it will if we turn on the -Wnullable-to-nonnull-conversion warning (found on some random StackOverflow posts, seemingly not configurable via the Xcode UI):

If you pass the return value from one nullable method directly into a nonnull argument, you'll get a warning:

NSString *deviceSecret = [storage secretForDevice:[storage objectForKey:ARTDeviceIdKey]]; // this gives "Implicit conversion from nullable pointer 'id _Nullable' to non-nullable pointer type 'ARTDeviceId * _Nonnull' (aka 'NSString *')"

If you explicitly add a nullability annotation to the intermediate variable, then the compiler will check that you've added the correct notation and it'll prevent you from passing as an argument when you shouldn't:

NSString *_Nonnull deviceId = [storage objectForKey:ARTDeviceIdKey]; // Implicit conversion from nullable pointer 'id _Nullable' to non-nullable pointer type 'NSString * _Nonnull'
NSString *deviceSecret = [storage secretForDevice:deviceId];

NSString *_Nullable deviceId = [storage objectForKey:ARTDeviceIdKey];
NSString *deviceSecret = [storage secretForDevice:deviceId]; // "Implicit conversion from nullable pointer 'NSString * _Nullable' to non-nullable pointer type 'ARTDeviceId * _Nonnull' (aka 'NSString *')"

But, the problem with this is that firstly it expects us to remember to explicitly add nullability annotations to all of our variables, and secondly I couldn't figure out how to "break out" of nullable land – that is, given a nullable variable, how do we convince the compiler that it is in fact not nil, and should hence be permitted to be passed to a nonnull argument? You'd expect this to be achievable via some sort of guard statement, e.g. an if that prevents the execution from proceeding if the value is nil, but my attempts at this didn't seem to convince the compiler. This means that turning on this warning flag would lead to a bunch of false positives in our codebase.

It's also worth mentioning that there is an "Enable Nullability Annotation Checks" (CLANG_UNDEFINED_BEHAVIOR_SANITIZER_NULLABILITY) setting that can be turned on in Xcode, although I couldn't see any difference that turning this on made.

Next steps

Continue investigating, I suppose. I don't have a good idea yet for how to progress. Might be worth diving deeper into the compiler docs, starting with the documentation on Clang’s nullability attributes. Can we change the default nullability for variables, which seem to currently behave as if they're all _Null_unspecified? I do wonder whether there's something simple that I'm missing here.

┆Issue is synchronized with this Jira Task by Unito

ably / ably-cocoa