Skip to content

Commit 988df5c

Browse files
timtebeektimtebeek
authored
UnnecessaryThrows should not change API for protected methods (#553)
* UnnecessaryThrows should not change API for protected methods For apache/maven#2291 Needs openrewrite/rewrite#5412 * Adjust logic following review
1 parent 1b942fa commit 988df5c

File tree

2 files changed

+128
-56
lines changed

2 files changed

+128
-56
lines changed

‎src/main/java/org/openrewrite/staticanalysis/UnnecessaryThrows.java‎

Lines changed: 66 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@
2929
import java.time.Duration;
3030
import java.util.*;
3131

32+
import static org.openrewrite.java.tree.J.Modifier.Type.*;
33+
3234
public class UnnecessaryThrows extends Recipe {
3335

3436
@Override
@@ -136,74 +138,82 @@ private void removeThrownTypes(JavaType.@Nullable Method type) {
136138

137139
return m;
138140
}
139-
};
140-
}
141141

142+
private Set<JavaType.FullyQualified> findExceptionCandidates(J.@Nullable MethodDeclaration method) {
142143

143-
private Set<JavaType.FullyQualified> findExceptionCandidates(J.@Nullable MethodDeclaration method) {
144+
if (method == null || method.getMethodType() == null || method.isAbstract() || method.isConstructor()) {
145+
return Collections.emptySet();
146+
}
144147

145-
if (method == null || method.getMethodType() == null || method.isAbstract() || method.isConstructor()) {
146-
return Collections.emptySet();
147-
}
148+
// Do not change the API of methods that may be overridden
149+
if (method.hasModifier(Protected) && !method.hasModifier(Final)) {
150+
J.ClassDeclaration cd = getCursor().firstEnclosing(J.ClassDeclaration.class);
151+
if (cd != null && !cd.hasModifier(Final)) {
152+
return Collections.emptySet();
153+
}
154+
}
155+
156+
//Collect all checked exceptions.
157+
Set<JavaType.FullyQualified> candidates = new TreeSet<>(Comparator.comparing(JavaType.FullyQualified::getFullyQualifiedName));
148158

149-
//Collect all checked exceptions.
150-
Set<JavaType.FullyQualified> candidates = new TreeSet<>(Comparator.comparing(JavaType.FullyQualified::getFullyQualifiedName));
159+
if (method.getThrows() != null) {
160+
for (NameTree exception : method.getThrows()) {
161+
if (exception.getType() == null || exception.getType() instanceof JavaType.Unknown) {
162+
return Collections.emptySet();
163+
}
164+
if (exception.getType() instanceof JavaType.FullyQualified && !TypeUtils.isAssignableTo("java.lang.RuntimeException", exception.getType())) {
165+
candidates.add(TypeUtils.asFullyQualified(exception.getType()));
166+
}
167+
}
168+
}
151169

152-
if (method.getThrows() != null) {
153-
for (NameTree exception : method.getThrows()) {
154-
if (exception.getType() == null || exception.getType() instanceof JavaType.Unknown) {
170+
if (candidates.isEmpty()) {
155171
return Collections.emptySet();
156172
}
157-
if (exception.getType() instanceof JavaType.FullyQualified && !TypeUtils.isAssignableTo("java.lang.RuntimeException", exception.getType())) {
158-
candidates.add(TypeUtils.asFullyQualified(exception.getType()));
173+
174+
//noinspection ConstantConditions
175+
if ((method.getMethodType().getDeclaringType() != null && method.getMethodType().getDeclaringType().getFlags().contains(Flag.Final)) ||
176+
method.isAbstract() || method.hasModifier(Static) ||
177+
method.hasModifier(Private) ||
178+
method.hasModifier(Final)) {
179+
//Consider all checked exceptions as candidates if the type/method are final or the method is private or static.
180+
return candidates;
159181
}
160-
}
161-
}
162-
163-
if (candidates.isEmpty()) {
164-
return Collections.emptySet();
165-
}
166-
167-
//noinspection ConstantConditions
168-
if ((method.getMethodType().getDeclaringType() != null && method.getMethodType().getDeclaringType().getFlags().contains(Flag.Final)) ||
169-
method.isAbstract() || method.hasModifier(J.Modifier.Type.Static) ||
170-
method.hasModifier(J.Modifier.Type.Private) ||
171-
method.hasModifier(J.Modifier.Type.Final)) {
172-
//Consider all checked exceptions as candidates if the type/method are final or the method is private or static.
173-
return candidates;
174-
}
175-
176-
//Remove any candidates that are defined in an overridden method.
177-
Optional<JavaType.Method> superMethod = TypeUtils.findOverriddenMethod(method.getMethodType());
178-
if (superMethod.isPresent()) {
179-
JavaType.Method baseMethod = superMethod.get();
180-
baseMethod.getThrownExceptions();
181-
for (JavaType baseException : baseMethod.getThrownExceptions()) {
182-
if (baseException instanceof JavaType.FullyQualified) {
183-
candidates.remove(baseException);
182+
183+
//Remove any candidates that are defined in an overridden method.
184+
Optional<JavaType.Method> superMethod = TypeUtils.findOverriddenMethod(method.getMethodType());
185+
if (superMethod.isPresent()) {
186+
JavaType.Method baseMethod = superMethod.get();
187+
baseMethod.getThrownExceptions();
188+
for (JavaType baseException : baseMethod.getThrownExceptions()) {
189+
if (baseException instanceof JavaType.FullyQualified) {
190+
candidates.remove(baseException);
191+
}
192+
}
184193
}
185-
}
186-
}
187-
if (!candidates.isEmpty()) {
188-
//Remove any candidates that are defined in Javadocs for the method.
189-
new JavaVisitor<Set<JavaType.FullyQualified>>() {
190-
@Override
191-
protected JavadocVisitor<Set<JavaType.FullyQualified>> getJavadocVisitor() {
192-
return new JavadocVisitor<Set<JavaType.FullyQualified>>(this) {
194+
if (!candidates.isEmpty()) {
195+
//Remove any candidates that are defined in Javadocs for the method.
196+
new JavaVisitor<Set<JavaType.FullyQualified>>() {
193197
@Override
194-
public Javadoc visitThrows(Javadoc.Throws aThrows, Set<JavaType.FullyQualified> candidates) {
195-
if (aThrows.getExceptionName() instanceof TypeTree) {
196-
JavaType.FullyQualified exceptionType = TypeUtils.asFullyQualified(((TypeTree) aThrows.getExceptionName()).getType());
197-
if (exceptionType != null) {
198-
candidates.remove(exceptionType);
198+
protected JavadocVisitor<Set<JavaType.FullyQualified>> getJavadocVisitor() {
199+
return new JavadocVisitor<Set<JavaType.FullyQualified>>(this) {
200+
@Override
201+
public Javadoc visitThrows(Javadoc.Throws aThrows, Set<JavaType.FullyQualified> candidates) {
202+
if (aThrows.getExceptionName() instanceof TypeTree) {
203+
JavaType.FullyQualified exceptionType = TypeUtils.asFullyQualified(((TypeTree) aThrows.getExceptionName()).getType());
204+
if (exceptionType != null) {
205+
candidates.remove(exceptionType);
206+
}
207+
}
208+
return super.visitThrows(aThrows, candidates);
199209
}
200-
}
201-
return super.visitThrows(aThrows, candidates);
210+
};
202211
}
203-
};
212+
}.visit(method, candidates);
204213
}
205-
}.visit(method, candidates);
206-
}
207-
return candidates;
214+
return candidates;
215+
}
216+
};
208217
}
218+
209219
}

‎src/test/java/org/openrewrite/staticanalysis/UnnecessaryThrowsTest.java‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -395,4 +395,66 @@ public static class MyException extends Exception { }
395395
)
396396
);
397397
}
398+
399+
@Issue("https://github.com/apache/maven/pull/2291")
400+
@Test
401+
void retainExceptionsForOverrides() {
402+
rewriteRun(
403+
//language=java
404+
java(
405+
"""
406+
import java.io.FileNotFoundException;
407+
class A {
408+
// Someone marked this protected, and added exceptions that implementers can optionally use
409+
protected void method() throws FileNotFoundException {
410+
}
411+
}
412+
"""
413+
),
414+
java(
415+
"""
416+
import java.io.FileNotFoundException;
417+
class B extends A {
418+
@Override
419+
protected void method() throws FileNotFoundException {
420+
}
421+
}
422+
"""
423+
),
424+
java(
425+
"""
426+
import java.io.FileNotFoundException;
427+
class C1 extends B {
428+
@Override
429+
protected final void method() throws FileNotFoundException {
430+
}
431+
}
432+
""",
433+
"""
434+
class C1 extends B {
435+
@Override
436+
protected final void method() {
437+
}
438+
}
439+
"""
440+
),
441+
java(
442+
"""
443+
import java.io.FileNotFoundException;
444+
final class C2 extends B {
445+
@Override
446+
protected void method() throws FileNotFoundException {
447+
}
448+
}
449+
""",
450+
"""
451+
final class C2 extends B {
452+
@Override
453+
protected void method() {
454+
}
455+
}
456+
"""
457+
)
458+
);
459+
}
398460
}

0 commit comments

Comments
 (0)