Skip to content

UnnecessaryThrows should not change API for protected methods #553

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
timtebeek merged 2 commits into from
May 10, 2025
Merged

UnnecessaryThrows should not change API for protected methods #553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22897d1
UnnecessaryThrows should not change API for protected methods
timtebeek May 9, 2025
22a3fbb
Adjust logic following review
timtebeek May 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 66 additions & 56 deletions src/main/java/org/openrewrite/staticanalysis/UnnecessaryThrows.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import java.time.Duration;
import java.util.*;

import static org.openrewrite.java.tree.J.Modifier.Type.*;

public class UnnecessaryThrows extends Recipe {

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

return m;
}
};
}

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

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

if (method == null || method.getMethodType() == null || method.isAbstract() || method.isConstructor()) {
return Collections.emptySet();
}
// Do not change the API of methods that may be overridden
if (method.hasModifier(Protected) && !method.hasModifier(Final)) {
J.ClassDeclaration cd = getCursor().firstEnclosing(J.ClassDeclaration.class);
if (cd != null && !cd.hasModifier(Final)) {
return Collections.emptySet();
}
}

//Collect all checked exceptions.
Set<JavaType.FullyQualified> candidates = new TreeSet<>(Comparator.comparing(JavaType.FullyQualified::getFullyQualifiedName));

//Collect all checked exceptions.
Set<JavaType.FullyQualified> candidates = new TreeSet<>(Comparator.comparing(JavaType.FullyQualified::getFullyQualifiedName));
if (method.getThrows() != null) {
for (NameTree exception : method.getThrows()) {
if (exception.getType() == null || exception.getType() instanceof JavaType.Unknown) {
return Collections.emptySet();
}
if (exception.getType() instanceof JavaType.FullyQualified && !TypeUtils.isAssignableTo("java.lang.RuntimeException", exception.getType())) {
candidates.add(TypeUtils.asFullyQualified(exception.getType()));
}
}
}

if (method.getThrows() != null) {
for (NameTree exception : method.getThrows()) {
if (exception.getType() == null || exception.getType() instanceof JavaType.Unknown) {
if (candidates.isEmpty()) {
return Collections.emptySet();
}
if (exception.getType() instanceof JavaType.FullyQualified && !TypeUtils.isAssignableTo("java.lang.RuntimeException", exception.getType())) {
candidates.add(TypeUtils.asFullyQualified(exception.getType()));

//noinspection ConstantConditions
if ((method.getMethodType().getDeclaringType() != null && method.getMethodType().getDeclaringType().getFlags().contains(Flag.Final)) ||
method.isAbstract() || method.hasModifier(Static) ||
method.hasModifier(Private) ||
method.hasModifier(Final)) {
//Consider all checked exceptions as candidates if the type/method are final or the method is private or static.
return candidates;
}
}
}

if (candidates.isEmpty()) {
return Collections.emptySet();
}

//noinspection ConstantConditions
if ((method.getMethodType().getDeclaringType() != null && method.getMethodType().getDeclaringType().getFlags().contains(Flag.Final)) ||
method.isAbstract() || method.hasModifier(J.Modifier.Type.Static) ||
method.hasModifier(J.Modifier.Type.Private) ||
method.hasModifier(J.Modifier.Type.Final)) {
//Consider all checked exceptions as candidates if the type/method are final or the method is private or static.
return candidates;
}

//Remove any candidates that are defined in an overridden method.
Optional<JavaType.Method> superMethod = TypeUtils.findOverriddenMethod(method.getMethodType());
if (superMethod.isPresent()) {
JavaType.Method baseMethod = superMethod.get();
baseMethod.getThrownExceptions();
for (JavaType baseException : baseMethod.getThrownExceptions()) {
if (baseException instanceof JavaType.FullyQualified) {
candidates.remove(baseException);

//Remove any candidates that are defined in an overridden method.
Optional<JavaType.Method> superMethod = TypeUtils.findOverriddenMethod(method.getMethodType());
if (superMethod.isPresent()) {
JavaType.Method baseMethod = superMethod.get();
baseMethod.getThrownExceptions();
for (JavaType baseException : baseMethod.getThrownExceptions()) {
if (baseException instanceof JavaType.FullyQualified) {
candidates.remove(baseException);
}
}
}
}
}
if (!candidates.isEmpty()) {
//Remove any candidates that are defined in Javadocs for the method.
new JavaVisitor<Set<JavaType.FullyQualified>>() {
@Override
protected JavadocVisitor<Set<JavaType.FullyQualified>> getJavadocVisitor() {
return new JavadocVisitor<Set<JavaType.FullyQualified>>(this) {
if (!candidates.isEmpty()) {
//Remove any candidates that are defined in Javadocs for the method.
new JavaVisitor<Set<JavaType.FullyQualified>>() {
@Override
public Javadoc visitThrows(Javadoc.Throws aThrows, Set<JavaType.FullyQualified> candidates) {
if (aThrows.getExceptionName() instanceof TypeTree) {
JavaType.FullyQualified exceptionType = TypeUtils.asFullyQualified(((TypeTree) aThrows.getExceptionName()).getType());
if (exceptionType != null) {
candidates.remove(exceptionType);
protected JavadocVisitor<Set<JavaType.FullyQualified>> getJavadocVisitor() {
return new JavadocVisitor<Set<JavaType.FullyQualified>>(this) {
@Override
public Javadoc visitThrows(Javadoc.Throws aThrows, Set<JavaType.FullyQualified> candidates) {
if (aThrows.getExceptionName() instanceof TypeTree) {
JavaType.FullyQualified exceptionType = TypeUtils.asFullyQualified(((TypeTree) aThrows.getExceptionName()).getType());
if (exceptionType != null) {
candidates.remove(exceptionType);
}
}
return super.visitThrows(aThrows, candidates);
}
}
return super.visitThrows(aThrows, candidates);
};
}
};
}.visit(method, candidates);
}
}.visit(method, candidates);
}
return candidates;
return candidates;
}
};
}

}
62 changes: 62 additions & 0 deletions src/test/java/org/openrewrite/staticanalysis/UnnecessaryThrowsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,4 +395,66 @@ public static class MyException extends Exception { }
)
);
}

@Issue("https://github.com/apache/maven/pull/2291")
@Test
void retainExceptionsForOverrides() {
rewriteRun(
//language=java
java(
"""
import java.io.FileNotFoundException;
class A {
// Someone marked this protected, and added exceptions that implementers can optionally use
protected void method() throws FileNotFoundException {
}
}
"""
),
java(
"""
import java.io.FileNotFoundException;
class B extends A {
@Override
protected void method() throws FileNotFoundException {
}
}
"""
),
java(
"""
import java.io.FileNotFoundException;
class C1 extends B {
@Override
protected final void method() throws FileNotFoundException {
}
}
""",
"""
class C1 extends B {
@Override
protected final void method() {
}
}
"""
),
java(
"""
import java.io.FileNotFoundException;
final class C2 extends B {
@Override
protected void method() throws FileNotFoundException {
}
}
""",
"""
final class C2 extends B {
@Override
protected void method() {
}
}
"""
)
);
}
}