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

[Bridge] Support nullability annotations in bridged methods #1796

Closed
wants to merge 1 commit into from

Conversation

ide
Copy link
Contributor

@ide ide commented Jun 30, 2015

Fixes a crash due to the selector regex not knowing about the nullability annotations. Adds support for both the core annotations __nullable and __nonnull plus their shorthand counterparts nullable and nonnull.

Objective-C allows the shorthand versions only at the front of a parameter type declaration like (nullable NSString *) but the regex will pick up (NSString * nullable) too. This shouldn't cause any adverse effects and I left the code this way to keep the regex readable.

Fixes #1795

Test Plan: Wrote a bridge method that uses a nullability annotation and verified that it didn't cause the app to crash:

RCT_EXPORT_METHOD(method:(nullable NSNumber *)reactTag)
{
}

Also added a nullable annotation to RCTTest.

Fixes a crash due to the selector regex not knowing about the nullability annotations. Adds support for both the core annotations `__nullable` and `__nonnull` plus their shorthand counterparts `nullable` and `nonnull`.

Objective-C allows the shorthand versions only at the front of a parameter type declaration like `(nullable NSString *)` but the regex will pick up `(NSString * nullable)` too. This shouldn't cause any adverse effects and I left the code this way to keep the regex readable.

Test Plan: Write a bridge method that uses a nullability annotation and verified that it didn't cause the app to crash:
```
RCT_EXPORT_METHOD(method:(nullable NSNumber *)reactTag)
{
}
```

Also added a nullable annotation to RCTTest.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 30, 2015
@nicklockwood
Copy link
Contributor

Thanks, I've been meaning to add this (along with typed collections/generics support).

What would be really cool though, is if the bridge enforced this by checking for NSNull values for nonnull arguments and threw a redbox.

Do you think it's feasible to add that to this PR, or would you prefer I land this as-is and create a new task?

@ide
Copy link
Contributor Author

ide commented Jun 30, 2015

Could you land this diff separately? Thinking about generics, there's a fair bit of overlap in the bookkeeping for nullability annotations and generics so it might make sense to plan for both features together.

@nicklockwood
Copy link
Contributor

👍🏽

@ide ide closed this in e3225f3 Jul 2, 2015
@ide ide deleted the nullability branch July 2, 2015 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants