RFR: 8348427: DeferredLintHandler API should use JCTree instead of DiagnosticPosition

[Maurizio Cimadamore]

On Thu, 23 Jan 2025 21:11:11 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

> The purpose of `DeferredLintHandler` is to allow `@SuppressWarnings` to be applied to warnings that are generated before `@SuppressWarnings` annotations themselves have been processed. The way this currently works is that warning callbacks are kept in a `HashMap` keyed by the innermost containing module, package, class, method, or variable declarations (in the form of a `JCModuleDecl`, `JCPackageDecl`, `JCClassDecl`, `JCMethodDecl`, or `JCVariableDecl`). Later, when the compiler executes the attribution phase and the lint categories suppressed at each declaration are known, the corresponding warning callbacks are provided with an appropriately configured `Lint` instance and "flushed".
> 
> However, the `DeferredLintHandler` API uses `DiagnosticPosition` instead of `JCTree` for registering and flushing deferred warnings. This opens the door for bugs where warnings are registered to an object which is not a declaration, and therefore ignored.
> 
> In fact, this occurs once in the code ([here](https://github.com/openjdk/jdk/blob/48ece0721489c1b357aaa81e89fe59f486079d15/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Annotate.java#L679)) where a `JCExpression` is being passed, although in this case the bug appears to be harmless (because annotation values can't contain any type of declaration).
> 
> The API should be tighted up, and furthermore an assertion should be added to verify that the JCTree being passed is actually a declaration supporting `@SuppressWarnings`.
> 
> In addition, there is a design flaw in the API: it's not possible to obtain the current immediate mode `Lint` object, so if an immediate mode `Lint` object is pushed/popped more than once, the second `Lint` object will overwrite the first.
> 
> To fix this, the API should be adjusted so the stack of current declarations and/or immediate mode `Lint` objects is managed by the `DeferredLintHandler` itself, by providing a `push()` and `pop()` methods in the API.

(My doubts re DeferredLintHandler are whether it's a good abstraction or not. It seems to provide, on paper, a good service: allow for lint warnings to be correctly reported in earlier phases of the compiler. However, I note that:
* this class seems to be serving too many masters -- the "immediate" mode seems to be a sign of that
* using this class correctly is rather finicky, and depends on which part of javac you are on. For instance, Flow doesn't use it, because it is after Attr... in other words, reporting of lint warnings is not centralized - and each components does what it needs to get the job done -- understandable, but leads to messy code (this also leads to Check::setLint)
* the synergy between Lint and DeferredLintHandler goes quite deep. For instance, flush has to be called in a place where `Lint` has been augmented with the contents of the relevant `SuppressWarnings`. All these dependencies make it quite hard to verify that the code does what it needs to do.

My general feeling here is that we'd be better off with a separate compilation step that runs after all the various front-end steps (e.g. after Flow). E.g. up to Flow, we keep accumulating lint warnings grouped by declaration. Then, in a single pass we take care of updating Lint with the correct `@SuppressWarning` found in a given declaration, and flush all the pending lint warnings for all that declaration. This would also mean that Attr and Check would be hypothetically be freed from Lint duties. Maybe this is too extreme, and we can't get there in one step - but I'd like to know what you think about the general direction (since we have already added more compilation steps in this area).

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

PR Comment: https://git.openjdk.org/jdk/pull/23281#issuecomment-2612299908