Merge pull request #6 from tpounds/master

Add new LO detector, Fix bugs in commons-lang 2.x detectors, Fix typos in messages.xml
mebigfatguy · Feb 12, 2013 · c27e5be · c27e5be
2 parents 752c9b5 + 24d2e7e
commit c27e5be
Show file tree

Hide file tree

Showing 5 changed files with 28 additions and 6 deletions.
diff --git a/etc/findbugs.xml b/etc/findbugs.xml
@@ -165,7 +165,7 @@
 
 	<Detector class="com.mebigfatguy.fbcontrib.detect.SuspiciousClusteredSessionSupport" speed="fast" reports="SCSS_SUSPICIOUS_CLUSTERED_SESSION_SUPPORT" />
 
-	<Detector class="com.mebigfatguy.fbcontrib.detect.LoggerOddities" speed="fast" reports="LO_SUSPECT_LOG_CLASS,LO_SUSPECT_LOG_PARAMETER,LO_STUTTERED_MESSAGE" />
+	<Detector class="com.mebigfatguy.fbcontrib.detect.LoggerOddities" speed="fast" reports="LO_LOGGER_LOST_EXCEPTION_STACK_TRACE,LO_SUSPECT_LOG_CLASS,LO_SUSPECT_LOG_PARAMETER,LO_STUTTERED_MESSAGE" />
 
 	<Detector class="com.mebigfatguy.fbcontrib.detect.IncorrectInternalClassUse" speed="fast" reports="IICU_INCORRECT_INTERNAL_CLASS_USE" />
 
@@ -356,6 +356,7 @@
 	<BugPattern abbrev="SCA" type="SCA_SUSPICIOUS_CLONE_ALGORITHM" category="CORRECTNESS" />
 	<BugPattern abbrev="WEM" type="WEM_WEAK_EXCEPTION_MESSAGING" category="STYLE" />
 	<BugPattern abbrev="SCSS" type="SCSS_SUSPICIOUS_CLUSTERED_SESSION_SUPPORT" category="CORRECTNESS" />
+   <BugPattern abbrev="LO" type="LO_LOGGER_LOST_EXCEPTION_STACK_TRACE" category="CORRECTNESS" />
 	<BugPattern abbrev="LO" type="LO_SUSPECT_LOG_CLASS" category="CORRECTNESS" />
 	<BugPattern abbrev="LO" type="LO_SUSPECT_LOG_PARAMETER" category="CORRECTNESS" />
 	<BugPattern abbrev="LO" type="LO_STUTTERED_MESSAGE" category="STYLE" />

diff --git a/etc/messages.xml b/etc/messages.xml
@@ -2815,6 +2815,19 @@
 		</Details>
 	</BugPattern>
 
+   <BugPattern type="LO_LOGGER_LOST_EXCEPTION_STACK_TRACE">
+      <ShortDescription>Method incorrectly passes exception as first argument to logger method</ShortDescription>
+      <LongDescription>Method {1} incorrectly passes exception as first argument to logger method</LongDescription>
+      <Details>
+         <![CDATA[
+         <p>This method passes an exception as the first argument to a logger method. The stack
+         trace is potentially lost due to the logger emitting the exception using toString(). It
+         is better to construct a log message with sufficient context and pass the exception as
+         the second argument to capture the stack trace.</p>
+         ]]>
+      </Details>
+   </BugPattern>
+
 	<BugPattern type="LO_SUSPECT_LOG_CLASS">
 		<ShortDescription>Method specifies an unrelated class when allocating a Logger</ShortDescription>
 		<LongDescription>Method {1} specifies an unrelated class when allocating a Logger</LongDescription>
@@ -3096,7 +3109,7 @@
 		<Details>
     			<![CDATA[
     			<p>This method tests a field to make sure it's not null before executing a conditional block of
-    			code. However in the conditional block is reassigns the field. It is likely that the guard
+    			code. However in the conditional block it reassigns the field. It is likely that the guard
     			should have been a check to see if the field is null, not that the field was not null.</p>
     			]]>
     		</Details>
@@ -3108,7 +3121,7 @@
 		<Details>
     			<![CDATA[
     			<p>This method tests a local variable to make sure it's not null before executing a conditional block of
-    			code. However in the conditional block is reassigns the local variable. It is likely that the guard
+    			code. However in the conditional block it reassigns the local variable. It is likely that the guard
     			should have been a check to see if the local variable is null, not that the local variable was not null.</p>
     			]]>
     		</Details>

diff --git a/src/com/mebigfatguy/fbcontrib/detect/CommonsEqualsBuilderToEquals.java b/src/com/mebigfatguy/fbcontrib/detect/CommonsEqualsBuilderToEquals.java
@@ -81,7 +81,7 @@ public void sawOpcode(int seen) {
 					String calledClass = stack.getStackItem(1).getSignature();
 					if ("Lorg/apache/commons/lang3/builder/EqualsBuilder;"
 							.equals(calledClass)
-							|| "org/apache/commons/lang/builder/EqualsBuilder"
+							|| "Lorg/apache/commons/lang/builder/EqualsBuilder;"
 									.equals(calledClass)) {
 						bugReporter.reportBug(new BugInstance(this,
 								"CEBE_COMMONS_EQUALS_BUILDER_ISEQUALS",

diff --git a/src/com/mebigfatguy/fbcontrib/detect/CommonsHashcodeBuilderToHashcode.java b/src/com/mebigfatguy/fbcontrib/detect/CommonsHashcodeBuilderToHashcode.java
@@ -81,7 +81,7 @@ public void sawOpcode(int seen) {
 					String calledClass = stack.getStackItem(0).getSignature();
 					if ("Lorg/apache/commons/lang3/builder/HashCodeBuilder;"
 							.equals(calledClass)
-							|| "org/apache/commons/lang/builder/HashCodeBuilder"
+							|| "Lorg/apache/commons/lang/builder/HashCodeBuilder;"
 									.equals(calledClass)) {
 						bugReporter.reportBug(new BugInstance(this,
 								"CHTH_COMMONS_HASHCODE_BUILDER_TOHASHCODE",

diff --git a/src/com/mebigfatguy/fbcontrib/detect/LoggerOddities.java b/src/com/mebigfatguy/fbcontrib/detect/LoggerOddities.java
@@ -266,7 +266,15 @@ public void sawOpcode(int seen) {
                 					}
                 				}
                 			}
-                		}
+                     } else if ("(Ljava/lang/Object;)V".equals(sig)) {
+                        final JavaClass clazz = stack.getStackItem(0).getJavaClass();
+                        if(clazz.instanceOf(THROWABLE_CLASS)) {
+                           bugReporter.reportBug(new BugInstance(this, "LO_LOGGER_LOST_EXCEPTION_STACK_TRACE", NORMAL_PRIORITY)
+                                 .addClass(this)
+                                 .addMethod(this)
+                                 .addSourceLine(this));
+                        }
+                     }
                 	}
                 }
             }