Suppresses RLC non-final field overwrite warning for safe constructor field initialization #7050

iamsanjaymalakar · 2025-04-18T10:43:57Z

No description provided.

… field initialization

kelloggm

Overall looks good, just a few minor comments. I'd like to take one more look before we merge, though.

kelloggm · 2025-04-18T13:38:46Z

checker/tests/resourceleak-firstinitconstructor/ConstructorChainingLeak.java

+  private @Owning FileInputStream s;
+
+  public ConstructorChainingLeak() throws Exception {
+    this(42);


Can you add a test like this one but where the this(42) call is replaced by a call to a super constructor? I'd like to at least codify the behavior in that scenario (I think it would be safe not to warn in that constructor, but it's okay if there's an FP now).

Added a test case for a constructor that calls super().
The suppression logic already handles super constructor calls correctly, so no false positive is reported solely due to the super() call.

kelloggm · 2025-04-18T13:41:51Z

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

+   * Determines whether the given assignment is the first write to a private field during a
+   * potential non-final owning field overwrite. This is used to suppress false positive resource
+   * leak warnings when it is safe to assume that a constructor initializes the field for the first
+   * time. This method returns {@code true} if: - the field is private, - it has no non-null inline


This Javadoc should be more clear that this method conservatively returns false unless it can prove that this is the first reassignment to a field.

I've updated the Javadoc.

kelloggm · 2025-04-18T13:43:08Z

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

+      if (member instanceof BlockTree) {
+        BlockTree block = (BlockTree) member;
+        if (block.isStatic()) continue;
+        boolean[] found = {false};


Why is found an array? Why not just a boolean?

Good catch — I used the array to work around Java's requirement that variables accessed from within inner classes be effectively final. I've updated the code to use AtomicBoolean instead, which takes care of the issue. Thanks!

mernst · 2025-04-18T14:42:17Z

@iamsanjaymalakar The typecheck job is not passing.

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

kelloggm

LGTM, I don't need to review again. Please address these comments and then request a review from Mike.

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

mernst · 2025-07-10T02:06:21Z

@iamsanjaymalakar Martin asked you to assign me when you had addressed his comments. Have you addressed them?

iamsanjaymalakar · 2025-07-14T00:55:05Z

@iamsanjaymalakar Martin asked you to assign me when you had addressed his comments. Have you addressed them?

Yes, I have. You can take a look.

…ork into 7049-dev

mernst

Please clarify some documentation, and update the code if necessary. (I didn't read the code because there is no point in doing so until the documentation is clear.)

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

Copilot

Pull Request Overview

The PR introduces a targeted suppression of RLC (Resource Leak Checker) warnings for safe first-time field initializations within constructors. The main purpose is to prevent false positive warnings when a private field is being initialized for the first time in a constructor, while still catching actual resource leaks in other scenarios.

Adds logic to detect and suppress warnings for first-time private field assignments in constructors
Implements comprehensive validation checks including constructor chaining, initializer blocks, and method calls
Includes extensive test coverage for various initialization patterns and edge cases

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
MustCallConsistencyAnalyzer.java	Implements the core suppression logic with field analysis and validation methods
ResourceLeakFirstInitConstructorTest.java	Adds JUnit test class for the new functionality
FirstAssignmentInConstructor.java	Test case for basic safe first assignment scenario
ExplicitNullInitializer.java	Test case for fields explicitly initialized to null
SuperCallFromConstructor.java	Test case for constructors that call super()
ConstructorChainingLeak.java	Test case for constructor chaining with this() calls
MethodCallBeforeAssignment.java	Test case for method calls before field assignment
PrivateMethodAssignmentLeak.java	Test case for field assignment within private methods
InstanceInitializerBlockLeak.java	Test case for instance initializer block assignments
InlineInitializerLeak.java	Test case for inline field initializers
PublicFieldLeak.java	Test case for public field assignments
ProtectedFieldLeak.java	Test case for protected field assignments
PackagePrivateFieldLeak.java	Test case for package-private field assignments

Comments suppressed due to low confidence (2)

checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:1683

[nitpick] The parameter name 'currentAssignment' is misleading since it represents the assignment tree being analyzed, not necessarily the current one in a sequence. Consider renaming to 'assignmentTree' or 'targetAssignment' for clarity.

      VariableElement field, MethodTree constructor, @FindDistinct Tree currentAssignment) {

checker/tests/resourceleak-firstinitconstructor/InlineInitializerLeak.java:10

This line will throw an IOException that is not handled in the field initializer. This could cause compilation issues or unexpected behavior during testing. Consider wrapping in a try-catch or using a mock/stub.

  private @Owning FileInputStream s = new FileInputStream("test.txt");

Copilot · 2025-07-18T14:34:11Z

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

+      if (member instanceof BlockTree) {
+        BlockTree block = (BlockTree) member;
+        if (block.isStatic()) continue;
+        // The variables accessed from within the inner class need to be effectively final, so


The comment mentions 'inner class' but the code uses an anonymous class (TreeScanner). Consider updating the comment to be more precise: 'The variables accessed from within the anonymous class need to be effectively final'.

Suggested change

// The variables accessed from within the inner class need to be effectively final, so

// The variables accessed from within the anonymous class need to be effectively final, so

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

mernst · 2025-07-20T15:09:44Z

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

+        Element receiverElement = TypesUtils.getTypeElement(receiver.getType());
+        if (Objects.equals(enclosingClassElement, receiverElement)) {
+          VariableElement lhsElement = lhs.getElement();
+          if (lhsElement.getModifiers().contains(Modifier.PRIVATE)


Method isFirstAssignmentToField is document to do this test, so move it into the method.

mernst · 2025-07-20T15:10:28Z

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

+      // (later writes/calls are checked at their own CFG points).
+      Element enclosingClassElement =
+          TreeUtils.elementFromDeclaration(enclosingMethodTree).getEnclosingElement();
+      if (ElementUtils.isTypeElement(enclosingClassElement)) {


How can this test fail? If it does, should an exception be thrown?

mernst · 2025-07-20T15:11:36Z

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

+    }
+
+    // (3) Disallow assignment in instance initializer blocks
+    for (Tree member : classTree.getMembers()) {


For efficiency, merge this into the above loop, like (5) and (6) are handled by a single loop.

mernst · 2025-07-20T15:12:44Z

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

+      if (expr == currentAssignment) {
+        break;
+      }
+      // (5) Prior assignment to the same field


I think this (or possibly some other bit of similar code) causes errors to be issued at the wrong location. See added test, which fails.

mernst · 2025-07-20T15:13:49Z

@iamsanjaymalakar Please take a look at my review comments.

Suppresses RLC non-final field overwrite warning for safe constructor…

158e737

… field initialization

iamsanjaymalakar self-assigned this Apr 18, 2025

iamsanjaymalakar requested a review from kelloggm April 18, 2025 10:44

kelloggm requested changes Apr 18, 2025

View reviewed changes

mernst changed the title ~~Suppresses RLC non-final field overwrite warning for safe constructor… #7049~~ Suppresses RLC non-final field overwrite warning for safe constructor field initialization Apr 18, 2025

Add @FindDistinct annotation

325e983

mernst reviewed Apr 18, 2025

View reviewed changes

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java Show resolved Hide resolved

Resolves PR review comments.

4cb90d7

iamsanjaymalakar assigned kelloggm and unassigned iamsanjaymalakar Jun 6, 2025

kelloggm previously approved these changes Jun 6, 2025

View reviewed changes

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java Show resolved Hide resolved

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java Outdated Show resolved Hide resolved

iamsanjaymalakar added 2 commits June 13, 2025 09:53

Merge branch 'master' into 7049-dev

9d4d403

Addresses PR comments.

c524542

iamsanjaymalakar dismissed kelloggm’s stale review via c524542 June 16, 2025 16:30

Merge ../checker-framework-branch-master into 7049-dev

a9a1e84

Merge branch 'master' into 7049-dev

dbd6447

iamsanjaymalakar assigned mernst and unassigned kelloggm Jul 14, 2025

mernst added 5 commits July 13, 2025 17:57

Merge branch '7049-dev' of github.com:iamsanjaymalakar/checker-framew…

29071ca

…ork into 7049-dev

Merge ../checker-framework-branch-master into 7049-dev

14aebc4

Punctuation

87b838c

Improve comments

8ad095b

Tweak coment and code

fa2aef4

mernst requested changes Jul 14, 2025

View reviewed changes

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java Outdated Show resolved Hide resolved

...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java Outdated Show resolved Hide resolved

mernst assigned iamsanjaymalakar and unassigned mernst Jul 14, 2025

Better comments.

96c47f8

iamsanjaymalakar assigned mernst Jul 15, 2025

mernst requested a review from Copilot July 18, 2025 14:33

Copilot AI reviewed Jul 18, 2025

View reviewed changes

mernst added 4 commits July 20, 2025 07:38

Merge ../checker-framework-branch-master into 7049-dev

9a70147

Add test showing error message at wrong location

4010022

Comments and variable name

4765ecf

Enhance test

143f3f4

mernst requested changes Jul 20, 2025

View reviewed changes

mernst removed their assignment Jul 20, 2025

Merge ../checker-framework-branch-master into 7049-dev

b11cd19

	// The variables accessed from within the inner class need to be effectively final, so
	// The variables accessed from within the anonymous class need to be effectively final, so

Suppresses RLC non-final field overwrite warning for safe constructor field initialization #7050

Are you sure you want to change the base?

Suppresses RLC non-final field overwrite warning for safe constructor field initialization #7050

Uh oh!

Conversation

iamsanjaymalakar commented Apr 18, 2025 • edited by mernst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kelloggm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mernst commented Apr 18, 2025

Uh oh!

Uh oh!

kelloggm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mernst commented Jul 10, 2025

Uh oh!

iamsanjaymalakar commented Jul 14, 2025

Uh oh!

mernst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mernst commented Jul 20, 2025

Uh oh!

Uh oh!

iamsanjaymalakar commented Apr 18, 2025 •

edited by mernst

Loading