Idea: use ANSI colors to highlight annotation differences

[vlsi]

I guess color-coding the error messages would help a lot to understand the failures.

I've implemented a Gradle plugin that color-codes test events from Gradle, and I found "bold", "bold+red foreground", "bold+blue foreground", "black bright foreground" produce good contrast in GitHub Actions (see sample), Travis (see sample), and in my terminal.

Unfortunately, it might be challenging to configure Windows terminals to support colors, so I guess it might need to be disabled by default on Windows. I tried different options to get it working in GitHub Actions+Windows, and it was challenging.

Sample message:

EnumerableValues.java:74: error: [argument.type.incompatible] inco
mpatible argument for parameter rowType of <init>.
    return new EnumerableValues(getCluster(), rowType, tuples, traitSet);
                                              ^
  found   : @Initialized @Nullable RelDataType
  required: @Initialized @NonNull RelDataType

I guess it would help if @Nullable and @NonNull were highlighted.

JdbcSchema.java:493: error: [return.type.incompatible] incompatible type
s in return.
    return getTypes().keySet();
                            ^
  type of expression: Set<@KeyFor("this.getTypes()") String>
  method return type: Set<String>

It would help if @KeyFor("this.getTypes()") was highlighted.

ViewTableMacro.java:99: error: [argument.type.incompatible] incompatible
argument for parameter names of path.
        parsed.table, Schemas.path(schema.root(), parsed.tablePath),
                                                        ^
  found   : @Initialized @Nullable ImmutableList<@Initialized @NonNull String>
  required: @Initialized @NonNull Iterable<@Initialized @NonNull String>

and so on.

WDYT?

[mernst]

Thanks for the suggestion.

To clarify: this is a suggestion about the Checker Framework's regular output, not about the output of the Checker Framework's test suite. And this suggestion would affect the Checker Framework's output only when a user uses Gradle (such as https://github.com/kelloggm/checkerframework-gradle-plugin) to run the Checker Framework, but not when the Checker Framework is run using Maven, Ant, javac, etc. Do I understand your proposal?

I agree that a message like this:

  found   : @Initialized @Nullable RelDataType
  required: @Initialized @NonNull RelDataType

is unnecessarily wordy. #2276 is a feature request to change that message to

  found   : @Nullable RelDataType
  required: @NonNull RelDataType

