djinni icon indicating copy to clipboard operation
djinni copied to clipboard

ObjC: List with nullable elements - crash

Open domasn opened this issue 8 years ago • 2 comments

First, thanks a lot for working on such a great tool! 🎉👏 My team uses it extensively and we have found an interesting case that causes a 100% crash.

Given a .djinni file:

test = interface +c {
    get_list_of_optionals(): list<optional<string>>;
}

A .h file is generated:

@interface Test : NSObject
- (nonnull NSArray<NSString *> *)getListOfOptionals;
@end

And a private .mm file is generated:

- (nonnull NSArray<NSString *> *) getListOfOptionals {
    try {
        auto r = _cppRefHandle.get()->get_list_of_optionals();
        return ::djinni::List<::djinni::Optional<std::experimental::optional, ::djinni::String>>::fromCpp(r);
    } DJINNI_TRANSLATE_EXCEPTIONS()
}

Trying to call getListOfOptionals containing a nullopt instance causes a crash in line:

return ::djinni::List<::djinni::Optional<std::experimental::optional, ::djinni::String>>::fromCpp(r);

Log:

*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[__NSArrayM insertObject:atIndex:]: object cannot be nil'

The problem comes from ObjC not supporting optional elements inside arrays.

Of course there are workarounds (using a container record that holds optional typed value), but it would be nice to look for a clean approach.

domasn avatar Jan 19 '17 10:01 domasn

Huh, I'm amazed that nobody's ever tested this or run into this before. As a non-ObjC-native I'm amazed that an array which can't hold nil is considered reasonable. Same is true for NSDictionary keys and values, I gather.

Seems like the "supported" way to do it would be to use NSNull objects in the array, but that seems to require much more specialty knowledge from the person using the array, to look for those objects by type rather than just testing the pointer as you'd use with other types of argument, so it's not an obvious solution. I wonder whether ObjC-natives in the community would find that reasonable to use, or be surprised when their code breaks.

I do feel like Djinni should at least report an error in this use case rather than generating code which can't be used.

artwyman avatar Jan 20 '17 05:01 artwyman

I've got some experimental code which will signal those errors (in resolver's buildMExpr() where we also disallow optional<optional<...>>. A drawback is that the resolver code isn't specific to a particular generator, so if I signal an error there, users of Djinni for languages other than ObjC wouldn't be able to use optional list/map values either, even though their languages support them. It would take some more restructuring to make the resolver know about the set of active generators, and/or implement a per-generator resolve pass. The whole concept of features disallowed in specific languages is a bit contrary to Djinni's original design, since the IDL was chosen to support a lowest-common-denominator across all languages.

I'm not going to prioritize this for the moment, since it seems like the value of just the error message isn't enough to justify the effort (though I'd accept a PR to that effect). A way to really support list<optional<...>> in ObjC would require a larger feature change.

artwyman avatar Jan 20 '17 05:01 artwyman