RFR JDK-8208470: Type annotations on inner type that is an array component
Liam Miller-Cushon
cushon at google.com
Thu Dec 13 01:05:27 UTC 2018
Hi Bernard,
On Tue, Sep 18, 2018 at 6:42 AM B. Blaser <bsrbnd at gmail.com> wrote:
> Please review the following fix for [1]:
> http://cr.openjdk.java.net/~bsrbnd/jdk8208470/webrev.00/
I'm not a Reviewer, but this approach looks good to me overall.
A couple of minor notes:
There was some previous discussion (
http://mail.openjdk.java.net/pipermail/compiler-dev/2018-July/012264.html)
about this part:
```
// Update positions
for (TypeCompound tc : annotations) {
if (tc.position == null)
tc.position = pos;
}
```
As far as I can tell it's redundant. Adding the following assertion to
`typeWithAnnotations` doesn't break any tests:
```
for (TypeCompound tc : annotations) {
Assert.check(tc.position == pos);
}
```
... which matches the comment in the logic for nested types in
`typeWithAnnotations` that "all annotations share the same position; modify
the first one", and suggests that could be cleaned up too. (
http://hg.openjdk.java.net/jdk/jdk/file/2626982cf4f7/src/jdk.compiler/share/classes/com/sun/tools/javac/code/TypeAnnotations.java#l525
)
```
- // All annotations share the same position; modify the
first one.
- Attribute.TypeCompound a = annotations.get(0);
- TypeAnnotationPosition p = a.position;
- p.location = p.location.appendList(depth.toList());
+ pos.location = pos.location.appendList(depth.toList());
```
I'm wondering if maybe we can remove that part of `rewriteArrayType`?
nit: arrayTypeTree is inconsistent about asserting on unexpected types vs.
just performing casts (e.g. it would throw a CCE for a non-array
JCAnnotatedType tree). Unless that assertion is defending against something
specific, consider just:
```
private JCArrayTypeTree arrayTypeTree(JCTree typetree) {
if (typetree.getKind() == JCTree.Kind.ANNOTATED_TYPE) {
typetree = ((JCAnnotatedType) typetree).underlyingType;
}
return (JCArrayTypeTree) typetree;
}
```
On Wed, Dec 12, 2018 at 6:56 AM B. Blaser <bsrbnd at gmail.com> wrote:
> Now, the question of Werner to be addressed in a separate issue is why
> isn't there any @TA on "new" expression warnings?
>
I did some debugging, and it looks like the rawtypes diagnostic
pretty-prints the attributed type, and at that point the type annotations
in `B [] arr = new @TA B [0];` have been deferred and not yet added to the
type (see Annotate#flush).
I'm not sure what the principled fix is, but forcing the deferred
annotations to be processed before emitting the diagnostics (by adding in
an explicit call to `flush()`) makes the diagnostics consistently include
type annotation information. In my opinion the effect on diagnostic quality
is fairly minor, and fixing that inconsistency could safely be deferred.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20181212/fce9b6f3/attachment.html>
More information about the compiler-dev
mailing list