RFR: 8348611: Eliminate DeferredLintHandler and emit warnings after attribution [v3]
Maurizio Cimadamore
mcimadamore at openjdk.org
Mon Jul 28 16:58:08 UTC 2025
On Sun, 20 Jul 2025 20:39:07 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:
>> This is a cleanup/refactoring of how lint warnings are logged and `@SuppressWarnings` annotations applied.
>>
>> A central challenge with lint warnings is that warnings can be generated during any compiler phase, but whether a particular lint warning is suppressed via `@SuppressWarnings` can't be known until after attribution. For example, the parser doesn't have enough information to interpret and apply `@SuppressWarnings("text-blocks")` to text blocks, or `@SuppressWarnings("preview")` to preview lexical features; instead, the `DeferredLintHandler` is used to workaround this limitation.
>>
>> In addition, several other factors complicate things:
>> * Knowing the current applicable `Lint` instance requires manually tracking it with each declaration visited and applying/removing the `@SuppressWarnings` annotation there, if any
>> * Some warnings are "suppressibly mandatory" (i.e., they are _emitted if not suppressed_ instead of _emitted if enabled_)
>> * Some warnings are "unsuppressibly mandatory" (e.g., the "internal proprietary API" warning)
>> * Some mandatory warnings are _aggregated_ into notes that are emitted at the end of compilation when not enabled
>> * Some warnings are _lint_ warnings, with a corresponding lint category, while others are just "plain" warnings
>> * Some lint warnings are suppressible via `@SuppressWarnings`, while others are only suppressible via `-Xlint:-foo` flags
>> * Speculative compilation requires holding log messages in purgatory until the speculation resolves, after which they are then either discarded or emitted. But this creates a tricky interaction with `DeferredLintHandler` because even after speculation is complete, we may still not yet know whether a warning should be suppressed.
>>
>> Previously the logic to get all of this right was non-obviously woven around the code base. In particular, you needed to know somehow whether or not to use `DeferredLintHandler`, and in what "mode".
>>
>> The overall goal of this PR is to simplify usage so that **no matter where you are in the compiler, you can just invoke `log.warning()` to log a warning** and (mostly) forget about all of the details listed above.
>
> 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
Overall, this is a great piece of work! The simplifications are evident throughout the entire codebase. I'm not sure `LintMapper` has reached its simplest possible form yet -- I've left some comments for you to consider -- I wonder if there's something we can do to capture the state transitions a little more explicitly there.
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java line 484:
> 482: * @param warning key for the localized warning message
> 483: */
> 484: public void logIfEnabled(LintWarning warning) {
I like it a lot that now there's "just warnings". It took a while to see this more clearly, but once you see it you can't unsee it :-)
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
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?
src/jdk.compiler/share/classes/com/sun/tools/javac/code/LintMapper.java line 283:
> 281: * nodes representing the top-level declarations in the file, and so on.
> 282: */
> 283: private static class DeclNode extends Span {
This also seems to suggest that some of the spans computed after parsing are later "promoted" to decl nodes. But the code doesn't work like this, instead it tracks spans and decls separately into the same FileInfo data structure, which I find a bit odd.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Annotate.java line 1079:
> 1077: private final Symbol sym;
> 1078:
> 1079: public TypeAnnotate(Env<AttrContext> env, Symbol sym) {
All these changes are truly great, as they are sign that you were able to remove a lot of coupling from the way things were implemented. E.g. the goal of this PR is not to "make annotation processing simpler", but turns out that, by centralizing the lint mapping logic in one place, other parts of the compiler also got simpler!
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?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 227:
> 225: void warnDeprecated(DiagnosticPosition pos, Symbol sym) {
> 226: Assert.check(!importSuppression);
> 227: LintWarning warningKey = sym.isDeprecatedForRemoval() ?
Another big win is the absence of all "is enabled?" checks in the new code. Meaning less potential for mistakes.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1:
> 1: /*
Another big win here -- code is much flatter!
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?)
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?
src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java line 196:
> 194: */
> 195: public void flushLintWaiters() {
> 196: lintWaitersMap.entrySet().removeIf(entry -> {
the use of `removeIf` seems to suggest that these not all warnings might be flushed at the same time? But this seems odd -- I'd expect that the `lintMapper` only gets new info in `Attr` -- after which if something doesn't have a `Lint`, well that shouldn't change?
-------------
PR Review: https://git.openjdk.org/jdk/pull/24584#pullrequestreview-3063577027
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237121469
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237230153
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237237978
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237264944
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237114296
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237142388
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237132978
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237149038
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237163998
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237209441
PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2237227213
More information about the kulla-dev
mailing list