The logic needed to implement #2276 might also be useful for implementing your suggestion.
Another way to implement your proposal would be using textual processing rather than the semantics of the types. (I'm not sure what implementation strategy, if any, you had in mind.)

To give benefits to users who are not using Gradle, would it be helpful to add a row of carets under the currently-printed messages, like the following? That could share code with your proposal.

EnumerableValues.java:74: error: [argument.type.incompatible] incompatible argument for parameter rowType of <init>.
    return new EnumerableValues(getCluster(), rowType, tuples, traitSet);
                                              ^
  found   : @Initialized @Nullable RelDataType
  required: @Initialized @NonNull  RelDataType
                         ^^^^^^^^^

JdbcSchema.java:493: error: [return.type.incompatible] incompatible types in return.
    return getTypes().keySet();
                            ^
  type of expression: Set<@KeyFor("this.getTypes()") String>
  method return type: Set<                           String>
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^

ViewTableMacro.java:99: error: [argument.type.incompatible] incompatible
argument for parameter names of path.
        parsed.table, Schemas.path(schema.root(), parsed.tablePath),
                                                        ^
  found   : @Initialized @Nullable ImmutableList<@Initialized @NonNull String>
  required: @Initialized @NonNull  Iterable     <@Initialized @NonNull String>
                         ^^^^^^^^^

[vlsi]

this is a suggestion about the Checker Framework's regular output

I thought that you use the same output formatting for both Maven, Gradle, and other plugins, so I guess you could improve formatter once and improve messages for all the users.

I use Checker Framework via Gradle only, however, that does not imply I want something Gradle-specific. Gradle was just an example.

[vlsi]

Just to clarify: coloring works universal across Maven, Gradle, and others, and it is quite easy to implement (see https://en.wikipedia.org/wiki/ANSI_escape_code ). The most complicated part for me was to select the colors that work well by default :)

[vlsi]

JdbcSchema.java:493: error: [return.type.incompatible] incompatible types in return.
    return getTypes().keySet();
                            ^
  type of expression: Set<@KeyFor("this.getTypes()") String>
  method return type: Set<                           String>
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^

That would indeed help (e.g. for those who have a hard time distinguishing colors at all). However, adding colors helps as well.

Note: sometimes the messages are extremely long, so "ading spaces" could make it even worse :(

PartiallyOrderedSet.java:127: error: [argument.type.incompatible] incompatible a
rgument for parameter parentFunction of <init>.
    this(ordering, new HashMap<>(collection.size() * 3 / 2), null, null);
                                                                   ^
  found   : @Initialized @Nullable null
  required: @Initialized @NonNull Function<E[ extends @Initialized @Nullable Object super @Initialized @NonNull Void], @Initialized @NonNull Iterable<E[ extends @Initiali
zed @Nullable Object super @Initialized @NonNull Void]>>

TopologicalOrderIterator.java:45: error: [return.type.incompatible] incomp
atible types in return.
    return () -> new TopologicalOrderIterator<>(graph);
                 ^
  type of expression: @Initialized @NonNull TopologicalOrderIterator<V extends @Initialized @Nullable Object, E extends @Initialized @NonNull DefaultEdge>.@Initialized @N
onNull TopologicalOrderIterator<V extends @Initialized @Nullable Object, E extends @Initialized @NonNull DefaultEdge>
  method return type: @Initialized @NonNull Iterator<V extends @Initialized @Nullable Object>

DelegatingInvocationHandler.java:57: error: [override.param.invalid] Incompatibl
e parameter type for args.
      Object[] args) throws Throwable {
               ^
  Method
    @Initialized @NonNull Object invoke(@Initialized @NonNull DelegatingInvocationHandler this, @Initialized @NonNull Object p0, @Initialized @NonNull Method p1, @Initial
ized @NonNull Object @Initialized @NonNull [] p2) throws @Initialized @NonNull Throwable in org.apache.calcite.util.DelegatingInvocationHandler
  cannot override
    @Initialized @Nullable Object invoke(@Initialized @NonNull InvocationHandler this, @Initialized @NonNull Object p0, @Initialized @NonNull Method p1, @Initialized @Nul
lable Object @Initialized @NonNull [] p2) throws @Initialized @NonNull Throwable in java.lang.reflect.InvocationHandler
  found   : @Initialized @NonNull Object @Initialized @NonNull []
  required: @Initialized @Nullable Object @Initialized @NonNull []

[mernst]

Thanks for the clarification. I now understand that the coloring proposal is relevant to all output, not just Gradle.

I agree that coloring is useful in addition to the row of carets. Both could be implemented using similar logic, I think.

I think we would want to enable the coloring with a command-line argument (or query the output to see what kind of terminal it is or whether it's a file) rather than always outputting the color escapes.

[vlsi]

I think we would want to enable the coloring with a command-line argument

That is true. There might be a default behavior (e.g. enable color if not on Windows, or even disabled by default), and there needs to be a command-line option to disable it.

What I think is users might fail to realize there's a color-enabling option, so nobody would notice there's such a feature, so I'm inclined to enable such features by default.

I know Gradle does not provide a way to detect if the terminal supports ANSI codes yet (see gradle/gradle#11133 )

[vlsi]

By the way, git does have --color-words option as well. What it does it prints both new and old text in the same line, and it uses special markers (e.g. [- ... -] vs [+ ... +]) to clarify what was removed and what was aded.

Then

  Method
    @Initialized @NonNull Object invoke(@Initialized @NonNull DelegatingInvocationHandler this, @Initialized @NonNull Object p0, @Initialized @NonNull Method p1, @Initial
ized @NonNull Object @Initialized @NonNull [] p2) throws @Initialized @NonNull Throwable in org.apache.calcite.util.DelegatingInvocationHandler
  cannot override
    @Initialized @Nullable Object invoke(@Initialized @NonNull InvocationHandler this, @Initialized @NonNull Object p0, @Initialized @NonNull Method p1, @Initialized @Nul
lable Object @Initialized @NonNull [] p2) throws @Initialized @NonNull Throwable in java.lang.reflect.InvocationHandler
  found   : @Initialized @NonNull Object @Initialized @NonNull []
  required: @Initialized @Nullable Object @Initialized @NonNull []

could become as follows. The signatures almost match, so printing both in the same line with extra markup for the diffs might work well.

    @Initialized [-@Nullable-][+@NonNull+] Object invoke(@Initialized @NonNull [-InvocationHandler-][+DelegatingInvocationHandler+] this
, @Initialized @NonNull Object p0, @Initialized @NonNull Method p1
, @Initialized [-@Nullable-][+@NonNull+] Object @Initialized @NonNull [] p2
) throws @Initialized @NonNull Throwable

  found   : @Initialized @NonNull Object @Initialized @NonNull []
  required: @Initialized @Nullable Object @Initialized @NonNull []

[mernst]

Cool. Thanks for the additional details. I agree this would be a useful feature.

[vlsi]

One more sample:

interface...
  @Nullable T aggregate(Queryable<T> source,
      FunctionExpression<Function2<@Nullable T, T, T>> selector);

implementation
  @Override public T aggregate(
      Queryable<T> source,
      FunctionExpression<Function2<T, T, T>> selector) {
    throw new UnsupportedOperationException();
  }

error:

core/src/main/java/org/apache/calcite/prepare/QueryableRelBuilder.java:135:
error: [override.param.invalid] Incompatible parameter type for selector.
      FunctionExpression<Function2<T, T, T>> selector) {
                                             ^
  Method
    T extends @Initialized @Nullable Object aggregate(@Initialized @NonNull QueryableRelBuilder<
T extends @Initialized @Nullable Object> this, @Initialized @NonNull Queryable<
T extends @Initialized @Nullable Object> p0, @Initialized @NonNull FunctionExpression<@Initialized @NonNull Function2<
T extends @Initialized @Nullable Object, T extends @Initialized @Nullable Object, T extends @Initialized @Nullable Object>> p1) 
in org.apache.calcite.prepare.QueryableRelBuilder
  cannot override
    T extends @Initialized @Nullable Object aggregate(@Initialized @NonNull QueryableFactory<
T extends @Initialized @Nullable Object> this, @Initialized @NonNull Queryable<
T extends @Initialized @Nullable Object> p0, @Initialized @NonNull FunctionExpression<@Initialized @NonNull Function2<
T extends @Initialized @Nullable Object, T extends @Initialized @Nullable Object, T extends @Initialized @Nullable Object>> p1) 
in org.apache.calcite.linq4j.QueryableFactory
  found   : @Initialized @NonNull FunctionExpression<@Initialized @NonNull Function2<
T[ extends @Initialized @Nullable Object super @Initialized @NonNull Void], 
T[ extends @Initialized @Nullable Object super @Initialized @NonNull Void], 
T[ extends @Initialized @Nullable Object super @Initialized @NonNull Void]>>
  required: @Initialized @NonNull FunctionExpression<@Initialized @NonNull Function2<
T[ extends @Initialized @Nullable Object super @Initialized @Nullable Void], 
T[ extends @Initialized @Nullable Object super @Initialized @NonNull Void], 
T[ extends @Initialized @Nullable Object super @Initialized @NonNull Void]>>

[vlsi]

One more sample:

SqlNullTreatmentOperator.java:46: error: [override.param.invalid] Incompatible parameter type for operands.
      SqlParserPos pos, SqlNode... operands) {
                                   ^
  Method
    @Initialized @NonNull SqlCall createCall(@Initialized @NonNull SqlNullTreatmentOperator this, @Initialized @NonNull SqlLiteral p0, @Initialized @NonNull SqlParserPos p1, @Initialized @NonNull SqlNode @Initialized @NonNull [] p2) in org.apache.calcite.sql.SqlNullTreatmentOperator
  cannot override
    @Initialized @NonNull SqlCall createCall(@Initialized @NonNull SqlOperator this, @Initialized @Nullable SqlLiteral p0, @Initialized @NonNull SqlParserPos p1, @Initialized @Nullable SqlNode @Initialized @NonNull [] p2) in org.apache.calcite.sql.SqlOperator
  found   : @Initialized @NonNull SqlNode @Initialized @NonNull []
  required: @Initialized @Nullable SqlNode @Initialized @NonNull []

It looks like Incompatible parameter type for $parameterName, and Incompatible return type error messages should print found vs required before printing the full signature.

In other words:

SqlNullTreatmentOperator.java:46: error: [override.param.invalid] Incompatible parameter type for operands.
      SqlParserPos pos, SqlNode... operands) {
                                   ^

  found   : @NonNull SqlNode @NonNull []
  required: @Nullable SqlNode @NonNull []
  Did you mean @Nullable SqlNode... operands ?

  Method
    @Initialized @NonNull SqlCall createCall(@Initialized @NonNull SqlNullTreatmentOperator this, @Initialized @NonNull SqlLiteral p0, @Initialized @NonNull SqlParserPos p1, @Initialized @NonNull SqlNode @Initialized @NonNull [] p2) in org.apache.calcite.sql.SqlNullTreatmentOperator
  cannot override
    @Initialized @NonNull SqlCall createCall(@Initialized @NonNull SqlOperator this, @Initialized @Nullable SqlLiteral p0, @Initialized @NonNull SqlParserPos p1, @Initialized @Nullable SqlNode @Initialized @NonNull [] p2) in org.apache.calcite.sql.SqlOperator