Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated nullability for Swift. #509

Merged
merged 1 commit into from Nov 29, 2016
Merged

Conversation

E-B-Smith
Copy link
Contributor

I made the callback params nullable for Swift compatibility.

@aaustin
Copy link
Contributor

aaustin commented Nov 23, 2016

Hmm - I don't think params can ever be null though, which is why I set them to _Nonnull. Can you share the line where they return null values?

@aaustin
Copy link
Contributor

aaustin commented Nov 23, 2016

Also, you need to tag someone if you want this reviewed. Maybe you didn't?

@E-B-Smith
Copy link
Contributor Author

Accidental nil values being passed to a call back can be tricky to detect, and are a certain crash on the Swift side, so I made the parameters nullable for the time being to prevent crashes.

@ahmednawar
Copy link
Contributor

IIUC, all server responses should hit this method https://github.com/BranchMetrics/ios-branch-deep-linking/blob/master/Branch-SDK/Branch-SDK/BNCEncodingUtils.m#L389-L415 which would return {} in case of nil or bad data

@E-B-Smith
Copy link
Contributor Author

I'm thinking of this, instance:

https://github.com/BranchMetrics/ios-branch-deep-linking/blob/master/Branch-SDK/Branch-SDK/Branch.m#L1302-L1304

        if (self.sessionInitWithParamsCallback) {
            self.sessionInitWithParamsCallback([self getLatestReferringParams], nil);
        }

If [self getLatestReferringParams] return nil for some reason, it's a crash in Swift.

@aaustin
Copy link
Contributor

aaustin commented Nov 24, 2016

I think it's impossible for that to happen? https://github.com/BranchMetrics/ios-branch-deep-linking/blob/master/Branch-SDK/Branch-SDK/Branch.m#L775 and https://github.com/BranchMetrics/ios-branch-deep-linking/blob/master/Branch-SDK/Branch-SDK/BNCEncodingUtils.m#L389. Since we already have those flags and the parter wrote in saying we don't, I think there's a different root cause at play here. Let's wait for him to reply to us.

@E-B-Smith
Copy link
Contributor Author

The best solution is to add strict nullability checking throughout the SDK which will allow the static analyzer to catch these cases for us. Checking all the edge cases can be pretty time consuming for us humans but is easy for the analyzer.

Until then, I think we should change the 'nullable' flag here to prevent accidental partner crashes. It's the fastest fix and prevents some pretty awful partner crashes.

@E-B-Smith E-B-Smith merged commit 16b65c9 into staging Nov 29, 2016
@E-B-Smith E-B-Smith deleted the Callbacks-Nullable-GH-507 branch November 29, 2016 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants