Skip to content

Commit

Permalink
Resorce Leak compiler error/warning (detection) has some problem (ecl…
Browse files Browse the repository at this point in the history 
…ipse-jdt#2257)

fixes eclipse-jdt#2207

+ treat body of an expression lambda like a return statement
+ fetch owning bits from the lambda descriptor
  • Loading branch information
@stephan-herrmann
stephan-herrmann authored Mar 31, 2024
1 parent 36da2cb commit c717e88
Show file tree
Hide file tree
Showing 4 changed files with 189 additions and 29 deletions.
16 changes: 10 additions & 6 deletions ...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/LambdaExpression.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -617,12 +617,16 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, fi
this.scope.problemReporter().shouldReturn(returnTypeBinding, this);
}
}
} else { // Expression
if (currentScope.compilerOptions().isAnnotationBasedNullAnalysisEnabled
&& lambdaInfo.reachMode() == FlowInfo.REACHABLE)
{
Expression expression = (Expression)this.body;
checkAgainstNullAnnotation(flowContext, expression, flowInfo, expression.nullStatus(lambdaInfo, flowContext));
} else if (this.body instanceof Expression expression ){
if (lambdaInfo.reachMode() == FlowInfo.REACHABLE) {
CompilerOptions compilerOptions = currentScope.compilerOptions();
if (compilerOptions.isAnnotationBasedNullAnalysisEnabled) {
checkAgainstNullAnnotation(flowContext, expression, flowInfo, expression.nullStatus(lambdaInfo, flowContext));
}
if (compilerOptions.analyseResourceLeaks) {
long owningTagBits = this.descriptor.tagBits & TagBits.AnnotationOwningMASK;
ReturnStatement.anylizeCloseableReturnExpression(expression, currentScope, owningTagBits, flowContext, flowInfo);
}
}
}
return flowInfo;
Expand Down
53 changes: 31 additions & 22 deletions ...se.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ReturnStatement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import org.eclipse.jdt.internal.compiler.ASTVisitor;
import org.eclipse.jdt.internal.compiler.codegen.*;
import org.eclipse.jdt.internal.compiler.flow.*;
import org.eclipse.jdt.internal.compiler.impl.CompilerOptions;
import org.eclipse.jdt.internal.compiler.impl.Constant;
import org.eclipse.jdt.internal.compiler.lookup.*;

Expand Down Expand Up @@ -89,29 +90,14 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
if (this.expression != null) {
flowInfo = this.expression.analyseCode(currentScope, flowContext, flowInfo);
this.expression.checkNPEbyUnboxing(currentScope, flowContext, flowInfo);
if (flowInfo.reachMode() == FlowInfo.REACHABLE && currentScope.compilerOptions().isAnnotationBasedNullAnalysisEnabled)
checkAgainstNullAnnotation(currentScope, flowContext, flowInfo, this.expression);
if (currentScope.compilerOptions().analyseResourceLeaks) {
boolean returnWithoutOwning = false;
boolean useOwningAnnotations = currentScope.compilerOptions().isAnnotationBasedResourceAnalysisEnabled;
FakedTrackingVariable trackingVariable = FakedTrackingVariable.getCloseTrackingVariable(this.expression, flowInfo, flowContext, useOwningAnnotations);
if (trackingVariable != null) {
long owningTagBits = 0;
boolean delegatingToCaller = true;
if (useOwningAnnotations) {
owningTagBits = methodScope.referenceMethodBinding().tagBits & TagBits.AnnotationOwningMASK;
returnWithoutOwning = owningTagBits == 0;
delegatingToCaller = (owningTagBits & TagBits.AnnotationNotOwning) == 0;
}
if (methodScope != trackingVariable.methodScope && delegatingToCaller)
trackingVariable.markClosedInNestedMethod();
if (delegatingToCaller) {
// by returning the method passes the responsibility to the caller:
flowInfo = FakedTrackingVariable.markPassedToOutside(currentScope, this.expression, flowInfo, flowContext, true);
}
if (flowInfo.reachMode() == FlowInfo.REACHABLE) {
CompilerOptions compilerOptions = currentScope.compilerOptions();
if (compilerOptions.isAnnotationBasedNullAnalysisEnabled)
checkAgainstNullAnnotation(currentScope, flowContext, flowInfo, this.expression);
if (compilerOptions.analyseResourceLeaks) {
long owningTagBits = methodScope.referenceMethodBinding().tagBits & TagBits.AnnotationOwningMASK;
flowInfo = anylizeCloseableReturnExpression(this.expression, currentScope, owningTagBits, flowContext, flowInfo);
}
// don't wait till after this statement, because then flowInfo would be DEAD_END & thus cannot serve nullStatus any more:
FakedTrackingVariable.cleanUpUnassigned(currentScope, this.expression, flowInfo, returnWithoutOwning);
}
}
this.initStateIndex =
Expand Down Expand Up @@ -194,6 +180,29 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
flowContext.expireNullCheckedFieldInfo();
return FlowInfo.DEAD_END;
}

public static FlowInfo anylizeCloseableReturnExpression(Expression returnExpression, BlockScope scope,
long owningTagBits, FlowContext flowContext, FlowInfo flowInfo) {
boolean returnWithoutOwning = false;
boolean useOwningAnnotations = scope.compilerOptions().isAnnotationBasedResourceAnalysisEnabled;
FakedTrackingVariable trackingVariable = FakedTrackingVariable.getCloseTrackingVariable(returnExpression, flowInfo, flowContext, useOwningAnnotations);
if (trackingVariable != null) {
boolean delegatingToCaller = true;
if (useOwningAnnotations) {
returnWithoutOwning = owningTagBits == 0;
delegatingToCaller = (owningTagBits & TagBits.AnnotationNotOwning) == 0;
}
if (scope.methodScope() != trackingVariable.methodScope && delegatingToCaller)
trackingVariable.markClosedInNestedMethod();
if (delegatingToCaller) {
// by returning the method passes the responsibility to the caller:
flowInfo = FakedTrackingVariable.markPassedToOutside(scope, returnExpression, flowInfo, flowContext, true);
}
}
// don't wait till after this statement, because then flowInfo would be DEAD_END & thus cannot serve nullStatus any more:
FakedTrackingVariable.cleanUpUnassigned(scope, returnExpression, flowInfo, returnWithoutOwning);
return flowInfo;
}
@Override
public boolean doesNotCompleteNormally() {
return true;
Expand Down
123 changes: 122 additions & 1 deletion ...mpiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakAnnotatedTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1459,5 +1459,126 @@ static void test() {
""",
null,
false);
}
}
public void testGH2207_2() {
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[] {
"ResourceLeakTest.java",
"""
import org.eclipse.jdt.annotation.Owning;
class RC implements AutoCloseable {
public void close() {}
}
interface ResourceProducer {
@Owning RC newResource();
}
public class ResourceLeakTest {
public void test() {
consumerOK(() -> new RC());
}
void consumerOK(ResourceProducer producer) {
try (RC ac = producer.newResource()) {
System.out.println(ac);
}
}
void consumerNOK(ResourceProducer producer) {
RC ac = producer.newResource();
}
}
"""
},
"""
----------
1. ERROR in ResourceLeakTest.java (at line 20)
RC ac = producer.newResource();
^^
Resource leak: \'ac\' is never closed
----------
""",
options);
}
public void testGH2207_3() {
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[] {
"ResourceLeakTest.java",
"""
import org.eclipse.jdt.annotation.NotOwning;
class RC implements AutoCloseable {
public void close() {}
}
interface ResourceProducer {
@NotOwning RC newResource();
}
public class ResourceLeakTest {
public void test(@NotOwning RC rcParm) {
consumer(() -> new RC());
consumer(() -> rcParm);
}
void consumer(ResourceProducer producer) {
RC ac = producer.newResource();
}
}
"""
},
"""
----------
1. ERROR in ResourceLeakTest.java (at line 12)
consumer(() -> new RC());
^^^^^^^^
Resource leak: \'<unassigned Closeable value>\' is never closed
----------
""",
options);
}
public void testGH2207_4() {
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[] {
"ResourceLeakTest.java",
"""
import org.eclipse.jdt.annotation.NotOwning;
class RC implements AutoCloseable {
public void close() {}
}
interface ResourceProducer {
@NotOwning RC newResource();
}
public class ResourceLeakTest {
public void test(@NotOwning RC rcParm) {
consumer(() -> new RC());
consumer(() -> rcParm);
}
void consumer(ResourceProducer producer) {
RC ac = producer.newResource();
}
}
"""
},
"""
----------
1. ERROR in ResourceLeakTest.java (at line 12)
consumer(() -> new RC());
^^^^^^^^
Resource leak: \'<unassigned Closeable value>\' is never closed
----------
""",
options);
}
}
26 changes: 26 additions & 0 deletions ....tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7147,4 +7147,30 @@ void test(int sw) {
"----------\n",
options);
}
public void testGH2207_1() {
// relevant only since 19, where ExecutorService implements AutoCloseable
Map options = getCompilerOptions();
options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.ERROR);
options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR);
options.put(CompilerOptions.OPTION_ReportExplicitlyClosedAutoCloseable, CompilerOptions.ERROR);
runLeakTest(
new String[] {
"ResourceLeakTest.java",
"""
import java.util.Optional;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
public class ResourceLeakTest {
protected ExecutorService es;
public ExecutorService t_supplier_lambda_returned(ExecutorService executor) {
return Optional.ofNullable(executor).orElseGet(() -> Executors.newCachedThreadPool());
}
}
"""
},
"",
options);
}
}

0 comments on commit c717e88

Please sign in to comment.