Snapchat / djinni

A tool for generating cross-language type declarations and interface bindings. Djinni's new home is in the Snapchat org.
Apache License 2.0
166 stars 44 forks source link

Value of Objective C Future/Promise is unexpectedly nillable #169

Closed jb-gcx closed 1 week ago

jb-gcx commented 1 month ago

What is the problem

My understanding of the concept of djinnis Future/Promise implementation is that a future will always result in either a value of the given type or an exception. The get method waits for a result to be present and then either returns the value or throws the exception. So by the time get returns, a value must be present.

This is already the case for the Cpp implementation of Future/Promise. If I want to have a nullable result, I must use Future<std::optional<int>>.

The ObjC implementation allows calls to setValue with a nullable value, regardless of the value type. Therefore get also needs to return a nullable value. I would love to change this, because it doesn't match the implementation in other platforms and goes against my understanding of the concept. It also makes usage from Swift harder/weirder, because we always need to unwrap a value that is expected to be always present.

Possible solution

I would most like to simply change DJFuture::get and DJPromise::setValue to return and accept nonnull values.

However, I'm guessing the value was originally made nullable for a reason. Namely: what if the user wants the future to return something nullable? In C++ this is easy to achieve with Future<optional<...>>. I lack ObjC knowledge here, but I'm guessing there is no way to dictate nullability via template arg in ObjC, which is why the value was made nullable by default. Is this true? Is there no other way?

Other observations

I've created this PR in my fork to showcase these issues, see the breaking tests in the CI. For reference, I'll copy the test code and CI output here as well:

Test code

- (void)testFutureObjCNullable {
    DJPromise<NSNumber *> *p1 = [[DJPromise alloc] init];
    DJFuture<NSNumber *> *f1 = [p1 getFuture];
    [p1 setValue:nil];

    DJPromise<NSNumber *> *p2 = [[DJPromise alloc] init];
    DJFuture<NSNumber *> *f2 = [p2 getFuture];
    [p2 setValue];

    // setValue:nil doesn't set the same value as setValue without parameters.
    // Is this expected? Is it good practice to use [NSNull null], like setValue does?
    // It seems problematic to me, but I lack the ObjC knowledge.
    XCTAssertEqual([f1 get], [f2 get]);
}

- (void)testFutureRoundtripWithNil {
    // This shows a crash that occurs when passing nil into a promise that, on cpp side, doesn't expect nil values
    DJPromise<NSNumber *> *p = [[DJPromise alloc] init];
    DJFuture<NSNumber *> *f = [p getFuture];
    DJFuture<NSString *> *f2 = [DBTestHelpers futureRoundtrip:f];
    [p setValue:nil];
}

- (void)testFutureRoundtripWithNSNull {
    // This shows a crash that occurs when passing [NSNull null] into a promise that, on cpp side, doesn't expect nil values
    DJPromise<NSNumber *> *p = [[DJPromise alloc] init];
    DJFuture<NSNumber *> *f = [p getFuture];
    DJFuture<NSString *> *f2 = [DBTestHelpers futureRoundtrip:f];
    [p setValue];
}

CI output

Test Case '-[DBAsyncTests testFutureObjCNullable]' started.
/private/tmp/test-suite/handwritten-src/objc/tests/DBAsyncTest.m:66: error: -[DBAsyncTests testFutureObjCNullable] : (([f1 get]) equal to ([f2 get])) failed: ("{length = 8, bytes = 0x0000000000000000}") is not equal to ("{length = 8, bytes = 0x008c990002000000}")
Test Case '-[DBAsyncTests testFutureObjCNullable]' failed (0.726 seconds).

...

Test Case '-[DBAsyncTests testFutureRoundtripWithNil]' started.
Assertion failed: (x), function toCpp, file DJIMarshal+Private.h, line 47.

Restarting after unexpected exit, crash, or test timeout in -[DBAsyncTests testFutureRoundtripWithNil]; summary will include totals from previous launches.

...

