Skip to content

Commit b0d7d80

Browse files
vorburgervorburger
committed
add new SLF4J_GET_STACK_TRACE detector (fixes KengoTODA#70)
incl. adjustments for code review feedback * delete useless inline comment * add @Item.SpecialKind annotation * rename getIsOfInterestKind to getKindOfInterest * rename isGetStackTrace to getStackTraceKind * rename AbstractDetectorForParameterArray2 to AbstractExtendedDetectorForParameterArray
1 parent a9380ef commit b0d7d80

13 files changed

+199
-131
lines changed

‎CHANGELOG.md

+2
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ This changelog follows [Keep a Changelog v1.0.0](https://keepachangelog.com/en/1
44

55
## Unreleased
66

7+
- SLF4J_GET_STACK_TRACE bug pattern
8+
79
## 1.4.1 - 2018-06-17
810
### Changed
911

‎README.md

+20
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,26 @@ class Foo {
192192
}
193193
```
194194

195+
## SLF4J_GET_STACK_TRACE
196+
197+
This pattern reports on the use of `Throwable#getStackTrace()`. This is most
198+
typically wrong and a misunderstanding of the slf4j API, as normally you just
199+
provide the Throwable instance as the last argument, instead of using getStackTrace().
200+
201+
```java
202+
class Foo {
203+
private final Logger logger = LoggerFactory.getLogger(getClass());
204+
void method() {
205+
// invalid: needless 'e.getStackTrace()'
206+
logger.info("Error occured. Stack is {}", e.getStackTrace());
207+
208+
// valid
209+
logger.info("Error occured.", e);
210+
}
211+
}
212+
```
213+
214+
195215
# How to use with Maven
196216

197217
To use this product, please configure your spotbugs-maven-plugin like below.

‎bug-pattern/.gitignore

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
/.apt_generated_tests/

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

+38
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
package jp.skypencil.findbugs.slf4j;
2+
3+
import static org.apache.bcel.Const.INVOKEVIRTUAL;
4+
5+
import com.google.common.base.Objects;
6+
import edu.umd.cs.findbugs.BugReporter;
7+
import edu.umd.cs.findbugs.OpcodeStack.Item;
8+
import jp.skypencil.findbugs.slf4j.parameter.AbstractExtendedDetectorForParameterArray;
9+
10+
/**
11+
* FindBugs (now SpotBugs) Detector for (ab)use of {@link Throwable#getStackTrace()} in SFL4j logger.
12+
*
13+
* @author Michael Vorburger.ch
14+
*/
15+
public class ManualGetStackTraceDetector extends AbstractExtendedDetectorForParameterArray {
16+
17+
@Item.SpecialKind
18+
private final int getStackTraceKind = Item.defineNewSpecialKind("use of throwable getStackTrace");
19+
20+
public ManualGetStackTraceDetector(BugReporter bugReporter) {
21+
super(bugReporter, "SLF4J_GET_STACK_TRACE");
22+
}
23+
24+
@Override
25+
@Item.SpecialKind
26+
protected int getKindOfInterest() {
27+
return getStackTraceKind;
28+
}
29+
30+
@Override
31+
protected boolean isWhatWeWantToDetect(int seen) {
32+
return seen == INVOKEVIRTUAL
33+
&& !stack.isTop()
34+
&& getThrowableHandler().checkThrowable(getStack().getStackItem(0))
35+
&& Objects.equal("getStackTrace", getNameConstantOperand());
36+
}
37+
38+
}

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

+12-66
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,16 @@
11
package jp.skypencil.findbugs.slf4j;
22

3+
import static org.apache.bcel.Const.INVOKEVIRTUAL;
4+
35
import com.google.common.base.Objects;
4-
import jp.skypencil.findbugs.slf4j.parameter.AbstractDetectorForParameterArray2;
5-
import jp.skypencil.findbugs.slf4j.parameter.ArrayData;
6-
import edu.umd.cs.findbugs.BugInstance;
76
import edu.umd.cs.findbugs.BugReporter;
87
import edu.umd.cs.findbugs.OpcodeStack.CustomUserValue;
98
import edu.umd.cs.findbugs.OpcodeStack.Item;
10-
import javax.annotation.Nullable;
11-
import jp.skypencil.findbugs.slf4j.parameter.AbstractDetectorForParameterArray;
12-
import jp.skypencil.findbugs.slf4j.parameter.ArrayDataHandler.Strategy;
9+
import jp.skypencil.findbugs.slf4j.parameter.AbstractExtendedDetectorForParameterArray;
10+
import jp.skypencil.findbugs.slf4j.parameter.ArrayData;
1311

1412
@CustomUserValue
15-
public final class ManualMessageDetector extends AbstractDetectorForParameterArray {
13+
public final class ManualMessageDetector extends AbstractExtendedDetectorForParameterArray {
1614

1715
@Item.SpecialKind
1816
private final int isMessage = Item.defineSpecialKind("message generated by throwable object");
@@ -22,38 +20,8 @@ public ManualMessageDetector(BugReporter bugReporter) {
2220
}
2321

2422
@Override
25-
protected Strategy createArrayCheckStrategy() {
26-
return (storedItem, arrayData, index) -> {
27-
if (arrayData == null) {
28-
return;
29-
}
30-
31-
if (storedItem.getSpecialKind() == isMessage) {
32-
arrayData.mark(true);
33-
}
34-
35-
// Let developer logs exception message, only when argument does not have throwable instance
36-
// https://github.com/KengoTODA/findbugs-slf4j/issues/31
37-
if (index == arrayData.getSize() - 1 && !getThrowableHandler().checkThrowable(storedItem)) {
38-
arrayData.mark(false);
39-
}
40-
};
41-
}
42-
43-
@Override
44-
public void afterOpcode(int seen) {
45-
boolean isInvokingGetMessage = isInvokingGetMessage(seen);
46-
super.afterOpcode(seen);
47-
48-
if (isInvokingGetMessage && !stack.isTop()) {
49-
stack.getStackItem(0).setSpecialKind(isMessage);
50-
}
51-
}
52-
53-
54-
@Override
55-
protected int getIsOfInterestKind() {
56-
return isMessage;
23+
protected int getKindOfInterest() {
24+
return isMessage;
5725
}
5826

5927
@Override
@@ -64,33 +32,11 @@ protected boolean isReallyOfInterest(Item storedItem, ArrayData arrayData, int i
6432
&& !getThrowableHandler().checkThrowable(storedItem));
6533
}
6634

67-
private boolean isInvokingGetMessage(int seen) {
68-
return seen == INVOKEVIRTUAL
69-
&& !stack.isTop()
70-
&& getThrowableHandler().checkThrowable(getStack().getStackItem(0))
71-
&& (Objects.equal("getMessage", getNameConstantOperand())
72-
|| Objects.equal("getLocalizedMessage", getNameConstantOperand()));
73-
}
74-
7535
@Override
76-
protected void onLog(@Nullable String format, @Nullable ArrayData arrayData) {
77-
if (arrayData == null || !arrayData.isMarked()) {
78-
return;
36+
protected boolean isWhatWeWantToDetect(int seen) {
37+
return seen == INVOKEVIRTUAL && !stack.isTop()
38+
&& getThrowableHandler().checkThrowable(getStack().getStackItem(0))
39+
&& (Objects.equal("getMessage", getNameConstantOperand())
40+
|| Objects.equal("getLocalizedMessage", getNameConstantOperand()));
7941
}
80-
BugInstance bugInstance =
81-
new BugInstance(this, "SLF4J_MANUALLY_PROVIDED_MESSAGE", NORMAL_PRIORITY)
82-
.addSourceLine(this)
83-
.addClassAndMethod(this)
84-
.addCalledMethod(this);
85-
getBugReporter().reportBug(bugInstance);
86-
}
87-
88-
@Override
89-
protected boolean isWhatWeWantToDetect(int seen) {
90-
return seen == INVOKEVIRTUAL && !stack.isTop()
91-
&& getThrowableHandler().checkThrowable(getStack().getStackItem(0))
92-
&& (Objects.equal("getMessage", getNameConstantOperand())
93-
|| Objects.equal("getLocalizedMessage", getNameConstantOperand()));
94-
}
95-
9642
}

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

-65
This file was deleted.

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

+65
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
package jp.skypencil.findbugs.slf4j.parameter;
2+
3+
import edu.umd.cs.findbugs.BugInstance;
4+
import edu.umd.cs.findbugs.BugReporter;
5+
import edu.umd.cs.findbugs.OpcodeStack.Item;
6+
import javax.annotation.Nullable;
7+
import jp.skypencil.findbugs.slf4j.parameter.ArrayDataHandler.Strategy;
8+
9+
public abstract class AbstractExtendedDetectorForParameterArray extends AbstractDetectorForParameterArray {
10+
11+
private final String bugPatternName;
12+
13+
public AbstractExtendedDetectorForParameterArray(BugReporter bugReporter, String bugPatternName) {
14+
super(bugReporter);
15+
this.bugPatternName = bugPatternName;
16+
}
17+
18+
@Override
19+
protected final Strategy createArrayCheckStrategy() {
20+
return (storedItem, arrayData, index) -> {
21+
if (arrayData == null) {
22+
return;
23+
}
24+
25+
if (storedItem.getSpecialKind() == getKindOfInterest()) {
26+
arrayData.mark(true);
27+
}
28+
29+
if (!isReallyOfInterest(storedItem, arrayData, index)) {
30+
arrayData.mark(false);
31+
}
32+
};
33+
}
34+
35+
protected boolean isReallyOfInterest(Item storedItem, ArrayData arrayData, int index) {
36+
return true;
37+
}
38+
39+
@Override
40+
public final void afterOpcode(int seen) {
41+
boolean isInvokingGetMessage = isWhatWeWantToDetect(seen);
42+
super.afterOpcode(seen);
43+
44+
if (isInvokingGetMessage && !stack.isTop()) {
45+
stack.getStackItem(0).setSpecialKind(getKindOfInterest());
46+
}
47+
}
48+
49+
@Override
50+
protected final void onLog(@Nullable String format, @Nullable ArrayData arrayData) {
51+
if (arrayData == null || !arrayData.isMarked()) {
52+
return;
53+
}
54+
BugInstance bugInstance = new BugInstance(this,
55+
bugPatternName, NORMAL_PRIORITY)
56+
.addSourceLine(this).addClassAndMethod(this)
57+
.addCalledMethod(this);
58+
getBugReporter().reportBug(bugInstance);
59+
}
60+
61+
abstract protected boolean isWhatWeWantToDetect(int seen);
62+
63+
abstract protected int getKindOfInterest();
64+
65+
}

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

+1
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,4 @@
77
0 BugPattern SLF4J_ILLEGAL_PASSED_CLASS
88
0 BugPattern SLF4J_SIGN_ONLY_FORMAT
99
0 BugPattern SLF4J_MANUALLY_PROVIDED_MESSAGE
10+
0 BugPattern SLF4J_GET_STACK_TRACE

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

+3
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111
<Detector class="jp.skypencil.findbugs.slf4j.StaticLoggerDetector" reports="SLF4J_LOGGER_SHOULD_BE_NON_STATIC" speed="fast" />
1212
<Detector class="jp.skypencil.findbugs.slf4j.IllegalPassedClassDetector" reports="SLF4J_ILLEGAL_PASSED_CLASS" speed="fast" />
1313
<Detector class="jp.skypencil.findbugs.slf4j.ManualMessageDetector" reports="SLF4J_MANUALLY_PROVIDED_MESSAGE,SLF4J_FORMAT_SHOULD_BE_CONST" speed="fast" />
14+
<Detector class="jp.skypencil.findbugs.slf4j.ManualGetStackTraceDetector" reports="SLF4J_GET_STACK_TRACE" speed="fast" />
15+
1416
<BugPattern type="SLF4J_PLACE_HOLDER_MISMATCH" abbrev="SLF4J" category="CORRECTNESS" />
1517
<BugPattern type="SLF4J_FORMAT_SHOULD_BE_CONST" abbrev="SLF4J" category="CORRECTNESS" />
1618
<BugPattern type="SLF4J_UNKNOWN_ARRAY" abbrev="SLF4J" category="CORRECTNESS" />
@@ -20,5 +22,6 @@
2022
<BugPattern type="SLF4J_ILLEGAL_PASSED_CLASS" abbrev="SLF4J" category="CORRECTNESS" />
2123
<BugPattern type="SLF4J_SIGN_ONLY_FORMAT" abbrev="SLF4J" category="BAD_PRACTICE" />
2224
<BugPattern type="SLF4J_MANUALLY_PROVIDED_MESSAGE" abbrev="SLF4J" category="BAD_PRACTICE" />
25+
<BugPattern type="SLF4J_GET_STACK_TRACE" abbrev="SLF4J" category="BAD_PRACTICE" />
2326

2427
</FindbugsPlugin>

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

+18
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,12 @@
4343
</Details>
4444
</Detector>
4545

46+
<Detector class="jp.skypencil.findbugs.slf4j.ManualGetStackTraceDetector">
47+
<Details>
48+
Finds log which uses Throwable#getStackTrace.
49+
</Details>
50+
</Detector>
51+
4652
<BugPattern type="SLF4J_PLACE_HOLDER_MISMATCH">
4753
<ShortDescription>Count of placeholder is not equal to count of parameter</ShortDescription>
4854
<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
146152
</Details>
147153
</BugPattern>
148154

155+
<BugPattern type="SLF4J_GET_STACK_TRACE">
156+
<ShortDescription>Do not use Throwable#getStackTrace.</ShortDescription>
157+
<LongDescription>
158+
Do not have use Throwable#getStackTrace. It is enough to provide throwable instance as the last argument, then binding will log its message.
159+
</LongDescription>
160+
<Details>
161+
<![CDATA[
162+
<p>Do not use Throwable#getStackTrace.</p>
163+
]]>
164+
</Details>
165+
</BugPattern>
166+
149167
<BugCode abbrev="SLF4J">SLF4J</BugCode>
150168
</MessageCollection>

‎test-case/.gitignore

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
/.apt_generated_tests/

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

+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
package pkg;
2+
3+
import org.slf4j.Logger;
4+
import org.slf4j.LoggerFactory;
5+
6+
public class UsingGetStackTrace {
7+
8+
private final Logger logger = LoggerFactory.getLogger(getClass());
9+
10+
void method(RuntimeException ex) {
11+
logger.error("Failed to determine The Answer to Life, The Universe and Everything: {}; cause: {}", 27, ex.getStackTrace());
12+
}
13+
}

0 commit comments

Comments
 (0)