Skip to content

Commit 191f62b

Browse files
author
Ádám Balogh
committed
Fix for Issue 2040
This PR fixes issue spotbugs#2040 by limiting the errors reported by the detector `ThrowingExceptions` to cases where `java.lang.Exception` or `lava.lang.Throwable` appears in the exception specification of th method but it is neither inherited from an overridden method nor is is coming from another method that the method invokes. Syntathic methods are also omitted as well as methods which throw generic exceptions.
1 parent 68e7dca commit 191f62b

File tree

8 files changed

+259
-10
lines changed

8 files changed

+259
-10
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
3939
- Fixed UI freezes in Eclipse on bug count decorations update ([#285](https://github.com/spotbugs/spotbugs/issues/285))
4040
- Bumped log4j from 2.17.1 to 2.17.2 ([#1960](https://github.com/spotbugs/spotbugs/pull/1960))
4141
- Bumped gson from 2.8.9 to 2.9.0 ([#1960](https://github.com/spotbugs/spotbugs/pull/1966))
42+
- Fixed detector `ThrowingExcpetions` by omitting cases where throwing `java.lang.Exception` or `java.lang.Throwable` is unavoidable ([#2040](https://github.com/spotbugs/spotbugs/pull/2040))
4243

4344
### Added
4445
* New detector `FindInstanceLockOnSharedStaticData` for new bug type `SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA`. This detector reports a bug if an instance level lock is used to modify a shared static data. (See [SEI CERT rule LCK06-J](https://wiki.sei.cmu.edu/confluence/display/java/LCK06-J.+Do+not+use+an+instance+lock+to+protect+shared+static+data))

‎spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2040Test.java

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
package edu.umd.cs.findbugs.detect;
2+
3+
import edu.umd.cs.findbugs.AbstractIntegrationTest;
4+
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
5+
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
6+
import org.junit.Test;
7+
8+
import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
9+
import static org.hamcrest.MatcherAssert.assertThat;
10+
11+
public class Issue2040Test extends AbstractIntegrationTest {
12+
@Test
13+
public void test() {
14+
performAnalysis("ghIssues/issue2040/Base.class",
15+
"ghIssues/issue2040/Derived.class",
16+
"ghIssues/issue2040/Interface.class",
17+
"ghIssues/issue2040/Generic.class",
18+
"ghIssues/issue2040/Caller.class");
19+
BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
20+
.bugType("THROWS_METHOD_THROWS_CLAUSE_THROWABLE")
21+
.build();
22+
assertThat(getBugCollection(), containsExactly(1, bugTypeMatcher));
23+
24+
bugTypeMatcher = new BugInstanceMatcherBuilder()
25+
.bugType("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION")
26+
.build();
27+
assertThat(getBugCollection(), containsExactly(1, bugTypeMatcher));
28+
}
29+
}

‎spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ThrowingExceptions.java

+173-10
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,20 @@
33
import edu.umd.cs.findbugs.BugInstance;
44
import edu.umd.cs.findbugs.BugReporter;
55
import edu.umd.cs.findbugs.OpcodeStack;
6+
import edu.umd.cs.findbugs.ba.AnalysisContext;
67
import edu.umd.cs.findbugs.ba.XMethod;
78
import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
89

10+
import java.util.Arrays;
11+
import java.util.Optional;
12+
import java.util.stream.Stream;
13+
914
import org.apache.bcel.Const;
15+
import org.apache.bcel.classfile.Code;
1016
import org.apache.bcel.classfile.ExceptionTable;
17+
import org.apache.bcel.classfile.JavaClass;
1118
import org.apache.bcel.classfile.Method;
19+
import org.apache.commons.lang3.StringUtils;
1220

1321

1422
public class ThrowingExceptions extends OpcodeStackDetector {
@@ -18,19 +26,63 @@ public ThrowingExceptions(BugReporter bugReporter) {
1826
this.bugReporter = bugReporter;
1927
}
2028

29+
private JavaClass klass;
30+
private String exceptionThrown = null;
31+
32+
@Override
33+
public void visit(JavaClass obj) {
34+
exceptionThrown = null;
35+
klass = obj;
36+
}
37+
2138
@Override
2239
public void visit(Method obj) {
23-
ExceptionTable exceptionTable = obj.getExceptionTable();
24-
if (exceptionTable != null) {
25-
String[] exceptionNames = exceptionTable.getExceptionNames();
26-
for (String exception : exceptionNames) {
27-
if ("java.lang.Exception".equals(exception)) {
28-
reportBug("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", getXMethod());
29-
} else if ("java.lang.Throwable".equals(exception)) {
30-
reportBug("THROWS_METHOD_THROWS_CLAUSE_THROWABLE", getXMethod());
31-
}
40+
exceptionThrown = null;
41+
42+
if (obj.isSynthetic()) {
43+
return;
44+
}
45+
46+
Stream<String> exceptionStream = null;
47+
48+
String signature = obj.getGenericSignature();
49+
if (signature != null) {
50+
String[] exceptions = StringUtils.substringsBetween(signature, "^", ";");
51+
if (exceptions != null) {
52+
exceptionStream = Arrays.stream(exceptions)
53+
.filter(s -> s.charAt(0) == 'L')
54+
.map(s -> s.substring(1).replace('/', '.'));
55+
}
56+
} else {
57+
ExceptionTable exceptionTable = obj.getExceptionTable();
58+
if (exceptionTable != null) {
59+
exceptionStream = Arrays.stream(exceptionTable.getExceptionNames());
3260
}
3361
}
62+
63+
if (exceptionStream != null) {
64+
Optional<String> exception = exceptionStream
65+
.filter(s -> "java.lang.Exception".equals(s) || "java.lang.Throwable".equals(s))
66+
.findAny();
67+
if (exception.isPresent() && !parentThrows(obj, exception.get())) {
68+
exceptionThrown = exception.get();
69+
}
70+
}
71+
72+
if (obj.getCode() == null) {
73+
if (exceptionThrown != null) {
74+
reportBug("java.lang.Exception".equals(exceptionThrown) ? "THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION"
75+
: "THROWS_METHOD_THROWS_CLAUSE_THROWABLE", getXMethod());
76+
}
77+
}
78+
}
79+
80+
@Override
81+
public void visitAfter(Code obj) {
82+
if (exceptionThrown != null) {
83+
reportBug("java.lang.Exception".equals(exceptionThrown) ? "THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION"
84+
: "THROWS_METHOD_THROWS_CLAUSE_THROWABLE", getXMethod());
85+
}
3486
}
3587

3688
@Override
@@ -42,10 +94,121 @@ public void sawOpcode(int seen) {
4294
reportBug("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", getXMethod());
4395
}
4496
}
97+
} else if (exceptionThrown != null &&
98+
seen == Const.INVOKEVIRTUAL ||
99+
seen == Const.INVOKEINTERFACE ||
100+
seen == Const.INVOKESTATIC) {
101+
XMethod calledMethod = getXMethodOperand();
102+
if (calledMethod == null) {
103+
return;
104+
}
105+
106+
String[] thrownExceptions = calledMethod.getThrownExceptions();
107+
if (thrownExceptions != null && Arrays.stream(thrownExceptions)
108+
.anyMatch(s -> s.replace('/', '.').equals(exceptionThrown))) {
109+
exceptionThrown = null;
110+
}
111+
45112
}
46113
}
47114

48115
private void reportBug(String bugName, XMethod method) {
49-
bugReporter.reportBug(new BugInstance(this, bugName, NORMAL_PRIORITY).addClass(this).addMethod(method));
116+
bugReporter.reportBug(new BugInstance(this, bugName, LOW_PRIORITY).addClass(this).addMethod(method));
117+
}
118+
119+
private boolean parentThrows(Method method, String exception) {
120+
return parentThrows(klass, method, exception);
121+
}
122+
123+
private boolean parentThrows(JavaClass clazz, Method method, String exception) {
124+
JavaClass ancestor;
125+
boolean throwsEx = false;
126+
try {
127+
ancestor = clazz.getSuperClass();
128+
if (ancestor != null) {
129+
Optional<Method> superMethod = Arrays.stream(ancestor.getMethods())
130+
.filter(m -> method.getName().equals(m.getName()) && sameSignature(method, m))
131+
.findAny();
132+
if (superMethod.isPresent()) {
133+
throwsEx = Arrays.stream(superMethod.get().getExceptionTable().getExceptionNames())
134+
.anyMatch(s -> exception.equals(s));
135+
} else {
136+
throwsEx = parentThrows(ancestor, method, exception);
137+
}
138+
}
139+
140+
for (JavaClass intf : clazz.getInterfaces()) {
141+
Optional<Method> superMethod = Arrays.stream(intf.getMethods())
142+
.filter(m -> method.getName().equals(m.getName()) && sameSignature(method, m))
143+
.findAny();
144+
if (superMethod.isPresent()) {
145+
throwsEx |= Arrays.stream(superMethod.get().getExceptionTable().getExceptionNames())
146+
.anyMatch(s -> exception.equals(s));
147+
} else {
148+
throwsEx |= parentThrows(intf, method, exception);
149+
}
150+
}
151+
} catch (ClassNotFoundException e) {
152+
AnalysisContext.reportMissingClass(e);
153+
}
154+
return throwsEx;
155+
}
156+
157+
private boolean sameSignature(Method child, Method parent) {
158+
String genSig = parent.getGenericSignature();
159+
if (genSig == null) {
160+
return child.getSignature().equals(parent.getSignature());
161+
}
162+
163+
String sig = child.getSignature();
164+
int genIndex = 1;
165+
char genSigChar = genSig.charAt(genIndex);
166+
int index = 1;
167+
char sigChar = sig.charAt(index);
168+
while (genSigChar != ')' && sigChar != ')') {
169+
switch (genSigChar) {
170+
default:
171+
if (sigChar != genSigChar) {
172+
return false;
173+
}
174+
++index;
175+
++genIndex;
176+
break;
177+
case 'L':
178+
int genSigObjEnd = genSig.indexOf(';', genIndex);
179+
int sigObjEnd = sig.indexOf(';', index);
180+
if (!genSig.substring(genIndex, genSigObjEnd - genIndex)
181+
.equals(sig.substring(index, sigObjEnd - index))) {
182+
return false;
183+
}
184+
genIndex = genSigObjEnd + 1;
185+
index = sigObjEnd + 1;
186+
break;
187+
case 'T':
188+
if (sigChar != 'L') {
189+
return false;
190+
}
191+
genSigObjEnd = genSig.indexOf(';', genIndex);
192+
sigObjEnd = sig.indexOf(';', index);
193+
genIndex = genSigObjEnd + 1;
194+
index = sigObjEnd + 1;
195+
break;
196+
}
197+
genSigChar = genSig.charAt(genIndex);
198+
sigChar = sig.charAt(index);
199+
}
200+
201+
if (sigChar != genSigChar) {
202+
return false;
203+
}
204+
205+
genSigChar = genSig.charAt(++genIndex);
206+
sigChar = sig.charAt(++index);
207+
208+
if (genSigChar == 'T') {
209+
return sigChar == 'L';
210+
} else {
211+
return genSig.substring(genIndex).equals(sig.substring(index));
212+
}
50213
}
51214
}

‎spotbugsTestCases/src/java/ghIssues/issue2040/Base.java

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
package ghIssues.issue2040;
2+
3+
public class Base {
4+
public void method() throws Throwable {
5+
throw new Throwable();
6+
}
7+
}

‎spotbugsTestCases/src/java/ghIssues/issue2040/Caller.java

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
package ghIssues.issue2040;
2+
3+
public class Caller {
4+
public void method() throws Throwable {
5+
Base b = new Base();
6+
b.method();
7+
}
8+
}

‎spotbugsTestCases/src/java/ghIssues/issue2040/Derived.java

+30
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
package ghIssues.issue2040;
2+
3+
import java.util.concurrent.Callable;
4+
import java.util.concurrent.ExecutorService;
5+
import java.util.concurrent.Executors;
6+
7+
public class Derived extends Base implements Interface, Callable<Integer> {
8+
@Override
9+
public void method() throws Throwable {
10+
super.method();
11+
}
12+
13+
@Override
14+
public void iMethod() throws Exception {
15+
return;
16+
}
17+
18+
public <T extends Throwable> void genericMethod() throws T {
19+
20+
}
21+
22+
public Integer call() throws Exception {
23+
return 0;
24+
}
25+
26+
public void lambda() {
27+
ExecutorService es = Executors.newCachedThreadPool();
28+
es.submit(() -> { return null; });
29+
}
30+
}

‎spotbugsTestCases/src/java/ghIssues/issue2040/Generic.java

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
package ghIssues.issue2040;
2+
3+
public class Generic<E extends Exception> {
4+
void method() throws E {
5+
}
6+
}

‎spotbugsTestCases/src/java/ghIssues/issue2040/Interface.java

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
package ghIssues.issue2040;
2+
3+
public interface Interface {
4+
public void iMethod() throws Exception;
5+
}

0 commit comments

Comments
 (0)