Tighten NSFileHandle retained NSError annotations

[danjboyd]

Summary

• Annotate the three NSFileHandle URL factory NSError out-parameters as nullable storage.
• Keep the change scoped to the retained URL factory family.

Validation

• scripts/run-phase15i-nsfilehandle-import-shape-audit.sh
• scripts/run-phase15i-nsfilehandle-nserror-runtime-gate.sh
• scripts/run-phase15i-nsfilehandle-promotion-validation.sh
• PATH=/usr/GNUstep/System/Tools:$PATH make check testobj=NSFileHandle/general.m

[fredkiefer]

Hi @danjboyd
you opened three PR that seem to build on top of each other. It would be easier to review if you stated this in the comments or even better waited for the first to get merged and then provided the second and so on.

[danjboyd]

Thanks, agreed. These three PRs are stacked and intended for review/merge in order:\n\n1. #605 Phase 15I NSFileHandle NSError annotation slice\n2. #606 Phase 15J invalid keyed-unarchive runtime repair\n3. #607 Phase 15J NSKeyedArchiver/NSKeyedUnarchiver NSError annotation slice\n\nI opened them together to keep the current GNUstep Swift validation stack visible, but #606 and #607 can wait until #605 lands if that is easier for review.

[rfm]

I did a long review for this a few days ago, but it seems that it somehow got lost (perhaps I failed to notice an error response when I submitted it).

In short, the basic changes look good, but don't compile on even the limited set of test systems we use on github. It's a key requirement for GNUstep as a framework that it is portable code which makes it easier for developers to write portable code.

In this case it looks like the issue is compiler compatibility, and I think it's because the compatibility macros for nullability are in NSObjCRuntime.h (I don't know why, it probably seemed a good idea at the time). A quick fix would probably be to ensure that all the files which fail to compile import that header. A better fix might be to move those compatibility macros to a more commonly imported header file. Given that GSConfig.h is supposed to encapsulate portability issues found at configure time, perhaps we should put these (and other) portability macros dealing with compiler compatibility in GSConfig.h.in?

[DTW-Thalion]

As I have been playing with portability between clang and GCC lately, this might help:

The two GCC jobs (Ubuntu x64 GCC, Windows x64 MinGW GCC) fail to compile while the clang jobs pass:

NSData.h:109:4: error: unknown type name 'nonnull'
NSData.h:109:11: error: expected ')' before 'id'

_Nullable, _Nonnull and NS_ASSUME_NONNULL_BEGIN are guarded on __has_feature(nullability) in NSObjCRuntime.h and expand to nothing under gcc, so those annotations build fine. The bare nonnull keyword has no such fallback and gcc's Objective-C front end does not recognise it, so it reads nonnull as a type name. GSObjCRuntime.m includes NSData.h first, so the Additions subproject aborts there.

Three declarations use the bare form:

NSData.h  + (nonnull instancetype) dataWithBytes: ...
NSData.h  - (nonnull instancetype) initWithBytes: ...
NSURL.h   - (nonnull instancetype) init;

NS_ASSUME_NONNULL_BEGIN is already in effect at those points, so an unannotated instancetype return is implicitly nonnull. Dropping the bare nonnull (or spelling it _Nonnull) keeps the annotation on clang and builds on gcc.