RFR: 8348611: Eliminate DeferredLintHandler and emit warnings after attribution [v3]

Mon Jul 28 17:53:19 UTC 2025

On Mon, 28 Jul 2025 16:41:59 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Archie Cobbs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 89 commits:
>> 
>>  - Add DEFAULT_ENABLED flags to fix some mandatory warnings.
>>  - Merge branch 'master' into JDK-8348611 to fix conflicts.
>>  - Merge branch 'master' into JDK-8348611 to fix conflicts.
>>  - Merge branch 'master' into JDK-8348611 to fix conflicts.
>>  - Merge branch 'MandatoryWarningCleanup' into JDK-8348611
>>  - Address review suggestions.
>>  - Remove assumptions about mandatoryness from the MandatoryWarningAggregator.
>>  - Merge branch 'MandatoryWarningCleanup' into JDK-8348611
>>  - Merge branch 'master' into MandatoryWarningCleanup
>>  - Merge branch 'MandatoryWarningCleanup' into JDK-8348611
>>  - ... and 79 more: https://git.openjdk.org/jdk/compare/cab51596...91202f7b
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/LintMapper.java line 55:
> 
>> 53:  *
>> 54:  * <p>
>> 55:  * Because {@code @SuppressWarnings} is a Java symbol, in general this mapping can't be be
> 
> Suggestion:
> 
>  * Because {@code @SuppressWarnings} is a Java symbol, in general this mapping can't be

Oops, thanks. Fixed in d4322d92d1f.

> src/jdk.compiler/share/classes/com/sun/tools/javac/code/LintMapper.java line 178:
> 
>> 176:      * Instances evolve through three states:
>> 177:      * <ul>
>> 178:      *  <li>Before the file has been completely parsed, {@link #topSpans} is null.
> 
> Maybe we can capture all these interesting state transitions with a sealed hierarchy? E.g. at different stages it seems like we might need different info attached to this "info" note -- which might suggest using different subclasses?

Yes, this probably could take advantage of those newer language features. I'll take a look at this and your other comment about `FileInfo` and post an update soon.

Thanks very much for the detailed review and comments so far!

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 1:
> 
>> 1: /*
> 
> Now that `Attr` no longer depends on `Check::lint` (yayyy!) can we make that field private?

Indeed - good catch! Fixed in d4322d92d1f.

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 265:
> 
>> 263: 
>> 264:         // Short circuit if this calculation is unnecessary
>> 265:         if (!lintMapper.lintAt(env.toplevel.sourcefile, env.tree.pos()).get().isEnabled(THIS_ESCAPE))
> 
> Question -- this code does the shortcircuiting using `lintMapper` -- but I saw code in `Check` that does shortcircuiting using `lint` (e.g. `Check::checkPotentiallyAmbiguousOverloads`). If such short-circuit operation are somewhat frequent (to avoid expensive computation logic) perhaps we should have a "blessed" (and less verbose) way to do it?
> 
> (Unfortunately, I'm aware that `checkPotentiallyAmbiguousOverloads` is called when `lintMapper` yet doesn't know whether lint warnings are suppressed or not -- this seems a problem?)

The subtle difference between `ThisEscapeAnalyzer` and `Check::checkPotentiallyAmbiguousOverloads` is that `ThisEscapeAnalyzer` runs during the compiler's `WARN` phase, whereas `checkPotentiallyAmbiguousOverloads` runs during the `ATTR` phase, and only during the `WARN` phase it is guaranteed that `LintMapper.lintAt()` will always be able to return a non-empty value.

Side note: as you may remember we had an earlier discussion about the idea of moving warnings from the `ATTR` phase to the `WARN` phase - the main "pro" being that there is no longer any concern about whether the applicable `Lint` instance has been mapped yet, so you don't need to track it manually (like `Check` does). I played with trying to do this but it was too heavy of a lift at the time (and probably still is) but it's a potential long term idea. The downside however (as you pointed out) is that during `ATTR` you have all the information you need to compute the warnings already "in front of you", whereas if you deferred that warning calculation to the `WARN` you would have to find it again (but maybe most of it is available?). Anyway, for now this is all "for future study".

> src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java line 174:
> 
>> 172:                   category.annotationSuppression ?                      // else emit if the category is not suppressed, where
>> 173:                     !lint.isSuppressed(category) :                      // ...suppression happens via @SuppressWarnings
>> 174:                     !options.isLintDisabled(category);                  // ...suppression happens via -Xlint:-category
> 
> Can this be rewritten as `!rootLint().isSuppressed(category)` ? E.g. how many different ways do we have to query the same thing?

There is a quirk about that: `rootLint().isSuppressed()` always returns `false` no matter what (see the last line of `Lint.initializeRootIfNeeded()`).

In other words, the question that `isSuppressed()` is answering is not "Is this category suppressed somehow?" but rather "Is this category suppressed by a `@SuppressWarnings` annotation?"

So in the case of a warning enabled by default:
* For the root `Lint` instance, "suppression" happens via `-Xlint:-foo`
* For non-root `Lint` instances, "suppression" happens via `@SuppressWarnings("foo")`

Could this be simplified somehow? Probably... but that would be an independent cleanup.

> the use of `removeIf` seems to suggest that these not all warnings might be flushed at the same time? 

Correct - "in theory" they might not all be flushed at the same time, because the entries in `lintWaitersMap` are organized on a per-source file basis, whereas lint mappings are discovered on a per-top level declaration basis. In practice they probably are.... but I didn't want to impose any implicit restrictions on when it was safe to invoke `Log.reportOutstandingWarnings()`. So, call this defensive coding :)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237441525
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237441827
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237440503
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237440802
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237441054
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237441313