Add test for ComparableMapEntry implements Comparable, Map.Entry

[vlsi]

I have the following failure (checkerframework 3.7.0) in Calcite

https://github.com/apache/calcite/blob/64a0ca71038da14f0f883f169aa95df00d909e06/core/src/main/java/org/apache/calcite/util/Pair.java#L41-L42

core/src/main/java/org/apache/calcite/util/Pair.java:44: error: [type.argument.type.incompatible] incompatible type argument for type parameter T1 of Pair.
    implements Comparable<Pair<T1, T2>>, Map.Entry<T1, T2>, Serializable {
                               ^
  found   : T1[ extends @UnknownKeyFor Object super @KeyForBottom Void]
  required: [extends @UnknownKeyFor Object super @UnknownKeyFor null]

core/src/main/java/org/apache/calcite/util/Pair.java:44: error: [type.argument.type.incompatible] incompatible type argument for type parameter T2 of Pair.
    implements Comparable<Pair<T1, T2>>, Map.Entry<T1, T2>, Serializable {
                                   ^
  found   : T2[ extends @UnknownKeyFor Object super @KeyForBottom Void]
  required: [extends @UnknownKeyFor Object super @UnknownKeyFor null]

Let me see if this test reproduces the issue.

[mernst]

@vlsi FYI, your job is currently 10th in the queue to be run: https://dev.azure.com/checkerframework/checkerframework/_build?definitionId=5&_a=summary

If you wish to avoid a long wait, you can enable CI for your fork, and then you don't need to make a pull request in order to do testing.

[vlsi]

ok, at least this change reproduces the problem: https://dev.azure.com/checkerframework/checkerframework/_build/results?buildId=2876&view=logs&j=11811b1c-d14b-5529-93a4-5f2e1e50e37a&t=c6036049-c59d-531d-b0de-bbcb93026718&l=1083

[mernst]

@vlsi CI passes (after a added a missing import statement). Is that what you expected? Can you clarify what you expected and how the Checker Framework's behavior differs from that? Thanks.

[vlsi]

I expected the test to fail like it fails for the current Calcite's Pair class (see the PR description)

[mernst]

I expected the test to fail like it fails for the current Calcite's Pair class (see the PR description)

Focusing just on this pull request and the file that it adds, what is the bug in the code and what error message should the Nullness Checker issue to warn a user about the bug? Thanks!

[vlsi]

If this PR succeeds then it means it fails to reproduce the issue I have when running checker with Calcite code.

[mernst]

If this PR succeeds then it means it fails to reproduce the issue I have when running checker with Calcite code.

I agree.

I think that means that you didn't minimize the Calcite code in such a way to reproduce the issue. Perhaps you removed too much code. Could you please try minimizing it again?

(You may already know this, but I'll mention it in case it is helpful: When minimizing an issue, I usually find it best to start with the full file and remove code bit-by-bit while maintaining the error. That is, I don't start with just the one line on which the error was issued.)

[vlsi]

Just in case: I know what test case minimization is, and I know what fuzzing is, I'm on a program committee for the largest testing conference in Russia https://heisenbug-moscow.ru/en/ which begins tomorrow. Thanks for explaining though.

I agree more analysis is needed to figure out what went wrong, however, unfortunately, the time is limited 🤷‍♂️ . As of now, I'm like ~380 nullness errors left in Calcite :core module. I would re-iterate on harder cases as I fix easier ones. Then Calcite developers would have something to look at (and "review" or whatever).

[mernst]

Thanks for the clarification. I meant no offense, but wanted to be sure that we were on the same page. It can be difficult to determine this when communicating by email, especially without other context.

Good luck with the conference tomorrow! I hope it goes well.

My main question is what to do with this issue. It does not seem actionable to me right now. I think you will be much more efficient in minimizing Calcite bugs than I can be, given your greater knowledge of the Calcite codebase. But I understand that you are busy and cannot minimize every bug. (Just as I cannot minimize every bug.) I want to reiterate that we do appreciate your bug reports and suggestions!

Should we close this issue until someone can produce a smaller reproduction? Or do you want to leave it open as a reminder that someone should do that?

[vlsi]

Whatever you like. I think I've configured Azure pipeline in my fork, so I could use it to reproduce the issue. Initially I created the PR to get access to CI.

At the end of the day, I hope nullness annotations are merged to Calcite, and in order to do that I would need to either fix the errors or suppress them, so I don't really think someone else would come up with the same issue sooner than myself.

[vlsi]

Just for reference, I tried 3.7.1 and the error is still the same.

[vlsi]

The error was not there in 3.6.1, and it first appeared in 3.7.0

[smillst]

One possible work around is to explicitly annotation T1 and T2: <@UnknownKeyFor T1 extends @UnknownKeyFor Object>. I'm going to close this pull request. Please open an issue if you are able to find a small example.

[mernst]

I'm sorry you were not able to reproduce the problem. That is frustrating.

By the way, if you just want to test a small example, an alternative to opening a pull request or making a change on your branch is to paste it into http://eisop.uwaterloo.ca/live/ .

[vlsi]

http://eisop.uwaterloo.ca/live/ is nice.

It looks like the error is not reproducible with 3.8.0

However, 3.8.0 seems to have another issue: https://github.com/typetools/checker-framework/issues/4048