Picnic loves Error Prone:
producing high-quality and consistent Java code
Error Prone Support is now open source! Check out the announcement.
Picnic is changing the way people do groceries. We’re an app-only supermarket, delivering the highest quality products for the best price to our customers. To do this, we meticulously design and build products for our customers and internal users. From providing real-time delivery tracking to building software for our warehouse management, meanwhile delighting our customers every single day. This takes a lot of software development, so how are we able to achieve this at high quality with a modest team of engineers?
Java development at Picnic
Within Picnic, we have a Platform team that supports our Java developers, from writing code using shared libraries to CI/CD. Across Picnic, we have a strong code reviewing culture to guard the quality of our codebase. But this isn’t enough: no matter how experienced a developer is, one can always overlook something. Additionally, repetitive style comments distract from reviewing the code’s functionality. Like other companies, we use static analysis tools to automate away some of this toil. To that end, we integrate static analysis tools into our development workflow. For Java development, this means adopting tools like Checkstyle, SonarQube, Maven’s Enforcer plugin, Policeman’s Forbidden API Checker, and many more.
These tools help detect bugs before they enter our codebase. In addition, it saves time for the pull request reviewers because they don’t have to point out these issues. Even better, it saves the frustration of solving or pointing out the same issues over and over again. Seeing the boost such tools give us, these questions arose: how can we achieve a similar degree of automation for bug patterns that we see occurring frequently, as opposed to relying on checks that existing tools provide? And, can we move from tooling that only points out problems, to tooling that actually fixes them?
High-quality code and uniform codebase
Let’s take a simple example: a Java developer wants to check if a string is empty. Okay, easy you think: str.length() == 0, or would it be safer to use str.isEmpty()? And what if it’s null? So shouldn’t we be using "".equals(str)…?
As you can see, there are many ways to do something simple. A lot of effort is wasted debating the best way — only to repeat this in another team, on another day. You can write guidelines (followed by some, ignored by others), or rely on the discipline of developers to enforce a single way during reviews. But that simply doesn’t scale.
Does this even matter? We think so. First of all, enforcing coding standards can prevent bugs and production incidents. But that’s not all. Having a consistent codebase across your organization has major benefits, such as increased onboarding speed for new joiners and less friction when contributing to another team’s code. Especially when you look beyond core Java and start considering how APIs of libraries can be misused, the existing static analysis tools don’t really offer any help.
So, we needed something to support Picnic’s opinionated way of writing Java code. Let’s automate this!
Introducing Error Prone
At Picnic, we have adopted Error Prone, a Java compiler plugin created by Google. It’s capable of automated checking and refactoring of Java code. By analyzing the code’s abstract syntax tree (AST), Error Prone is able to identify bugs and anti-patterns and, where possible, produce suggested fixes.
Issues are flagged by emitting a compilation warning or error. These messages provide additional information and show the developer how to improve the code. Additionally, Error Prone can produce patch files with the suggested fixes, optionally applying the changes to the codebase immediately.
Let’s take a simple example where valid Java code doesn’t quite do what you want. It’s easy to forget the throw keyword before the initialization of an exception. This code compiles, but no exception will be thrown:
When compiling your code with Error Prone installed as a compiler plugin, however, you will get the following error:
[ERROR] Example.java:[3,35][3,25] [DeadException] Exception created but not thrown
[ERROR] (see https://errorprone.info/bugpattern/DeadException)
[ERROR] Did you mean 'throw new IllegalArgumentException();'?As a developer, you don’t just get a generic error message, but a fully contextualized suggested fix to boot. What’s not to like?
These so-called BugChecks are provided out of the box, but you can also create your own! This extensibility is what makes Error Prone so attractive to us, and we have created many of our own BugChecks on top of Error Prone. However, writing BugChecks means traversing (and possibly manipulating) ASTs of Java code, which isn’t entirely trivial. You can see this already in the implementation of the DeadException BugCheck. Keep in mind this check is relatively straightforward compared to other BugChecks.
Refaster
So, writing a check in Error Prone can be challenging, as you need to work directly with ASTs and need to have some knowledge of Error Prone’s APIs.
For that reason, Google created Refaster, which ships together with Error Prone. Refaster is a tool that allows defining rewrite rules in Java using before- and after-templates. Refaster then scans the codebase for code that matches the structure of the before-template. The matching code is subsequently rewritten in accordance with the associated after-template.
An example might help, so let’s revisit the empty-string checking conundrum we discussed earlier in this article. Below we have a Refaster template that identifies any String-typed expression on which the method equals is called with the argument "" (in the first @BeforeTemplate annotated method), and replaces said method call with an invocation of the nullary method isEmpty (as expressed in the @AfterTemplate annotated method). A second before-template is also provided to match the case where developers use .length() == 0 to check for an empty string. Such expressions are also rewritten to the after-template.
With Refaster templates there’s no fiddling with ASTs required: you can express patterns in concrete Java syntax. Refaster can then apply changes to an entire codebase in a manner of seconds!
Currently, Refaster can only be used to rewrite Java expressions and statements; it does not support modifying other source code constructs such as annotations, variable declarations, return types, method names, or method signatures.
Unfortunately, none of Google’s templates are open-sourced even though developers have been asking for it. At Picnic, we’ve created a lot of Refaster templates to automatically fix anti-patterns in our code.
Rolling out Picnic’s Error Prone Support
Our Error Prone Support project collects all custom BugChecks and Refaster templates that we want to apply company-wide. Introducing Error Prone – including our own extensions – across all of our Java codebases is a major undertaking. It means getting every repository up to standards (we have about 2 million lines of Java code!) so the code conforms to all checks and templates until no errors or warnings are raised.
Before you can continuously enable this tooling, you need to establish a baseline. However, applying all the rules at once could result in huge pull requests with many code changes. No one likes to review a pull request that has over 3000 lines of code changed, so we took this challenge seriously. For each of our 50+ Java repositories, together with the teams, the Java Platform team gradually applied the BugChecks and templates by spreading the changes over multiple pull requests for teams.
At the same time, we carefully checked the warnings and errors to see whether we could improve the checks or templates, closing the feedback loop. The goal for each check is to have zero false positives. Besides that, we started the conversation with the developers to ask for feedback and have discussions about the preferred way of writing code.
Rewriting 37.000 lines of code was a breeze. By rewriting our code, we significantly improve its consistency and we enforce best practices.
As a concrete example, take the usage of LocalDateTime. One team was used to writing LocalDateTime.of(0,0), where another preferred LocalDateTime.MIDNIGHT, and yet another LocalDateTime.MIN. We asked Picnic’s #java-coders Slack community to express their preference, and the result of the poll was clear: let’s use LocalDateTime.MIN. With a Refaster template, we can easily enforce this choice and automatically change all of these occurrences in one go.
Even better than creating a uniform approach, is that we also catch actual bugs using our BugChecks and Refaster templates! Take for example this piece of code, where a developer inadvertently used the %s placeholder syntax in an SLF4J log statement, where it should be a {} placeholder. And, for the second log statement, provided only a single argument where the formatted string expects two:
Through our custom SLF4J BugCheck, these problems are caught at compile time rather than silently ignored (!) at run-time.
[WARNING] Example.java:[5,14] [Slf4jLogStatement] SLF4J log statement placeholders are of the form `{}`, not `%s`
Did you mean 'LOG.error("with-'{}'-placeholder", o);'?
[WARNING] Example.java:[6,14] [Slf4jLogStatement] Log statement contains 2 placeholders, but specifies 1 matching argument(s)With Error Prone we finally found the tool that allows us to take static analysis to the next level for our whole tech team.
The future of Error Prone Support
Picnic’s Java Platform team is leading the effort on implementing Error Prone across Picnic. Additionally, they’re increasing its usefulness with custom BugChecks and Refaster templates in the Error Prone Support project with every release.
Now, we’re happy to report that developers across Picnic are contributing to the Error Prone Support project and enthusiastic about the direction we’re heading. (Amazing stickers may or may not have been involved in this process.). The more checks and templates we add, the better our codebases become.
But you may wonder: how does all of this apply to me or my company? It should not come as a surprise that we highly recommend adopting Error Prone in your development cycle. Since it’s a plain Java compiler plugin, you can achieve this regardless of which build tooling you use.
Which leaves our collection of BugChecks and Refaster templates. Ideally everyone would curate their own perfect set, because every company is different. Still, we feel that many of our checks and templates are useful for the larger Java community outside of Picnic. So… stay tuned for our next step, which includes open sourcing Error Prone Support!
