diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties index c9839a9b7..7f7d33fc6 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/gradle/wrapper/gradle-wrapper.properties @@ -1,7 +1,7 @@ distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionUrl=https\://services.gradle.org/distributions/gradle-8.12-bin.zip -distributionSha256Sum=7a00d51fb93147819aab76024feece20b6b84e420694101f276be952e08bef03 +distributionUrl=https\://services.gradle.org/distributions/gradle-8.12.1-bin.zip +distributionSha256Sum=8d97a97984f6cbd2b85fe4c60a743440a347544bf18818048e611f5288d46c94 networkTimeout=10000 zipStoreBase=GRADLE_USER_HOME zipStorePath=wrapper/dists diff --git a/src/main/java/org/openrewrite/staticanalysis/AbstractClassPublicConstructor.java b/src/main/java/org/openrewrite/staticanalysis/AbstractClassPublicConstructor.java new file mode 100644 index 000000000..c1a8e363a --- /dev/null +++ b/src/main/java/org/openrewrite/staticanalysis/AbstractClassPublicConstructor.java @@ -0,0 +1,66 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.staticanalysis; + +import org.openrewrite.ExecutionContext; +import org.openrewrite.Recipe; +import org.openrewrite.TreeVisitor; +import org.openrewrite.internal.ListUtils; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.tree.J; + +import java.util.Collections; +import java.util.Set; + +import static org.openrewrite.java.tree.J.Modifier.Type.*; + +public class AbstractClassPublicConstructor extends Recipe { + @Override + public String getDisplayName() { + return "Constructors of an `abstract` class should not be declared `public`"; + } + + @Override + public String getDescription() { + return "Constructors of `abstract` classes can only be called in constructors of their subclasses. " + + "Therefore the visibility of `public` constructors are reduced to `protected`."; + } + + @Override + public Set getTags() { + return Collections.singleton("RSPEC-S5993"); + } + + @Override + public TreeVisitor getVisitor() { + return new JavaIsoVisitor() { + @Override + public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) { + J.ClassDeclaration cd = super.visitClassDeclaration(classDecl, ctx); + if (cd.hasModifier(Abstract)) { + return cd.withBody(cd.getBody().withStatements(ListUtils.map(cd.getBody().getStatements(), st -> { + if (st instanceof J.MethodDeclaration && ((J.MethodDeclaration) st).isConstructor()) { + return ((J.MethodDeclaration) st).withModifiers(ListUtils.map(((J.MethodDeclaration) st).getModifiers(), + mod -> mod.getType() == Public ? mod.withType(Protected) : mod)); + } + return st; + }))); + } + return cd; + } + }; + } +} diff --git a/src/main/java/org/openrewrite/staticanalysis/CatchClauseOnlyRethrows.java b/src/main/java/org/openrewrite/staticanalysis/CatchClauseOnlyRethrows.java index 6a23f2ee4..6eef79123 100755 --- a/src/main/java/org/openrewrite/staticanalysis/CatchClauseOnlyRethrows.java +++ b/src/main/java/org/openrewrite/staticanalysis/CatchClauseOnlyRethrows.java @@ -27,9 +27,10 @@ import org.openrewrite.java.tree.TypeUtils; import java.time.Duration; +import java.util.List; import java.util.Set; -import static java.util.Collections.singleton; +import static java.util.Collections.*; import static org.openrewrite.staticanalysis.csharp.CSharpFileChecker.isInstanceOfCs; public class CatchClauseOnlyRethrows extends Recipe { @@ -78,14 +79,10 @@ public J.Try visitTry(J.Try tryable, ExecutionContext ctx) { J.Try t = super.visitTry(tryable, ctx); return t.withCatches(ListUtils.map(t.getCatches(), (i, aCatch) -> { if (onlyRethrows(aCatch)) { - // if a subsequent catch is a wider exception type and doesn't rethrow, we should - // keep this one + // if a subsequent catch is a wider exception type and doesn't rethrow, we should keep this one for (int j = i + 1; j < tryable.getCatches().size(); j++) { J.Try.Catch next = tryable.getCatches().get(j); - if (hasWiderExceptionType(aCatch, next)) { - if (onlyRethrows(next)) { - return null; - } + if (isAnyAssignableTo(getJavaTypes(next), getJavaTypes(aCatch)) && !onlyRethrows(next)) { return aCatch; } } @@ -95,17 +92,23 @@ public J.Try visitTry(J.Try tryable, ExecutionContext ctx) { })); } - private boolean hasWiderExceptionType(J.Try.Catch aCatch, J.Try.Catch next) { + private List getJavaTypes(J.Try.Catch next) { if (next.getParameter().getType() instanceof JavaType.MultiCatch) { JavaType.MultiCatch multiCatch = (JavaType.MultiCatch) next.getParameter().getType(); - for (JavaType throwableType : multiCatch.getThrowableTypes()) { - if (TypeUtils.isAssignableTo(throwableType, aCatch.getParameter().getType())) { + return multiCatch.getThrowableTypes(); + } + return next.getParameter().getType() == null ? emptyList() : singletonList(next.getParameter().getType()); + } + + private boolean isAnyAssignableTo(List nextTypes, List aCatchTypes) { + for (JavaType aCatchType : aCatchTypes) { + for (JavaType nextType : nextTypes) { + if (TypeUtils.isAssignableTo(nextType, aCatchType)) { return true; } } - return false; } - return TypeUtils.isAssignableTo(next.getParameter().getType(), aCatch.getParameter().getType()); + return false; } private boolean onlyRethrows(J.Try.Catch aCatch) { @@ -116,9 +119,8 @@ private boolean onlyRethrows(J.Try.Catch aCatch) { Expression exception = ((J.Throw) aCatch.getBody().getStatements().get(0)).getException(); - // In C# an implicit rethrow is possible - if (isInstanceOfCs(getCursor().firstEnclosing(SourceFile.class)) && - exception instanceof J.Empty) { + // In C# an implicit rethrow is possible, which means a `throw` statement without a variable + if (isInstanceOfCs(getCursor().firstEnclosing(SourceFile.class)) && exception instanceof J.Empty) { return true; } diff --git a/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java b/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java index cd6286c10..2bb79480b 100644 --- a/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java +++ b/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java @@ -69,7 +69,7 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) J.MethodInvocation mi = (J.MethodInvocation) super.visitMethodInvocation(method, ctx); if (VALUE_OF.matches(mi) && mi.getArguments().size() == 1) { Expression argument = mi.getArguments().get(0); - if (argument instanceof J.Binary) { + if (argument instanceof J.Binary && removeValueOfForStringConcatenation(argument)) { return JavaTemplate.builder("(#{any(java.lang.String)})").build() .apply(updateCursor(mi), mi.getCoordinates().replace(), argument); } else if (TypeUtils.isString(argument.getType()) || removeValueOfForStringConcatenation(argument)) { diff --git a/src/main/java/org/openrewrite/staticanalysis/RemoveExtraSemicolons.java b/src/main/java/org/openrewrite/staticanalysis/RemoveExtraSemicolons.java index 45d8c1e93..e4089af2c 100644 --- a/src/main/java/org/openrewrite/staticanalysis/RemoveExtraSemicolons.java +++ b/src/main/java/org/openrewrite/staticanalysis/RemoveExtraSemicolons.java @@ -36,7 +36,11 @@ public String getDisplayName() { @Override public String getDescription() { - return "Optional semicolons at the end of try-with-resources are also removed."; + //language=markdown + return "Removes not needed semicolons. Semicolons are considered not needed:\n" + + "* Optional semicolons at the end of try-with-resources,\n" + + "* after the last enum value if no field or method is defined,\n" + + "* no statement between two semicolon."; } @Override diff --git a/src/main/java/org/openrewrite/staticanalysis/UnnecessaryCatch.java b/src/main/java/org/openrewrite/staticanalysis/UnnecessaryCatch.java index e8c758b26..81bb6058c 100644 --- a/src/main/java/org/openrewrite/staticanalysis/UnnecessaryCatch.java +++ b/src/main/java/org/openrewrite/staticanalysis/UnnecessaryCatch.java @@ -24,6 +24,7 @@ import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.J.NewClass; import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.TypeUtils; @@ -36,13 +37,21 @@ public class UnnecessaryCatch extends Recipe { @Option(displayName = "Include `java.lang.Exception`", - description = "Whether to include java.lang.Exception in the list of checked exceptions to remove. " + - "Unlike other checked exceptions, `java.lang.Exception` is also the superclass of unchecked exceptions. " + - "So removing `catch(Exception e)` may result in changed runtime behavior in the presence of unchecked exceptions. " + - "Default `false`", + description = "Whether to include `java.lang.Exception` in the list of checked exceptions to remove. " + + "Unlike other checked exceptions, `java.lang.Exception` is also the superclass of unchecked exceptions. " + + "So removing `catch(Exception e)` may result in changed runtime behavior in the presence of unchecked exceptions. " + + "Default `false`", required = false) boolean includeJavaLangException; + @Option(displayName = "Include `java.lang.Throwable`", + description = "Whether to include `java.lang.Throwable` in the list of exceptions to remove. " + + "Unlike other checked exceptions, `java.lang.Throwable` is also the superclass of unchecked exceptions. " + + "So removing `catch(Throwable e)` may result in changed runtime behavior in the presence of unchecked exceptions. " + + "Default `false`", + required = false) + boolean includeJavaLangThrowable; + @Override public String getDisplayName() { return "Remove catch for a checked exception if the try block does not throw that exception"; @@ -80,6 +89,18 @@ public J.Try visitTry(J.Try tryable, ExecutionContext ctx) { AtomicBoolean missingTypeInformation = new AtomicBoolean(false); //Collect any checked exceptions thrown from the try block. new JavaIsoVisitor() { + @Override + public NewClass visitNewClass(NewClass newClass, Integer integer) { + JavaType.Method methodType = newClass.getMethodType(); + if (methodType == null) { + //Do not make any changes if there is missing type information. + missingTypeInformation.set(true); + } else { + thrownExceptions.addAll(methodType.getThrownExceptions()); + } + return super.visitNewClass(newClass, integer); + } + @Override public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Integer integer) { JavaType.Method methodType = method.getMethodType(); @@ -108,6 +129,9 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Integ if (!includeJavaLangException && TypeUtils.isOfClassType(parameterType, "java.lang.Exception")) { return aCatch; } + if (!includeJavaLangThrowable && TypeUtils.isOfClassType(parameterType, "java.lang.Throwable")) { + return aCatch; + } for (JavaType e : thrownExceptions) { if (TypeUtils.isAssignableTo(e, parameterType)) { return aCatch; @@ -117,7 +141,6 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Integ return null; })); } - }; } } diff --git a/src/main/resources/META-INF/rewrite/common-static-analysis.yml b/src/main/resources/META-INF/rewrite/common-static-analysis.yml index c1cbb5417..c70ab5ec6 100644 --- a/src/main/resources/META-INF/rewrite/common-static-analysis.yml +++ b/src/main/resources/META-INF/rewrite/common-static-analysis.yml @@ -20,6 +20,7 @@ name: org.openrewrite.staticanalysis.CommonStaticAnalysis displayName: Common static analysis issues description: Resolve common static analysis issues (also known as SAST issues). recipeList: + - org.openrewrite.staticanalysis.AbstractClassPublicConstructor # - org.openrewrite.staticanalysis.AddSerialVersionUidToSerializable - org.openrewrite.staticanalysis.AtomicPrimitiveEqualsUsesGet - org.openrewrite.staticanalysis.BigDecimalDoubleConstructorRecipe diff --git a/src/test/java/org/openrewrite/staticanalysis/AbstractClassPublicConstructorTest.java b/src/test/java/org/openrewrite/staticanalysis/AbstractClassPublicConstructorTest.java new file mode 100644 index 000000000..985c34a76 --- /dev/null +++ b/src/test/java/org/openrewrite/staticanalysis/AbstractClassPublicConstructorTest.java @@ -0,0 +1,103 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.staticanalysis; + +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.Issue; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +@SuppressWarnings("java:S2699") +class AbstractClassPublicConstructorTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new AbstractClassPublicConstructor()); + } + + @DocumentExample + @Test + void replacePublicByProtected() { + rewriteRun( + //language=java + java( + """ + abstract class Test { + public Test() { + } + } + """, + """ + abstract class Test { + protected Test() { + } + } + """ + ) + ); + } + + @Test + void noReplaceOnNonAbstractClass() { + rewriteRun( + //language=java + java( + """ + class Test { + public Test() { + } + } + """ + ) + ); + } + + @Test + void noReplaceOnPackageProtectedConstructor() { + rewriteRun( + //language=java + java( + """ + abstract class Test { + Test() { + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/449") + @Test + void noReplaceOnNestedClasses() { + rewriteRun( + //language=java + java( + """ + abstract class Test { + static class Nested { + public Nested() { + } + } + } + """ + ) + ); + } +} diff --git a/src/test/java/org/openrewrite/staticanalysis/CatchClauseOnlyRethrowsTest.java b/src/test/java/org/openrewrite/staticanalysis/CatchClauseOnlyRethrowsTest.java index 46990490f..81c812a95 100755 --- a/src/test/java/org/openrewrite/staticanalysis/CatchClauseOnlyRethrowsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/CatchClauseOnlyRethrowsTest.java @@ -45,7 +45,7 @@ void foo() throws IOException { new FileReader("").read(); } catch (IOException e) { throw new IOException("another message", e); - } catch(Exception e) { + } catch (Exception e) { throw new Exception("another message"); } } @@ -70,9 +70,9 @@ void foo() throws IOException { new FileReader("").read(); } catch (IOException e) { throw e; - } catch(Exception e) { + } catch (Exception e) { System.out.println(e.getMessage()); - } catch(Throwable t) { + } catch (Throwable t) { t.printStackTrace(); } } @@ -83,7 +83,60 @@ void foo() throws IOException { } @Test - void catchShouldBePreservedBecauseLessSpecificCatchFollowsWithMultiCast() { + void catchShouldBePreservedBecauseLessSpecificCatchFollowsLater() { + rewriteRun( + //language=java + java( + """ + import java.io.FileReader; + import java.io.IOException; + import java.io.FileNotFoundException; + + class A { + void foo() throws IOException { + try { + new FileReader("").read(); + } catch (FileNotFoundException e) { + throw e; + } catch (IOException e) { + throw e; + } catch (Exception e) { + System.out.println(e.getMessage()); + } + } + } + """ + ) + ); + } + + @Test + void multiCatchShouldBePreservedBecauseLessSpecificCatchFollows() { + rewriteRun( + //language=java + java( + """ + import java.io.FileReader; + import java.io.IOException; + + class A { + void foo() throws IOException { + try { + new FileReader("").read(); + } catch (IOException | RuntimeException e) { + throw e; + } catch (Exception e) { + System.out.println(e.getMessage()); + } + } + } + """ + ) + ); + } + + @Test + void catchShouldBePreservedBecauseLessSpecificCatchFollowsWithMultiCatch() { rewriteRun( //language=java java( @@ -97,7 +150,32 @@ void foo() throws IOException { new FileReader("").read(); } catch (IOException e) { throw e; - } catch(Exception | Throwable t) { + } catch (Exception | Throwable t) { + t.printStackTrace(); + } + } + } + """ + ) + ); + } + + @Test + void mltiCatchShouldBePreservedBecauseLessSpecificCatchFollowsWithMultiCatch() { + rewriteRun( + //language=java + java( + """ + import java.io.FileReader; + import java.io.IOException; + + class A { + void foo() throws IOException { + try { + new FileReader("").read(); + } catch (IOException | RuntimeException e) { + throw e; + } catch (Exception | Throwable t) { t.printStackTrace(); } } @@ -157,9 +235,9 @@ void foo() throws IOException { new FileReader("").read(); } catch (FileNotFoundException e) { throw e; - } catch(IOException | ArrayIndexOutOfBoundsException e) { + } catch (IOException | ArrayIndexOutOfBoundsException e) { throw e; - } catch(Exception e) { + } catch (Exception e) { throw e; } } @@ -196,7 +274,7 @@ void foo() throws IOException { new FileReader("").read(); } catch (FileNotFoundException e) { throw e; - } catch(IOException | ArrayIndexOutOfBoundsException e) { + } catch (IOException | ArrayIndexOutOfBoundsException e) { throw new IOException("another message", e); } } diff --git a/src/test/java/org/openrewrite/staticanalysis/NoValueOfOnStringTypeTest.java b/src/test/java/org/openrewrite/staticanalysis/NoValueOfOnStringTypeTest.java index bf320e782..17bde044e 100644 --- a/src/test/java/org/openrewrite/staticanalysis/NoValueOfOnStringTypeTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/NoValueOfOnStringTypeTest.java @@ -143,6 +143,23 @@ static void count(int i) { ); } + @Test + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/456") + void valueOfOnNonStringPrimitiveWithBinaryArgument() { + rewriteRun( + //language=java + java( + """ + class Test { + static void method(int i) { + String s = String.valueOf(41 + 1); + } + } + """ + ) + ); + } + @Test @Issue("https://github.com/openrewrite/rewrite/issues/1200") void valueOfIsMethodInvocationPartOfBinary() { diff --git a/src/test/java/org/openrewrite/staticanalysis/RemoveExtraSemicolonsTest.java b/src/test/java/org/openrewrite/staticanalysis/RemoveExtraSemicolonsTest.java index 1de29550e..06ccb8641 100644 --- a/src/test/java/org/openrewrite/staticanalysis/RemoveExtraSemicolonsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/RemoveExtraSemicolonsTest.java @@ -68,7 +68,7 @@ void enumSemicolonsWithOtherStatements() { public enum FRUITS { BANANA, APPLE; - + void hiFruit() {} } """ @@ -215,6 +215,7 @@ int test() { } @Test + @DocumentExample void repeatedSemicolon() { rewriteRun( //language=java diff --git a/src/test/java/org/openrewrite/staticanalysis/UnnecessaryCatchTest.java b/src/test/java/org/openrewrite/staticanalysis/UnnecessaryCatchTest.java index 5b785cc60..3395bd29d 100644 --- a/src/test/java/org/openrewrite/staticanalysis/UnnecessaryCatchTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/UnnecessaryCatchTest.java @@ -27,7 +27,7 @@ class UnnecessaryCatchTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { - spec.recipe(new UnnecessaryCatch(false)); + spec.recipe(new UnnecessaryCatch(false, false)); } @DocumentExample @@ -140,6 +140,33 @@ public void fred() throws IOException { ); } + @Test + void doNotRemoveThrownExceptionFromConstructor() { + rewriteRun( + //language=java + java( + """ + import java.io.IOException; + + public class AnExample { + public void method() { + try { + new Fred(); + } catch (IOException e) { + System.out.println("an exception!"); + } + } + + public static class Fred { + public Fred() throws IOException {} + } + + } + """ + ) + ); + } + @Test void doNotRemoveJavaLangException() { rewriteRun( @@ -159,4 +186,24 @@ void method() { ) ); } + + @Test + void doNotRemoveJavaLangThrowable() { + rewriteRun( + //language=java + java( + """ + class Scratch { + void method() { + try { + throw new RuntimeException(); + } catch (Throwable e) { + System.out.println("an exception!"); + } + } + } + """ + ) + ); + } }