[ANN] InspectionGadgets 0.0.1 released


Announcing the first, beta release of InspectionGadgets, available at http://www.intellij.org/twiki/bin/view/Main/InspectionGadgets.

The InspectionGadgets plugin extends the IDEA inspection system, providing over 140 new code inspections and interactive warnings. These inspections provide insight into the small-scale structure of your code, find (and in some cases automatically fix) bugs and code weaknesses before compilation, assist in the enforcement of coding standards, and enable more productive code reviews. These code inspections are available both in batch mode, via the Tools/Inspect Code... panel, and in interactive mode, as "yellow-line" warnings during editing. The inspections implemented here have been chosen from popular coding standards, as well as from existing commercially available and open-source code inspection tools. It is hoped and planned that IDEA+InspectionGadgets will grow to become the most full-featured platform available for static analysis of Java and J2EE programs. Simply put, I want to kill a lot of bugs with this thing.

Inspections provided:

Naming conventions
Class naming convention (min. length, max. length, regex)
Interface naming convention (min. length, max. length, regex)
Instance variable naming convention (min. length, max. length, regex)
Static variable naming convention (min. length, max. length, regex)
Constant naming convention (min. length, max. length, regex)
Instance method naming convention (min. length, max. length, regex)
Static method naming convention (min. length, max. length, regex)
Local Variable naming convention (min. length, max. length, regex)
Parameter naming convention (min. length, max. length, regex)
Exception class name doesn't end with Exception
Non-exception class name ends with 'Exception'
Class name prefixed with package name
Method name same as class name
Method name same as parent class name

Probable bugs
Assignment used as condition
Statement with empty body
Covariant compareTo()
Covariant equals()
Floating point equality comparison
'compareto()' instead of 'compareTo()'
'hashcode()' instead of 'hashCode()'
Fallthrough in 'switch' statement
'switch' statement without 'default' branch
'default' not last case in 'switch'
Object comparison using ==, instead of '.equals()'(*)
String comparison using ==, instead of '.equals()'(*)
Result of InputStream.read() ignored
Loop statement that doesn't loop
Text label in 'switch' statement
Assignment to null
Octal and decimal integers in same array

Potentially confusing code constructs
Assignment to method parameter
Assignment to for-loop parameter
Nested assignment
'break' statement
'break' statement with label
'continue' statement
'continue' statement with label
Conditional expression (?:)
Nested conditional expression
Long literal ending with 'l' instead of 'L'(*)
Value of ++ or -- used
Switch statement with too many branches
Nested 'switch' statement
Chained method calls
Nested method calls

Abstraction issues
'instanceof' a concrete class
Collection declared by class, not interface
"Magic number" (i.e. numeric value used without declaration)

Class structure
Class may be interface
constructor not 'protected' in 'abstract' class(*)
Class without constructor
'final' method in 'final' class(*)
'public' constructor in non-'public' class(*)

Encapsulation issues
Public field
Package-visible field
Protected field
Return of Collection or array field
Assignment to Collection or array field from parameter

Visibility issues
Field names hides field in superclass
Inner class field hides outer class field
Parameter hides member variable
Local variable hides member variable

Finalization issues
finalize() not declared 'protected'(*)
finalize() doesn't call super.finalize()
finalize() called explicitly

Error handling
'catch' generic class
Empty 'catch' block
Empty 'finally' block
'throw' inside 'finally' block
'return' inside 'finally' block
'throw' generic class
Checked exception class
Unchecked exception class

Verbose or redundant code constructs
Redundant boolean comparison(*)
Assignment replacable with operator assignment(*)
Unnecessary 'if' statement(*)
Unnecessary parentheses(*)
Unnecessary 'this' qualifier(*)
Unnecessary interface modifier(*)
Unnecessary 'return' statement(*)
Unnecessary semicolon(*)

Code style issues
Statement bodies without {}(*)
Constant on left side of comparison(*)
Constant on right side of comparison(*)
expression.equals("literal") rather than "literal".equals(expression)
Missorted modifers(*)
C-style array declaration

Serialization issues
Serializable class without 'readObject()' and 'writeObject()'
Serializable class without serialVersionUID
'readObject()' or 'writeObject()' not declared 'private'(*)
'serialVersionUID' field not declared 'static final'(*)
'readResolve()' or 'writeReplace()' not declared 'protected'(*)

