Extending checkstyle to detect objetc comparison with == operator.

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Extending checkstyle to detect objetc comparison with == operator.

benoit.delayen
Hello,

I am currently working on a project which was partly developped by java newbies and we encountered several bugs which had for root cause object comparison using == instead of .equals().

Those objects (1 class + 1 subclass) are from a cached repository so they would work for unit testing but would start showing erratic behavior after cache invalidations.

I wanted to use checkstyle on this project to mainly improve code quality and enforce standards but i would also like to extend it to detect those erroneous comparisons.

Bad code would look like

RepositoryItem item1 = ...
RepositoryItem item2 = ...

if (item1 == item2) {
    doCriticalStuff();
}

Is it possible to extend checkstyle to detect this ?

I have started working on a check class (largely inspired from StringLiteralEquality) but i am unsure on how to check the instance type. I have the AST items ready (aka. getFirstChild() + getFirstChild().nextSibling() ). But i dont know how to proceed to detect that they are instances of RepositoryItem or one of their subclass.

I would appreciate any help on this matter.


Thanks in advance,
Benoit.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Checkstyle-user mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/checkstyle-user
Reply | Threaded
Open this post in threaded view
|

Re: Extending checkstyle to detect objetc comparison with == operator.

Oliver Burn
I would recommend using FindBugs for these types of bugs.

On Fri, Sep 26, 2008 at 01:14, <[hidden email]> wrote:
Hello,

I am currently working on a project which was partly developped by java newbies and we encountered several bugs which had for root cause object comparison using == instead of .equals().

Those objects (1 class + 1 subclass) are from a cached repository so they would work for unit testing but would start showing erratic behavior after cache invalidations.

I wanted to use checkstyle on this project to mainly improve code quality and enforce standards but i would also like to extend it to detect those erroneous comparisons.

Bad code would look like

RepositoryItem item1 = ...
RepositoryItem item2 = ...

if (item1 == item2) {
   doCriticalStuff();
}

Is it possible to extend checkstyle to detect this ?

I have started working on a check class (largely inspired from StringLiteralEquality) but i am unsure on how to check the instance type. I have the AST items ready (aka. getFirstChild() + getFirstChild().nextSibling() ). But i dont know how to proceed to detect that they are instances of RepositoryItem or one of their subclass.

I would appreciate any help on this matter.


Thanks in advance,
Benoit.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Checkstyle-user mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/checkstyle-user


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Checkstyle-user mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/checkstyle-user
Reply | Threaded
Open this post in threaded view
|

RE : Extending checkstyle to detect objetc comparison with == operator.

Arnaud Roques-2
In reply to this post by benoit.delayen
Hello,

In http://checkstyle.sourceforge.net/5.x/writingchecks.html, about limitations :
There are basically only two of them:
  • You cannot determine the type of an expression.
  • You cannot see the content of other files.
And indeed, it is very difficult to determine the type of an expression...
So in your example,

if (item1==item2) {..}

It is difficult to know which are the type of item1 and item2. Difficult, but not impossible, if item1 and/or item2 are local variable or fields of the current class, you *can* determine the type.

But in :

if (someObject.foo1() == item2) {...}
You will not be able to determine the type of foo1().


[hidden email] a écrit :
Hello,

I am currently working on a project which was partly developped by java newbies and we encountered several bugs which had for root cause object comparison using == instead of .equals().

Those objects (1 class + 1 subclass) are from a cached repository so they would work for unit testing but would start showing erratic behavior after cache invalidations.

I wanted to use checkstyle on this project to mainly improve code quality and enforce standards but i would also like to extend it to detect those erroneous comparisons.

Bad code would look like

RepositoryItem item1 = ...
RepositoryItem item2 = ...

if (item1 == item2) {
doCriticalStuff();
}

Is it possible to extend checkstyle to detect this ?

I have started working on a check class (largely inspired from StringLiteralEquality) but i am unsure on how to check the instance type. I have the AST items ready (aka. getFirstChild() + getFirstChild().nextSibling() ). But i dont know how to proceed to detect that they are instances of RepositoryItem or one of their subclass.

I would appreciate any help on this matter.


Thanks in advance,
Benoit.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Checkstyle-user mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/checkstyle-user


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Checkstyle-user mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/checkstyle-user

Reply | Threaded
Open this post in threaded view
|