Test Case '-[DBAsyncTests testFutureRoundtripWithNSNull]' started.
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[NSNull intValue]: unrecognized selector sent to instance 0x200998c00'
*** First throw call stack:
(
    0   CoreFoundation                      0x000000019401accc __exceptionPreprocess + 176
    1   libobjc.A.dylib                     0x0000000193b02788 objc_exception_throw + 60
    2   CoreFoundation                      0x00000001940cd02c -[NSObject(NSObject) __retain_OA] + 0
    3   CoreFoundation                      0x0000000193f84cdc ___forwarding___ + 1580
    4   CoreFoundation                      0x0000000193f845f0 _CF_forwarding_prep_0 + 96
    5   djinni-objc-tests                   0x0000000102899334 _ZN6djinni3I325unboxEP8NSNumber + 40
    6   djinni-objc-tests                   0x00000001028992e4 _ZN6djinni9PrimitiveINS_3I32EiE5Boxed5toCppEP8NSNumber + 108
    7   djinni-objc-tests                   0x00000001028e9b74 ___ZN6djinni13FutureAdaptorINS_3I32EE5toCppEP8DJFuture_block_invoke + 104
    8   djinni-objc-tests                   0x00000001029283d0 __17-[DJFuture then:]_block_invoke + 120
    9   djinni-objc-tests                   0x0000000102928c74 -[DJPromise updateAndCallResultHandler:] + 440
    10  djinni-objc-tests                   0x0000000102928fac -[DJPromise setValue:] + 144
    11  djinni-objc-tests                   0x00000001029290ec -[DJPromise setValue] + 72
    12  djinni-objc-tests                   0x0000000102884614 -[DBAsyncTests testFutureRoundtripWithNSNull] + 136
    13  CoreFoundation                      0x0000000193f86434 __invoking___ + 148
    14  CoreFoundation                      0x0000000193f862b4 -[NSInvocation invoke] + 428
    15  XCTestCore                          0x0000000100e8a154 +[XCTFailableInvocation invokeStandardConventionInvocation:completion:] + 68
    16  XCTestCore                          0x0000000100e8a108 __90+[XCTFailableInvocation invokeInvocation:withTestMethodConvention:lastObservedErrorIssue:]_block_invoke_3 + 28
    17  XCTestCore                          0x0000000100e89860 __81+[XCTFailableInvocation invokeWithAsynchronousWait:lastObservedErrorIssue:block:]_block_invoke + 360
    18  XCTestCore                          0x0000000100e42970 +[XCTSwiftErrorObservation observeErrorsInBlock:] + 280
    19  XCTestCore                          0x0000000100e895f0 +[XCTFailableInvocation invokeWithAsynchronousWait:lastObservedErrorIssue:block:] + 228
    20  XCTestCore                          0x0000000100e89df0 +[XCTFailableInvocation invokeInvocation:withTestMethodConvention:lastObservedErrorIssue:] + 540
    21  XCTestCore                          0x0000000100e8a1f8 +[XCTFailableInvocation invokeInvocation:lastObservedErrorIssue:] + 72
    22  XCTestCore                          0x0000000100e7b3f8 __24-[XCTestCase invokeTest]_block_invoke_2 + 88
    23  XCTestCore                          0x0000000100e51278 -[XCTMemoryChecker _assertInvalidObjectsDeallocatedAfterScope:] + 84
    24  XCTestCore                          0x0000000100e7e8d4 -[XCTestCase assertInvalidObjectsDeallocatedAfterScope:] + 92
    25  XCTestCore                          0x0000000100e7b358 __24-[XCTestCase invokeTest]_block_invoke.96 + 172
    26  XCTestCore                          0x0000000100e394b8 -[XCTestCase(XCTIssueHandling) _caughtUnhandledDeveloperExceptionPermittingControlFlowInterruptions:caughtInterruptionException:whileExecutingBlock:] + 168
    27  XCTestCore                          0x0000000100e7ae80 -[XCTestCase invokeTest] + 764
    28  XCTestCore                          0x0000000100e7c908 __26-[XCTestCase performTest:]_block_invoke.149 + 36
    29  XCTestCore                          0x0000000100e394b8 -[XCTestCase(XCTIssueHandling) _caughtUnhandledDeveloperExceptionPermittingControlFlowInterruptions:caughtInterruptionException:whileExecutingBlock:] + 168
    30  XCTestCore                          0x0000000100e7c3ac __26-[XCTestCase performTest:]_block_invoke.134 + 552
    31  XCTestCore                          0x0000000100e5d844 +[XCTContext _runInChildOfContext:forTestCase:markAsReportingBase:block:] + 180
    32  XCTestCore                          0x0000000100e5d72c +[XCTContext runInContextForTestCase:markAsReportingBase:block:] + 104
    33  XCTestCore                          0x0000000100e7be04 -[XCTestCase performTest:] + 308
    34  XCTestCore                          0x0000000100e240f0 -[XCTest runTest] + 48
    35  XCTestCore                          0x0000000100e60adc -[XCTestSuite runTestBasedOnRepetitionPolicy:testRun:] + 68
    36  XCTestCore                          0x0000000100e609a4 __27-[XCTestSuite performTest:]_block_invoke + 164
    37  XCTestCore                          0x0000000100e60408 __59-[XCTestSuite _performProtectedSectionForTest:testSection:]_block_invoke + 48
    38  XCTestCore                          0x0000000100e5d844 +[XCTContext _runInChildOfContext:forTestCase:markAsReportingBase:block:] + 180
    39  XCTestCore                          0x0000000100e5d72c +[XCTContext runInContextForTestCase:markAsReportingBase:block:] + 104
    40  XCTestCore                          0x0000000100e60378 -[XCTestSuite _performProtectedSectionForTest:testSection:] + 180
    41  XCTestCore                          0x0000000100e60658 -[XCTestSuite performTest:] + 220
    42  XCTestCore                          0x0000000100e240f0 -[XCTest runTest] + 48
    43  XCTestCore                          0x0000000100e60adc -[XCTestSuite runTestBasedOnRepetitionPolicy:testRun:] + 68
    44  XCTestCore                          0x0000000100e609a4 __27-[XCTestSuite performTest:]_block_invoke + 164
    45  XCTestCore                          0x0000000100e60408 __59-[XCTestSuite _performProtectedSectionForTest:testSection:]_block_invoke + 48
    46  XCTestCore                          0x0000000100e5d844 +[XCTContext _runInChildOfContext:forTestCase:markAsReportingBase:block:] + 180
    47  XCTestCore                          0x0000000100e5d72c +[XCTContext runInContextForTestCase:markAsReportingBase:block:] + 104
    48  XCTestCore                          0x0000000100e60378 -[XCTestSuite _performProtectedSectionForTest:testSection:] + 180
    49  XCTestCore                          0x0000000100e60658 -[XCTestSuite performTest:] + 220
    50  XCTestCore                          0x0000000100e240f0 -[XCTest runTest] + 48
    51  XCTestCore                          0x0000000100e60adc -[XCTestSuite runTestBasedOnRepetitionPolicy:testRun:] + 68
    52  XCTestCore                          0x0000000100e609a4 __27-[XCTestSuite performTest:]_block_invoke + 164
    53  XCTestCore                          0x0000000100e60408 __59-[XCTestSuite _performProtectedSectionForTest:testSection:]_block_invoke + 48
    54  XCTestCore                          0x0000000100e5d844 +[XCTContext _runInChildOfContext:forTestCase:markAsReportingBase:block:] + 180
    55  XCTestCore                          0x0000000100e5d72c +[XCTContext runInContextForTestCase:markAsReportingBase:block:] + 104
    56  XCTestCore                          0x0000000100e60378 -[XCTestSuite _performProtectedSectionForTest:testSection:] + 180
    57  XCTestCore                          0x0000000100e60658 -[XCTestSuite performTest:] + 220
    58  XCTestCore                          0x0000000100e240f0 -[XCTest runTest] + 48
    59  XCTestCore                          0x0000000100e26538 __89-[XCTTestRunSession executeTestsWithIdentifiers:skippingTestsWithIdentifiers:completion:]_block_invoke + 104
    60  XCTestCore                          0x0000000100e5d844 +[XCTContext _runInChildOfContext:forTestCase:markAsReportingBase:block:] + 180
    61  XCTestCore                          0x0000000100e5d72c +[XCTContext runInContextForTestCase:markAsReportingBase:block:] + 104
    62  XCTestCore                          0x0000000100e263f8 -[XCTTestRunSession executeTestsWithIdentifiers:skippingTestsWithIdentifiers:completion:] + 296
    63  XCTestCore                          0x0000000100e9e12c __72-[XCTExecutionWorker enqueueTestIdentifiersToRun:testIdentifiersToSkip:]_block_invoke_2 + 136
    64  XCTestCore                          0x0000000100e9e2c0 -[XCTExecutionWorker runWithError:] + 132
    65  XCTestCore                          0x0000000100e5a000 __25-[XCTestDriver _runTests]_block_invoke.264 + 56
    66  XCTestCore                   
libc++abi: terminating due to uncaught exception of type NSException

Restarting after unexpected exit, crash, or test timeout in -[DBAsyncTests testFutureRoundtripWithNSNull]; summary will include totals from previous launches.
LiFengSC commented 1 month ago

Yes you are right, setValue 's parameter should be nonnull. For void futures, the version without parameter should be used.

jb-gcx commented 1 month ago

@LiFengSC I'm happy to make a PR out of this.

One more question: is there any way to prevent calling setValue without parameter for promises with non-optional values? Or to make setValue take a nullable parameter if and only if the promises value is optional? DJPromise<nonnull Foo *> doesn't compile and I'm not sure how powerful ObjC generics are in this regard 🤔

LiFengSC commented 1 month ago

TBH I don't know. My understanding is ObjC generics is not as powerful as C++ templates that we can completely disable the undesirable form.