False Positive Potential Resource Leak Warnings - @NotOwning Not Working

Correctly + respect an explicit @NotOwning during field analysis + update testSharedField() to show that @NotOwning field is "safe" fixes eclipse-jdt#2635
stephan-herrmann · Sep 24, 2024 · a1001be · a1001be
1 parent 59bc0be
commit a1001be
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 10 deletions.
diff --git a/...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/FieldDeclaration.java b/...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/FieldDeclaration.java
@@ -111,11 +111,13 @@ public FlowInfo analyseCode(MethodScope initializationScope, FlowContext flowCon
 			&& this.binding != null
 			&& FakedTrackingVariable.isCloseableNotWhiteListed(this.binding.type))
 	{
+		long owningTagBits = this.binding.tagBits & TagBits.AnnotationOwningMASK;
 		if (this.binding.isStatic()) {
 			initializationScope.problemReporter().staticResourceField(this);
-		} else if ((this.binding.tagBits & TagBits.AnnotationOwning) == 0) {
+		} else if (owningTagBits == 0) {
 			initializationScope.problemReporter().shouldMarkFieldAsOwning(this);
-		} else if (!this.binding.declaringClass.hasTypeBit(TypeIds.BitAutoCloseable|TypeIds.BitCloseable)) {
+		} else if (owningTagBits == TagBits.AnnotationOwning
+				&& !this.binding.declaringClass.hasTypeBit(TypeIds.BitAutoCloseable|TypeIds.BitCloseable)) {
 			initializationScope.problemReporter().shouldImplementAutoCloseable(this);
 		}
 	}

diff --git a/...mpiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakAnnotatedTests.java b/...mpiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakAnnotatedTests.java
@@ -629,7 +629,7 @@ public class A {
 				PrintWriter fOther;
 				@Owning PrintWriter fProper;
 				boolean useField = false;
-				A(PrintWriter writer1, PrintWriter writer2, @Owning PrintWriter writer3) {
+				A(@Owning PrintWriter writer1, PrintWriter writer2, @Owning PrintWriter writer3) {
 					fWriter = writer1;
 					fOther = writer2;
 					fProper = writer3;
@@ -639,21 +639,21 @@ public class A {
 		},
 		"""
 		----------
-		1. WARNING in A.java (at line 5)
-			@NotOwning PrintWriter fWriter;
-			                       ^^^^^^^
-		It is recommended to mark resource fields as '@Owning' to ensure proper closing
-		----------
-		2. WARNING in A.java (at line 6)
+		1. WARNING in A.java (at line 6)
 			PrintWriter fOther;
 			            ^^^^^^
 		It is recommended to mark resource fields as '@Owning' to ensure proper closing
 		----------
-		3. WARNING in A.java (at line 7)
+		2. WARNING in A.java (at line 7)
 			@Owning PrintWriter fProper;
 			                    ^^^^^^^
 		Class with resource fields tagged as '@Owning' should implement AutoCloseable
 		----------
+		3. ERROR in A.java (at line 9)
+			A(@Owning PrintWriter writer1, PrintWriter writer2, @Owning PrintWriter writer3) {
+			                      ^^^^^^^
+		Resource leak: 'writer1' is never closed
+		----------
 		""",
 		options
 		);
@@ -1703,4 +1703,66 @@ public class ClassWithStatics {
 		""",
 		options);
 }
+public void testGH2635() {
+	Map<String, String> options = getCompilerOptions();
+	options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.ERROR);
+	options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR);
+	options.put(CompilerOptions.OPTION_ReportExplicitlyClosedAutoCloseable, CompilerOptions.ERROR);
+	runLeakTestWithAnnotations(
+		new String[] {
+			"owning_test/OwningTest.java",
+			"""
+			package owning_test;
+
+			import java.io.FileInputStream;
+			import java.io.FileNotFoundException;
+			import java.io.IOException;
+			import java.io.InputStream;
+
+			import org.eclipse.jdt.annotation.Owning;
+
+			public class OwningTest implements AutoCloseable {
+
+				@Owning
+				private InputStream fileInputStream;
+
+				@SuppressWarnings("unused")
+				private NotOwningTest cacheUser;
+
+				public void initialise() throws FileNotFoundException {
+				  fileInputStream = new FileInputStream("test.txt");
+				  cacheUser = new NotOwningTest(fileInputStream);
+				}
+
+				@Override
+				public void close() throws IOException {
+					fileInputStream.close();
+				}
+
+			}
+			""",
+			"owning_test/NotOwningTest.java",
+			"""
+			package owning_test;
+
+			import java.io.InputStream;
+
+			import org.eclipse.jdt.annotation.NotOwning;
+
+			public class NotOwningTest {
+
+				// If get a warning here - "It is recommended to mark resource fields as '@Owning' to ensure proper closing"
+				@NotOwning
+				private InputStream cacheInputStream;
+
+				NotOwningTest(InputStream aCacheInputStream) {
+					cacheInputStream = aCacheInputStream;
+				}
+
+			}
+			"""
+		},
+		"",
+		options);
+}
 }