RFR: 8291643: Consider omitting type annotations from type error diagnostics [v2]

[Liam Miller-Cushon]

On Wed, 6 Mar 2024 20:35:20 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`.
>
> Liam Miller-Cushon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Use Type stripping logic added for JDK-8042981
>  - Merge branch 'master' into JDK-8291643
>  - Updates based on review thread
>    
>    * Support optionally printing types with type annotations
>    * Move handling of types in AbstractDiagnosticFormatter
>    * Make annotation removal more robust, don't rely on stripMetadata
>  - 8291643: Consider omitting type annotations from type error diagnostics

The example my colleage @cpovirk shared in https://github.com/openjdk/jdk/pull/16578#issuecomment-2088992273 was

> As an interested party, one thing that I'd highlight is that the tests and examples shown here are by nature simpler than what we see in real-world errors, such as this one:
>
> ```
> com/google/common/collect/MutableClassToInstanceMap.java:169: error: incompatible types: Map<Class<? extends @org.checkerframework.checker.nullness.qual.NonNull B>,B> cannot be converted to Map<? extends Class<B>,? extends B>
>   where @org.checkerframework.checker.nullness.qual.NonNull B is a type-variable:
>     @org.checkerframework.checker.nullness.qual.NonNull B extends @org.checkerframework.checker.nullness.qual.Nullable Object declared in class MutableClassToInstanceMap
> ```
> That could be reduced to merely:
>
> ```
> com/google/common/collect/MutableClassToInstanceMap.java:169: error: incompatible types: Map<Class<? extends B>,B> cannot be converted to Map<? extends Class<B>,? extends B>
>   where B is a type-variable:
>     B extends Object declared in class MutableClassToInstanceMap
> ```
> Note also that the first line of the current message, by including `@org.checkerframework.checker.nullness.qual.NonNull` in the one type but not the other, may suggest to some readers that the nullness annotations are part of the reason for the error, when in fact the error has nothing to do with nullness.

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

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