How To Find Bugs, Part 2: Well, this is somewhat confusing and frustrating

LMAX Exchange

Last time we implemented a minimal detector, and I presented the code for the detector as a fait accompli. Let’s take a closer look at it.

import java.nio.file.Files;

import edu.umd.cs.findbugs.BugInstance;
import edu.umd.cs.findbugs.BugReporter;
import edu.umd.cs.findbugs.BytecodeScanningDetector;
import edu.umd.cs.findbugs.classfile.ClassDescriptor;
import edu.umd.cs.findbugs.classfile.DescriptorFactory;
import edu.umd.cs.findbugs.classfile.MethodDescriptor;

public class FilesLinesDetector extends BytecodeScanningDetector {
  private static final ClassDescriptor JAVA_NIO_FILES =
    DescriptorFactory.createClassDescriptor(Files.class);
  final BugReporter bugReporter;

  public FilesLinesDetector(final BugReporter bugReporter)
  {
    this.bugReporter = bugReporter;
  }
 
  @Override
  public void sawMethod()
  {
    MethodDescriptor invokedMethod = getMethodDescriptorOperand();
    ClassDescriptor invokedObject = getClassDescriptorOperand();
    if(invokedMethod != null && 
       "lines".equals(invokedMethod.getName()) && 
       JAVA_NIO_FILES.equals(invokedObject))
    {
      bugReporter.reportBug(
      new BugInstance(this, "FILES_LINES_CALLED", HIGH_PRIORITY)
            .addClassAndMethod(this).addSourceLine(this));
    }
  }
}

Let’s start here:

public class FilesLinesDetector extends BytecodeScanningDetector

A Findbugs detector is defined as a class implementing the Detector interface. There are a number of abstract classes which provide some scaffolding on which to build a detector, and ByteCodeScanningDetector is one of them.

At first, when I started writing detectors, I was picking detector classes to extend based on how specific they appeared to be to the problem I was trying to solve. Over time, I found that everything I could do with more specific detectors, I could also do with ByteCodeScanningDetector, and it usually looked cleaner. In addition, that way I only had to learn the quirks of one detector, and this is an API with a ton of quirks.

In short: I always extend ByteCodeScanningDetector.

public FilesLinesDetector(final BugReporter bugReporter)
{
    this.bugReporter = bugReporter;
}

A detector isn’t just a class implementing the Detector interface: it also has to have a constructor which takes a BugReporter as its only argument.

public void sawMethod()

ByteCodeScanningDetector has a ton of methods, many inherited. The Javadoc isn’t always terribly helpful here.

There are two common patterns of naming here: visitXXX and sawXXX. It appears that visitXXX means declaration, and sawXXX means evaluation. For example, visitMethod() means that we’ve just hit a method declaration, whereas sawMethod() means we’ve just hit a method call. So, as we’re looking for method calls, that means we want to do our detecting in sawMethod().

There are some other gotchas in here: for example, sawMethod() will be hit every time we call a method, but only if the type of the object that’s being invoked is class. If, however, we’re calling a method on an interface, then sawMethod() won’t be called: instead sawIMethod() will be called.

MethodDescriptor invokedMethod = getMethodDescriptorOperand();

Of course, all the detector visit/saw methods are no-arg void methods. That’s because the detector tracks the current state, and provides access to that state via a variety of methods.

So, we’ve just hit a method call, and we want to know what method it was. We notice there is a method getMethodDescriptor(): this is not what we want. getMethodDescriptor() returns a MethodDescriptor for the method that we’re currently in, not the method that’s being called. What we want is getMethodDescriptorOperand() – this returns the method descriptor for the operation we’re currently looking at, i.e. the method call.

MethodDescriptors and ClassDescriptors are one way Findbugs will represent methods/classes: you can also get XMethods and XClasses, and occasionally you’ll need to deal with BCEL types like Type and JavaClass. We’ll delve into the differences in a later post: for now, all you need to know is that descriptors are unique identifiers for a piece of code with a small amount of metadata (name, package, signature – that sort of thing).

private static final ClassDescriptor JAVA_NIO_FILES =
 DescriptorFactory.createClassDescriptor(Files.class);

if(invokedMethod != null && 
 "lines".equals(invokedMethod.getName()) && 
 JAVA_NIO_FILES.equals(invokedObject))

This is reasonably straightforward: if the method being called is on java.nio.Files, and the method is called “lines”, then that’s a bug (or so we assert).

Our life is made easier here by this being a static call: if this were a regular method call like equals(), we couldn’t assume that the class operand is the class on which the method was declared, and we’d need to find a way to reason about supertypes. Turns out, that’s not hard, but if Findbugs implements that for you, I haven’t found it.

Our life is also made easier because we don’t care about the signature. Let’s say we had access to an API which had two methods, one of which took a byte[] and another which took a byte[] and a charset, and you wanted to ensure a charset was always provided to avoid falling back on default charsets. In that case, you’d need to inspect the signature to determine if the sinful or virtuous overload is being called. We don’t have that problem here: we’re content banning the use of all overloads of Files.lines.

bugReporter.reportBug(
    new BugInstance(this, "FILES_LINES_CALLED", HIGH_PRIORITY)
             .addClassAndMethod(this).addSourceLine(this));

So now we’ve found a bug, we report it. The BugInstance requires a description and a priority: passing in the detector is optional. The description must tie up with values defined in findbugs.xml and messages.xml, and is used to look up the failure message.

Once we’ve created a bug instance, we can then add some metadata: in this case, where the bug can be found. There’s an absolute ton of these, and they aren’t all always valid. For example, if you’re examining a field definition, then addClassAndMethod() will blow up because you’re not in a method. You’d expect that source line would always be available, but as it happens, it isn’t.

I’d like to give a coherent overview on what can be done when, but unfortunately, that’s not entirely compatible with my experiences. The one golden rule, as always, is to test any changes you make to ensure that you are getting sensible results out.

And that concludes this brief tour of this most trivial of detectors, and with it, the basics of how to interact with Findbugs. Coming up next: how to find some slightly more interesting bug patterns.