Static Analysis with OCLint

Posted by on January 15, 2014

At Etsy, we’re big believers in making tools do our work for us.

On the mobile apps team we spend most of our time focused on building new features and thinking about how the features of Etsy fit into an increasingly mobile world. One of the great things about working at Etsy is that we have a designated period called Code Slush around the winter holidays where product development slows down and we can take stock of where we are and do things that we think are important or useful, but that don’t fit into our normal release cycles. Even though our apps team releases significantly less frequently than our web stack, making it easier to continue developing through the holiday season, we still find it valuable to take this time out at the end of the year.

This past slush we spent some of that time contributing to the OCLint project and integrating it into our development workflow. OCLint, as the name suggests, is a linter tool for Objective-C. It’s somewhat similar to the static analyzer that comes built into Xcode, and it’s built on the same clang infrastructure. OCLint is a community open source project and all of the changes to it we’re discussing have been contributed back and are available with the rest of OClint on their github page.

If you run OCLint on your code it will tell you things like, “This method is suspiciously long” or “The logic on this if statement looks funny”. In general, it’s great at identifying these sorts of code smells. We thought it would be really cool if we could extend it to find definite bugs and to statically enforce contracts in our code base. In the remainder of this post, we’re going to talk about what those checks are and how we take advantage of them, both in our code and in our development process.

Rules

Objective-C is a statically typed Object Oriented language. Its type system gets the job done, but it’s fairly primitive in certain ways. Often, additional contracts on a method are specified as comments. One thing that comes up sometimes is knowing what methods a subclass is required to implement. Typically this is indicated in a comment above the method.

For example, UIActivity.h contains the comment // override methods above a list of several of its methods.

This sort of contract is trivial to check at compile time, but it’s not part of the language, making these cases highly error prone. OCLint to the rescue! We added a check for methods that subclasses are required to implement. Furthermore, you can use the magic of Objective-C categories to mark up existing system libraries.

To mark declarations, oclint uses clang’s __attribute__((annotate(“”))) feature to pass information from your code to the checker.
To make these marks on a system method like the -activityType method in UIActivity, you would stick the following in a header somewhere:

@interface UIActivity (StaticChecks)
...
- (NSString *)activityType
__attribute__((annotate(“oclint:enforce[subclass must implement]”)));
...
@end

That __attribute__ stuff is ugly and hard to remember so we #defined it away.

#define OCLINT_SUBCLASS_MUST_IMPLEMENT 
__attribute__((annotate(“oclint:enforce[subclass must implement]”)))

Now we can just do:

@interface UIActivity (StaticChecks)
...
- (NSString *)activityType OCLINT_SUBCLASS_MUST_IMPLEMENT;
...
@end

 

We’ve contributed back a header file with these sorts of declarations culled from the documentation in UIKit that anyone using oclint can import into their project. We added this file into our project’s .pch file so it’s included in every one one of our classes automatically.

Some other checks we’ve added:

Protected Methods

This is a common feature in OO languages – methods that only a subclass and its children can call. Once again, this is usually indicated in Objective-C by comments or sometimes by sticking the declarations in a category in separate header. Now we can just tack on OCLINT_PROTECTED_METHOD at the end of the declaration. This makes the intent clear, obvious, and statically checked.

Prohibited Calls

This is another great way to embed institutional knowledge directly into the codebase. You can mark methods as deprecated using clang, but this is an immediate compiler error. We’ll talk more about our workflow later, but doing it through oclint allow us to migrate from old to new methods gradually and easily use things while debugging that we wouldn’t want to commit.

We have categories on NSArray and NSDictionary that we use instead of the built in methods, as discussed here. Marking the original library methods as prohibited lets anyone coming into our code base know that they should be using our versions instead of the built in ones. We also have a marker on NSLog, so that people don’t accidentally check in debug logs. Frequently the replacement for the prohibited call calls the prohibited call itself, but with a bunch of checks and error handling logic. We use oclint’s error suppression mechanism to hide the violation that would be generated by making the original call. This is more syntactically convenient than dealing with clang pragmas like you would have to using the deprecated attribute.

Ivar Assignment Outside Getters

We prefer to use properties whenever possible as opposed to bare ivar accesses. Among other things, this is more syntactically and semantically regular and makes it much easier to set breakpoints on changes to a given property when debugging.  This rule will emit an error when it sees an ivar assigned outside its getter, setter, or the owning class’s init method.

-isEquals: without -hash

In Cocoa, if you override the -isEquals: method that checks for object equality, it’s important to also override the -hash method. Otherwise you’ll see weird behavior in collections when using the object as a dictionary key. This check finds classes that implement -isEquals: without implementing -hash. This is another great example of getting contracts out of comments and into static checks.

Workflow

We think that oclint adds a lot of value to our development process, but there were a couple of barriers we had to deal with to make sure our team picked it up. First of all, any codebase written without oclint’s rules strictly enforced for all of development will have tons of minor violations. Sometimes the lower priority things it warns about are actually done deliberately to increase code clarity. To cut down on the noise we went through and disabled a lot of the rules, leaving only the ones we thought added significant value. Even with that, there were still a number of things it complained frequently about – things like not using Objective-C collection literals. We didn’t want to go through and change a huge amount of code all at once to get our violations down to zero, so we needed a way to see only the violations that were relevant to the current change. Thus, we wrote a little script to only run oclint on the changed files. This also allows us to easily mark something as no longer recommended without generating tons of noise, having to remove it entirely from our codebase, or fill up Xcode’s warnings and errors.

Finally, we wanted to make it super easy for our developers to start using it. We didn’t want to require them to run it manually before every commit. That would be just one more thing to forget and one more thing anyone joining our team would have to know about. Plus it’s kind of slow to run all of its checks on a large codebase. Instead, we worked together with our terrific testing and automation team to integrate it into our existing github pull request workflow. Now, whenever we make a pull request, it automatically kicks off a jenkins job that runs oclint on the changed files. When the job is done, it posts a summary a comment right to the pull request along with a link to the full report on jenkins. This ended up feeling very natural and similar to how we interact with the php code sniffer on our web stack.

Conclusion

We think oclint is a great way to add static checks to your Cocoa code. There are some interesting things going on with clang plugins and direct Xcode integration, but for now we’re going to stick with oclint. We like its base of existing rules, the ease of gradually applying its rules to our code base, and its reporting options and jenkins integration.

We also want to thank the maintainer and the other contributors for the hard work they’ve been put into the project. If you use these rules in interesting ways, or even boring ones, we’d love to hear about it. Interested in a working at a place that cares about the quality of its software and about solving its own problems instead of just letting them mount? Our team is hiring!

Posted by on January 15, 2014
Category: mobile Tags: , ,

2 Comments

Love your mobile posts! Keep em coming. Can you post your script for running OCLint on changed files in a pull request? I need to do the exact same thing. Thanks!

Love your mobile articles, keep em coming! Can you post your script that runs OCLint only on changed files in a pull request? I need to do the same thing. Thanks!!