8355753: @SuppressWarnings("this-escape") not respected for indirect leak via field #24932

archiecobbs · 2025-04-28T17:08:40Z

This PR corrects a bug in the logic for handling @SuppressWarnings("this-escape") when the leak is from a field initializer.

The ThisEscapeAnalyzer has to do some custom handling for @SuppressWarnings("this-escape") because when a constructor executes, the actual path of execution can jump around between multiple constructors, field initializers, and initialization blocks. The previous logic was somewhat ad hoc and contained at least one bug (this one), so this PR refactors it to fix the bug and also make the code clearer.

Now we "execute" field initializers and initialization blocks when we encounter super() invocations, just like the actual JVM does, and we move the logics for (a) applying suppression and (b) the "at most one warning per constructor or initializer" rule until after the analysis, so they are part of the existing warning filtering and de-duplication step.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8355753: @SuppressWarnings("this-escape") not respected for indirect leak via field (Bug - P4)

Reviewers

Vicente Romero (@vicente-romero-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24932/head:pull/24932
$ git checkout pull/24932

Update a local copy of the PR:
$ git checkout pull/24932
$ git pull https://git.openjdk.org/jdk.git pull/24932/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24932

View PR using the GUI difftool:
$ git pr show -t 24932

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24932.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-28T17:09:10Z

👋 Welcome back acobbs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-04-28T17:09:35Z

@archiecobbs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8355753: @SuppressWarnings("this-escape") not respected for indirect leak via field

Reviewed-by: vromero

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 13 new commits pushed to the master branch:

ae3d96a: 8357683: (process) SIGQUIT still blocked after JDK-8234262 with jdk.lang.Process.launchMechanism=FORK or VFORK
12ee80c: 8357675: Amend headless message
acd64ba: 8357626: RISC-V: Tighten up template interpreter method entry code
... and 10 more: https://git.openjdk.org/jdk/compare/727412d1b5f0764363ebd2ff600d4b7f0c99bb9b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-04-28T17:10:20Z

@archiecobbs The following label will be automatically applied to this pull request:

compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-04-28T17:12:58Z

Webrevs

bridgekeeper · 2025-05-26T19:18:13Z

@archiecobbs This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

archiecobbs · 2025-05-26T21:23:26Z

/touch

openjdk · 2025-05-26T21:24:22Z

@archiecobbs The pull request is being re-evaluated and the inactivity timeout has been reset.

vicente-romero-oracle · 2025-05-29T17:50:37Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java

+
+        // Sort warnings so redundant warnings immediately follow whatever they are redundant for, then remove them
+        warningList.sort(Warning::sortByStackFrames);
+        AtomicReference<Warning> previousRef = new AtomicReference<>();


just curious: why do we need to deal with AtomicReference here?

Hi @vicente-romero-oracle, thanks for taking a look.

The reason for the AtomicReference is that this lamba needs to be stateful (it needs to remember the most recent previous Warning that was also retained, if any), so if it wants to remain a lambda (i.e., and not a more verbose inner class) that state needs to be declared outside the lamba, and therefore it must be effectively final, which means there must be an extra level of indirection, etc. I guess it's a verbosity vs. clarity trade-off.

right, would an array be less heavy? not beautiful either

right, would an array be less heavy? not beautiful either

If by "heavy" you mean optimization/efficiency, honestly I have no idea about that... that gets into a level of JVM optimization granularity beyond my knowledge.

As for "beauty", yes using an array is a common trick for this, but IMHO doing so is more obscure than using AtomicReference because there is no "arrayness" here. You are just using the array as a thing that contains a single mutable reference - i.e., exactly what AtomicReference does. Of course, we don't care about the "atomic" part, so that's slightly confusing too.

There's no perfect answer. It makes me wonder... maybe someday we could make stateful lambdas easier, e.g.:

Runnable r = () -> { static int count; System.out.println("Invoked " + ++count + " times"); };

right, definitely there is a need for stateful lambdas

vicente-romero-oracle · 2025-05-29T19:22:41Z

test/langtools/tools/javac/warnings/ThisEscape.java

        }
    }
+
+    // JDK-8355753 - @SuppressWarnings("this-escape") not respected for indirect leak via field


nit: consider adding a similar test case with an additional constructor that is not annotated with the @SuppressAnnotations annotation so that the warning is issued

nit: consider adding a similar test case with an additional constructor that is not annotated with the @SuppressAnnotations annotation so that the warning is issued

and probably another variation doing something like:

private int x; { x = this.mightLeak(); }

Hi @vicente-romero-oracle, thanks for the suggestions. They should be implemented in 65e2679.

vicente-romero-oracle · 2025-05-29T20:13:25Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java

+                if (diff != 0)
+                    return diff;
+            }
+        };


Suggested change

};

}

Thanks - fixed in f79e35f.

vicente-romero-oracle · 2025-05-29T20:13:50Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java

+            return numExtra >= 0 &&
+              IntStream.range(0, that.stack.size())
+                .allMatch(index -> this.stack.get(numExtra + index).comparePos(that.stack.get(index)) == 0);
+        };


Suggested change

};

}

Thanks - fixed in f79e35f.

vicente-romero-oracle

lgtm

vicente-romero-oracle · 2025-05-30T13:40:56Z

@archiecobbs thanks for the updates

archiecobbs · 2025-05-30T14:42:13Z

Thanks for the review!

archiecobbs · 2025-05-30T14:42:21Z

/integrate

openjdk · 2025-05-30T14:42:38Z

Going to push as commit 26275a1.
Since your change was applied there have been 13 commits pushed to the master branch:

ae3d96a: 8357683: (process) SIGQUIT still blocked after JDK-8234262 with jdk.lang.Process.launchMechanism=FORK or VFORK
12ee80c: 8357675: Amend headless message
acd64ba: 8357626: RISC-V: Tighten up template interpreter method entry code
... and 10 more: https://git.openjdk.org/jdk/compare/727412d1b5f0764363ebd2ff600d4b7f0c99bb9b...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-30T14:42:45Z

@archiecobbs Pushed as commit 26275a1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Refactor ThisEscapeAnalyzer to correct bug in suppression logic.

5ef9606

openjdk bot added the rfr Pull request is ready for review label Apr 28, 2025

archiecobbs mentioned this pull request Apr 28, 2025

8348611: Eliminate DeferredLintHandler and emit warnings after attribution #24584

Closed

3 tasks

openjdk bot added the compiler compiler-dev@openjdk.org label Apr 28, 2025

vicente-romero-oracle reviewed May 29, 2025

View reviewed changes

archiecobbs added 2 commits May 29, 2025 17:08

Merge branch 'master' into JDK-8355753

b10fb8b

Add additional tests for warnings generated from fields.

65e2679

vicente-romero-oracle reviewed May 30, 2025

View reviewed changes

Remove extraneous semicolons.

f79e35f

vicente-romero-oracle approved these changes May 30, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 30, 2025

openjdk bot added the integrated Pull request has been integrated label May 30, 2025

openjdk bot closed this May 30, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 30, 2025

8355753: @SuppressWarnings("this-escape") not respected for indirect leak via field #24932

8355753: @SuppressWarnings("this-escape") not respected for indirect leak via field #24932

Uh oh!

Conversation

archiecobbs commented Apr 28, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 28, 2025

Uh oh!

openjdk bot commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 28, 2025

Uh oh!

mlbridge bot commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

bridgekeeper bot commented May 26, 2025

Uh oh!

archiecobbs commented May 26, 2025

Uh oh!

openjdk bot commented May 26, 2025

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!

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle commented May 30, 2025

Uh oh!

archiecobbs commented May 30, 2025

Uh oh!

archiecobbs commented May 30, 2025

Uh oh!

openjdk bot commented May 30, 2025

Uh oh!

openjdk bot commented May 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

archiecobbs commented Apr 28, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 28, 2025 •

edited

Loading

mlbridge bot commented Apr 28, 2025 •

edited

Loading