Threading issues
Double-checked locking
Call to notify() instead of notifyAll()
'wait()' not in loop
Call to Thread.run()
'synchronized' method
Synchronization on a non-final field
Nested 'synchronized' statement
Empty 'synchronized' statement

Method Metrics
Method with more than three negations
Method with multiple return points.
Method with too many parameters
Overly complex method
Overly nested method
Overly long method

Portability issues
Call to Runtime.exec()
Call to System.exit()
Call to System.getenv()
Hardcoded line separator
Hardcoded file separator
Native method

Internationalization issues
Hardcoded string literal
Character comparison
"Magic character" (i.e. character literal used without explicit declaration)
Call to Numeric .toString()
Call to Date.toString()
Call to Time.toString()
Call to String.compareTo()
Call to String.equalsIgnoreCase()
Call to String.equals()
String concatenation
Use of StringTokenizer

Performance issues
Field may be 'static'(*)
StringBuffer without initial capacity
Collection without initial capacity
String concatenation in loop
Multiply or divide by power of two(*)
Single character string concatenation(*)
Boolean constructor call(*)
Redundant String.toString()(*)
Redundant String constructor call(*)
StringBuffer.toString() in concatenation(*)
Tail recursion
Calls to System.gc() or Runtime.gc()

Code maturity issues
Use of System.out or System.err
Call to printStackTrace()

JUnit issues
Missing message on JUnit assertion
JUnit TestCase with non-trivial constructors
'setup()' instead of 'setUp()'
'teardown()' instead of 'tearDown()'
'suite()' method not declared 'static'

Please let me know of any problems, or ways to improve it.

42 comments
Comment actions Permalink

One more bug report. Class initializers aren't checked. Though the same is
true for most of embedded inspections. Just noticed.

--

Best regards,
Maxim Shafirov
JetBrains, Inc / IntelliJ Software
http://www.intellij.com
"Develop with pleasure!"


0
Comment actions Permalink

public class Foo {
public |
public int x;
}

ERROR - intellij.plugins.PluginManager -
ERROR - intellij.plugins.PluginManager - IntelliJ IDEA (Aurora)
Bu
ild #__BUILD_NUMBER__
ERROR - intellij.plugins.PluginManager - Internal version.
Compiled
29 July 2003 15:18
ERROR - intellij.plugins.PluginManager - JDK: 1.4.2-beta
ERROR - intellij.plugins.PluginManager - VM: Java HotSpot(TM)
Client
VM
ERROR - intellij.plugins.PluginManager - Vendor: Sun
Microsystems In
c.
ERROR - intellij.plugins.PluginManager - OS: Windows XP
ERROR - intellij.plugins.PluginManager - Last Action:
EditorComplete
Statement
ERROR - intellij.plugins.PluginManager -
java.lang.NullPointerException
at
com.siyeh.ig.style.MissortedModifiersInspection$MissortedModifiersVis
itor.isModifierListMissorted(MissortedModifiersInspection.java:193)
at
com.siyeh.ig.style.MissortedModifiersInspection$MissortedModifiersVis
itor.checkForMissortedModifiers(MissortedModifiersInspection.java:175)
at
com.siyeh.ig.style.MissortedModifiersInspection$MissortedModifiersVis
itor.visitField(MissortedModifiersInspection.java:169)
at
com.intellij.psi.impl.source.PsiFieldImpl.accept(PsiFieldImpl.java:33
5)
at
com.intellij.psi.impl.source.TreeWrapperPsiElement.acceptChildren(Tre
eWrapperPsiElement.java:63)
at
com.intellij.psi.PsiRecursiveElementVisitor.visitElement(PsiRecursive
ElementVisitor.java:7)
at
com.intellij.psi.JavaElementVisitor.visitClass(JavaElementVisitor.jav
a:47)
at
com.siyeh.ig.style.MissortedModifiersInspection$MissortedModifiersVis
itor.visitClass(MissortedModifiersInspection.java:137)
at
com.intellij.psi.impl.source.PsiClassImpl.accept(PsiClassImpl.java:39
2)
at com.siyeh.ig.ClassInspection.checkClass(ClassInspection.java:17)
at
com.intellij.codeInsight.daemon.impl.LocalInspectionsPass.collectInfo
rmation(LocalInspectionsPass.java:87)

--