RE : Extending checkstyle to detect objetc comparison with == operator.

Arnaud Roques-2
In reply to this post by benoit.delayen
Hello,

You can also use regular expression to do the job.

Indeed, you want to detect:

SomeClass variable [...anything...] variable == anythingButNULL

So if you assume that class name begins with an uppercase, variable with a lowercase, you can use:

^ *(?:[A-Z]\w+)\b +([a-z]\w*)\b[\s\S]*?\1 *==(?!null)(?! null)

And also:
SomeClass variable [...anything...] anythingButNULL == variable

^ *(?:[A-Z]\w+)\b +([a-z]\w*)\b[\s\S]*?==(?<!null==)(?<!null ==) *\1(?!\.)

I've attached a file with the correct module definition.



[hidden email] a écrit :
Hello,

I am currently working on a project which was partly developped by java newbies and we encountered several bugs which had for root cause object comparison using == instead of .equals().

Those objects (1 class + 1 subclass) are from a cached repository so they would work for unit testing but would start showing erratic behavior after cache invalidations.

I wanted to use checkstyle on this project to mainly improve code quality and enforce standards but i would also like to extend it to detect those erroneous comparisons.

Bad code would look like

RepositoryItem item1 = ...
RepositoryItem item2 = ...

if (item1 == item2) {
doCriticalStuff();
}

Is it possible to extend checkstyle to detect this ?

I have started working on a check class (largely inspired from StringLiteralEquality) but i am unsure on how to check the instance type. I have the AST items ready (aka. getFirstChild() + getFirstChild().nextSibling() ). But i dont know how to proceed to detect that they are instances of RepositoryItem or one of their subclass.

I would appreciate any help on this matter.


Thanks in advance,
Benoit.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Checkstyle-user mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/checkstyle-user


                <module name="Regexp">
                        <property name="format"
                                value="^ *(?:[A-Z]\w+)\b +([a-z]\w*)\b[\s\S]*?\1 *==(?!null)(?! null)" />
                        <property name="message" value="Suspect use of == with this variable latter" />
                        <property name="ignoreComments" value="true" />
                        <property name="illegalPattern" value="true" />
                </module>

                <module name="Regexp">
                        <property name="format"
                                value="^ *(?:[A-Z]\w+)\b +([a-z]\w*)\b[\s\S]*?==(?&lt;!null==)(?&lt;!null ==) *\1(?!\.)" />
                        <property name="message" value="Suspect use of == with this variable latter" />
                        <property name="ignoreComments" value="true" />
                        <property name="illegalPattern" value="true" />
                </module>

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Checkstyle-user mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/checkstyle-user

Reply | Threaded
Open this post in threaded view
|

Extending checkstyle 4.4

Arnaud Roques-2
Hello,
Checkstyle is really a great tool to improve code quality, and I've developped some checkers that other people may find usefull. (They have been used with JDK1.4).
If you have some idea to add in this list, please tell me.

I also apologize for the long post, but is there a better place to share checkers?



AvoidNegativeLogic 
 This checker avoid the usage of inverted logic. It looks for code like
 if (a != null) {
        System.out.println("not null");
 } else {
        System.out.println("null");
 }
 
and promotes the usage of
 if (a == null) {
        System.out.println("null");
 } else {
        System.out.println("not null");
 }
 
 

CollapseIfStatement 
 This checker looks for if statement with can be simplified.
For example:
 if (a != null) {
        if (b == null) {
                 System.err.println();
        }
 }
 
Can be simplified in:
 if (a != null && b == null) {
        System.err.println();
 }
 
 

DeclareVariableInItsBloc
Variable should be declared in the bloc that uses it.
For example:
 Object dummy;
 for (Iterator it = list.iterator(); it.hasNext();) {
        dummy = it.next();
        ...
 }
 (dummy not used anymore)
 
Can be changed in:
 for (Iterator it = list.iterator(); it.hasNext();) {
        Object dummy = it.next();
        ...
 }

 

HiddenCatchBloc
Very often, people catch exception and do not use the exception itself.
Example:
 try {
        ...
 } catch (IOException e) {
        return null;
 }
 
May be we should at least log the exception:
 try {
        ...
 } catch (IOException e) {
        log.error("IOError ", e);
        return null;
 }
 

