From c717e88e73455ef7a5b4b00e9540b2d0bb03e95c Mon Sep 17 00:00:00 2001 From: Stephan Herrmann Date: Sun, 31 Mar 2024 17:04:23 +0200 Subject: [PATCH] Resorce Leak compiler error/warning (detection) has some problem (#2257) fixes #2207 + treat body of an expression lambda like a return statement + fetch owning bits from the lambda descriptor --- .../compiler/ast/LambdaExpression.java | 16 ++- .../compiler/ast/ReturnStatement.java | 53 ++++---- .../ResourceLeakAnnotatedTests.java | 123 +++++++++++++++++- .../regression/ResourceLeakTests.java | 26 ++++ 4 files changed, 189 insertions(+), 29 deletions(-) diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/LambdaExpression.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/LambdaExpression.java index 2064e545c94..fc82da454d4 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/LambdaExpression.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/LambdaExpression.java @@ -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; diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ReturnStatement.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ReturnStatement.java index a68d4f039c0..8843c509800 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ReturnStatement.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ReturnStatement.java @@ -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.*; @@ -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 = @@ -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; diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakAnnotatedTests.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakAnnotatedTests.java index 35b2b88c8e3..06815088eaf 100644 --- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakAnnotatedTests.java +++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakAnnotatedTests.java @@ -1459,5 +1459,126 @@ static void test() { """, null, false); - } +} +public void testGH2207_2() { + Map 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 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: \'\' is never closed + ---------- + """, + options); +} +public void testGH2207_4() { + Map 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: \'\' is never closed + ---------- + """, + options); +} } diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java index 0406c8b4cef..d77d2917615 100644 --- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java +++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java @@ -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); +} }