Best regards,
Maxim Shafirov
JetBrains, Inc / IntelliJ Software
http://www.intellij.com
"Develop with pleasure!"



0
Comment actions Permalink

Dave Griffith wrote:
>> 1. String concatenation in loop:
>> The first '+' must be detected, the secord one shouldn't.
>

Yeah, I thought about this, but didn't see a particularly good way to
auto-detect. Is is just that you're using the concatenation in an
assignment? Harumph.

Maybe just add an option to ignore if the string value is declared within
the loop e.g. a literal, and therefore can never be worked on by the next
iteration of the loop?

N.


0
Comment actions Permalink

In the following, I get a "local variable hides member variable":

public class Foo {

private int bar;

public static void fooIt() {
int bar = 1; // marked as hiding this.bar
...
}
}

But the instance member "bar" is not really in scope in a static method, no?

0
Comment actions Permalink

Is there any way in which "Parameter hides member variable" can be
ignored when the following idiom is used?

private AccountData(Account account, boolean lockForUpdate) {
this.account = account;
this.lockForUpdate = lockForUpdate;
}

We don't use any prefix for member variables (since idea and all other
ide's make clear the scope of a variable without prefixes).

0
Comment actions Permalink


Hmmm. It would be trivial to not report instance variables inside static methods which hide instance variables. However, to the extent that the reason for these inspections is to highlight confusing and potentially bug-prone code, I would say that this sort of construct more than qualifies as worthy of reporting. I could put on the list a request to have a flag for this sort of thing, but unless I hear a groundswell of complaint it's likely to be a pretty low priority.

0
Comment actions Permalink


I do see this sort of construct a lot, and while I really strongly dislike it (preferring to use variable naming prefixes), I could certainly support it. Would a "Ignore for constructor parameters" checkbox be adequate, or would an "Ignore for setter parameters" be needed as well? The first is decidedly easier to implement than the second.

0
Comment actions Permalink

Dave Griffith wrote:

I would say that this sort
of construct more than qualifies as worthy of reporting.


I agree completely. Given some more code that obscures the
declaration(s), it would be easy for me to make a mistake and think that
I'm using a static field "bar" even though that field isn't really
static. But maybe I'm easily confused or something :)

0
Comment actions Permalink


Dave Griffith wrote:

Hmmm. It would be trivial to not report instance variables inside static methods which hide instance variables. However, to the extent that the reason for these inspections is to highlight confusing and potentially bug-prone code, I would say that this sort of construct more than qualifies as worthy of reporting. I could put on the list a request to have a flag for this sort of thing, but unless I hear a groundswell of complaint it's likely to be a pretty low priority.


Ok, I buy that. Probably the static-local variable should have a
different name than the class member. But its unclear what the possible
bug might be, other than thinking the method you are coding is not static.

0
Comment actions Permalink


Dave Griffith wrote:

I do see this sort of construct a lot, and while I really strongly dislike it (preferring to use variable naming prefixes), I could certainly support it. Would a "Ignore for constructor parameters" checkbox be adequate, or would an "Ignore for setter parameters" be needed as well? The first is decidedly easier to implement than the second.


Well, when not using prefixes both are really required.

Speaking on prefixes, we used to use them before we got heavy into
testing. Now those types of silly errors simply don't happen anymore
(well, not for long anyway). I believe idea even flags assigment to self
as a "silly assignment". So in the end, prefixes are just noise.

0
Comment actions Permalink

With version 0.0.2 I'm still getting a noise problem here - try the code:

public class Test extends JComponent
{
public void repaint(long tm, int x, int y, int width, int height)
{
super.repaint(tm, x, y, width, height);
}
}

I get a warning on x, y, width and height even though they're package
private variables of Component. Is this the situation you're describing in
the second part of your post?

Cheers,
N.

Dave Griffith wrote:

This is a bug. I was already filtering out "private" variables from
being hidden by locals and parameters. I'll put on the list to
filter out package-visible variables if they are in a different
package.

>

Also, there's no filtering on hiding by subclass or inner class,
intentionally. If you have a private instance variable which has the
same name as the private instance variable of your parent, it'll be
flagged. I had thought this was the desired behaviour, but am
willing to take arguments on it.

>

Thanks!



0
Comment actions Permalink

Nope, it's a bug on my part. Thanks for catching it.

--Dave Griffith

0

Please sign in to leave a comment.