RFR: 8291643: Consider omitting type annotations from type error diagnostics

[Liam Miller-Cushon]

On Thu, 9 Nov 2023 00:47:10 GMT, Liam Miller-Cushon <cushon at openjdk.org> wrote:

> Hi,
> 
> Please consider this fix for [JDK-8291643](https://bugs.openjdk.org/browse/JDK-8291643), which causes javac to remove type-use annotations when formatting types in diagnostics.
> 
> For the example like the one in the bug, this change causes javac to emit the diagnostic like `incompatible types: List<String> cannot be converted to List<Number>` rather than `incompatible types: List<String> cannot be converted to List<@Nullable Number>`. Including the type annotations can be confusing, because they do not impact the type checking done by the compiler.
> 
> An alternative I considered was to remove type annotations from types for specific diagnostics, instead of doing it unconditionally in diagnostic formatting. I can revisit that if the current approach seems too broad.
> 
> The test update to `test/langtools/tools/javac/lambda/LambdaConv25.out` is because a `ForAll` is being formatted, and `stripMetadata()` uses a `StructuralTypeMapping` which rewrites away the `ForAll`.

I have been looking at some other issues related to diagnostics and type annotations (e.g. [JDK-8043226](https://bugs.openjdk.org/browse/JDK-8043226)), and I am now wondering if it would make sense to preserve type annotations on times some of the time. I think stripping them is a good default (the majority of diagnostics about types are not related to type annotations), but do you have thoughts on letting diagnostics opt-in to preserving the annotations?

One possibility would be to have a trivial wrapper around the types used in diagnostic arguments, and then have the diagnostic formatter unwrap them and leave the annotations on:

https://github.com/openjdk/jdk/blob/a95062b39a431b4937ab6e9e73de4d2b8ea1ac49/src/jdk.compiler/share/classes/com/sun/tools/javac/util/AbstractDiagnosticFormatter.java#L202-L203

I am now also wondering if `AbstractDiagnosticFormatter` would be a better place to call `stripMetdata()` than where it currently is in `JCDIagnostic`.

What do you think? (And sorry for no thinking of these issues until after opening the PR.)

---

> > The test update to `test/langtools/tools/javac/lambda/LambdaConv25.out` is because a `ForAll` is being formatted, and `stripMetadata()` uses a `StructuralTypeMapping` which rewrites away the `ForAll`.
>
> I guess your patch have uncovered a bug in stripMetadata

I wasn't sure if would be considered a bug or not, but it was unexpected. I'm not sure preserving the `ForAll` makes the diagnostic clearer, but it's also possible to update `stripMetadata()` to preserve it.

The following would preserve the original diagnostics for `LambdaConv25`, do you have a preference?


diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java
index e295096bb6d..b914d5e4a43 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java
@@ -446,7 +446,12 @@ public Type visitTypeVar(TypeVar t, Void aVoid) {
             public Type visitWildcardType(WildcardType wt, Void aVoid) {
                 return super.visitWildcardType((WildcardType)wt.typeNoMetadata(), aVoid);
             }
-        };
+
+            @Override
+            public Type visitForAll(ForAll t, Void aVoid) {
+                return new ForAll(t.tvars, t.qtype.accept(this, aVoid));
+            }
+    };

     public Type preannotatedType() {
         return addMetadata(new Annotations());

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

PR Comment: https://git.openjdk.org/jdk/pull/16578#issuecomment-1804914364