RFR: 8043226: Better diagnostics for non-applicable type annotations

[Werner Dietl]

On Fri, 10 Nov 2023 01:21:28 GMT, Liam Miller-Cushon <cushon at openjdk.org> wrote:

> Hi,
> 
> Please consider this improvement to diagnostics on inadmissable type annotations.
> 
> [JDK-8057683: clarify error messages trying to annotate scoping](https://bugs.openjdk.org/browse/JDK-8057683) is closely related to JDK-8043226 and I think could be made a duplicate of that bug. [JDK-8057683: improve ordering of errors with type annotations](https://bugs.openjdk.org/browse/JDK-8057683) is also closely related, and this PR partially fixes the issues described in that bug.
> 
> I have some notes on details of the proposed changes below.
> 
> ---
> 
> Currently javac reports 'scoping construct cannot be annotated' diagnostics for type annotations at locations where they are not admissable. 
> 
> As discussed in [JDK-8043226](https://bugs.openjdk.org/browse/JDK-8043226) and [JDK-8057683](https://bugs.openjdk.org/browse/JDK-8057683), the current language is unclear. The 'scoping construct' language was used in JSR-308 discussions but didn't end up in the final specification, JLS §9.7.4 talks about where annotations are 'admissable', and that language is mirrored in JVMS §4.7.20.2.
> 
> This change updates the diagnostics to state that type annotations 'not admissible at this location' and removed the reference to 'scoping constructs'. Additionally, the diagnostic now includes an explanation of a location in the type where annotations would be admissible, to make it easier to understand how to annotate qualified type names.
> 
> Before:
> 
> 
> test/langtools/tools/javac/annotations/typeAnnotations/failures/CantAnnotateScoping.java:38: error: scoping construct cannot be annotated with type-use annotation: @TA
>     @TA Outer.SInner osi;
>              ^
> 
> 
> After:
> 
> 
> test/langtools/tools/javac/annotations/typeAnnotations/failures/CantAnnotateScoping.java:38: error: type annotations are not admissible at this location: @TA
>     @TA Outer.SInner osi;
>              ^
>   (to annotate a qualified type, write Outer. at TA SInner)
> 
> 
> ---
> 
> Attribution currently assumes `@TA java.lang.Object` in `List<@TA java.lang.Object>` will be a type (not a package), and reports a resolution failure when it can't find a class named `java`.
> 
> This change modifies attribution to search for packages as well as types when resolving annotated types, and relies on the subsequent error checking for annotated types to report that type annotations are not admissible at that location.
> 
> This also improves the ordering of the diagnostic in the output, since attribution errors are report before type annotation validation errors in the l...

Thanks for working on this! Improving the compiler error messages is a very important task.
I'm not a reviewer, so just some comments from going through the code (I didn't compile and run it yet).

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 5274:

> 5272:                 attribTree(tree.underlyingType, env, new ResultInfo(KindSelector.TYP_PCK, Type.noType));
> 5273:         if (underlyingType.hasTag(PACKAGE)) {
> 5274:             // Type annotations are not admissible on packages, but we handle packages here to be to

Suggestion:

            // Type annotations are not admissible on packages, but we handle packages here to

src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties line 3233:

> 3231: # 0: fragment, 1: symbol, 2: type
> 3232: compiler.err.type.annotation.inadmissible=\
> 3233:     type annotations are not admissible at this location: {0}\n\

Suggestion:

compiler.err.type.annotation.not.applicable=\
    type annotations are not applicable at this location: {0}\n\


I don't know whether there are concrete guidelines for this or whether "admissible" is what the JLS uses for this.
Looking at other annotation-related error messages in this file, most use "applicable" and "not applicable" (one key uses `inapplicable`, but the message still uses `not applicable`).
There are no other uses of `admissible` in this file.
There is also `annotation.not.valid.for.type` which uses `annotation not valid for an element of type {0}`, which could be another option.

Also, not not sure whether it would be `are not applicable *at* this location`, `are not applicable *on* this location`, or something else.

test/langtools/tools/javac/annotations/typeAnnotations/failures/CantAnnotatePackages.out line 1:

> 1: CantAnnotatePackages.java:14:18: compiler.err.type.annotation.inadmissible: (compiler.misc.type.annotation.1: @TA), java.lang, @TA java.lang.Object

In this output, isn't `java.lang` the `1: symbol` and `@TA java.lang.Object` the `2: type` in the error message?
Won't the `to annotate a qualified type, write {1}.{2}` then output `java.lang. at TA java.lang.Object`, which isn't what we would want.
Am I parsing this incorrectly?

-------------

PR Review: https://git.openjdk.org/jdk/pull/16592#pullrequestreview-1809460770
PR Review Comment: https://git.openjdk.org/jdk/pull/16592#discussion_r1444890016
PR Review Comment: https://git.openjdk.org/jdk/pull/16592#discussion_r1444978083
PR Review Comment: https://git.openjdk.org/jdk/pull/16592#discussion_r1445001491