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

Avatar

Thomas Johnson

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.

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

Any opinions, news, research, analyses, prices or other information ("information") contained on this Blog, constitutes marketing communication and it has not been prepared in accordance with legal requirements designed to promote the independence of investment research. Further, the information contained within this Blog does not contain (and should not be construed as containing) investment advice or an investment recommendation, or an offer of, or solicitation for, a transaction in any financial instrument. LMAX Exchange has not verified the accuracy or basis-in-fact of any claim or statement made by any third parties as comments for every Blog entry.

LMAX Exchange will not accept liability for any loss or damage, including without limitation to, any loss of profit, which may arise directly or indirectly from use of or reliance on such information. No representation or warranty is given as to the accuracy or completeness of the above information. While the produced information was obtained from sources deemed to be reliable, LMAX Exchange does not provide any guarantees about the reliability of such sources. Consequently any person acting on it does so entirely at his or her own risk. It is not a place to slander, use unacceptable language or to promote LMAX Exchange or any other FX, Spread Betting and CFD provider and any such postings, excessive or unjust comments and attacks will not be allowed and will be removed from the site immediately.

LMAX Exchange will clearly identify and mark any content it publishes or that is approved by LMAX Exchange.

FX and CFDs are leveraged products that can result in losses exceeding your deposit. They are not suitable for everyone so please ensure you fully understand the risks involved. The information on this website is not directed at residents of the United States of America, Australia (we will only deal with Australian clients who are "wholesale clients" as defined under the Corporations Act 2001), Canada (although we may deal with Canadian residents who meet the "Permitted Client" criteria), Singapore or any other jurisdiction where FX trading and/or CFD trading is restricted or prohibited by local laws or regulations.