RFR: 8359493: Refactor how aggregated mandatory warnings are handled in the compiler [v4]

[Maurizio Cimadamore]

On Tue, 1 Jul 2025 16:03:33 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>> 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.
>
> Archie Cobbs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - Merge branch 'master' into MandatoryWarningCleanup
>  - Move (most) SOURCE_LEVEL flags into compiler.properties.
>  - Add DiagnosticFlag support to compiler.properties and put AGGREGATE there.
>  - Apply some review suggestions.
>  - Refactor MandatoryWarningHandler to just be an aggregator.

make/langtools/tools/propertiesparser/resources/templates.properties line 93:

> 91: diagnostic.flags=\n\
> 92:   '  'Stream.of({0})\n\
> 93:   '    '.map(s -> s.replace(''-'', ''_''))\n\

These transformations could be also done in Java code inside `ClassGenerator` right?

make/langtools/tools/propertiesparser/resources/templates.properties line 96:

> 94:   '    '.map(s -> s.toUpperCase(Locale.ROOT))\n\
> 95:   '    '.map(DiagnosticFlag::valueOf)\n\
> 96:   '    '.toArray(DiagnosticFlag[]::new)

If instead of an array we returned a list, or a set, maybe we could use empty list/set if there's no flag (instead of `null` as we do now?)

src/jdk.compiler/share/classes/com/sun/tools/javac/util/JCDiagnostic.java line 512:

> 510:         DiagnosticType type;
> 511: 
> 512:         /** A set of diagnostic flags to be automatically added to newly created JCDiagnostics (if not null). */

As commented elsewhere -- should we use `null` or something like an empty list/set?

src/jdk.compiler/share/classes/com/sun/tools/javac/util/JCDiagnostic.java line 585:

> 583: 
> 584:         public boolean hasFlag(DiagnosticFlag flag) {
> 585:             return flags != null && Arrays.asList(flags).contains(flag);

This is where it seems that encoding as a possibly null array has a cost

src/jdk.compiler/share/classes/com/sun/tools/javac/util/JCDiagnostic.java line 685:

> 683:         if (diagnosticInfo.flags != null) {
> 684:             for (DiagnosticFlag flag : diagnosticInfo.flags) {
> 685:                 this.flags.add(flag);

This could be `addAll` (if flags were not arrays)

src/jdk.compiler/share/classes/com/sun/tools/javac/util/MandatoryWarningAggregator.java line 131:

> 129:      *                that may be generated, or null to infer from the lint category.
> 130:      */
> 131:     public MandatoryWarningAggregator(Log log, Source source, LintCategory lc, String prefix) {

I think we should call it just `WarningAggregator`. While for now the cases where we aggregate are mandatory warnings, it is not clear to me that we want to give this coupling much importance.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25810#discussion_r2182380617
PR Review Comment: https://git.openjdk.org/jdk/pull/25810#discussion_r2182382167
PR Review Comment: https://git.openjdk.org/jdk/pull/25810#discussion_r2182385914
PR Review Comment: https://git.openjdk.org/jdk/pull/25810#discussion_r2182387337
PR Review Comment: https://git.openjdk.org/jdk/pull/25810#discussion_r2182388765
PR Review Comment: https://git.openjdk.org/jdk/pull/25810#discussion_r2182392534