Skip to content

Commit

Permalink
False Positive Potential Resource Leak Warnings - @NotOwning Not Working
Browse files Browse the repository at this point in the history
Correctly

+ respect an explicit @NotOwning during field analysis
+ update testSharedField() to show that @NotOwning field is "safe"

fixes #2635
  • Loading branch information
stephan-herrmann committed Aug 17, 2024
1 parent d116b06 commit c107810
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
);
Expand Down Expand Up @@ -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);
}
}

0 comments on commit c107810

Please sign in to comment.