ImmutableField
This checker detects private fields that are unused in a class and promotes the usage of final for fields that are never changed.
 

ImportControl
This check control that import are valid into a java file, depending on its package.
Example, to control that classes into com.foo2 (and subpackages) does not import com.foo1:
 <module name="cs.ImportControl">
        <property name="importThis" value="com\.foo1" />
        <property name="notByPackages" value="com\.foo2" />
 </module>
 
Another example, to control that only classes into com.dummy2 (and subpackages) does import com.dummy1:
 <module name="cs.ImportControl">
        <property name="importThis" value="com\.dummy1" />
       
 <property name="onlyByPackages" value="com\.dummy2" />
 </module>
 

InstanceofIsNeverNull
When an object is an instanceof something, it cannot be null.
So the following code :
if (obj != null && obj instanceof List) {...}
should be changed into:
if (obj instanceof List) {...}

NewIsNeverNull
When an object is created with new, it is never null.
So the following code :
 MyClass obj = new MyClass();
 if (obj != null) {
        // Some code
 }
 
should be changed into:
 
 MyClass obj = new MyClass();
 // Some code

PatternFilter
This filter looks like SuppressionFilter, but the configuration is done in the filter itself, and not into another XML file.
 <module name="Checker">
        <module
 name="cs.PatternFilter">
                 <property name="ignoreFiles"
                         value="(.*(&lt;=Managed)(&lt;=View)Bean\.java)|(.*Home\.java)" />
                 <property name="modules" value="TabCharacter|WhitespaceAround" />
        </module>
        <module name="TreeWalker">
                 ...
        </module>
 </module>
 

TooEarlyVariableDeclaration
This checker try to detect too early variable declaration:
 
int i = 0, j = 4; // Error: Declare variables on separate lines
 
int i;
i = 4; // those two lines can be merged into "int i = 4"
 
int i = 0; // this affectation is useless
i = 4;
 
int i; // Declare variable as close as possible to where they are used
[...A lot of code that does NOT use i...]
i = 6;

UnitTestMethods
This checker is really great for Test Driven Developpement. It checks that every non private method of a class is JUnit tested.
Example of module configuration:
 <module name="cs.UnitTestMethods">
        <property name="severity" value="warning" />
        <property name="filesToCheck"
                 value="^(.*[/\\])src([/\\]dummy[/\\]\w+)\.java$" />
        <property name="testCases"
                 value="$1test$2Test.java" />
        <property name="minSemicolon"
                 value="2" />
 </module>
 
This example looks for every Java file in src/dummy or src\dummy. Non-private methods that have more than 2 semicolons must have a test method. The TestCase must be in test/dummy directory.

UnnecessarilyElse
This checker detects unnecessarily else statement. Example:
 if (a==null) {
        // some code
        return null;
 } else {
        // other things
 }
 
which can be changed into:
 if (a==null) {
        // some code
        return null;
 }
 // other things

UnnecessaryParentheses
This checker detects some unnecessary parentheses which are not detected by the standard check of CheckStyle. Up to now, it only deals with ==,!=,&& and ||
It detects the following code:
((a==null) && (b!=null))
 
which is better read this way (except for people coming from LISP :-)
 
(a==null && b!=null)

UnusedLocalVariable
This checker detects unused local variable. The detection is not 100% accurate.
  • If the checker declares a variable as unused, it means that the variable is really unused.
  • But in some (rare) cases, an unused variable can be undetected by the checker. For example, if you call the variable foo and that you also have some method called foo() used.


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Checkstyle-user mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/checkstyle-user


cs.jar (77K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Extending checkstyle 4.4

Lars Kühne-3
Hi Arnaud,

wow, this list looks really impressive. If you're comfortable sharing the the sources under LGPL and including our standard license header in your source files, you might want to contribute your code to our patch tracker, so they could end up in the normal checkstyle distribution. Just make sure that you follow the guidelines in our "contributing" document.

I am not aware of any other place to share checkers.

Cheers,
Lars

On Mon, Sep 29, 2008 at 6:00 PM, Arnaud Roques wrote:
Hello,
Checkstyle is really a great tool to improve code quality, and I've developped some checkers that other people may find usefull. (They have been used with JDK1.4).
If you have some idea to add in this list, please tell me.


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Checkstyle-user mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/checkstyle-user