8370800: Downgrade cant.attach.type.annotations diagnostics to warnings

[cushon]

Hi, please consider this fix for JDK-8370800: Downgrade cant.attach.type.annotations diagnostics to warnings.

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.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8370800: Downgrade cant.attach.type.annotations diagnostics to warnings (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28018/head:pull/28018
$ git checkout pull/28018

Update a local copy of the PR:
$ git checkout pull/28018
$ git pull https://git.openjdk.org/jdk.git pull/28018/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28018

View PR using the GUI difftool:
$ git pr show -t 28018

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28018.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back cushon! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@cushon The following label will be automatically applied to this pull request:

• compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (c65a7ac5)
• 01: Full - Incremental (0a6c9590)
• 00: Full (8a5aba58)

[lahodaj]

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.

But, even if we decided to ignore the missing class error, the implementation is, sadly, wrong. We cannot just ignore the CompletionFailure, as that will never be thrown again for the given ClassSymbol. And then javac may proceed to generate a classfile for a broken input. For example, changing the test to:

    void testMissingEnclosingType() throws Exception {
        String annoSrc =
                """
                import static java.lang.annotation.ElementType.TYPE_USE;
                import java.lang.annotation.Target;
                @Target(TYPE_USE)
                @interface Anno {}

                class A<E> {}

                class B {
                  public @Anno A<String> a;
                }
                """;
        String cSrc =
                """
                class C {
                  B b;
                  public void test() {
                      b.a.toString();
                  }
                }
                """;

        Path base = Paths.get(".");
        Path src = base.resolve("src");
        tb.createDirectories(src);
        tb.writeJavaFiles(src, annoSrc, cSrc);
        Path out = base.resolve("out");
        tb.createDirectories(out);
        new JavacTask(tb).outdir(out).files(tb.findJavaFiles(src)).run();

        // now if we remove A.class javac should not crash
        tb.deleteFiles(out.resolve("A.class"));

        List<String> log =
                new JavacTask(tb)
                        .outdir(out)
                        .classpath(out)
                        .options(/*"-Werror", */"-XDrawDiagnostics")
                        .files(src.resolve("C.java"))
                        .run(Expect.FAIL)
                        .writeAll()
                        .getOutputLines(Task.OutputKind.DIRECT);

        var expectedOutput =
                List.of(
                        "B.class:-:-: compiler.warn.cant.attach.type.annotations: @Anno, B, a,"
                                + " (compiler.misc.class.file.not.found: A)",
                        "- compiler.err.warnings.and.werror",
                        "1 error",
                        "1 warning");
        if (!expectedOutput.equals(log)) {
            throw new Exception("expected output not found: " + log);
        }
    }

leads to:

An exception has occurred in the compiler (26-internal). Please file a bug against the Java compiler via the Java bug reporting page (https://bugreport.java.com) after checking the Bug Database (https://bugs.java.com) for duplicates. Include your program, the following diagnostic, and the parameters passed to the Java compiler in your report. Thank you.
java.lang.ClassCastException: class com.sun.tools.javac.code.Symbol$ClassSymbol cannot be cast to class com.sun.tools.javac.code.Symbol$MethodSymbol (com.sun.tools.javac.code.Symbol$ClassSymbol and com.sun.tools.javac.code.Symbol$MethodSymbol are in module jdk.compiler of loader 'app')
	at jdk.compiler/com.sun.tools.javac.comp.TransTypes.visitApply(TransTypes.java:931)
	at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodInvocation.accept(JCTree.java:1869)
	at jdk.compiler/com.sun.tools.javac.tree.TreeTranslator.translate(TreeTranslator.java:58)
	at jdk.compiler/com.sun.tools.javac.comp.TransTypes.translate(TransTypes.java:450)
...

I think that if you really want to ignore the CompletionFailures 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.

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.)

[cushon]

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 CompletionFailures 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 CompletionFailures, 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.