RFR: 8370800: Downgrade cant.attach.type.annotations diagnostics to warnings [v2]

[Liam Miller-Cushon]

On Wed, 29 Oct 2025 08:49:24 GMT, Liam Miller-Cushon <cushon at openjdk.org> wrote:

>> Hi, please consider this fix for [JDK-8370800: Downgrade cant.attach.type.annotations diagnostics to warnings](https://bugs.openjdk.org/browse/JDK-8370800).
>> 
>> As discussed in the, this reduces the compatibility impact of these diagnostics for builds that deliberately omit transitive annotation dependencies, for example if they are only referenced through javadoc `@link` tags, or by frameworks that conditionally load the classes.
>> 
>> The PR changes the existing error diagnostic to an unconditional warning. Another alternative would be to make it an optional xlint diagnostic, perhaps as part of `-Xlint:classfile`, or as another category.
>
> Liam Miller-Cushon has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Use DeferredCompletionFailureHandler

Thanks very much for the review!

> So, overall, I am not convinced this is a good move. Yes, we have some existing cases where missing stuff produces just warnings in the class reader, but these are cases where annotations, or their attributes, are missing. Not when the actual field/method type is missing. I.e. in the test case, not producing an error for missing `@Anno` would seem more or less OK to me, but ignoring errors for missing type `A` makes much less sense to me.

I had been thinking about it similarly, that it would be better to report and error and just add the missing transitive deps.

I've heard feedback about a couple of cases where the code owners didn't want to do that, because the deps were only used for thinks like `@link` tags or for optional / provided framework dependencies.

Overall it might make sense to move this back to a draft and collect more feedback in the bug.

> I think that if you really want to ignore the `CompletionFailure`s at this point, `DeferredCompletionFailureHandler` needs to be used to re-set the `ClassSymbol` for `A` to the original state. `speculativeCodeHandler` might be usable for this (look how it is used in `DeferredAttr`). `b.a.toString();` in the above testcase would then hopefully produce a compile-time error correctly.

Thanks! I experimented with doing that and it avoids the crash, and I have pushed those changes to the PR, but I realize that doesn't fully solve the issues you raised and this needs more thought and discussion.

> Second problem is that catching the `CompletionFailure` at this place may leave some of the annotations unassigned, leading to an inconsistent model. Like, what if the type of the field is `@Anno Triple<@Anno Integer, @Anno A, @Anno String>` (where `A` is missing) - I may get some of the types with the annotation, and some without, no? Shouldn't the annotations be applied consistently? (It is an issue even now, but now javac reports an error, so it is less of a problem if the model is sub-optimal.)

Yes, I guess to continue with this approach of trying to recover from the `CompletionFailure`s, we'd want to push that handling into the logic for attaching annotations, and recover and continue attaching annotations where possible instead of stopping.

I do think that's a somewhat rare issue. If these diagnostics did end up getting downgraded to warnings, compilations that are relying on accurate type annotation information would likely want to promote them to errors. And the examples in the bug weren't generally trying to read the type annotations, they just wanted compilation to succeed with incomplete classpaths.

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

PR Comment: https://git.openjdk.org/jdk/pull/28018#issuecomment-3460423634