Override nullability annotations in local NDK headers

[marcprux]

Over the past few weeks I've been submitting PRs to various projects to get them working with Android. Aside from adding import Android, by far the most frequent changes that need to be made are around the nullability annotations added in NDK 26 (see swiftlang/swift-corelibs-foundation#5024 and discussions at swiftlang/swift-corelibs-foundation#5010 and swiftlang/swift-corelibs-foundation#4850).

These annotations have broken previously-ported packages like weichsel/ZIPFoundation#250, and make it cumbersome to get things working again.

For example, something like this:

    _ = data.withUnsafeBytes { (buffer: UnsafeRawBufferPointer) in
        fwrite(buffer.baseAddress, buffer.count, 1, file)
    }

results in a build error:

main.swift:44:23: error: value of optional type 'UnsafeRawPointer?' must be unwrapped to a value of type 'UnsafeRawPointer'
42 | 
43 |     _ = data.withUnsafeBytes { (buffer: UnsafeRawBufferPointer) in
44 |         fwrite(buffer.baseAddress, buffer.count, 1, file)
   |                       |- error: value of optional type 'UnsafeRawPointer?' must be unwrapped to a value of type 'UnsafeRawPointer'
   |                       |- note: coalesce using '??' to provide a default when the optional value contains 'nil'
   |                       `- note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
45 |     }
46 | }

and needs to either be force-unwrapped or changed to something like this:

    _ = data.withUnsafeBytes { (buffer: UnsafeRawBufferPointer) in
        #if os(Android)
        fwrite(buffer.baseAddress!, buffer.count, 1, file)
        #else
        fwrite(buffer.baseAddress, buffer.count, 1, file)
        #endif
    }

Some of these nullability annotations are incorrect (android/ndk#2098), and if they are fixed in the future ("If bionic's wrong about the annotation we'll get them fixed for r28 though"), they will break all the client Swift packages again when the next Swift Android SDK ships with an updated NDK 28.

One compatibility-maximizing solutions might be that, since we started shipping the NDK headers along with the Swift 6 Android SDK, we could in theory get rid of the (arguably incorrect) _Nonnull annotations for some of these functions. For example, if we patch android-27c-sysroot/usr/include/stdio.h to change:

size_t fwrite(const void* _Nonnull __buf, size_t __size, size_t __count, FILE* _Nonnull __fp);

to remove the _Nonnull __buf :

size_t fwrite(const void* __buf, size_t __size, size_t __count, FILE* _Nonnull __fp);

Then we could go back to the simple (and Darwin/Glibc/Windows-compatible) form that was possible before NDK 26:

    _ = data.withUnsafeBytes { (buffer: UnsafeRawBufferPointer) in
        fwrite(buffer.baseAddress, buffer.count, 1, file)
    }

[finagolfin]

I see this differently. While one can certainly make the case that these null errors by devs are rare, as Elliot Hughes does in the linked NDK issue, I still think there is value in smoking those out through updating the source to check for null values.

In your example, I usually update those situations to the following instead:

_ = data.withUnsafeBytes { (buffer: UnsafeRawBufferPointer) in
        fwrite(buffer.baseAddress!, buffer.count, 1, file)
    }

That will work for all platforms and fail out in the Swift caller if data is somehow nil or unaddressable, rather than hoping the C callee catches it for us. While other platforms may not require that force unwrap, they will still work fine with it.

I'll note that your linked NDK issue was closed with no changes to the annotations, though you may disagree with that outcome. There have certainly been fixes needed, so I'm not disagreeing with you that mistakes were made, but I don't think the churn was that large.

If you're building all these Swift packages against your Skip SDK bundle, feel free to experiment with these changes there if you like. I'm simply loath to make such changes here and try to keep this NDK sysroot pristine from upstream. It bothers me that I had to change that single libc++ header to get C++ interoperability to work and plan to look into it for an upstream fix instead.

[marcprux]

Understood. TBH, I wasn't confident that it was a good idea in the first place, but I wanted to get the possibility on the record.