add new SLF4J_GET_STACK_TRACE detector (fixes #70)

CHANGELOG.md

@@ -7,6 +7,7 @@ This changelog follows [Keep a Changelog v1.0.0](https://keepachangelog.com/en/1
 ### Added
 
 - Support for new SonarQube v6.7 LTS
+- add new SLF4J_GET_STACK_TRACE bug pattern
 
 ### Changed
 

README.md

@@ -192,6 +192,26 @@ class Foo {
 }
 ```
 
+## SLF4J_GET_STACK_TRACE
+
+This pattern reports on the use of `Throwable#getStackTrace()`.  This is most
+typically wrong and a misunderstanding of the slf4j API, as normally you just
+provide the Throwable instance as the last argument, instead of using getStackTrace().
+
+```java
+class Foo {
+    private final Logger logger = LoggerFactory.getLogger(getClass());
+    void method() {
+        // invalid: needless 'e.getStackTrace()'
+        logger.info("Error occured. Stack is {}", e.getStackTrace());
+
+        // valid
+        logger.info("Error occured.", e);
+    }
+}
+```
+
+
 # How to use with Maven
 
 To use this product, please configure your spotbugs-maven-plugin like below.

bug-pattern/.gitignore

@@ -0,0 +1 @@
+/.apt_generated_tests/

bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/ManualGetStackTraceDetector.java

@@ -0,0 +1,38 @@
+package jp.skypencil.findbugs.slf4j;
+
+import static org.apache.bcel.Const.INVOKEVIRTUAL;
+
+import com.google.common.base.Objects;
+import edu.umd.cs.findbugs.BugReporter;
+import edu.umd.cs.findbugs.OpcodeStack.Item;
+import jp.skypencil.findbugs.slf4j.parameter.AbstractExtendedDetectorForParameterArray;
+
+/**
+ * FindBugs (now SpotBugs) Detector for (ab)use of {@link Throwable#getStackTrace()} in SFL4j logger.
+ *
+ * @author Michael Vorburger.ch
+ */
+public class ManualGetStackTraceDetector extends AbstractExtendedDetectorForParameterArray {
+
+    @Item.SpecialKind
+    private final int getStackTraceKind = Item.defineNewSpecialKind("use of throwable getStackTrace");
+
+    public ManualGetStackTraceDetector(BugReporter bugReporter) {
+        super(bugReporter, "SLF4J_GET_STACK_TRACE");
+    }
+
+    @Override
+    @Item.SpecialKind
+    protected int getKindOfInterest() {
+        return getStackTraceKind;
+    }
+
+    @Override
+    protected boolean isWhatWeWantToDetect(int seen) {
+        return seen == INVOKEVIRTUAL
+                && !stack.isTop()
+                && getThrowableHandler().checkThrowable(getStack().getStackItem(0))
+                && Objects.equal("getStackTrace", getNameConstantOperand());
+    }
+
+}

bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/ManualMessageDetector.java

@@ -3,74 +3,40 @@
 import static org.apache.bcel.Const.INVOKEVIRTUAL;
 
 import com.google.common.base.Objects;
-import edu.umd.cs.findbugs.BugInstance;
 import edu.umd.cs.findbugs.BugReporter;
 import edu.umd.cs.findbugs.OpcodeStack.CustomUserValue;
 import edu.umd.cs.findbugs.OpcodeStack.Item;
-import javax.annotation.Nullable;
-import jp.skypencil.findbugs.slf4j.parameter.AbstractDetectorForParameterArray;
+import jp.skypencil.findbugs.slf4j.parameter.AbstractExtendedDetectorForParameterArray;
 import jp.skypencil.findbugs.slf4j.parameter.ArrayData;
-import jp.skypencil.findbugs.slf4j.parameter.ArrayDataHandler.Strategy;
 
 @CustomUserValue
-public final class ManualMessageDetector extends AbstractDetectorForParameterArray {
+public final class ManualMessageDetector extends AbstractExtendedDetectorForParameterArray {
+
   @Item.SpecialKind
   private final int isMessage = Item.defineSpecialKind("message generated by throwable object");
 
   public ManualMessageDetector(BugReporter bugReporter) {
-    super(bugReporter);
+    super(bugReporter, "SLF4J_MANUALLY_PROVIDED_MESSAGE");
   }
 
   @Override
-  protected Strategy createArrayCheckStrategy() {
-    return new Strategy() {
-      @Override
-      public void store(Item storedItem, ArrayData arrayData, int index) {
-        if (arrayData == null) {
-          return;
-        }
-
-        if (storedItem.getSpecialKind() == isMessage) {
-          arrayData.mark(true);
-        }
-
-        // Let developer logs exception message, only when argument does not have throwable instance
-        // https://github.com/KengoTODA/findbugs-slf4j/issues/31
-        if (index == arrayData.getSize() - 1 && !getThrowableHandler().checkThrowable(storedItem)) {
-          arrayData.mark(false);
-        }
-      }
-    };
+  protected int getKindOfInterest() {
+    return isMessage;
   }
 
   @Override
-  public void afterOpcode(int seen) {
-    boolean isInvokingGetMessage = isInvokingGetMessage(seen);
-    super.afterOpcode(seen);
-
-    if (isInvokingGetMessage && !stack.isTop()) {
-      stack.getStackItem(0).setSpecialKind(isMessage);
-    }
-  }
-
-  private boolean isInvokingGetMessage(int seen) {
-    return seen == INVOKEVIRTUAL
-        && !stack.isTop()
-        && getThrowableHandler().checkThrowable(getStack().getStackItem(0))
-        && (Objects.equal("getMessage", getNameConstantOperand())
-            || Objects.equal("getLocalizedMessage", getNameConstantOperand()));
+  protected boolean isReallyOfInterest(Item storedItem, ArrayData arrayData, int index) {
+      // Let developer logs exception message, only when argument does not have throwable instance
+      // https://github.com/KengoTODA/findbugs-slf4j/issues/31
+      return !(index == arrayData.getSize() - 1
+              && !getThrowableHandler().checkThrowable(storedItem));
   }
 
   @Override
-  protected void onLog(@Nullable String format, @Nullable ArrayData arrayData) {
-    if (arrayData == null || !arrayData.isMarked()) {
-      return;
+  protected boolean isWhatWeWantToDetect(int seen) {
+    return seen == INVOKEVIRTUAL && !stack.isTop()
+      && getThrowableHandler().checkThrowable(getStack().getStackItem(0))
+      && (Objects.equal("getMessage", getNameConstantOperand())
+        || Objects.equal("getLocalizedMessage", getNameConstantOperand()));
     }
-    BugInstance bugInstance =
-        new BugInstance(this, "SLF4J_MANUALLY_PROVIDED_MESSAGE", NORMAL_PRIORITY)
-            .addSourceLine(this)
-            .addClassAndMethod(this)
-            .addCalledMethod(this);
-    getBugReporter().reportBug(bugInstance);
-  }
 }

bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/parameter/AbstractExtendedDetectorForParameterArray.java

@@ -0,0 +1,65 @@
+package jp.skypencil.findbugs.slf4j.parameter;
+
+import edu.umd.cs.findbugs.BugInstance;
+import edu.umd.cs.findbugs.BugReporter;
+import edu.umd.cs.findbugs.OpcodeStack.Item;
+import javax.annotation.Nullable;
+import jp.skypencil.findbugs.slf4j.parameter.ArrayDataHandler.Strategy;
+
+public abstract class AbstractExtendedDetectorForParameterArray extends AbstractDetectorForParameterArray {
+
+    private final String bugPatternName;
+
+    public AbstractExtendedDetectorForParameterArray(BugReporter bugReporter, String bugPatternName) {
+      super(bugReporter);
+      this.bugPatternName = bugPatternName;
+    }
+
+    @Override
+    protected final Strategy createArrayCheckStrategy() {
+      return (storedItem, arrayData, index) -> {
+        if (arrayData == null) {
+          return;
+        }
+
+        if (storedItem.getSpecialKind() == getKindOfInterest()) {
+          arrayData.mark(true);
+        }
+
+        if (!isReallyOfInterest(storedItem, arrayData, index)) {
+          arrayData.mark(false);
+        }
+      };
+    }
+
+    protected boolean isReallyOfInterest(Item storedItem, ArrayData arrayData, int index) {
+        return true;
+    }
+
+    @Override
+    public final void afterOpcode(int seen) {
+      boolean isInvokingGetMessage = isWhatWeWantToDetect(seen);
+      super.afterOpcode(seen);
+
+      if (isInvokingGetMessage && !stack.isTop()) {
+        stack.getStackItem(0).setSpecialKind(getKindOfInterest());
+      }
+    }
+
+    @Override
+    protected final void onLog(@Nullable String format, @Nullable ArrayData arrayData) {
+      if (arrayData == null || !arrayData.isMarked()) {
+        return;
+      }
+      BugInstance bugInstance = new BugInstance(this,
+            bugPatternName, NORMAL_PRIORITY)
+            .addSourceLine(this).addClassAndMethod(this)
+            .addCalledMethod(this);
+      getBugReporter().reportBug(bugInstance);
+    }
+
+    abstract protected boolean isWhatWeWantToDetect(int seen);
+
+    abstract protected int getKindOfInterest();
+
+}

bug-pattern/src/main/resources/bugrank.txt

@@ -7,3 +7,4 @@
 0 BugPattern SLF4J_ILLEGAL_PASSED_CLASS
 0 BugPattern SLF4J_SIGN_ONLY_FORMAT
 0 BugPattern SLF4J_MANUALLY_PROVIDED_MESSAGE
+0 BugPattern SLF4J_GET_STACK_TRACE

bug-pattern/src/main/resources/findbugs.xml

@@ -11,6 +11,8 @@
         <Detector class="jp.skypencil.findbugs.slf4j.StaticLoggerDetector" reports="SLF4J_LOGGER_SHOULD_BE_NON_STATIC" speed="fast" />
         <Detector class="jp.skypencil.findbugs.slf4j.IllegalPassedClassDetector" reports="SLF4J_ILLEGAL_PASSED_CLASS" speed="fast" />
         <Detector class="jp.skypencil.findbugs.slf4j.ManualMessageDetector" reports="SLF4J_MANUALLY_PROVIDED_MESSAGE,SLF4J_FORMAT_SHOULD_BE_CONST" speed="fast" />
+        <Detector class="jp.skypencil.findbugs.slf4j.ManualGetStackTraceDetector" reports="SLF4J_GET_STACK_TRACE" speed="fast" />
+
         <BugPattern type="SLF4J_PLACE_HOLDER_MISMATCH" abbrev="SLF4J" category="CORRECTNESS" />
         <BugPattern type="SLF4J_FORMAT_SHOULD_BE_CONST" abbrev="SLF4J" category="CORRECTNESS" />
         <BugPattern type="SLF4J_UNKNOWN_ARRAY" abbrev="SLF4J" category="CORRECTNESS" />
@@ -20,5 +22,6 @@
         <BugPattern type="SLF4J_ILLEGAL_PASSED_CLASS" abbrev="SLF4J" category="CORRECTNESS" />
         <BugPattern type="SLF4J_SIGN_ONLY_FORMAT" abbrev="SLF4J" category="BAD_PRACTICE" />
         <BugPattern type="SLF4J_MANUALLY_PROVIDED_MESSAGE" abbrev="SLF4J" category="BAD_PRACTICE" />
+        <BugPattern type="SLF4J_GET_STACK_TRACE" abbrev="SLF4J" category="BAD_PRACTICE" />
 
 </FindbugsPlugin>

bug-pattern/src/main/resources/messages.xml

@@ -43,6 +43,12 @@
     </Details>
   </Detector>
 
+  <Detector class="jp.skypencil.findbugs.slf4j.ManualGetStackTraceDetector">
+    <Details>
+      Finds log which uses Throwable#getStackTrace.
+    </Details>
+  </Detector>
+
   <BugPattern type="SLF4J_PLACE_HOLDER_MISMATCH">
     <ShortDescription>Count of placeholder is not equal to count of parameter</ShortDescription>
     <LongDescription>Count of placeholder({5}) is not equal to count of parameter({4})</LongDescription>
@@ -146,5 +152,17 @@ You cannot use array which is provided as method argument or returned from other
     </Details>
   </BugPattern>
 
+  <BugPattern type="SLF4J_GET_STACK_TRACE">
+    <ShortDescription>Do not use Throwable#getStackTrace.</ShortDescription>
+    <LongDescription>
+      Do not have use Throwable#getStackTrace. It is enough to provide throwable instance as the last argument, then binding will log its message.
+    </LongDescription>
+    <Details>
+<![CDATA[
+<p>Do not use Throwable#getStackTrace.</p>
+]]>
+    </Details>
+  </BugPattern>
+
   <BugCode abbrev="SLF4J">SLF4J</BugCode>
 </MessageCollection>

test-case/.gitignore

@@ -0,0 +1 @@
+/.apt_generated_tests/

test-case/src/main/java/pkg/UsingGetStackTrace.java

@@ -0,0 +1,13 @@
+package pkg;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class UsingGetStackTrace {
+
+    private final Logger logger = LoggerFactory.getLogger(getClass());
+
+    void method(RuntimeException ex) {
+        logger.error("Failed to determine The Answer to Life, The Universe and Everything: {}; cause: {}", 27, ex.getStackTrace());
+    }
+}

test-case/src/test/java/jp/skypencil/findbugs/slf4j/UsingGetStackTraceTest.java

@@ -0,0 +1,25 @@
+package jp.skypencil.findbugs.slf4j;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.Multimap;
+import java.util.Collections;
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Test for {@link ManualGetStackTraceDetector}.
+ *
+ * @author Michael Vorburger.ch
+ */
+public class UsingGetStackTraceTest {
+
+    @Test
+    public void testExceptionMethods() throws Exception {
+        Map<String, Integer> expected = Collections.singletonMap("SLF4J_GET_STACK_TRACE", 1);
+        Multimap<String, String> longMessages = new XmlParser().expect(pkg.UsingGetStackTrace.class, expected);
+        assertThat(longMessages).containsEntry("SLF4J_GET_STACK_TRACE",
+                "Do not have use Throwable#getStackTrace. It is enough to provide throwable instance as the last argument, then binding will log its message.");
+    }
+
+}