Skip to content

Commit

Permalink
add new SLF4J_GET_STACK_TRACE detector (fixes KengoTODA#70)
Browse files Browse the repository at this point in the history
  • Loading branch information
vorburger committed Mar 14, 2018
1 parent d1a2c62 commit 33f4abd
Show file tree
Hide file tree
Showing 7 changed files with 127 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package jp.skypencil.findbugs.slf4j;

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.AbstractDetectorForParameterArray2;

/**
* FindBugs (now SpotBugs) Detector for (ab)use of {@link Throwable#getStackTrace()} in SFL4j logger.
*
* @author Michael Vorburger.ch
*/
public class ManualGetStackTraceDetector extends AbstractDetectorForParameterArray2 {

@Item.SpecialKind
private final int isGetStackTrace = Item.defineNewSpecialKind("use of throwable getStackTrace");

public ManualGetStackTraceDetector(BugReporter bugReporter) {
super(bugReporter, "SLF4J_GET_STACK_TRACE");
}

@Override
protected int getIsOfInterestKind() {
return isGetStackTrace;
}

@Override
protected boolean isWhatWeWantToDetect(int seen) {
// return false;
return seen == INVOKEVIRTUAL
&& !stack.isTop()
&& getThrowableHandler().checkThrowable(getStack().getStackItem(0))
&& Objects.equal("getStackTrace", getNameConstantOperand());
}

}
1 change: 1 addition & 0 deletions bug-pattern/src/main/resources/bugrank.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
3 changes: 3 additions & 0 deletions bug-pattern/src/main/resources/findbugs.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Expand All @@ -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>
18 changes: 18 additions & 0 deletions bug-pattern/src/main/resources/messages.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
13 changes: 13 additions & 0 deletions test-case/src/main/java/pkg/UsingGetStackTrace.java
Original file line number Diff line number Diff line change
@@ -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; {}", 27, ex.getStackTrace());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package jp.skypencil.findbugs.slf4j;

import edu.umd.cs.findbugs.BugInstance;
import java.util.function.Predicate;
import org.hamcrest.BaseMatcher;
import org.hamcrest.Description;

/**
* Hamcrest matcher for FindBugs (now SpotBugs) {@link BugInstance}.
*
* @author Michael Vorburger.ch
*/
public final class BugInstanceMatcher extends BaseMatcher<BugInstance> {

private final Predicate<BugInstance> predicate;

public BugInstanceMatcher(Predicate<BugInstance> predicate) {
this.predicate = predicate;
}

@Override
public final boolean matches(Object item) {
return predicate.test((BugInstance) item);
}

@Override
public void describeTo(Description description) {
description.appendText("BugInstance must match expected");
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package jp.skypencil.findbugs.slf4j;

import com.youdevise.fbplugins.tdd4fb.DetectorAssert;
import edu.umd.cs.findbugs.BugReporter;
import edu.umd.cs.findbugs.Detector;
import org.junit.jupiter.api.Test;
import pkg.UsingGetStackTrace;

/**
* Test for {@link ManualGetStackTraceDetector}.
*
* @author Michael Vorburger.ch
*/
public class UsingGetStackTraceTest {

@Test
public void testExceptionMethods() throws Exception {
BugReporter bugReporter = DetectorAssert.bugReporterForTesting();
Detector detector = new ManualGetStackTraceDetector(bugReporter);
DetectorAssert.assertNoBugsReported(UsingGetStackTrace.class, detector, bugReporter);
DetectorAssert.assertBugReported(UsingGetStackTrace.class, detector, bugReporter, new BugInstanceMatcher(
bugInstance -> bugInstance.getBugPattern().getAbbrev().equals("SLF4J_GET_STACK_TRACE")));
}

}

0 comments on commit 33f4abd

Please sign in to comment.