Skip to content

Commit

Permalink
Revisit ownership of static fields of a resource type
Browse files Browse the repository at this point in the history 
fixes eclipse-jdt#2161
+ warn about static resource fields (if annotations are enabled)
+ no warning on any field of whitelisted type
+ warn on field initialization in static block, but not at field
+ resolve left-over from bug 552521: no warnings in unreachable code
+ make new problems from PR eclipse-jdt#1716 suppressable (token = "resource")
Tests:
+ in annotation mode annotate some fields as @owning for equal result
  • Loading branch information
@stephan-herrmann
stephan-herrmann committed Apr 4, 2024
1 parent 58696fc commit 8f751d3
Show file tree
Hide file tree
Showing 10 changed files with 214 additions and 74 deletions.
2 changes: 2 additions & 0 deletions org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/core/compiler/IProblem.java
View file
Expand Up @@ -2068,6 +2068,8 @@ public interface IProblem {
int OverrideReducingParamterOwning = Internal + 1266;
/** @since 3.37 */
int OverrideAddingReturnOwning = Internal + 1267;
/** @since 3.38 */
int StaticResourceField = Internal + 1268;

// terminally
/** @since 3.14 */
Expand Down
15 changes: 12 additions & 3 deletions ....core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/FakedTrackingVariable.java
View file
Expand Up @@ -242,6 +242,8 @@ private static FakedTrackingVariable getRiskyCloseTrackerAt(LocalVariableBinding
* @return a new {@link FakedTrackingVariable} or null.
*/
public static FakedTrackingVariable getCloseTrackingVariable(Expression expression, FlowInfo flowInfo, FlowContext flowContext, boolean useAnnotations) {
if (flowInfo.reachMode() != FlowInfo.REACHABLE)
return null;
while (true) {
if (expression instanceof CastExpression)
expression = ((CastExpression) expression).expression;
Expand Down Expand Up @@ -425,6 +427,8 @@ private static void preConnectTrackerAcrossAssignment(ASTNode location, LocalVar
* See Bug 358903 - Filter practically unimportant resource leak warnings
*/
public static void analyseCloseableAllocation(BlockScope scope, FlowInfo flowInfo, FlowContext flowContext, AllocationExpression allocation) {
if (flowInfo.reachMode() != FlowInfo.REACHABLE)
return;
// client has checked that the resolvedType is an AutoCloseable, hence the following cast is safe:
if (((ReferenceBinding)allocation.resolvedType).hasTypeBit(TypeIds.BitResourceFreeCloseable)) {
// remove unnecessary attempts (closeable is not relevant)
Expand Down Expand Up @@ -842,8 +846,8 @@ public static void handleResourceFieldAssignment(BlockScope scope, FlowInfo flow
if (fieldReference.receiver.isThis())
field = fieldReference.binding;
}
if (field != null) {
if (!field.isStatic() && (field.tagBits & TagBits.AnnotationOwning) != 0) {
if (field != null&& (field.tagBits & TagBits.AnnotationNotOwning) == 0) { // assignment to @NotOwned has no meaning
if ((field.tagBits & TagBits.AnnotationOwning) != 0) {
rhsTrackVar.markNullStatus(flowInfo, flowContext, FlowInfo.NON_NULL);
} else {
rhsTrackVar.markAsShared();
Expand Down Expand Up @@ -1008,6 +1012,11 @@ protected static int getNullStatusFromMessageSend(Expression expression) {
public static void cleanUpAfterAssignment(BlockScope currentScope, int lhsBits, Expression expression) {
// remove all remaining track vars with no original binding

boolean useAnnotations = currentScope.compilerOptions().isAnnotationBasedResourceAnalysisEnabled;
if (useAnnotations && (lhsBits & Binding.FIELD) != 0) {
return;
}

// unwrap uninteresting nodes:
while (true) {
if (expression instanceof Assignment)
Expand All @@ -1034,7 +1043,7 @@ else if (expression instanceof CastExpression)
LocalVariableBinding local = expression.localVariableBinding();
if (local != null
&& ((lhsBits & Binding.FIELD) != 0)
&& !currentScope.compilerOptions().isAnnotationBasedResourceAnalysisEnabled
&& !useAnnotations
&& local.closeTracker != null) {
local.closeTracker.withdraw(); // TODO: may want to use local.closeTracker.markPassedToOutside(..,true)
}
Expand Down
6 changes: 4 additions & 2 deletions ...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/FieldDeclaration.java
View file
Expand Up @@ -109,9 +109,11 @@ public FlowInfo analyseCode(MethodScope initializationScope, FlowContext flowCon
}
if (options.isAnnotationBasedResourceAnalysisEnabled
&& this.binding != null
&& this.binding.type.hasTypeBit(TypeIds.BitAutoCloseable|TypeIds.BitCloseable))
&& FakedTrackingVariable.isCloseableNotWhiteListed(this.binding.type))
{
if ((this.binding.tagBits & TagBits.AnnotationOwning) == 0) {
if (this.binding.isStatic()) {
initializationScope.problemReporter().staticResourceField(this);
} else if ((this.binding.tagBits & TagBits.AnnotationOwning) == 0) {
initializationScope.problemReporter().shouldMarkFieldAsOwning(this);
} else if (!this.binding.declaringClass.hasTypeBit(TypeIds.BitAutoCloseable|TypeIds.BitCloseable)) {
initializationScope.problemReporter().shouldImplementAutoCloseable(this);
Expand Down
2 changes: 1 addition & 1 deletion ...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/MessageSend.java
View file
Expand Up @@ -159,7 +159,7 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
yieldQualifiedCheck(currentScope);
// recording the closing of AutoCloseable resources:
CompilerOptions compilerOptions = currentScope.compilerOptions();
boolean analyseResources = compilerOptions.analyseResourceLeaks;
boolean analyseResources = compilerOptions.analyseResourceLeaks && flowInfo.reachMode() == FlowInfo.REACHABLE;
if (analyseResources) {
if (nonStatic) {
// closeable.close()
Expand Down
6 changes: 4 additions & 2 deletions ...lipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/IrritantSet.java
View file
Expand Up @@ -182,8 +182,10 @@ public class IrritantSet {
STATIC_METHOD
.set(CompilerOptions.MethodCanBePotentiallyStatic);
RESOURCE
.set(CompilerOptions.PotentiallyUnclosedCloseable)
.set(CompilerOptions.ExplicitlyClosedAutoCloseable);
.set(CompilerOptions.PotentiallyUnclosedCloseable
|CompilerOptions.ExplicitlyClosedAutoCloseable)
.set(CompilerOptions.InsufficientResourceManagement
|CompilerOptions.IncompatibleOwningContract);
INCOMPLETE_SWITCH.set(CompilerOptions.MissingDefaultCase);
String suppressRawWhenUnchecked = System.getProperty("suppressRawWhenUnchecked"); //$NON-NLS-1$
if (suppressRawWhenUnchecked != null && "true".equalsIgnoreCase(suppressRawWhenUnchecked)) { //$NON-NLS-1$
Expand Down
9 changes: 9 additions & 0 deletions ...dt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java
View file
Expand Up @@ -699,6 +699,7 @@ public static int getIrritant(int problemID) {
case IProblem.NotOwningResourceField:
case IProblem.OwningFieldInNonResourceClass:
case IProblem.OwningFieldShouldImplementClose:
case IProblem.StaticResourceField:
return CompilerOptions.InsufficientResourceManagement;
case IProblem.OverrideReducingParamterOwning:
case IProblem.OverrideAddingReturnOwning:
Expand Down Expand Up @@ -10324,6 +10325,14 @@ public void shouldMarkFieldAsOwning(ASTNode location) {
location.sourceStart,
location.sourceEnd);
}
public void staticResourceField(FieldDeclaration fieldDeclaration) {
this.handle(
IProblem.StaticResourceField,
NoArgument,
NoArgument,
fieldDeclaration.sourceStart,
fieldDeclaration.sourceEnd);
}
public void shouldImplementAutoCloseable(ASTNode location) {
char[] name = this.options.owningAnnotationName[this.options.owningAnnotationName.length-1];
String[] args = { String.valueOf(name) };
Expand Down
1 change: 1 addition & 0 deletions ...jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/messages.properties
View file
Expand Up @@ -920,6 +920,7 @@
1265 = Class with resource fields tagged as ''@{0}'' should implement ''close()''
1266 = Unsafe redefinition, super method tagged this parameter as ''@{0}''
1267 = Unsafe redefinition, super method is not tagged as ''@{0}''
1268 = It is not recommended to hold a resource in a static field

# Java9 - Module declaration related
1300 = {0} cannot be resolved to a module
Expand Down
2 changes: 2 additions & 0 deletions ....compiler/src/org/eclipse/jdt/core/tests/compiler/regression/CompilerInvocationTests.java
View file
Expand Up @@ -1037,6 +1037,7 @@ class ProblemAttributes {
expectedProblemAttributes.put("StaticMemberOfParameterizedType", new ProblemAttributes(CategorizedProblem.CAT_TYPE));
expectedProblemAttributes.put("StaticMethodRequested", new ProblemAttributes(CategorizedProblem.CAT_MEMBER));
expectedProblemAttributes.put("StaticMethodShouldBeAccessedStatically", new ProblemAttributes(CategorizedProblem.CAT_MEMBER));
expectedProblemAttributes.put("StaticResourceField", new ProblemAttributes(CategorizedProblem.CAT_POTENTIAL_PROGRAMMING_PROBLEM));
expectedProblemAttributes.put("StringConstantIsExceedingUtf8Limit", new ProblemAttributes(CategorizedProblem.CAT_INTERNAL));
expectedProblemAttributes.put("SuperAccessCannotBypassDirectSuper", new ProblemAttributes(CategorizedProblem.CAT_TYPE));
expectedProblemAttributes.put("SuperCallCannotBypassOverride", new ProblemAttributes(CategorizedProblem.CAT_MEMBER));
Expand Down Expand Up @@ -2156,6 +2157,7 @@ class ProblemAttributes {
expectedProblemAttributes.put("StaticMemberOfParameterizedType", SKIP);
expectedProblemAttributes.put("StaticMethodRequested", SKIP);
expectedProblemAttributes.put("StaticMethodShouldBeAccessedStatically", SKIP);
expectedProblemAttributes.put("StaticResourceField", new ProblemAttributes(JavaCore.COMPILER_PB_RECOMMENDED_RESOURCE_MANAGEMENT));
expectedProblemAttributes.put("StringConstantIsExceedingUtf8Limit", SKIP);
expectedProblemAttributes.put("SuperAccessCannotBypassDirectSuper", SKIP);
expectedProblemAttributes.put("SuperCallCannotBypassOverride", SKIP);
Expand Down
140 changes: 133 additions & 7 deletions ...mpiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakAnnotatedTests.java
View file
Expand Up @@ -147,7 +147,21 @@ protected String potentialLeakOrCloseNotShownAtExit(String resourceName) {
protected String potentialOrDefiniteLeak(String string) {
return "Resource leak: '"+string+"' is never closed\n";
}

protected String fieldDeclPrefix() {
return "@org.eclipse.jdt.annotation.Owning "; // intentionally no linebreak
}
/** Override to add annotation to some tests from the super class. */
@Override
protected void runConformTest(String[] testFiles, String expectedOutput, Map<String, String> customOptions) {
testFiles = addAnnotationSources(testFiles);
super.runConformTest(testFiles, expectedOutput, customOptions);
}
/** Override to add annotation to some tests from the super class. */
@Override
protected void runLeakTest(String[] testFiles, String expectedCompileError, Map options) {
testFiles = addAnnotationSources(testFiles);
super.runLeakTest(testFiles, expectedCompileError, options);
}
private void runLeakTestWithAnnotations(String[] testFiles, String expectedProblems, Map<String, String> options) {
runLeakTestWithAnnotations(testFiles, expectedProblems, options, true);
}
Expand All @@ -160,14 +174,18 @@ private void runLeakTestWithAnnotations(String[] testFiles, String expectedProbl
options.put(CompilerOptions.OPTION_ReportExplicitlyClosedAutoCloseable, CompilerOptions.INFO);
if (options.get(CompilerOptions.OPTION_ReportInsufficientResourceManagement).equals(CompilerOptions.IGNORE))
options.put(CompilerOptions.OPTION_ReportInsufficientResourceManagement, CompilerOptions.INFO);
int length = testFiles.length;
System.arraycopy(testFiles, 0, testFiles = new String[length+4], 4, length);
testFiles[0] = OWNING_JAVA;
testFiles[1] = OWNING_CONTENT;
testFiles[2] = NOTOWNING_JAVA;
testFiles[3] = NOTOWNING_CONTENT;
testFiles = addAnnotationSources(testFiles);
runLeakTest(testFiles, expectedProblems, options, shouldFlushOutputDirectory);
}
private String[] addAnnotationSources(String[] testFiles) {
int length = testFiles.length;
System.arraycopy(testFiles, 0, testFiles = new String[length+4], 0, length);
testFiles[length+0] = OWNING_JAVA;
testFiles[length+1] = OWNING_CONTENT;
testFiles[length+2] = NOTOWNING_JAVA;
testFiles[length+3] = NOTOWNING_CONTENT;
return testFiles;
}

@Override
protected String getTest056e_log() {
Expand Down Expand Up @@ -1587,4 +1605,112 @@ void consumer(ResourceProducer producer) {
""",
options);
}
public void testGH2161_staticBlock() {
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[] {
"ClassWithStatics.java",
"""
import org.eclipse.jdt.annotation.*;
class RC implements AutoCloseable {
public void close() {}
}
public class ClassWithStatics {
private static AutoCloseable f1;
protected static @Owning AutoCloseable f2;
public static @NotOwning AutoCloseable f3;
static @SuppressWarnings("resource") @Owning AutoCloseable fSilent;
static {
f1 = new RC();
System.out.print(f1); // avoid unused warning
f2 = new RC();
f3 = new RC();
fSilent = new RC();
}
}
"""
},
"""
----------
1. INFO in ClassWithStatics.java (at line 7)
private static AutoCloseable f1;
^^
It is not recommended to hold a resource in a static field
----------
2. INFO in ClassWithStatics.java (at line 8)
protected static @Owning AutoCloseable f2;
^^
It is not recommended to hold a resource in a static field
----------
3. INFO in ClassWithStatics.java (at line 9)
public static @NotOwning AutoCloseable f3;
^^
It is not recommended to hold a resource in a static field
----------
4. ERROR in ClassWithStatics.java (at line 13)
f1 = new RC();
^^^^^^^^
Mandatory close of resource \'<unassigned Closeable value>\' has not been shown
----------
5. ERROR in ClassWithStatics.java (at line 16)
f3 = new RC();
^^^^^^^^
Resource leak: \'<unassigned Closeable value>\' is never closed
----------
""",
options);
}
public void testGH2161_initializers() {
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[] {
"ClassWithStatics.java",
"""
import org.eclipse.jdt.annotation.*;
import java.io.StringWriter;
class RC implements AutoCloseable {
public void close() {}
}
public class ClassWithStatics {
private static AutoCloseable f1 = new RC();
protected static @Owning AutoCloseable f2 = new RC();
public static @NotOwning AutoCloseable f3 = new RC();
static @SuppressWarnings("resource") @Owning AutoCloseable fSilent = new RC();
static StringWriter sw = new StringWriter(); // no reason to complain: white listed
static {
System.out.print(f1); // avoid unused warning :)
}
}
"""
},
"""
----------
1. INFO in ClassWithStatics.java (at line 8)
private static AutoCloseable f1 = new RC();
^^
It is not recommended to hold a resource in a static field
----------
2. INFO in ClassWithStatics.java (at line 9)
protected static @Owning AutoCloseable f2 = new RC();
^^
It is not recommended to hold a resource in a static field
----------
3. INFO in ClassWithStatics.java (at line 10)
public static @NotOwning AutoCloseable f3 = new RC();
^^
It is not recommended to hold a resource in a static field
----------
""",
options);
}
}

0 comments on commit 8f751d3

Please sign in to comment.