Thrift 2905 & - Modern Objective-C & Swift Support

[kdubb]

This pull request is really just to start a conversation about how to move forward with the changes I've made to the Thrift Cocoa library and generator.

The nature of the changes are that they are big & a bit destructive. Unfortunately there wasn't much getting around that. Hopefully working with the community we can agree on the best way to integrate these changes. Maybe just as another "language" binding instead of replacing the current "cocoa".

Goals

• Modernize generated code
• Increase asynchronous support (both blocks and promises)
• Remove use of deprecated classes & paradigms (e.g NSException, NSURLConnection)
• Generate Swift client & server code

Major Changes

• Addes Swift code generator

• Use NSError based error & exception handling
• Optionally generate PromiseKit methods in the asynchronous clients
• Change asynchronous clients to be fully multi-thread safe
• Added THTTPSessionTransport, asynchronous HTTP transport that works with NSURLSession
• Use NS_ENUM enumerations (with standard format)
• Use immutable collection types (e.g. NSArray) in service methods
• Struct fields & their "set" flags now use properties exclusively
• Helper classes now include the service name to fix duplicate naming issue when two services has the same method name
• NSCopying is implemented correctly for structs
• Simple constants are now "exported" public variables
• Optionally use CocoaPods style imports with Podspec (e.g. <Thrift/File.h>)
• Removed instance variables from public headers
• Remove retain/release stubs
• Remove all deallocs

Why?

We made some very large changes. Here's why...

NSException to NSError

The use of NSException has been very problematic for us. Aside from the direction of Apple, which is to use exceptions for "fatal errors" only, it comes with a host of issues. First, Clang still produces incorrect code in many cases when using exceptions. Take this code for example...

executeBlockAndReturnValue(^{
  @try {
    return value;
  }
  @catch(NSException *e) {
  }
  // should be a return statement on this branch, Clang produces no error/warning
});

Clang should produce an error but doesn't. This is only a nuisance though, much worse are the ARC failures we've found when catching exceptions up the stack. Secondly, Swift has no support for exceptions (and with Swift 2 we've seen it won't ever use NSException).

It's for these reasons we changed the the Cocoa library to use NSError, all of the generated (synchronous) methods now follow the Apple guidelines and return a BOOL or object pointer with a last error parameter of type NSError**; return values of NO or nil mean the error parameter is populated and the call failed (or and exception was thrown from the server). These changes have been made throughout the generated and library code.

Client Method Mapping

• Methods returning basic types are mapped to NSNumber/NSValue - nil reports call failure/exception
• Methods returning VOID are mapped to return BOOL - NO reports call failure/exception
• Methods returning Struct/String/Binary return the object value as normal - nil reports call failure/exception

NSError & Swift 2

These changes conveniently take the form of Swift 2's error handling system. This means that protocols and transports can be used from and written in Swift and will be compatible with the current & future directions of Swift.

ARC Only

ARC is here to stay; Apple deprecated everything else a few years ago. All code related to maintaining backwards compatibly with non-ARC mode has been removed.

Mutl-Threaded Asynchronous Transports

The current implementations means that clients cannot be used in different threads. This means the user is left to synchronize access to them or, as in our case, build a client per thread to ensure parallelism.

Our changes have reengineered the way asynchronous works. Now, asynchronous clients accept only protocol and async-transport factories. Each call allocates a new transport and protocol binds them before use. This provides the best options for parallelism with a fairly small overhead.

A note on performance... the alternative approaches, which we used in the past, required thread local storage inside the transport which meant an NSDictionary lookup (i.e. threadDictionary) for each read/write/flush. The new approach only requires 2 simple object allocations. Much improved.

NSURLSession (NSURLConnection is now deprecated)

The new THTTPSessionTransport replaces the old THTTPClient. It's both asynchronous and uses the new NSURLSession classes for conformance in the new iOS/OSX versions.

NS_ENUM & Swift

The generated code now generates properly formatted enumerations using the NS_ENUM macro & no underscores. This is to ensure they work with code completion in objective-c and are imported into Swift with the best possible syntax.

[Jens-G]

Re cocoa/objc/swift: I see. Leave it as it is.

Question: Are the eror codes from TTransportError.h (and mybe others) only used internally or are they visible to the other side? If they are public, they probably should match the usual Thrift error numbers, like in this C++ code, otherwise a (for example) C++ client will have a hard time dealing with them.

[kdubb]

With regard to the C++ and Cocoa exception codes matching... It doesn't seem like the exceptions are being thrown in the same circumstances in most cases. Maybe we should look at standardizing these codes in general?

[Jens-G]

Maybe we should look at standardizing these codes in general?

Good idea :-) In fact, the exception codes are already standardized for all standard Thrift exceptions (TTransport/Protocol/ApplicationException). If you compare with other languages you'll find they all match.

[kdubb]

I've brought the exception reporting in line with other languages. I looked into Java & C++ to see the error codes they for all. Historically TApplicationError seems to have been correct in the past and I trampled on it by changing the codes themselves. The protocol & transport errors needed a bit of shuffling but now they are using a meaningful code in the correct context. I've kept some of the extended error information we were traditionally reporting but put it into variables that do not escape the current environment.

[jriskin]

First, thanks for all the modernization!
I had a few issues with this release.

