To keep it interesting, this time I'd like to tell about our approach to developing and polishing diagnostic rules for PVS-Studio Java. You will learn how we keep existing warnings consistent across releases and why the new ones aren't too weird. I'll also share a bit of inside information on what plans we, the Java team, have for the future, and show you a few interesting (and a few plain) bugs we found using the diagnostics from the upcoming release.
Naturally, every new diagnostic rule starts as an idea. And since the Java analyzer is the most recent branch of PVS-Studio, we mainly steal ideas from the C/C++ and C# teams. But it's okay: we come up with our own rules too (and some are suggested by our users, for which we are greatly thankful) and then the other teams steal them from us. Circulation of ideas, you know.
In most cases, implementing rules in the code is very much a pipeline process. You create a file with a bunch of synthetic examples, manually mark spots with bugs, arm yourself with the debugger and just keep traversing the syntax tree until you are sick and have covered all the cases you came up with. Some rules are surprisingly simple (for example, diagnostic rule V6063 is literally just a few lines long), while others have complex logic that takes quite a while to get right.
But it's only the beginning. As you know, we don't favor synthetic examples much because they are extremely poor indicators of diagnostics' behavior in real-life circumstances. By the way, a significant portion of examples used in our unit tests comes from real projects: it's almost impossible to come up with every possible case all on your own. What's more, unit tests keep us from losing warnings triggered by examples in the documentation. Yes, it happened before, but shh, don't tell anyone.
Anyway, we need to somehow find warnings in real projects first. And we also have to make sure that:
And this is where, much like a knight upon a horse (slightly limping, but we are working on that), SelfTester enters the scene. Its main and sole purpose is to automatically check a collection of projects against the "reference model" in the version control system and report any newly triggered warnings and warnings that have been removed or changed. That is, to provide us with diffs on the analyzer's report and show related code in the projects. As of now, SelfTester for Java has a collection of 62 open-source projects of ancient versions, with projects like DBeaver, Hibernate, and Spring among them. One full run through the collection takes about 2–2.5 hours, which is surely painful, but it can't be helped.
In the screenshot above, the "green" projects are those that haven't changed. As for the "red" projects, we manually study each diff generated on them and, if it's correct, approve it by clicking that "Approve" button at the bottom. Actually, the PVS-Studio distribution will be built only if SelfTester's output is completely green. This is how, basically, we maintain consistency of results across different versions.
SelfTester also helps us weed off tons of false positives even before releasing the diagnostic. This is what our typical rule development algorithm looks like:
Fortunately, we don't have to do full runs of SelfTester – and, consequently, endure the "2–2.5 hour" wait – too often. Every now and then luck fails us and we get warnings on large projects like Sakai and Apache Hive. That means it's time to have coffee – a whole lot of coffee. Or, as an alternative, to work on the documentation, but not many are willing.
"Why use unit tests at all when you have such a great tool?"
Because tests are significantly faster. Wait a few minutes and get a result. Besides, they help with figuring out which part of the rule exactly has broken. And finally, not all possible warnings associated with a given rule necessarily get caught in SelfTester's projects, but we still need to check them.
I originally started this section with the words, "The versions of the projects checked by SelfTester are pretty old, so most of the bugs I'll be showing must have been fixed by now." What a surprise it was for me to discover that all of them – to the last bug – were still there. Each single one, literally. And that means two things:
For those of you willing to dig deeper, I'll include links to the particular versions that we check.
P.S. What I've said above doesn't mean that static analysis is good for catching only harmless bugs in rarely used code. Remember that what we check are release (or near-release) versions, i.e. versions where most of the critical bugs were already found and fixed by developers and testers (and in some cases, I'm afraid, by users too) manually, which is a long, expensive, and painful process. We talk more about this in the article "Errors that static code analysis does not find because it is not used".
The "V6080 Consider checking for misprints. It's possible that an assigned variable should be checked in the next condition" diagnostic was already included in version 7.08 but didn't come up in our articles, so now is a good time to take a look at it.
public class Menu
{
private Map<String, List<String>> menus = new HashMap<String, List<String>>();
public void putMenuItem(String menu, String item)
{
List<String> items = menus.get(menu);
if (item == null) // <=
{
items = new ArrayList<String>();
menus.put(menu, items);
}
items.add(item);
}
....
}This is a classic example of a "key-collection" dictionary – and a classic typo. The developer wanted a collection to be created to initialize a mapping if it doesn't exist, but wrote the wrong variable name, ending up with not just a faulty method but a NullPointerException in the last line as well. In Java 8 and later, it's better to use the computeIfAbsent method to implement this type of dictionary:
public class Menu
{
private Map<String, List<String>> menus = new HashMap<String, List<String>>();
public void putMenuItem(String menu, String item)
{
List<String> items = menus.computeIfAbsent(menu, key -> new ArrayList<>());
items.add(item);
}
....
}One of the diagnostics to be included in the next release checks for correct implementation of the "double-checked locking" pattern. Among SelfTester's collection of projects, Glassfish turned out to be the record-holder: PVS-Studio found a total of 10 bugs of this type in it. Let's have some fun now! Look at the snippet below and try to spot two of those bugs. You may refer to the documentation for a clue: "V6082 Unsafe double-checked locking". Or, if you don't feel like hunting bugs at the moment, go straight to the end of the article.
EjbComponentAnnotationScanner.java
public class EjbComponentAnnotationScanner
{
private Set<String> annotations = null;
public boolean isAnnotation(String value)
{
if (annotations == null)
{
synchronized (EjbComponentAnnotationScanner.class)
{
if (annotations == null)
{
init();
}
}
}
return annotations.contains(value);
}
private void init()
{
annotations = new HashSet();
annotations.add("Ljavax/ejb/Stateless;");
annotations.add("Ljavax/ejb/Stateful;");
annotations.add("Ljavax/ejb/MessageDriven;");
annotations.add("Ljavax/ejb/Singleton;");
}
....
}Polishing diagnostics is not only about modifying their code with the aim to catch more suspicious spots or get rid of false positives. Manual method annotation for data-flow analysis plays an important part in the analyzer development too. For instance, you can mark a certain library method as always returning a non-null value. When making a new diagnostic, we discovered – by pure accident – that we don't have an annotation for the Map#clear() method. In addition to the outright silly code patterns that the "V6009 Collection is empty. The call of the 'clear' function is senseless" diagnostic is now able to detect, we also found a nice typo.
protected void after()
{
this.metricsById.clear();
this.metricsById.clear();
}Dictionary re-clearing might not look like an error at first. We would have even thought it was just an accidentally duplicated line if we hadn't looked just a bit further – right at the next method.
protected void after()
{
this.metricsById.clear();
this.metricsById.clear();
}
public Metric getByKey(String key)
{
Metric res = metricsByKey.get(key);
....
}Exactly. This class has two fields with similar names metricsById and metricsByKey. I bet the developer wanted the after method to clear both dictionaries, but either autocompletion failed them or they mechanically typed the same name one more time. As a result, the two dictionaries which originally stored linked data will get desynchronized after calling the after method.
Another diagnostic to appear in the next version is "V6084 Suspicious return of an always empty collection". It's quite easy to forget to add elements to a collection, especially when each element must be initialized first. I can tell from my own experience that such bugs usually result in strange program behavior or some functionality missing rather than a crash.
public List getDaySelectItems()
{
List selectDays = new ArrayList();
Integer[] d = this.getDays();
for (int i = 0; i < d.length; i++)
{
SelectItem selectDay = new SelectItem(d[i], d[i].toString());
}
return selectDays;
}Curiously, there are other methods in this class that look very similar to the one above but don't have the same bug in them. For example:
public List getMonthSelectItems()
{
List selectMonths = new ArrayList();
Integer[] m = this.getMonths();
for (int i = 0; i < m.length; i++)
{
SelectItem selectMonth = new SelectItem(m[i], m[i].toString());
selectMonths.add(selectMonth);
}
return selectMonths;
}Apart from a bunch of not-so-interesting internal tasks, we are currently considering adding diagnostics for the Spring Framework to our Java analyzer. The framework is not only the working horse of Java developers but also has quite a few non-obvious nuances that may easily turn into pitfalls. We are not quite sure yet as to what form exactly these diagnostics will take in the end or when it will happen or if it's even going to happen at all. But we are dead sure that we need ideas to start with and Spring-based open-source projects to include in SelfTester. If you have something worthy in mind, please let us know! The more stuff we'll gather, the more focus it will get.
To wind up, here are the two mistakes in Glassfish implementation of double-checked locking which I asked you to try to find earlier:
What harm can they do? The documentation will help you figure it all out.