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

Wed Aug 13 15:11:22 UTC 2025

On Mon, 11 Aug 2025 19:23:59 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 97 commits:
> 
>  - Merge branch 'master' into JDK-8348611 to fix conflict.
>  - Remove an unnecessary field.
>  - More simplification of LintMapper per review suggestions.
>  - More simplification of LintMapper per review suggestions.
>  - More refactoring to simplify LintMapper per review.
>  - Refactor LintMapper to clean up internal type hierarchy per review.
>  - Address a couple of review comments.
>  - Merge branch 'master' into JDK-8348611
>  - Add DEFAULT_ENABLED flags to fix some mandatory warnings.
>  - Merge branch 'master' into JDK-8348611 to fix conflicts.
>  - ... and 87 more: https://git.openjdk.org/jdk/compare/8cd79752...18eaa8ef

Hi Jan, thanks for taking a look.

> nice to see `ThisEscapeAnalyzer` is short-circuited, we'll see if there are more than needs to be short-circuited, but probably good to do it when there's a need, rather than pro-actively.

Right - and to elaborate on this a bit: We added the new `WARN` phase a while back as a comfortable home for `ThisEscapeAnalyzer` and any other future non-trivial warning calculators. Since `WARN` comes after `ATTR`, all attribution and `Lint` calculations are complete, and so you can always ask, "Is Lint category X enabled here... or here... or over there...?" without worry.

> the semantics for `BaseFileManager`/`Locations` is slightly complex (the `Context` there may be a different `Context` than used by the rest of javac, e.g. when created through `javax.tools.JavaCompiler.getStandardFileManager`). I _think_ the current code is OK with that - the warnings should be reported using the root Lint, but I am not sure if there are any tests for that. I'll see if I can add a test, but it may be done separately.

Ah, I didn't realize that. In any case it should have the same semantics as before, because in both cases (before and after this change), the `Lint` and the `Log` singletons derive from the same `Context`.

> Just for information, do you know/recall what's the exact path to get here with JShell?

If you throw an `AssertionError` in that particular case, you get a failure in `ToolEnablePreviewTest.java` that looks like this (but I haven't investigated it any further):

test ToolEnablePreviewTest.testCompilerTestFlag(): failure [406ms]
java.lang.AssertionError: unknown source file: WrappedJavaFileObject[jdk.jshell.MemoryFileManager$SourceMemoryJavaFileObject[string:///$NeverUsedName$.java]]
	at jdk.compiler/com.sun.tools.javac.util.Log$DiagnosticHandler.lambda$flushLintWaiters$0(Log.java:201)
	at java.base/java.util.Collection.removeIf(Collection.java:581)
	at jdk.compiler/com.sun.tools.javac.util.Log$DiagnosticHandler.flushLintWaiters(Log.java:196)
	at jdk.compiler/com.sun.tools.javac.util.Log.reportOutstandingWarnings(Log.java:832)
	at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.errorCount(JavaCompiler.java:583)
	at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.invocationHelper(JavacTaskImpl.java:177)
	at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.parse(JavacTaskImpl.java:248)
	at jdk.jshell/jdk.jshell.TaskFactory$ParseTask.parse(TaskFactory.java:384)
	at jdk.jshell/jdk.jshell.TaskFactory$ParseTask.<init>(TaskFactory.java:373)
	at jdk.jshell/jdk.jshell.TaskFactory.lambda$parse$0(TaskFactory.java:149)
	at jdk.jshell/jdk.jshell.TaskFactory.lambda$runTask$1(TaskFactory.java:218)
	at jdk.compiler/com.sun.tools.javac.api.JavacTaskPool.getTask(JavacTaskPool.java:194)
	at jdk.jshell/jdk.jshell.TaskFactory.runTask(TaskFactory.java:211)
	at jdk.jshell/jdk.jshell.TaskFactory.parse(TaskFactory.java:145)
	at jdk.jshell/jdk.jshell.TaskFactory.parse(TaskFactory.java:245)
	at jdk.jshell/jdk.jshell.CompletenessAnalyzer.lambda$scan$1(CompletenessAnalyzer.java:96)
	at jdk.jshell/jdk.jshell.CompletenessAnalyzer$Parser.disambiguateDeclarationVsExpression(CompletenessAnalyzer.java:769)
	at jdk.jshell/jdk.jshell.CompletenessAnalyzer$Parser.parseUnit(CompletenessAnalyzer.java:669)
	at jdk.jshell/jdk.jshell.CompletenessAnalyzer.scan(CompletenessAnalyzer.java:97)
	at jdk.jshell/jdk.jshell.SourceCodeAnalysisImpl.analyzeCompletion(SourceCodeAnalysisImpl.java:199)
	at jdk.jshell/jdk.internal.jshell.tool.JShellTool.isComplete(JShellTool.java:1356)
	at jdk.jshell/jdk.internal.jshell.tool.JShellTool.getInput(JShellTool.java:1297)
	at jdk.jshell/jdk.internal.jshell.tool.JShellTool.run(JShellTool.java:1246)
	at jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1031)
	at jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.run(JShellToolBuilder.java:259)
	at ReplToolTesting.testRawRun(ReplToolTesting.java:319)
	at ReplToolTesting.testRaw(ReplToolTesting.java:302)
	at ReplToolTesting.test(ReplToolTesting.java:254)
	at ReplToolTesting.test(ReplToolTesting.java:241)
	at ReplToolTesting.test(ReplToolTesting.java:233)
	at ReplToolTesting.test(ReplToolTesting.java:224)
	at ToolEnablePreviewTest.testCompilerTestFlag(ToolEnablePreviewTest.java:97)

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

PR Comment: https://git.openjdk.org/jdk/pull/24584#issuecomment-3184318491