diff --git a/src/main/java/sorald/processor/UnclosedResourcesProcessor.java b/src/main/java/sorald/processor/UnclosedResourcesProcessor.java index d6bb67c53..1f4f24f2a 100644 --- a/src/main/java/sorald/processor/UnclosedResourcesProcessor.java +++ b/src/main/java/sorald/processor/UnclosedResourcesProcessor.java @@ -1,9 +1,12 @@ package sorald.processor; import java.util.List; +import java.util.Objects; +import java.util.stream.Stream; import sorald.annotations.ProcessorAnnotation; import spoon.reflect.code.CtAssignment; import spoon.reflect.code.CtBlock; +import spoon.reflect.code.CtCatch; import spoon.reflect.code.CtConstructorCall; import spoon.reflect.code.CtExpression; import spoon.reflect.code.CtLocalVariable; @@ -12,6 +15,7 @@ import spoon.reflect.code.CtTryWithResource; import spoon.reflect.code.CtVariableWrite; import spoon.reflect.declaration.CtElement; +import spoon.reflect.reference.CtLocalVariableReference; import spoon.reflect.reference.CtVariableReference; @ProcessorAnnotation(key = 2095, description = "Resources should be closed") @@ -30,7 +34,7 @@ protected void repairInternal(CtConstructorCall element) { } } - private void createCtTryWithResource(CtElement parent, CtLocalVariable variable) { + private void createCtTryWithResource(CtElement parent, CtLocalVariable variable) { CtTryWithResource tryWithResource = getFactory().createTryWithResource(); tryWithResource.addResource(variable); @@ -49,6 +53,9 @@ private void createCtTryWithResource(CtElement parent, CtLocalVariable variable) } else { encloseResourceInTryBlock((CtStatement) parent, tryWithResource); } + + cleanFinalizerAndCatchersOfReferencesToLocalVariableWithName( + tryWithResource, variable.getSimpleName()); } /** @@ -111,4 +118,28 @@ private CtBlock moveStatementsToNewBlock( } return block; } + + private void cleanFinalizerAndCatchersOfReferencesToLocalVariableWithName( + CtTry ctTry, String variableName) { + var blocksToClean = + Stream.concat( + Stream.of(ctTry.getFinalizer()), + ctTry.getCatchers().stream().map(CtCatch::getBody)) + .filter(Objects::nonNull); + blocksToClean.forEach( + block -> + deleteStatementsInBlockThatReferenceLocalVariableWithName( + block, variableName)); + } + + private void deleteStatementsInBlockThatReferenceLocalVariableWithName( + CtBlock block, String variableName) { + List> varRefs = + block.filterChildren(CtLocalVariableReference.class::isInstance).list(); + + varRefs.stream() + .filter(ref -> variableName.equals(ref.getSimpleName())) + .map(ref -> ref.getParent(CtStatement.class)) + .forEach(CtStatement::delete); + } } diff --git a/src/test/resources/processor_test_files/2095_UnclosedResources/BadCloseInFinally.java b/src/test/resources/processor_test_files/2095_UnclosedResources/BadCloseInFinally.java new file mode 100644 index 000000000..5195b2cbc --- /dev/null +++ b/src/test/resources/processor_test_files/2095_UnclosedResources/BadCloseInFinally.java @@ -0,0 +1,40 @@ +/* +Sometimes there is an "insufficient" close in the finalizer. In this example, bw1.close() might +throw, so bw2.close() may not execute. In such cases, Sonar flags the bw2 initialization as an +unclosed resource, but when we inline that into a resources block we must also make sure to clean +out any references to the variable in the finalizer. + */ + +import java.io.BufferedWriter; +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; + +public class BadCloseInFinally { + + public static void saveTo(File file1, File file2) { + BufferedWriter bw1 = null; + BufferedWriter bw2 = null; + try { + bw1 = new BufferedWriter(new FileWriter(file1)); + bw1.write("Write some stuff to file 1"); + + bw2 = new BufferedWriter(new FileWriter(file2)); // Noncompliant + bw2.write("Write some stuff to file 2"); + } catch (IOException e) { + e.printStackTrace(); + } finally { + System.out.println("done"); + try { + if (bw1 != null) { + bw1.close(); + } + if (bw2 != null) { + bw2.close(); + } + } catch (IOException e) { + e.printStackTrace(); + } + } + } +} \ No newline at end of file diff --git a/src/test/resources/processor_test_files/2095_UnclosedResources/BadCloseInFinally.java.expected b/src/test/resources/processor_test_files/2095_UnclosedResources/BadCloseInFinally.java.expected new file mode 100644 index 000000000..fb33eeb20 --- /dev/null +++ b/src/test/resources/processor_test_files/2095_UnclosedResources/BadCloseInFinally.java.expected @@ -0,0 +1,35 @@ +/* +Sometimes there is an "insufficient" close in the finalizer. In this example, bw1.close() might +throw, so bw2.close() may not execute. In such cases, Sonar flags the bw2 initialization as an +unclosed resource, but when we inline that into a resources block we must also make sure to clean +out any references to the variable in the finalizer. + */ + +import java.io.BufferedWriter; +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; + +public class BadCloseInFinally { + + public static void saveTo(File file1, File file2) { + BufferedWriter bw1 = null; + try (BufferedWriter bw2 = new BufferedWriter(new FileWriter(file2))) { + bw1 = new BufferedWriter(new FileWriter(file1)); + bw1.write("Write some stuff to file 1"); + + bw2.write("Write some stuff to file 2"); + } catch (IOException e) { + e.printStackTrace(); + } finally { + System.out.println("done"); + try { + if (bw1 != null) { + bw1.close(); + } + } catch (IOException e) { + e.printStackTrace(); + } + } + } +} \ No newline at end of file diff --git a/src/test/resources/processor_test_files/2095_UnclosedResources/ReferenceInCatcher.java b/src/test/resources/processor_test_files/2095_UnclosedResources/ReferenceInCatcher.java new file mode 100644 index 000000000..745bcc17b --- /dev/null +++ b/src/test/resources/processor_test_files/2095_UnclosedResources/ReferenceInCatcher.java @@ -0,0 +1,27 @@ +/* +When an unclosed resource is referenced in a catcher, we must ensure that those references are +removed when the resource is inlined into the resource list. + */ + +import java.io.BufferedWriter; +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; + +public class ReferenceInCatcher { + + public static void saveTo(File file) throws IOException { + BufferedWriter bw = null; + try { + bw = new BufferedWriter(new FileWriter(file)); // Noncompliant + bw.write("Write some stuff to file"); + } catch (IOException e) { + e.printStackTrace(); + if (bw != null) { + bw.close(); + } + } finally { + System.out.println("done"); + } + } +} diff --git a/src/test/resources/processor_test_files/2095_UnclosedResources/ReferenceInCatcher.java.expected b/src/test/resources/processor_test_files/2095_UnclosedResources/ReferenceInCatcher.java.expected new file mode 100644 index 000000000..c78c496de --- /dev/null +++ b/src/test/resources/processor_test_files/2095_UnclosedResources/ReferenceInCatcher.java.expected @@ -0,0 +1,22 @@ +/* +When an unclosed resource is referenced in a catcher, we must ensure that those references are +removed when the resource is inlined into the resource list. + */ + +import java.io.BufferedWriter; +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; + +public class ReferenceInCatcher { + + public static void saveTo(File file) throws IOException { + try (BufferedWriter bw = new BufferedWriter(new FileWriter(file))) { + bw.write("Write some stuff to file"); + } catch (IOException e) { + e.printStackTrace(); + } finally { + System.out.println("done"); + } + } +} diff --git a/src/test/resources/processor_test_files/2095_UnclosedResources/ReferenceInCatcherAndFinalizerToFieldWithSameNameAsResource.java b/src/test/resources/processor_test_files/2095_UnclosedResources/ReferenceInCatcherAndFinalizerToFieldWithSameNameAsResource.java new file mode 100644 index 000000000..5770f6ea2 --- /dev/null +++ b/src/test/resources/processor_test_files/2095_UnclosedResources/ReferenceInCatcherAndFinalizerToFieldWithSameNameAsResource.java @@ -0,0 +1,32 @@ +/* +When the processor inlines a local variable declaration into the resources block, it also cleans +up referenses to said variable in the catchers and finalizer. This test case includes a field +with the same name as the inlined variable, and is meant to verify that we don't accidentally +delete references to this unrelated field. + */ + +import java.io.BufferedWriter; +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; + +public class ReferenceInCatcherAndFinalizerToFieldWithSameNameAsResource { + private int bw = 2; + + public void saveTo(File file) throws IOException { + BufferedWriter bw = null; + try { + bw = new BufferedWriter(new FileWriter(file)); // Noncompliant + bw.write("Write some stuff to file"); + } catch (IOException e) { + e.printStackTrace(); + System.out.println(this.bw); // reference to field; should not be cleaned! + if (bw != null) { // reference to local variable; should be cleaned! + bw.close(); + } + } finally { + System.out.println(this.bw); // reference to field; should not be cleaned! + System.out.println("done"); + } + } +} \ No newline at end of file diff --git a/src/test/resources/processor_test_files/2095_UnclosedResources/ReferenceInCatcherAndFinalizerToFieldWithSameNameAsResource.java.expected b/src/test/resources/processor_test_files/2095_UnclosedResources/ReferenceInCatcherAndFinalizerToFieldWithSameNameAsResource.java.expected new file mode 100644 index 000000000..9e78d5dc7 --- /dev/null +++ b/src/test/resources/processor_test_files/2095_UnclosedResources/ReferenceInCatcherAndFinalizerToFieldWithSameNameAsResource.java.expected @@ -0,0 +1,27 @@ +/* +When the processor inlines a local variable declaration into the resources block, it also cleans +up referenses to said variable in the catchers and finalizer. This test case includes a field +with the same name as the inlined variable, and is meant to verify that we don't accidentally +delete references to this unrelated field. + */ + +import java.io.BufferedWriter; +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; + +public class ReferenceInCatcherAndFinalizerToFieldWithSameNameAsResource { + private int bw = 2; + + public void saveTo(File file) throws IOException { + try (BufferedWriter bw = new BufferedWriter(new FileWriter(file))) { + bw.write("Write some stuff to file"); + } catch (IOException e) { + e.printStackTrace(); + System.out.println(this.bw); // reference to field; should not be cleaned! + } finally { + System.out.println(this.bw); // reference to field; should not be cleaned! + System.out.println("done"); + } + } +} \ No newline at end of file