In part one of three, I covered the basics of making a quickfix pattern for a bug -
starting with parsing the Abstract Syntax Tree (AST) and ending with creating a new
ASTNode to replace the broken code.
In part two, this tutorial, we will be tackling a slightly more complicated bug that needs resolving:
Randoms are not super secure.
In cases where they need to be less guessable, it is better to either use a SecureRandom (at the cost of speed), or simply initialize a Random with a long from SecureRandom (mitigates time-based attacks and faster, but still not super secure).
The user will have to decide which one of the two suggested fixes applies, so we need to give them a choice. This tutorial will show how to offer two quickfixes as well as customizing the descriptions to give more information.
If you've already done the first part, you should have an Eclipse project
Otherwise, download this preconfigured Eclipse workspace and then install the dev version of FindBugs from
If you want to do this in hard mode, follow the dev setup instructions for fb-contrib quickfixes - it will amount to basically the same thing.
Just like last time, we'll start by modifying the plugin.xml file. The main difference you will notice is that we register two fixes for the same resolution. This will make both fix options pop up on the list of resolutions.
To make two separate fixes, we'll need to override
setOptions() to distinguish between them and then code in the separate fixes.
TypeBindings for the visitor implementation below. This method handles the parsing of any arguments you pass in. The arguments will be parsed as comma-separated, if you need more than one. And we simply use the parsed argument from above to determine the fix to apply. This call is from the FindBugs plugin.eclipse.quickfix.util.ASTUtil class and will add, if it doesn't already exist, a reference to SecureRandom. This is a simple way to create a constructor invocation of SecureRandom. One downside is that it won't deal with imports, potentially leaving a compilation error. Line 31 corrects this oversight. Makes a call to
new Random()Chains a newly created SecureRandom to a call of nextLong:
nextLong()Takes the output of the long generated from the
SecureRandomand uses that to initialize the
Randomobject we made in 42-44.
The only tricky part to this so far is the creation of the new ASTNodes. Remember that ClassInstanceCreations are when a constructor is called (i.e. a class instance is made). Do not be tempted by the (more logically named) ConstructorInvocation.
This visitor is very straight forward because we only have to look for when a Random is created.
The last piece is supplying custom descriptions, which simply involves overriding the aptly named getDescriptions method.
I recently started a project called fb-contrib quickfixes that focuses on creating quickfixes for FindBugs-detected bugs. At first, it was a bit difficult, because, to my knowledge, there isn't a nice friendly "how to make a quickfix" tutorial, so I thought I'd make one.
In this tutorial, we will make the following quickfix.
This is a good start if you want to contribute quickfixes yourself to the project or if you are interested in a hands-on exercise with abstract syntax trees (ASTs, for short - see below).
It fixes a bug called LSC_LITERAL_STRING_COMPARISON which, as you can see, is easily fixed by swapping the argument (the
String literal) and the
String variable (which could be null).
You will be writing software for Eclipse to perform the quickfixes, so I'll start discussing how Eclipse deals with Java source code - Abstract Syntax Trees (ASTs). The Wikipedia article covers it well, but basically, ASTs are a semantic representation of the source code - Eclipse knows what each line is, whether it's an if statement, a variable initialization statement, etc. Let's look at an example of some source code and its representative AST.
The outer-most ASTNode is a TypeDeclaration for the class
TypeDeclarations have some bookkeeping things like modifiers, a name, whether it's an interface or not, but we mostly care about the stuff inside of it, lines 3-7, which are the BodyDeclarations.
Remember, in a class, you can define fields, methods and other types, so that is what these are - an ordered list of all fields, methods and types defined in ForQuickFixes. This happens to just be a single MethodDeclaration that corresponds to our
MethodDeclarations can also have bookkeeping things, like a name, modifiers, a list of arguments (of type SingleVariableDeclaration), but if we are looking to fix Literal String Comparisons, we only really care about what goes on inside the method - the body. The body is a Block, which is basically a wrapper for a bunch of Statements.
are basically executable lines of code, so we run into them pretty frequently.
In fact, our code snippet has
statements, an IfStatement and an ExpressionStatement corresponding to
Statements generally have other Statements nested inside of them, as well as Expressions, the other key building block that you will encounter a lot. For example, the IfStatement is composed of other Statements and Expressions like this:
The Expression we are interested in is
args.equals("version"), which happens to be a MethodInvocation and it is currently the rightOperand of an InfixExpression.
This MethodInvocation has a name, an expression (aka the object upon which the method is invoked), and a
List<Type> of arguments.
The one argument is a StringLiteral.
Discovering, reasoning about, and navigating this AST is much easier (for me, at least) when I use a tool called ASTView, which is an Eclipse plugin.
An important tool for navigating the AST is the Visitor pattern. Basically what we will do is create an object which will be given to the AST. The AST will ping the object with notifications as it navigates the passed-in object through the entire the tree.
So, if we are interested in MethodInvocations, we will make the visiting object have a
visit(MethodInvocation node) method that will be called for every MethodInvocation in the AST, giving us a chance to poke around with all of the nodes.
For this tutorial, I'm providing an already setup Eclipse workspace, downloadable here. As of writing, the 3.0.0 version of the FindBugs plugin does not support extension, but a dev version does. Here is a compiled dev version (with fb-contrib bundled). Uninstall FindBugs from Eclipse (if already installed) and then unzip the bundle and install it from Eclipse. See http://stackoverflow.com/a/16074606/1447621 for more details. If you want to do this in hard mode, follow the dev setup instructions for fb-contrib quickfixes - it will amount to basically the same thing.
Read the two prerequisite sections above if you haven't already. You may also wish to install ASTView from above.
First, we need to start by registering our bug pattern to be fixed. We do this by updating the plugin.xml file, which is responsible for declaring how we are extending other Eclipse plugins (among other things).
Now, we'll make the class we promised to make in plugin.xml -
BugResolution, the superclass has two methods that we are required to overwrite.
Moreover, we will
true, but, if you find yourself not needing to verify types, return
false, to improve performance. This method will be called when the user has opted to fix the bug. So, given these three variables, we need to find the buggy code and create a fix. These three lines will appear in most every quickfix. What do they do? They find the node that contains the bug (line #12) and then traverse that node with the custom visitor (#13-14). This is our custom node visitor. ASTVisitor has a bunch of visit methods, but they are all basically empty by default. We'll override the ones we want next.
Now, we'll code the visitor.
The purpose of the visitor will be to find the incorrect
also identify the
String literal and the
String variable so they can be swapped.
Since we are looking for a
MethodInvocation to fix, we will override
visit(MethodInvocation) as seen below:
arguments()) are non-parameterized. When we cast them (e.g. to a
List<Expression>), this makes a compiler warning. This annotation silences said warning. The return value indicates whether or not you want to traverse the subtree (true is the default value for all visit() calls). We return false when we have found the MethodInvocation we are looking for, true otherwise. To see if the MethodInvocation we are looking at is the one we want to fix, we check to see if the name matches something in our list and the first (and only) argument has a literal value. It is technically possible for this logic to find (and then fix) the wrong method. For the purposes of this tutorial, I've decided to omit it, but if you want to see the type-checking version, see this gist.
Now that we've identified the problemmatic code, we'll need to make a fixed version and then replace it.
setExpression(visitor.stringLiteralExpression)would throw an
IllegalArgumentException. The code example shows two ways to deal with that - making a brand new node (lines 17-18) or using rewrite.createMoveTarget (lines 20+21). The third way I know of is with rewrite.createCopyTarget, but I don't know how this differs from the MoveTarget - they seem to do the same thing.
While I'd like to say this was all easily created, it was a bit of a struggle. ASTView helped some, but really what was useful was setting breakpoints, stepping through the various visitors and exploring the nodes with the Eclipse Debugging tools. Hopefully, this guide has made it easier to get a feel for the Eclipse quickfix platform than I had. Along the way, I did run into some issues that others should not repeat:
Foo f = new Foo()is a ClassInstanceCreation, not a ConstructorInvocation: ConstructorInvocations, as it turns out, refer to an invocation of constructor from a similar constructor like
this(someVar,otherVar). This was annoying and confusing for me when I was trying to deal with fixing constructors.
Stay tuned for the next iteration of this tutorial where we look to have a slightly more complicated example. Here's a sneak preview: