8359493: Refactor how aggregated mandatory warnings are handled in the compiler #25810

archiecobbs · 2025-06-13T21:25:54Z

The compiler's handling of the aggregation of mandatory warnings into "notes" at the end of compilation can be refactored to simplify the code.

The JCDiagnostic class supports flags that alter how warnings are handled, e.g., MANDATORY, NON_DEFERRABLE, etc. So instead of having to log aggregated mandatory warnings through a separate channel (the MandatoryWarningHandler), these warnings could instead be logged just like any other warning, but with an AGGREGATED flag added. The actual aggregation can then be handled "behind the scenes" by the logging subsystem.

This will also make it easier to implement @SuppressAnnotations support for parser/tokenizer warnings which require aggregated mandatory warning notes such as warnings for preview features.

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

Issues

JDK-8359493: Refactor how aggregated mandatory warnings are handled in the compiler (Sub-task - P4)
JDK-8350514: Refactor MandatoryWarningHandler to support dynamic verbosity (Sub-task - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25810

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

Using diff file

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

Using Webrev

Link to Webrev Comment

archiecobbs · 2025-06-13T21:26:21Z

/issue add JDK-8350514

bridgekeeper · 2025-06-13T21:26:27Z

👋 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-06-13T21:26:41Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-06-13T21:27:19Z

@archiecobbs
Adding additional issue to issue list: 8350514: Refactor MandatoryWarningHandler to support dynamic verbosity.

openjdk · 2025-06-13T21:27:52Z

@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-06-13T21:38:18Z

Webrevs

01: Full - Incremental (81670b14)
00: Full (75d9edf5)

mcimadamore · 2025-06-23T09:36:22Z

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

            }
        }
+        Optional.ofNullable(warningKey)


I think I'd prefer a regular if (warningKey != null) { log.mandatoryWarning(...) }, as it seems generally "flatter".

It's a style thing and I'm happy either way. I'll fix.

mcimadamore · 2025-06-23T09:37:04Z

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

The changes in this class are really really good! Lot of complex, coupled, stateful code disappears :-)

mcimadamore · 2025-06-23T09:48:03Z

src/jdk.compiler/share/classes/com/sun/tools/javac/util/AbstractLog.java

     */
-    public void warning(Warning warningKey) {
-        report(diags.warning(source, null, warningKey));
+    public void warning(Warning warningKey, DiagnosticFlag... flags) {


I'm not sure about these changes to the warning method. The reason being that this PR doesn't seem to require such changes. And note that, with this change, all calls to Log.warning become variadic -- meaning the compiler will have to at least allocate an empty array. I'm not sure if this is enough to cause a regression, but if not necessary, I'd prefer to revert these changes, also noting that error and note have variants that do not accept flags.

(This comment also applies to your question about mandatoryWarning()).

This PR is a precursor to #24584 which is going to add another flag (DEFAULT_ENABLED). The goal here is to encapsulate all the site-specific peculiarities of logging in these flags to make the code clearer.

Let me know how you'd prefer to approach this. E.g., if you're worried about useless empty array allocations, we could just have another method override (so we'd have one that takes flags and one that doesn't). If you're worried about consistency in the API we can add a flags option to all of the other log methods. Etc. I'm happy to do it however makes the most sense.

I'd prefer to address the DEFAULT_ENABLED in a separate (and standalone, if possible) PR, to make sure we're really going the right direction about this. My general feeling is that all these properties are ultimately tied to the lint category --- so, once you pick a category, you kind of know whether it's aggregated, or defaultEnabled, or whatever -- modelling this as a flag makes it look as if you can also emit a preview warning w/o the aggregation behavior, which I'm not sure makes sense?

My general feeling is that all these properties are ultimately tied to the lint category --- so, once you pick a category, you kind of know whether it's aggregated, or defaultEnabled, or whatever -- modelling this as a flag makes it look as if you can also emit a preview warning w/o the aggregation behavior, which I'm not sure makes sense?

The flags can't always be tied to the category (for example, compiler.warn.preview.feature.use.classfile is logged with MANDATORY but not DEFAULT_ENABLED), but it looks like they could be tied to the specific warning.

This could use our existing property file comment mechanism. What do you think about that idea? Example:

# 0: message segment (feature) # lint: preview # diag-flags: mandatory, default-enabled, aggregate compiler.warn.preview.feature.use=\ {0} is a preview feature and may be removed in a future release. # 0: file object (classfile), 1: string (expected version) # lint: preview # diag-flags: mandatory, aggregate compiler.warn.preview.feature.use.classfile=\ class file for {0} uses preview features of Java SE {1}.

Then we could eliminate Log.mandatoryWarning() too.

I was thinking about capturing flags in the file too (but then hoped we could just ride the category). Doing what you suggest is a possibility -- my only "fear" is that by reflecting the flags in the compiler.properties file we'll make the current flag classification look more permanent than what it probably is (e.g. it seems to me there's a lot of ad-hocness to this classification, due to the ad-hoc behavior we're now trying to sort out). Anyway, perhaps worth doing it, even as if an exercise, as it will certainly make asymmetries between various flags pop out more -- then we can ask whether that asymmetry is there for a reason and needs to be supported going forward, or if we can make things a little less ad-hoc.

Re. default-enabled, I suspect this is used for preview language feature warnings -- e.g. warnings to be issued when (a) preview features have been enabled and (b) the use of a preview language feature has been detected. As JEP 12 says, while it's ok to use suppressible warnings for preview APIs, it is not ok to use suppressible warnings for language features. So, I believe what JEP 12 says is not "default-enabled", but really "always-enabled", or "unsuppressible". (unfortunately the JEP text does some confusion as it uses the term lint warning to refer to the unsuppressible warning, when in reality, in javac, the term lint is used for warnings that can be suppressed).

Also, are there mandatory warnings for which there's no aggregation?

Thanks for your thoughts on that. I will work on a properties file version of this so we can take a look at it. I think this should help make things as "clean" as they can be, while still preserving the existing (slightly wonky) behavior.

Regarding the JEP, there may indeed be some disagreement between what it specifies and what's implemented. If so, adding these flags to the properties file should make that disagreement (a) more obvious and (b) easy to fix.

Also, are there mandatory warnings for which there's no aggregation?

Yes, I believe these are the only two:

compiler.warn.sun.proprietary
compiler.warn.preview.feature.use.classfile

mcimadamore · 2025-06-23T09:49:08Z

src/jdk.compiler/share/classes/com/sun/tools/javac/util/AbstractLog.java

     */
-    public void mandatoryWarning(DiagnosticPosition pos, Warning warningKey) {
-        report(diags.mandatoryWarning(source, pos, warningKey));
+    public void mandatoryWarning(DiagnosticPosition pos, Warning warningKey, DiagnosticFlag... flags) {


Note: this method is already some kind of special wrapper around Log.warning which always sets the mandatory flag. I wonder if, maybe, we could just tweak this method to take a boolean parameter telling whether to aggregate or not, and then deal with the flags internally?

mcimadamore · 2025-06-23T09:52:58Z

src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java

+
+    private MandatoryWarningAggregator aggregatorFor(LintCategory lc) {
+        return switch (lc) {
+        case PREVIEW -> aggregators.computeIfAbsent(lc, c -> new MandatoryWarningAggregator(this, Source.instance(context), c));


Can we save Source as an instance variable? I'm mildly worried about using the context instance field (which is used for lazy init of lint) more generally. (In fact, I was even thinking if it would make sense to have a Supplier<Lint> field that we create at construction, so that we don't have to save the context field at all.

Can we save Source as an instance variable? I'm mildly worried about using the context instance field (which is used for lazy init of lint) more generally. (In fact, I was even thinking if it would make sense to have a Supplier<Lint> field that we create at construction, so that we don't have to save the context field at all.

I guess I don't understand the issue - isn't this kind of usage the whole point of having a Context singleton?

We can certainly grab the reference to Source eagerly (which means sometimes we would be grabbing it unnecessarily - i.e., any time there are no preview warnings) but beyond that I'm not sure what additional changes would buy us.

Put another way: what are you trying to optimize for with that question? How would Supplier<Lint> be better than a Context (plus Lint.instance()) which is the more standard way to achieve the same thing?

What I mean is that, typically, javac components only use context as a constructor parameter, and they initialize all the context-dependent stuff they need from there. It is rather strange to see classes to "hold onto" their context parameter and use it later. Sometimes (as in this case) this is unavoidable (as initializing too early would lead to cycles) -- but I believe the status quo is to do this only where strictly necessary, and the Source.instance(context) does not feel necessary to me. (separately, each call to instance does a lookup on a map, so there's also an efficiency angle to try and limit this where possible).

Supplier is not better -- but it avoids the context field, which in turns avoid the "temptation" to use it when not necessary. It is not mandatory you change the code to use a supplier though, as we don't have precedents for that kind of thing -- but limiting context lookups as much as possible is defo how javac code generally works -- and the main thing I'm bringing up in this comment.

Thinking about this, I realized I have had this assumption in my head: "There is a certain set of types which are compiler singletons (Resolve, Source, etc.). We assume that for any Context instance, there is only one associated singleton of any of those types. So while it would not necessarily be correct for random objects in the compiler to hold Context references, it's OK for any of these singletons to hold on to Context references because they are all part of the same singleton blob/cluster."

By that logic, Log holding a Context reference and then using it later is OK. But I agree there's no need to gratuitously do this when it's not necessary.

However, it turns out it's necessary for both Source and Lint to avoid dependencies. E.g., for Source we get this error if we try to acquire it eagerly:

java.lang.AssertionError: ignored flag: --source at jdk.compiler.interim/com.sun.tools.javac.util.Assert.error(Assert.java:162) at jdk.compiler.interim/com.sun.tools.javac.util.Assert.check(Assert.java:104) at jdk.compiler.interim/com.sun.tools.javac.util.Options.lambda$computeIfReady$0(Options.java:296) at jdk.compiler.interim/com.sun.tools.javac.util.Options.notifyListeners(Options.java:265) at jdk.compiler.interim/com.sun.tools.javac.main.Arguments.processArgs(Arguments.java:367) at jdk.compiler.interim/com.sun.tools.javac.main.Arguments.init(Arguments.java:202) at jdk.compiler.interim/com.sun.tools.javac.main.Main.compile(Main.java:238) at jdk.compiler.interim/com.sun.tools.javac.main.Main.compile(Main.java:178) at jdk.compiler.interim/com.sun.tools.javac.main.JavacToolProvider.run(JavacToolProvider.java:54) at javacserver.server.Server.runCompiler(Server.java:239) at javacserver.server.Server.handleRequest(Server.java:208) at javacserver.server.Server.lambda$start$0(Server.java:172) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1095) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:619) at java.base/java.lang.Thread.run(Thread.java:1447)

This is because Log is needed so early in the startup process.

mcimadamore · 2025-06-23T10:04:55Z

src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java

+          .map(MandatoryWarningAggregator::aggregationNotes)
+          .flatMap(List::stream)
+          .forEach(this::report);
+        aggregators.clear();


Is there a "cleanup race" here? E.g. this method clears the aggregator enum map. But then Log.clear walks that map and calls clear on each aggregator -- but if the map is empty, nothing will get cleared?

Is there a "cleanup race" here? E.g. this method clears the aggregator enum map. But then Log.clear walks that map and calls clear on each aggregator -- but if the map is empty, nothing will get cleared?

Yep - there is a bit of inconsistency here: reportOutstandingNotes() is treating aggregators as things that are created, used, and then discarded, whereas log.clear() is treating them as being created once and then used, cleared and re-used multiple times. Logically it's the same result but it looks weird. I'll fix to just clear the map in both cases.

mcimadamore

Great work as usual. Moving the aggregation logic to Log seems generally a big win, both for clients and for us, as it seems generally easier, in the new code, to guarantee that all mandatory warnings get the same treatment.

Refactor MandatoryWarningHandler to just be an aggregator.

75d9edf

archiecobbs mentioned this pull request Jun 13, 2025

8348611: Eliminate DeferredLintHandler and emit warnings after attribution #24584

Draft

3 tasks

openjdk bot added the compiler [email protected] label Jun 13, 2025

archiecobbs marked this pull request as ready for review June 13, 2025 21:32

openjdk bot added the rfr Pull request is ready for review label Jun 13, 2025

mcimadamore reviewed Jun 23, 2025

View reviewed changes

Apply some review suggestions.

81670b1

8359493: Refactor how aggregated mandatory warnings are handled in the compiler #25810

Are you sure you want to change the base?

8359493: Refactor how aggregated mandatory warnings are handled in the compiler #25810

Conversation

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

Uh oh!

Progress

Issues

Reviewing

Uh oh!

archiecobbs commented Jun 13, 2025

Uh oh!

bridgekeeper bot commented Jun 13, 2025

Uh oh!

openjdk bot commented Jun 13, 2025

Uh oh!

openjdk bot commented Jun 13, 2025

Uh oh!

openjdk bot commented Jun 13, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcimadamore Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

archiecobbs Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mcimadamore Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcimadamore Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mcimadamore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

archiecobbs commented Jun 13, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 13, 2025 •

edited

Loading

mcimadamore Jun 23, 2025 •

edited

Loading

archiecobbs Jun 23, 2025 •

edited

Loading

mcimadamore Jun 23, 2025 •

edited

Loading

mcimadamore Jun 23, 2025 •

edited

Loading