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.

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change requires a CSR request matching fixVersion 28 to be approved (needs to be created)
• 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

• 07: Full - Incremental (ecad620d)
• 06: Full - Incremental (39c4741f)
• 05: Full - Incremental (74512b21)
• 04: Full - Incremental (d78c3425)
• 03: Full - Incremental (2781df57)
• 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.

[openjdk]

⚠️ @cushon This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

[cushon]

I added a comment to https://bugs.openjdk.org/browse/JDK-8370800 with some more analysis. I think the core of the new behaviour that is surprising and potentially undesirable is

javac is more eagerly completing symbols that are referenced in the API of any libraries referenced by the current compilation, if those APIs have type annotations

I wonder if a better approach here is to continue to report cant.attach.type.annotations as an unconditional error, but to defer the work down by addTypeAnnotationsToSymbol until the symbol is completed, instead of doing it unconditionally when annotations are completed after reading the class.

That would mean that if the only place javac needing a missing class was to attach type annotations, a regular compilation would succeed, but a compilation with an annotation processor that tried to read those type annotations would still get a cant.attach.type.annotations error.

I have uploaded a new draft where addTypeAnnotationsToSymbol is deferred until the annotated symbol is completed.

@lahodaj do you think that approach might have any merit?

[cushon]

I wonder if a better approach here is to continue to report cant.attach.type.annotations as an unconditional error, but to defer the work down by addTypeAnnotationsToSymbol until the symbol is completed, instead of doing it unconditionally when annotations are completed after reading the class.

I have been doing some more testing with this and it seems to work. I adjusted the initial approach to ensure it's only deferring attaching type annotations to symbol completion for non-class members, classes are still completed eagerly, which has the desired behaviour for type annotations and also avoids interactions with how annotation processing resets completers for class symbols.

[cushon]

I have realized this approach doesn't work for all cases, because there are public APIs that allow accessing type annotations on an ExecutableElement that don't result in the underlying MethodSymbol getting completed. It would be possible to fix that by ensuring that methods like MethodSymbol#asType call complete().

[cushon]

I have realized this approach doesn't work for all cases, because there are public APIs that allow accessing type annotations on an ExecutableElement that don't result in the underlying MethodSymbol getting completed

In particular, JavacElements#getAllMembers doesn't complete the elements it returns. Element#getEnclosedElements does complete the members it returns, and many annotation processors use #getEnclosedElements to get ExecutableElements they process, which partly explains why this was working in many cases.

getAllMembers could be updated to do completion, but there may be other paths that return ExecutableElements that would need similar updates, the way that methods that return or operate on class symbols all ensure the class has been completed.

in general I'm still unsure if using the completion mechanism for MethodSymbol is an acceptable approach.

[lahodaj]

I was looking into this a bit more. I am afraid I don't see any really good solution. So, out of the not-so-good solutions, the original solution seems least problematic. (With the DeferredCompletionFailureHandler, of course, we can't ignore the CompletionFailures completely.) I suspect this probably should be documented in a CSR.

@jddarcy, what do you think?

Sorry for the fuss.

[openjdk]

@jddarcy has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@cushon please create a CSR request for issue JDK-8370800 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[cushon]

I am leaning towards holding off on changes here until there's a clear consensus about which of the approaches in is best.

Are there any more thoughts on the approaches discussed in #28018 (comment)?

I think using symbol completion has the most desirable behaviour, but completers have more implementation complexity and risk. Downgrading the diagnostics to warnings is easier to reason about, but doesn't as precisely address the problem.

[bridgekeeper]

@cushon This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[cushon]

/touch

I've heard of at least one more recent example of a cant.attach.type.annotations diagnostic requiring a workaround. I think the symbol completion option here has the best ergonomics, but also still have concerns about the downsides of using field/method completers for this. I'd like to leave this open for a bit longer to keep thinking about it.

[openjdk]

@cushon The pull request is being re-evaluated and the inactivity timeout has been reset.

[bridgekeeper]

@cushon This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[cushon]

/touch

[openjdk]

@cushon The pull request is being re-evaluated and the inactivity timeout has been reset.

[bridgekeeper]

@cushon This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[cushon]

/touch

[openjdk]

@cushon The pull request is being re-evaluated and the inactivity timeout has been reset.

[bridgekeeper]

@cushon This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[cushon]

/touch

[openjdk]

@cushon The pull request is being re-evaluated and the inactivity timeout has been reset.

[bridgekeeper]

@cushon This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@cushon This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[cushon]

/open

[openjdk]

@cushon This pull request is now open

[bridgekeeper]

@cushon This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[cushon]

/touch

[openjdk]

@cushon The pull request is being re-evaluated and the inactivity timeout has been reset.