RFR: 8348427: DeferredLintHandler API should use JCTree instead of DiagnosticPosition [v2]

[Archie Cobbs]

On Thu, 6 Feb 2025 22:10:57 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>>> > learn a little bit more about this. It looks like the only time immediate mode is used explicitly (i.e., other due to `DeferredLintHandler`'s default state) is when resolving imports ([here](https://github.com/openjdk/jdk/blob/89e5e7ab73472b7d02aac5b8b0c7e9f26db6ec32/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TypeEnter.java#L368)). In that case, of course, there can be no enclosing `@SuppressWarnings` so that makes sense.
>>> 
>>> Can we use a similar "flush" strategy ? E.g. make the warnings be part of an import declaration, and then process them as part of a flush? While it's good that you knocked one of the two usages off -- it's a bit sad to have this machinery "just" for one case.
>>> 
>>> Separately, I wonder if warnings issued from import statements should be suppressible as well (perhaps piggy backing on the toplevel class `@SuppressWarnings` uhmmm). It seems bad that there's some warnings that can't be suppressed programmatically.
>> 
>> I think it would be useful to understand which warnings can be generated during `resolveImports`. @jddarcy  pointed me at:
>> 
>> https://openjdk.org/jeps/211
>> 
>> See related change:
>> 
>> https://hg.openjdk.org/jdk9/jdk9/langtools/rev/82384454947c
>> 
>> Where we explicitly disable deprecation warnings on import since 9. Is there any other?
>
>> > > learn a little bit more about this. It looks like the only time immediate mode is used explicitly (i.e., other due to `DeferredLintHandler`'s default state) is when resolving imports ([here](https://github.com/openjdk/jdk/blob/89e5e7ab73472b7d02aac5b8b0c7e9f26db6ec32/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TypeEnter.java#L368)). In that case, of course, there can be no enclosing `@SuppressWarnings` so that makes sense.
>> > 
>> > 
>> > Can we use a similar "flush" strategy ? E.g. make the warnings be part of an import declaration, and then process them as part of a flush? While it's good that you knocked one of the two usages off -- it's a bit sad to have this machinery "just" for one case.
>> > Separately, I wonder if warnings issued from import statements should be suppressible as well (perhaps piggy backing on the toplevel class `@SuppressWarnings` uhmmm). It seems bad that there's some warnings that can't be suppressed programmatically.
>> 
>> I think it would be useful to understand which warnings can be generated during `resolveImports`. @jddarcy pointed me at:
>> 
>> https://openjdk.org/jeps/211
>> 
>> See related change:
>> 
>> https://hg.openjdk.org/jdk9/jdk9/langtools/rev/82384454947c
>> 
>> Where we explicitly disable deprecation warnings on import since 9. Is there any other?
> 
> If this was indeed the only possible case, there is a possible silver lining, because either warnings in imports are enabled (e.g. `--source 8`) in which case we can just report them immediately, as they can't be suppressed, or they are disabled (e.g. `--source >= 9`) in which case no warning should even be generated, period.
> 
> But I'm sure reality is more complex than that :-)

Hi @mcimadamore,

Thanks for the review & comments!

> Can we use a similar "flush" strategy ? E.g. make the warnings be part of an import declaration, and then process them as part of a flush? While it's good that you knocked one of the two usages off -- it's a bit sad to have this machinery "just" for one case.

Agreed - it seems to be a result of mixing the ordering of attributing things and flushing the `DeferredLintHandler`. An example is this program:

import java.security.PolicySpi;
public class A {

    @SuppressWarnings("deprecation")
    private PolicySpi foo;

    @SuppressWarnings("deprecation")
    public void m() throws Exception {
        A.class.newInstance();
    }
}

If you comment out the one remaining `deferredLintHandler.pushImmediate()` invocation (in `TypeEnter.java`), then watch watch happens when compiling the above program, you'll see that `checkDeprecated()` is invoked twice for `PolicySpi`, and the first time (`other` = `unnamed package`) the current `Lint` instance is correct but the deferred instance is incorrect, and the second time (`other` = `A`) the reverse is true; when it's invoked for `newInstance()` (only once), the current instance is correct but the deferred instance is incorrect. So - all still very confusing to me.

"When to flush" is indeed a key issue for other reasons too. For example, it appears that `JCTree` nodes tend to randomly disappear after attribution (well, it only seems random to me so far), so that deferring warning analysis for too long can set you up for a `NullPointerException` chasing exercise. A simple example is in `Gen.genClass()` where all of a class' declarations are discarded after the classfile is generate (whether that matters depends on the compiler execution mode), and there are others which I'm still chasing down.

> I think it would be useful to understand which warnings can be generated during resolveImports. @jddarcy pointed me at: https://openjdk.org/jeps/211

I ran across this as well and accommodated it my prototype by effectively pretending there is a `@SuppressWarnings("deprecation", "removal", "preview")` on every `import` statement if source ≥ 9. This is possible because the `@SuppressWarning` analysis is a completely separate step.

> Separately, I wonder if warnings issued from import statements should be suppressible as well (perhaps piggy backing on the toplevel class `@SuppressWarnings` uhmmm). It seems bad that there's some warnings that can't be suppressed programmatically.

It would be nice if the compiler allowed `@SuppressWarnings` in more places. If/when it ever does, the proposed refactoring would hopefully make it easier to implement.

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

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