RFR JDK-8208470: Type annotations on inner type that is an array component

[B. Blaser]

Hi Liam,

On Thu, 13 Dec 2018 at 02:05, Liam Miller-Cushon <cushon at google.com> wrote:
>
> 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.

Thanks for your feedback, I'll wait for a Reviewer approval before
integrating it.

> 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:

Good, but this isn't a guarantee.
If we want to clean up this part, I suggest to address this in a separate issue?

> ```
> for (TypeCompound tc : annotations) {
>     Assert.check(tc.position == pos);
> }
> ```

And I think we should live with the assertion above for a while before
eventually dropping it...

> ... 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());
> ```

This cleanup seems harmless to me and I guess I like it, but maybe we
should address all of them in another issue?

> I'm wondering if maybe we can remove that part of `rewriteArrayType`?

I'd play defensively by deferring it as discussed above.

> 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;
> }
> ```

This method is reinstated from:

http://hg.openjdk.java.net/jdk9/dev/langtools/rev/62e285806e83#l6.515

I'm not its author, but I imagine that the assertion is only
protecting from malformed trees?

That said, I think I like your suggestion which doesn't seem too dangerous...
But if we are unsure, we could also live with the assertion for a
while and address this as part of the cleanup issue?

> 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).

Great explanation, thanks for the debugging!

> 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.

I'm of your opinion. This is a minor cosmetic issue and the output is
consistent enough at my mind.

Thanks for your help,
Bernard