Type annotations on inner type that is an array components
Werner Dietl
wdietl at gmail.com
Mon Jul 30 03:31:31 UTC 2018
Thanks to Liam for pointing out how to run the tests and, of course, I
had missed a case: annotations on the array dimensions.
I re-added the arrayTypeTree helper method that was removed in
http://hg.openjdk.java.net/jdk9/dev/langtools/rev/62e285806e83
and tier1 tests are now clean, except for some failures that already
existed for me.
Updated patch below.
Best,
cu, WMD.
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/TypeAnnotations.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/TypeAnnotations.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/TypeAnnotations.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/TypeAnnotations.java
@@ -50,7 +50,6 @@
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Type.ModuleType;
-import com.sun.tools.javac.code.TypeMetadata.Entry.Kind;
import com.sun.tools.javac.comp.Annotate;
import com.sun.tools.javac.comp.Attr;
import com.sun.tools.javac.comp.AttrContext;
@@ -423,8 +422,9 @@
return type;
}
- if (type.hasTag(TypeTag.ARRAY))
- return rewriteArrayType((ArrayType)type, annotations, pos);
+ if (type.hasTag(TypeTag.ARRAY)) {
+ return rewriteArrayType(typetree, (ArrayType)type,
annotations, pos);
+ }
if (type.hasTag(TypeTag.TYPEVAR)) {
return type.annotatedType(onlyTypeAnnotations);
@@ -536,15 +536,17 @@
*
* SIDE EFFECT: Update position for the annotations to be {@code pos}.
*/
- private Type rewriteArrayType(ArrayType type,
List<TypeCompound> annotations, TypeAnnotationPosition pos) {
+ private Type rewriteArrayType(JCTree typetree, ArrayType type,
+ List<TypeCompound> annotations, TypeAnnotationPosition pos) {
ArrayType tomodify = new ArrayType(type);
- ArrayType res = tomodify;
+ final ArrayType res = tomodify;
List<TypePathEntry> loc = List.nil();
// peel one and update loc
Type tmpType = type.elemtype;
loc = loc.prepend(TypePathEntry.ARRAY);
+ JCTree tmpTypeTree = arrayTypeTree(typetree).elemtype;
while (tmpType.hasTag(TypeTag.ARRAY)) {
ArrayType arr = (ArrayType)tmpType;
@@ -556,37 +558,32 @@
tmpType = arr.elemtype;
loc = loc.prepend(TypePathEntry.ARRAY);
+ tmpTypeTree = arrayTypeTree(tmpTypeTree).elemtype;
}
+ // Update positions
+ // All annotations share the same position; modify the first one.
+ TypeCompound tc = annotations.get(0);
+ tc.position = pos;
+ tc.position.location = loc;
+
// Fix innermost element type
- Type elemType;
- if (tmpType.getMetadata() != null) {
- List<TypeCompound> tcs;
- if (tmpType.getAnnotationMirrors().isEmpty()) {
- tcs = annotations;
- } else {
- // Special case, lets prepend
- tcs =
annotations.appendList(tmpType.getAnnotationMirrors());
- }
- elemType = tmpType.cloneWithMetadata(tmpType
- .getMetadata()
- .without(Kind.ANNOTATIONS)
- .combine(new TypeMetadata.Annotations(tcs)));
- } else {
- elemType = tmpType.cloneWithMetadata(new
TypeMetadata(new TypeMetadata.Annotations(annotations)));
- }
- tomodify.elemtype = elemType;
-
- // Update positions
- for (TypeCompound tc : annotations) {
- if (tc.position == null)
- tc.position = pos;
- tc.position.location = loc;
- }
+ tomodify.elemtype = typeWithAnnotations(tmpTypeTree,
tmpType, annotations, annotations, pos);
return res;
}
+ private JCArrayTypeTree arrayTypeTree(JCTree typetree) {
+ if (typetree.getKind() == JCTree.Kind.ARRAY_TYPE) {
+ return (JCArrayTypeTree) typetree;
+ } else if (typetree.getKind() == JCTree.Kind.ANNOTATED_TYPE) {
+ return (JCArrayTypeTree) ((JCAnnotatedType)
typetree).underlyingType;
+ } else {
+ Assert.error("Could not determine array type from
type tree: " + typetree);
+ return null;
+ }
+ }
+
/** Return a copy of the first type that only differs by
* inserting the annotations to the left-most/inner-most type
* or the type given by stopAt.
@@ -707,9 +704,7 @@
List<JCTree> path,
JCLambda currentLambda,
int outer_type_index,
- ListBuffer<TypePathEntry> location)
- {
-
+ ListBuffer<TypePathEntry> location) {
// Note that p.offset is set in
// com.sun.tools.javac.jvm.Gen.setTypeAnnotationPositions(int)
@@ -1399,37 +1394,25 @@
scan(tree.elems);
}
-
- private void findTypeCompoundPosition(JCTree tree, JCTree
frame, List<Attribute.TypeCompound> annotations) {
+ private void findPosition(JCTree tree, JCTree frame,
List<JCAnnotation> annotations) {
if (!annotations.isEmpty()) {
final TypeAnnotationPosition p =
- resolveFrame(tree, frame, frames,
currentLambda, 0, new ListBuffer<>());
- for (TypeCompound tc : annotations)
- tc.position = p;
- }
- }
-
- private void findPosition(JCTree tree, JCTree frame,
List<JCAnnotation> annotations) {
- if (!annotations.isEmpty())
- {
- final TypeAnnotationPosition p =
resolveFrame(tree, frame, frames, currentLambda,
0, new ListBuffer<>());
setTypeAnnotationPos(annotations, p);
}
}
- private void setTypeAnnotationPos(List<JCAnnotation>
annotations, TypeAnnotationPosition position)
- {
+ private void setTypeAnnotationPos(List<JCAnnotation>
annotations, TypeAnnotationPosition position) {
// attribute might be null during DeferredAttr;
// we will be back later.
for (JCAnnotation anno : annotations) {
- if (anno.attribute != null)
+ if (anno.attribute != null) {
((Attribute.TypeCompound)
anno.attribute).position = position;
+ }
}
}
-
@Override
public String toString() {
return super.toString() + ": sigOnly: " + sigOnly;
On Sun, Jul 29, 2018 at 7:26 PM Werner Dietl <wdietl at gmail.com> wrote:
>
> Hi Bernard,
>
> thanks for looking into this bug!
>
> I created a bug:
> https://bugs.openjdk.java.net/browse/JDK-8208470
> Please let me know if I missed some information.
>
> I was a bit worried about the different changes in your first patch.
> Based on the missing method invocation you uncovered in your second
> email, I made the patch below.
> (It includes a few whitespace and style cleanups, and removes a dead method.)
> Overall, `rewriteArrayType` is much easier to read after the change, I hope.
>
> How do I run tier1 tests? I looked a bit in the compiler team
> documentation, but didn't find current instructions.
>
> Please let me know if I should send the patch in a different format or
> if you have any comments!
>
> Thanks,
> cu, WMD.
>
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/TypeAnnotations.java
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/TypeAnnotations.java
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/TypeAnnotations.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/TypeAnnotations.java
> @@ -50,7 +50,6 @@
> import com.sun.tools.javac.code.Symbol.VarSymbol;
> import com.sun.tools.javac.code.Symbol.MethodSymbol;
> import com.sun.tools.javac.code.Type.ModuleType;
> -import com.sun.tools.javac.code.TypeMetadata.Entry.Kind;
> import com.sun.tools.javac.comp.Annotate;
> import com.sun.tools.javac.comp.Attr;
> import com.sun.tools.javac.comp.AttrContext;
> @@ -423,8 +422,9 @@
> return type;
> }
>
> - if (type.hasTag(TypeTag.ARRAY))
> - return rewriteArrayType((ArrayType)type, annotations, pos);
> + if (type.hasTag(TypeTag.ARRAY)) {
> + return rewriteArrayType(typetree, (ArrayType)type,
> annotations, pos);
> + }
>
> if (type.hasTag(TypeTag.TYPEVAR)) {
> return type.annotatedType(onlyTypeAnnotations);
> @@ -536,15 +536,17 @@
> *
> * SIDE EFFECT: Update position for the annotations to be {@code pos}.
> */
> - private Type rewriteArrayType(ArrayType type,
> List<TypeCompound> annotations, TypeAnnotationPosition pos) {
> + private Type rewriteArrayType(JCTree typetree, ArrayType type,
> + List<TypeCompound> annotations, TypeAnnotationPosition pos) {
> ArrayType tomodify = new ArrayType(type);
> - ArrayType res = tomodify;
> + final ArrayType res = tomodify;
>
> List<TypePathEntry> loc = List.nil();
>
> // peel one and update loc
> Type tmpType = type.elemtype;
> loc = loc.prepend(TypePathEntry.ARRAY);
> + JCTree tmpTypeTree = ((JCArrayTypeTree) typetree).elemtype;
>
> while (tmpType.hasTag(TypeTag.ARRAY)) {
> ArrayType arr = (ArrayType)tmpType;
> @@ -556,33 +558,17 @@
>
> tmpType = arr.elemtype;
> loc = loc.prepend(TypePathEntry.ARRAY);
> + tmpTypeTree = ((JCArrayTypeTree) tmpTypeTree).elemtype;
> }
>
> + // Update positions
> + // All annotations share the same position; modify the first one.
> + TypeCompound tc = annotations.get(0);
> + tc.position = pos;
> + tc.position.location = loc;
> +
> // Fix innermost element type
> - Type elemType;
> - if (tmpType.getMetadata() != null) {
> - List<TypeCompound> tcs;
> - if (tmpType.getAnnotationMirrors().isEmpty()) {
> - tcs = annotations;
> - } else {
> - // Special case, lets prepend
> - tcs =
> annotations.appendList(tmpType.getAnnotationMirrors());
> - }
> - elemType = tmpType.cloneWithMetadata(tmpType
> - .getMetadata()
> - .without(Kind.ANNOTATIONS)
> - .combine(new TypeMetadata.Annotations(tcs)));
> - } else {
> - elemType = tmpType.cloneWithMetadata(new
> TypeMetadata(new TypeMetadata.Annotations(annotations)));
> - }
> - tomodify.elemtype = elemType;
> -
> - // Update positions
> - for (TypeCompound tc : annotations) {
> - if (tc.position == null)
> - tc.position = pos;
> - tc.position.location = loc;
> - }
> + tomodify.elemtype = typeWithAnnotations(tmpTypeTree,
> tmpType, annotations, annotations, pos);
>
> return res;
> }
> @@ -707,9 +693,7 @@
> List<JCTree> path,
> JCLambda currentLambda,
> int outer_type_index,
> - ListBuffer<TypePathEntry> location)
> - {
> -
> + ListBuffer<TypePathEntry> location) {
> // Note that p.offset is set in
> // com.sun.tools.javac.jvm.Gen.setTypeAnnotationPositions(int)
>
> @@ -1399,37 +1383,25 @@
> scan(tree.elems);
> }
>
> -
> - private void findTypeCompoundPosition(JCTree tree, JCTree
> frame, List<Attribute.TypeCompound> annotations) {
> + private void findPosition(JCTree tree, JCTree frame,
> List<JCAnnotation> annotations) {
> if (!annotations.isEmpty()) {
> final TypeAnnotationPosition p =
> - resolveFrame(tree, frame, frames,
> currentLambda, 0, new ListBuffer<>());
> - for (TypeCompound tc : annotations)
> - tc.position = p;
> - }
> - }
> -
> - private void findPosition(JCTree tree, JCTree frame,
> List<JCAnnotation> annotations) {
> - if (!annotations.isEmpty())
> - {
> - final TypeAnnotationPosition p =
> resolveFrame(tree, frame, frames, currentLambda,
> 0, new ListBuffer<>());
>
> setTypeAnnotationPos(annotations, p);
> }
> }
>
> - private void setTypeAnnotationPos(List<JCAnnotation>
> annotations, TypeAnnotationPosition position)
> - {
> + private void setTypeAnnotationPos(List<JCAnnotation>
> annotations, TypeAnnotationPosition position) {
> // attribute might be null during DeferredAttr;
> // we will be back later.
> for (JCAnnotation anno : annotations) {
> - if (anno.attribute != null)
> + if (anno.attribute != null) {
> ((Attribute.TypeCompound)
> anno.attribute).position = position;
> + }
> }
> }
>
> -
> @Override
> public String toString() {
> return super.toString() + ": sigOnly: " + sigOnly;
> On Wed, Jul 25, 2018 at 2:04 PM B. Blaser <bsrbnd at gmail.com> wrote:
> >
> > Hi,
> >
> > On 19 July 2018 at 21:23, Alex Buckley <alex.buckley at oracle.com> wrote:
> > > Hi Werner,
> > >
> > > On 7/18/2018 4:49 PM, Werner Dietl wrote:
> > >>
> > >> class ArrayOfInner {
> > >> class Inner {}
> > >>
> > >> @TA(1) ArrayOfInner. @TA(2) Inner oi;
> > >> @TA(3) ArrayOfInner. @TA(4) Inner [] oia;
> > >> @TA(5) Inner i;
> > >> @TA(6) Inner [] ia;
> > >> }
> > >>
> > >>
> > >> Compile with javac 8 and according to javap -v field ia has the
> > >> annotation:
> > >>
> > >> ArrayOfInner$Inner[] ia;
> > >> descriptor: [LArrayOfInner$Inner;
> > >> flags: (0x0000)
> > >> RuntimeVisibleTypeAnnotations:
> > >> 0: #10(#11=I#21): FIELD, location=[ARRAY, INNER_TYPE]
> > >>
> > >> Compile with javac 9, 10, or a recent 11 build and ia has the annotation:
> > >>
> > >> ArrayOfInner$Inner[] ia;
> > >> descriptor: [LArrayOfInner$Inner;
> > >> flags: (0x0000)
> > >> RuntimeVisibleTypeAnnotations:
> > >> 0: #10(#11=I#21): FIELD, location=[ARRAY]
> > >>
> > >> Note the missing INNER_TYPE location.
> > >>
> > >> I would like to argue that the Java 8 behavior was correct.
> > >
> > >
> > > Sounds right. Given source code that denotes the array type "Inner[]", the
> > > question is whether the component type of that array type is a nested type
> > > (ArrayOfInner.Inner) or not (just Inner). Logically, yes, it's a nested
> > > type, so the target_path item deserves an INNER_TYPE entry.
> > >
> > > This feels like the kind of detail we've been round and round on in the
> > > past. That said, I don't see any mails that would have led to javac changing
> > > how it emits target_path between 8 and 9. The closest suspect is a
> > > compiler-dev thread from Jan/Feb 2016 -- "Bug in encoding type annotations
> > > for supertype_targets" -- which spoke about the target_path being omitted
> > > entirely for an annotated nested type. (JDK-8148504 was filed against 8, but
> > > it's still open, so it can't have caused any collateral damage for a field
> > > type's annotation.)
> > >
> > > Alex
> >
> > Doing some archaeology in 'TypeAnnotations.java' I found a suspect
> > change-set for JDK-8031744:
> >
> > https://bugs.openjdk.java.net/browse/JDK-8031744
> >
> > The array logic has been bundled in 'rewriteArrayType()' but the
> > following line seems to have disappeared:
> >
> > http://hg.openjdk.java.net/jdk9/dev/langtools/rev/62e285806e83#l6.440
> >
> > This change-set might be the culprit but I haven't verified.
> >
> > Bernard
>
>
>
> --
> http://www.google.com/profiles/wdietl
--
http://www.google.com/profiles/wdietl
More information about the compiler-dev
mailing list