Suppresses RLC non-final field overwrite warning for safe constructor field initialization by iamsanjaymalakar · Pull Request #7050 · typetools/checker-framework

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

Summary by CodeRabbit

New Features
- Enhanced resource leak checker to suppress false-positive warnings when a private field is assigned for the first time within a constructor.
Tests
- Added comprehensive test suite covering various constructor initialization scenarios, including inline initializers, instance initializers, method calls, and conditional branching.

… field initialization

kelloggm

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

checker/tests/resourceleak-firstinitconstructor/ConstructorChainingLeak.java

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

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

…to 7049-dev

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

mernst

Thanks! This is looking solid. I have a few comments and questions, and then after another round I hope this can be merged.

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

mernst · 2026-04-09T00:33:37Z

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

    }
  }

+  /**


There are 400 lines of code about finding the first write. Given its size, I suggest abstracting it out into a separate file.

Refactored them into a separate file.

mernst · 2026-04-09T00:34:18Z

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

+      return false;
+    }
+    ClassTree classTree = TreePathUtil.enclosingClass(constructorPath);
+    if (classTree == null) {


When can this happen? Should this be removed, so that the Checker Framework throws an exception when it has an internal defect, rather than silently ignoring the error?

I threw an exception instead of null-check now.

mernst · 2026-04-09T00:44:35Z

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

+                Element lhsElement = TreeUtils.elementFromTree(lhs);
+                if (targetField == lhsElement) {
+                  isInitialized.set(true);
+                  return null;


Once isInitialized is set to true, there is no need to continue the visitor's traversal. So, use a visitor that permits early exit (short-circuiting).
Every visitor that can return early should use such an optimization.

Thanks for pointing it out, I've added the short-circuiting logic.

mernst · 2026-04-09T00:56:53Z

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

+
+  /**
+   * Scans {@code stmts} in source order to determine whether {@code targetAssignment} is definitely
+   * the first write to {@code targetField} in this constructor fragment.


It could also be in an initializer block, right?

No, it's only for constructor-body statements. I've clarified in the javadoc.

mernst · 2026-04-09T01:08:23Z

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

+   * exceptional path).
+   *
+   * @param tryTree the try statement to inspect
+   * @param targetField the field to check for assignments


I think it must be of reference (non-primitive) type. Is that correct?

mernst · 2026-04-09T01:09:43Z

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

+
+  /**
+   * Visitor that determines whether a given assignment in a constructor is definitely the first
+   * write to its field before any earlier assignment or side-effecting call. The entry point is


Do "earlier assignments" include static initializer blocks? I know your code handles that, but the comment should say whether this method handles it.

I've updated the comments.

mernst · 2026-04-09T01:09:45Z

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

+   *
+   * <p>This performs a single AST traversal over the provided tree fragment and is deliberately
+   * conservative: it does not reason about control flow or path feasibility. It is intended to be
+   * applied only to supported fragments (expressions and statement subtrees selected by the


Selected how?
How does the caller know what is legal to pass?

Updated the code comments.

mernst · 2026-04-09T01:11:54Z

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

+   * <p>This performs a single AST traversal over the provided tree fragment and is deliberately
+   * conservative: it does not reason about control flow or path feasibility. It is intended to be
+   * applied only to supported fragments (expressions and statement subtrees selected by the
+   * caller), not to arbitrary statements containing branching/looping constructs.


Consider clarifying whether this code is conservative or unsound if the contract ("only applied to supported fragments") is violated by the caller.

Updated the code comments.

mernst · 2026-04-09T01:13:04Z

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

+     * Scans {@code root} to determine whether {@code targetAssignment} is reached before any
+     * disqualifying event within this tree fragment.
+     *
+     * <p>This method is conservative and AST-only. It returns {@link


What does "AST-only" mean in this context?

Updated the code 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 Outdated 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 Outdated 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

mernst added 12 commits March 2, 2026 11:56

Put error key in brackets

57985ba

Merge ../checker-framework-fork-mernst-branch-more-square-brackets in…

481e926

…to 7049-dev

Put error key in brackets

4babfbd

Undo a change

7668ccc

Another pair of brackets

cb5f029

Merge ../checker-framework-fork-mernst-branch-more-square-brackets in…

7305395

…to 7049-dev

Merge ../checker-framework-branch-master into more-square-brackets

224f485

Add space around heading

775a2db

Merge ../checker-framework-branch-master into more-square-brackets

f26dca7

Merge ../checker-framework-fork-mernst-branch-more-square-brackets in…

8e127b5

…to 7049-dev

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

29b6694

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

e3f1c5d

mernst reviewed Mar 24, 2026

View reviewed changes

iamsanjaymalakar added 3 commits April 3, 2026 16:39

Addresses Mike's PR review comments.

b278f86

Merge branch 'master' into 7049-dev

d6b1e99

Merge branch 'master' into 7049-dev

9b70eec

iamsanjaymalakar requested a review from mernst April 4, 2026 07:23

iamsanjaymalakar assigned mernst and unassigned iamsanjaymalakar Apr 4, 2026

iamsanjaymalakar and others added 6 commits April 4, 2026 00:23

Merge branch 'master' into 7049-dev

4d88f93

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

f9b8a87

Add comment

db0b98b

Use named instanceof

4fccac1

Eliminate need for warning suppression

96dee63

Tweaks

f5420f7

mernst requested changes Apr 9, 2026

View reviewed changes

mernst assigned iamsanjaymalakar and unassigned mernst Apr 9, 2026

iamsanjaymalakar added 2 commits April 11, 2026 15:57

Addresses PR comments.

27703ee

Merge branch 'master' into 7049-dev

f55e800

                   }
                 }
+                /**

Conversation

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

Uh oh!

Summary by CodeRabbit

Uh oh!

kelloggm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mernst left a comment

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

iamsanjaymalakar commented Apr 18, 2025 •

edited by coderabbitai bot

Loading