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.

Question: looking more broadly at the deferred lint handler API, I see that in most cases (all but one) `report` is called with a lambda that ignores its `lint` object. This seems odd - consider this:


private void warnOnExplicitStrictfp(DiagnosticPosition pos) {
        DiagnosticPosition prevLintPos = deferredLintHandler.setPos(pos);
        try {
            deferredLintHandler.report(_ -> lint.logIfEnabled(log, pos, LintWarnings.Strictfp)); // <---------------
        } finally {
            deferredLintHandler.setPos(prevLintPos);
        }
    }


Is the above code correct? E.g. should the call on `lint.logIfEnabled` occur on the `lint` that is the parameter to the lambda expression?

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

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