1. had to delete the swift files
2. Using the new generator I had to remove instances of NSArray i just removed the typing for now
3. I'm having collisions with methods that I wouldn't suspect. For now i renamed all my properties, but shouldn't they some how be scoped to their class? I'm not entirely sure what's going on here.

[kdubb]

@jriskin
Can you send me a sample of your thrift file that shows the issues with 2 & 3? Although, I can hazard a guess at 2.

Also, what was the reason for needing to delete the swift files? Are you targeting an platform version that doesn't support it?

[jriskin]

1. I believe it has something to do with Xcode converting the project to create a bridging header. If I drag all the files at once in to the project, it just accepts them, if you drag them one at a time it asks about creating the bridging header. It's probably harmless, but since I don't have any other swift code in the project, rather than set that up, I just removed the swift files.
   https://developer.apple.com/library/ios/documentation/Swift/Conceptual/BuildingCocoaApps/MixandMatch.html
2. & 3 So push_id conflicts with a completely unrelated push_id method and post_ids are problem number 2.
   thrift:
   struct PushContentRequest {
   1: optional i32 push_id;
   2: optional list post_ids;
   3: optional i32 post_limit;
   4: optional i64 bookmark;
   5: optional i32 comment_limit;
   }

output header:
@interface CTZNAPIV2PushContentRequest : NSObject <TBase, NSCoding, NSCopying>

@Property (assign, nonatomic) SInt32 push_id;
@Property (assign, nonatomic) BOOL push_idIsSet;

• (void) unsetPush_id;

@Property (strong, nonatomic) NSMutableArray * post_ids;
@Property (assign, nonatomic) BOOL post_idsIsSet;

• (void) unsetPost_ids;

@Property (assign, nonatomic) SInt32 post_limit;
@Property (assign, nonatomic) BOOL post_limitIsSet;

• (void) unsetPost_limit;

@Property (assign, nonatomic) SInt64 bookmark;
@Property (assign, nonatomic) BOOL bookmarkIsSet;

• (void) unsetBookmark;

@Property (assign, nonatomic) SInt32 comment_limit;
@Property (assign, nonatomic) BOOL comment_limitIsSet;

• (void) unsetComment_limit;
• (instancetype) initWithPush_id: (SInt32) push_id post_ids: (NSArray *) post_ids post_limit: (SInt32) post_limit bookmark: (SInt64) bookmark comment_limit: (SInt32) comment_limit;

@EnD

[kdubb]

I assume from the way the IDL was pasted that all of the <i32> and <SInt32> have been scrubbed. Regardless, I believe I can see where the issue is with that.

For now i renamed all my properties, but shouldn't they some how be scoped to their class? I'm not
entirely sure what's going on here.

In Objective-C selectors are explicitly not scoped to their class. This has surprised me more times than I would like to believe.

[creker]

From the code it difficult to see what's the problem. Yes, in objc selectors are separate from the classes - they're shared between them and stored as plain strings. That creates problems only when you're dealing with id objects - compiler doesn't have type information. When you call method on id compiler will search for any class that contains the selector. But if your variable have a specific type then compiler will know what to do. Even with id I don't remember any conflicts - compiler just silently lets me call any selector that it can find which may crash at runtime because that particular object doesn't implement it.

[jriskin]

@creker ah, that makes some sense, I am using id for a couple of types of objects that I store in arrays and they don't resolve to a type until runtime. I'll revert all the code once i'm in a stable state and see if i can figure it out, for now i just renamed the conflicts.

Also, is it an issue that they are using 'id' as a variable name? That seems awfully unsafe in obj-c. I may push to see if i can get them to change this to post_id or something more reasonable.

e.g.
struct Post {
1: optional i32 id;

[creker]

Had a few problems with a field named description. It obviously conflicts with NSObject's one. Have to be careful about method conflicts and, of course, don't use keywords as fields names. As far as I know, clang is smart enough to understand that id is used as a name and not a keyword. But I'm still against such names.

[jriskin]

I completely agree, definitely going to make the case to have the team defining the thrift to change... i would think it might be a good idea in the generator to automatically prefix reserved keywords as an option when the programmer can't control the thrift.

[jriskin]

It also might be nice to use NSNumber objects, would solve the issue of storing them in NSArrays.

[creker]

You read my mind. Was thinking about both features you mention - somehow deal with conflicting names (shouldn't be too much of them) and store all value types in NSNumber. But latter will cause the fields to loose their type. Still, compiler options for both of them would be nice.

[kdubb]

The issue with the NSArray<SInt32> is just a problem with the generics specifier. It says SInt32 but all the code uses an NSNumber properly. I'll fix and commit ASAP.

[kdubb]

FWIW, we currently use a property named "id". I'll admit to being concerned when we first started using it but beyond looking awkward it has produced no issues.

[kdubb]

@jriskin PR #652 should fix your issues with NSArray<SInt32>. They should now, correctly, be generated as NSArray<NSNumber *>. It also fixes a number of other issues with constants of those types.

[jriskin]

Awesome! I'll check it out now. FYI, coercing the type solved my namespace issue. e.g.
for (id entry in self.feed.posts) {
if ([entry isKindOfClass:[CTZNPostModel class]]) {
// this next line creates a conflict on post_id since it's type is unknown at this point.
if([entry post_id] == self.current_post_id) {
// this fixes the problem...
if([(CTZNPostModel *)entry post_id] == self.current_post_id) {

Thanks for the quick fixes!