openjdk
8370800: Downgrade cant.attach.type.annotations diagnostics to warnings
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)
- [x] Change must not contain extraneous whitespace
- [x] 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
:wave: 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.
@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.
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)
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.)
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
@Annowould seem more or less OK to me, but ignoring errors for missing typeAmakes 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,DeferredCompletionFailureHandlerneeds to be used to re-set theClassSymbolforAto the original state.speculativeCodeHandlermight be usable for this (look how it is used inDeferredAttr).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
CompletionFailureat 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>(whereAis 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.
⚠️ @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).
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?
I wonder if a better approach here is to continue to report
cant.attach.type.annotationsas an unconditional error, but to defer the work down byaddTypeAnnotationsToSymboluntil 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.
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().
I have realized this approach doesn't work for all cases, because there are public APIs that allow accessing type annotations on an
ExecutableElementthat don't result in the underlyingMethodSymbolgetting 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.
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.
Thanks for taking another look, and I'm happy to write up a CSR. (If we go with a warning I would like to consider putting it in an -Xlint category, that detail could be discussed in the CSR if you're open to considering that.)
I'm curious if you'd be willing to share your thoughts on the alternative of lazily attaching the type annotations in MethodSymbol#complete, though? I think in theory I prefer that behaviour. In practice it seems like complete is only intended to be used for ClassSymbol, so using it for MethodSymbol is maybe a significant departure from that and would require 'hardening' MethodSymbol to ensure that it actually calls complete in all of the methods that would allow accessing type annotation information.
If it was only for methods (and possibly fields), then using the completers might work at this time, as we don't use completers on those for anything else (I think!). If we ever wanted to use the completers on methods/fields for anything else, then might get into a hard-to-solve state. (Overall, using completers and very carefully calling complete at specific times but not otherwise to avoid errors feels a bit subtle - the current behavior of completers is already pretty complex.)
For classes, completing is done often and at "arbitrary" points in time - we could not use the same trick there. And I am not sure if we could attach TAs on classes eagerly. E.g. enhancing your test with:
class B<T extends @Anno A<String>> {
..
would lead to a failure, while having:
class B<T extends A<String>> {
...
would not, I think.
I was thinking if we could attach the TAs without completing the types (which would allow us to keep the same behavior that is present without the TAs), but I didn't see a reasonable way to do that.
I am not sure if we could attach TAs on classes eagerly.
The feedback I've heard about these diagnostics has been for the field/method cases. I understand the use-case is something like the example below, where in practice the issues that have come up are all with method (and maybe fields), but not classes. I think eagerly attaching TAs to classes and only lazily attaching TAs for methods/fields would address the complaints.
// Api is packaged in a jar that does not include Foo or Bar, clients optionally bring their own implementations of those classes
class Api {
static void foo(Foo foo) {...}
static void bar(Bar foo) {...}
}
// Client depends on a jar providing API, and a jar providing Foo. Bar is not on the classpath.
class Client {
void (Api api, Foo foo) {
api.foo(foo);
}
}
@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!
/touch
I'm still unsure what the best approach here is.
in practice the issues that have come up are all with method (and maybe fields), but not classes. I think eagerly attaching TAs to classes and only lazily attaching TAs for methods/fields would address the complaints
I also recognize there are other downsides of using field/method completers (including that it would make it hard to use them for other things in the future, and it requires a lot of care to ensure completion happens at the right times).
I don't see a clear favourite among the options:
- Completers (see above)
- Downgrade to warnings
- missing types are ignored, annotations are missing from model elements
- some clients will be running with
-Werrorand will want to be warnings clean, which is an issue for library owners that are currently relying on the lenient handling of missing types referenced by fields/methods. Putting this behind an-Xlint:category doesn't fully solve that problem
- Do nothing for now
- avoids implementation tradeoffs around using completers, or downgrading diagnostics for missing classes
- doesn't address the problem for library owners
I think (1) best addresses the problem that's been reported by library authors. But I am not sure if that's going to have a bug tail of subtle completer issues, or cause problems later if there's another use-case for completers on fields/methods.
@cushon The pull request is being re-evaluated and the inactivity timeout has been reset.
Hmm. Catching up on this PR has been on to-do list for a while. Generally there are conditions in the JLS that justify rejecting (or accepting) a program.
What do the relevant specifications say here?
@jddarcy, I am afraid this is a bit in a gray zone. Consider this sequence of projects/files:
----------- ./prj1/src/A.java
public class A<T> {}
----------- ./prj2/src/B.java
import java.lang.annotation.ElementType;
import java.lang.annotation.Target;
import java.util.List;
public class B {
private void test (A<String> a) {}
}
@Target(ElementType.TYPE_USE)
@interface TA {}
----------- ./prj3/src/C.java
public class C {
public B b;
public void test() {
b.toString();
}
}
And the a sequence of compilations like (this is a standing behavior, unrelated to this PR):
$ javac -d prj1/classes prj1/src/A.java
$ javac -d prj2/classes -cp prj1/classes/ prj2/src/B.java
$ javac -d prj3/classes -cp prj2/classes/ prj3/src/C.java
note:
- there are no errors
Ais not on the compile classpath when compilingC, butBdepends onA
When compiling C, if javac looked at B in the source form, it would be uncompilable, as A would not be resolvable. But, since B is load from a classfile, javac doesn't proactively check whether A exists, until it has a "reason" to look for A (at which point it will find out it can't find the definition, and will produce a compile-time error).
AFAIK JLS sees compilation units only it their source form, so strictly speaking, I believe we could always produce an error for any missing class - but the long-standing behavior is that javac only looks for classes/interfaces that are referred to by classfiles if it has a reason to do so.
Now, consider I do this change in B.java:
private void test (A<String> a) {}
=>
private void test (@TA A<String> a) {}
The compilations now will be (JDK 25):
$ javac -d prj1/classes prj1/src/A.java
$ javac -d prj2/classes -cp prj1/classes/ prj2/src/B.java
$ javac -d prj3/classes -cp prj2/classes/ prj3/src/C.java
prj2/classes/B.class: error: Cannot attach type annotations @TA to B.test:
class file for A not found
1 error
And while technically speaking, I think javac can produce this error, there's a question whether it is reasonable. Especially in this case, where all that was done was adding a type annotation to a type in a private method, and the outcome is that the compilation passed before and now fails.
Another place this could be informed by the specification is the specification of runtime behaviour when loading and linking classes with references to missing classes. I don't have as good an understanding of JLS 12 and JVMS 5 as I'd like, but it was surprising to me how tolerant the runtime behaviour is of missing classes.
For example, the following program successfully calls a method whose descriptor references a return type that doesn't exist.
=== ./A.java ===
class A {}
=== ./B.java ===
import java.util.List;
class B {
A f(List<A> xs) {
return null;
}
}
=== ./T.java ===
import java.util.List;
class T {
public static void main(String[] args) {
new B();
new B().f(List.of());
var a = new B().f(List.of());
boolean b = a == new Object();
System.err.println("hello");
}
}
$ javac -g *.java; rm A.class; java T
hello