-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ECJ out of sync with JLS 9.6.4.1 #1097
ECJ out of sync with JLS 9.6.4.1 #1097
Conversation
@stephan-herrmann, this backs out your code and also advances it forward - May I request you to glance through the changes ? TIA. See comments beginning with #1092 (comment) for background although it turned out #1092 is completely orthogonal |
I was a bit confused by the network of JDK bugs, and about how to figure out which solution replaces which previous solutions, so I collected the changes directly from JLS:
Going from 8 to 17+ would have been easy. With the detour via 14 (which javac skipped?) more effort is needed, so from an outside look, backing out changes done for 14 sounds like the natural thing to do. My next question would be: should ecj follow any of this back and forth based on compliance settings, or should we assume the 17+ variant is simply the best and should be supported for all compliance levels? I'll take an inside look, too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections, just some questions for further clarification, opportunities for (future) cleanup etc.
org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Annotation.java
Outdated
Show resolved
Hide resolved
if ((metaTagBits & (TagBits.AnnotationTargetMASK)) != 0) { | ||
if ((metaTagBits & (TagBits.AnnotationForTypeParameter | TagBits.AnnotationForTypeUse)) == 0) { | ||
if ((metaTagBits & (TagBits.AnnotationTargetMASK)) == 0) { // In the absence of explicit target, applicable only to declaration sites | ||
if (targetType != AnnotationTargetTypeConstants.CLASS_TYPE_PARAMETER && targetType != AnnotationTargetTypeConstants.METHOD_TYPE_PARAMETER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my naive understanding, methods isRuntimeTypeVisible
and isRuntimeTypeInvisible
would only cover codegen for type annotations (declaration scenarii being covered by isRuntimeVisible
/isRuntimeInvisible
).
If that were true then parameter targetType
would be unnecessary, as in absence of a @Target
we can directly answer false
, can't we?
The only test that disagrees is the new testGH1096()
, challenging a no-target annotation on a type parameter.
Does this mean, annotations on type parameters are handled through the TYPE_USE-related code, rather than like "normal" declaration annotations?
Should we schedule a follow-up issue, to reconsider this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my naive understanding, methods
isRuntimeTypeVisible
andisRuntimeTypeInvisible
would only cover codegen for type annotations (declaration scenarii being covered byisRuntimeVisible
/isRuntimeInvisible
). If that were true then parametertargetType
would be unnecessary, as in absence of a@Target
we can directly answerfalse
, can't we?
Consider this program:
import java.lang.annotation.ElementType;
import java.lang.annotation.Target;
@Target(ElementType.TYPE_USE)
@interface TA {
}
@TA
public class X {
}
which reaches
SingleMemberAnnotation(Annotation).isRuntimeTypeInvisible(int) line: 777
ClassFile.generateRuntimeAnnotations(Annotation[], long) line: 4581
ClassFile.addAttributes() line: 423
TypeDeclaration.generateCode(ClassFile) line: 772
TypeDeclaration.generateCode(CompilationUnitScope) line: 831
and org.eclipse.jdt.core.tests.compiler.regression.TypeAnnotationTest.testGH1096()
which reaches the call stack:
MarkerAnnotation(Annotation).isRuntimeTypeInvisible(int) line: 777
TypeReference$AnnotationCollector.internalVisit(Annotation) line: 197
TypeReference$AnnotationCollector.visit(MarkerAnnotation, BlockScope) line: 244
MarkerAnnotation.traverse(ASTVisitor, BlockScope) line: 43
TypeParameter.getAllAnnotationContexts(int, int, List<AnnotationContext>) line: 72
ClassFile.completeMethodInfo(MethodBinding, int, int) line: 2455
MethodDeclaration(AbstractMethodDeclaration).generateCode(ClassFile) line: 377
MethodDeclaration(AbstractMethodDeclaration).generateCode(ClassScope, ClassFile) line: 292
TypeDeclaration.generateCode(ClassFile) line: 761
In both scenarios the method is supposed to return true.
What is a suitable value for the parameter targetType of the method
org.eclipse.jdt.internal.compiler.ast.Annotation.isRuntimeTypeInvisible(int) for the first call (stack) ??
(Despite it not being relevant - the parameter gets "touched" only when no target is specified) I simply invented a value that is outside of all possible type annotation target locations enumerated by table 4.7.20 of the JVMS.
There are only three meaningful values that influence the method's behavior:
targetType == AnnotationTargetTypeConstants.CLASS_TYPE_PARAMETER
targetType == AnnotationTargetTypeConstants.METHOD_TYPE_PARAMETER and
targetType being anything else - For that -1 given the symbolic name NonJVMS4_7_20_TargetLocation was my choice.
Better ideas welcome.
The only test that disagrees is the new
testGH1096()
, challenging a no-target annotation on a type parameter.Does this mean, annotations on type parameters are handled through the TYPE_USE-related code, rather than like "normal" declaration annotations?
Should we schedule a follow-up issue, to reconsider this?
....compiler/src/org/eclipse/jdt/core/tests/compiler/regression/RecordsRestrictedClassTest.java
Show resolved
Hide resolved
@@ -1472,12 +1464,15 @@ public void testDeprecation2() { // verify that deprecation warning does not sho | |||
"@interface T {\n" + | |||
" public int value() default -1;\n" + | |||
"}\n" + | |||
"interface I<@T(1) @T(2) K> {\n" + | |||
"interface I extends @T(1) Runnable {\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the test input changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to retain it as a negative test. Otherwise the code would compile fine with the new behavior implemented
@@ -19,6 +19,8 @@ | |||
|
|||
public interface AnnotationTargetTypeConstants { | |||
|
|||
int NonJVMS4_7_20_TargetLocation = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit at loss, where in JVMS exactly I should look for an explanation...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit at loss, where in JVMS exactly I should look for an explanation...
Per above, I need a sentinel value for the targetType parameter of org.eclipse.jdt.internal.compiler.ast.Annotation.isRuntimeTypeInvisible(int) method that is outside the legal range of values for type annotation target locations enumerated by JVMS Table 4.7.20 -
Better suggestions welcome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than a lot of hand waving (where I couldn't be sure whether or not I was talking bullshit), I experimented with disentangling how we handle type annotations and declaration annotations as much as possible. Please find my result in PR #1117 - which is created on top of this PR but pushed separately as not to interfere with your work here.
The key connection to this constant is: the invocations where the new constant was used could be removed altogether :)
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit at loss, where in JVMS exactly I should look for an explanation...
I have in the latest additional push, gotten rid of this head-scratch producing enumeration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than a lot of hand waving (where I couldn't be sure whether or not I was talking bullshit), I experimented with disentangling how we handle type annotations and declaration annotations as much as possible. Please find my result in PR #1117 - which is created on top of this PR but pushed separately as not to interfere with your work here.
The key connection to this constant is: the invocations where the new constant was used could be removed altogether :)
WDYT?
I will review this - I suggest let us do this in a follow up ticket - (I'll raise one)
Yes, javac never resolved https://bugs.openjdk.org/browse/JDK-8231436 which is still open that corresponds to the spec bug https://bugs.openjdk.org/browse/JDK-8231435 in JDK14 (which was superseded subsequently in JDK17)
IMO, it is best to skip supporting past behavior - it is not the case that a valid program ceases to be valid or changes meaning in some subtle manner - which would call for preserving old behavior with a compliance check. |
sounds good to me. |
NonJVMS4_7_20_TargetLocation.
clean-up on top of PR #1097 + reduce mixed handling of type/declaration annotations - when generating decl annos don't care about visibility as type anno + remove redundant filtering of decl annos + remove interim NonJVMS4_7_20_TargetLocation + replace SE7AnnotationTargetMASK with AnnotationForDeclarationMASK + clarify that AnnotationCollector does not traverse type parameter + a few clarifying code comments + update two tests in RepeatableAnnotationTest: - test026 extend the error message - testUnspecifiedTarget2 don't expect conflict, @t applies to all decls Co-authored-by: Srikanth Sankaran <srikanth.sankaran@advantest.com>
clean-up on top of PR eclipse-jdt#1097 + reduce mixed handling of type/declaration annotations - when generating decl annos don't care about visibility as type anno + remove redundant filtering of decl annos + remove interim NonJVMS4_7_20_TargetLocation + replace SE7AnnotationTargetMASK with AnnotationForDeclarationMASK + clarify that AnnotationCollector does not traverse type parameter + a few clarifying code comments + update two tests in RepeatableAnnotationTest: - test026 extend the error message - testUnspecifiedTarget2 don't expect conflict, @t applies to all decls Co-authored-by: Srikanth Sankaran <srikanth.sankaran@advantest.com>
* Reverse the changes made for https://bugs.eclipse.org/bugs/show_bug.cgi?id=552082 * Tweak for test failures in Jenkins * Implement the recent changes to JLS 9.6.4.1 * Incorporate review comment eclipse-jdt#1097 (comment) * Additional test case for https://bugs.eclipse.org/bugs/show_bug.cgi?id=568240 * Additional regression test for https://bugs.eclipse.org/bugs/show_bug.cgi?id=566803 * Review follow up: Get rid of the head-scratch producing enumerator NonJVMS4_7_20_TargetLocation.
clean-up on top of PR eclipse-jdt#1097 + reduce mixed handling of type/declaration annotations - when generating decl annos don't care about visibility as type anno + remove redundant filtering of decl annos + remove interim NonJVMS4_7_20_TargetLocation + replace SE7AnnotationTargetMASK with AnnotationForDeclarationMASK + clarify that AnnotationCollector does not traverse type parameter + a few clarifying code comments + update two tests in RepeatableAnnotationTest: - test026 extend the error message - testUnspecifiedTarget2 don't expect conflict, @t applies to all decls Co-authored-by: Srikanth Sankaran <srikanth.sankaran@advantest.com>
* Reverse the changes made for https://bugs.eclipse.org/bugs/show_bug.cgi?id=552082 * Tweak for test failures in Jenkins * Implement the recent changes to JLS 9.6.4.1 * Incorporate review comment eclipse-jdt#1097 (comment) * Additional test case for https://bugs.eclipse.org/bugs/show_bug.cgi?id=568240 * Additional regression test for https://bugs.eclipse.org/bugs/show_bug.cgi?id=566803 * Review follow up: Get rid of the head-scratch producing enumerator NonJVMS4_7_20_TargetLocation.
clean-up on top of PR eclipse-jdt#1097 + reduce mixed handling of type/declaration annotations - when generating decl annos don't care about visibility as type anno + remove redundant filtering of decl annos + remove interim NonJVMS4_7_20_TargetLocation + replace SE7AnnotationTargetMASK with AnnotationForDeclarationMASK + clarify that AnnotationCollector does not traverse type parameter + a few clarifying code comments + update two tests in RepeatableAnnotationTest: - test026 extend the error message - testUnspecifiedTarget2 don't expect conflict, @t applies to all decls Co-authored-by: Srikanth Sankaran <srikanth.sankaran@advantest.com>
What it does
Reverse the changes made for https://bugs.eclipse.org/bugs/show_bug.cgi?id=552082 as a first step
To sync up with JLS 9.6.4.1 by a subsequent commit to fix #1096
How to test
Author checklist