diff --git a/src/main/java/org/openrewrite/staticanalysis/UnnecessaryThrows.java b/src/main/java/org/openrewrite/staticanalysis/UnnecessaryThrows.java index 8a4aa20165..1fea4249fa 100644 --- a/src/main/java/org/openrewrite/staticanalysis/UnnecessaryThrows.java +++ b/src/main/java/org/openrewrite/staticanalysis/UnnecessaryThrows.java @@ -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 @@ -136,74 +138,82 @@ private void removeThrownTypes(JavaType.@Nullable Method type) { return m; } - }; - } + private Set findExceptionCandidates(J.@Nullable MethodDeclaration method) { - private Set 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 candidates = new TreeSet<>(Comparator.comparing(JavaType.FullyQualified::getFullyQualifiedName)); - //Collect all checked exceptions. - Set 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 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 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>() { - @Override - protected JavadocVisitor> getJavadocVisitor() { - return new JavadocVisitor>(this) { + if (!candidates.isEmpty()) { + //Remove any candidates that are defined in Javadocs for the method. + new JavaVisitor>() { @Override - public Javadoc visitThrows(Javadoc.Throws aThrows, Set candidates) { - if (aThrows.getExceptionName() instanceof TypeTree) { - JavaType.FullyQualified exceptionType = TypeUtils.asFullyQualified(((TypeTree) aThrows.getExceptionName()).getType()); - if (exceptionType != null) { - candidates.remove(exceptionType); + protected JavadocVisitor> getJavadocVisitor() { + return new JavadocVisitor>(this) { + @Override + public Javadoc visitThrows(Javadoc.Throws aThrows, Set 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; + } + }; } + } diff --git a/src/test/java/org/openrewrite/staticanalysis/UnnecessaryThrowsTest.java b/src/test/java/org/openrewrite/staticanalysis/UnnecessaryThrowsTest.java index 2040a141a0..88ae053a84 100644 --- a/src/test/java/org/openrewrite/staticanalysis/UnnecessaryThrowsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/UnnecessaryThrowsTest.java @@ -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() { + } + } + """ + ) + ); + } }