[CALCITE-4199] Add nullability annotations to the methods and fields, ensure consistency with checkerframework

[vlsi]

https://issues.apache.org/jira/browse/CALCITE-4199

CheckerFramework needs an annotated JDK (which might be slow to download), so it is not activated by default

Third-party issues:

• Order classes by recent failures and modification date when feeding them to javac compiler gradle/gradle#14332: Order classes by recent failures and modification date when feeding them to javac compiler
• Ability to skip class file parse errors policeman-tools/forbidden-apis#173: Ability to skip class file parse errors
• Infer annotations for receiver of instanceof checks typetools/checker-framework#3616: Infer annotations for receiver of instanceof checks
• False alarm when using "return null" and type is known to support nulls typetools/checker-framework#3614: False alarm when using "return null" and type is known to support nulls
• Crash in AbstractAtmComboVisitor visitArray_Typevar .. defaultAction typetools/checker-framework#3612: Crash in AbstractAtmComboVisitor visitArray_Typevar .. defaultAction

Corner cases:

• Nullness Checker: support Q.isEmpty - Q.poll logic typetools/checker-framework#399 checker framework does not understand if (queue.isEmpty()) { ... = queue.poll(); }, and it treats poll() as always nullable.
• Arrays vs annotations: https://twitter.com/tagir_valeev/status/1301443075080949760, https://twitter.com/tagir_valeev/status/1302173719398436864

[michaelmior]

Looks like this is currently failing but I assume that's expected? (Presumably because of missing annotations.)

[vlsi]

You are right, there's not enough annotations.
The idea was that once all the annotations are there, we could enforce the check for all the commits.

Feel free to pick it up.
I don't have the bandwidth in the nearest future :-/

[michaelmior]

@vlsi Unfortunately I'm in the same situation bandwidth-wise but I'll keep that in mind.

[zinking]

You are right, there's not enough annotations.
The idea was that once all the annotations are there, we could enforce the check for all the commits.

Feel free to pick it up.
I don't have the bandwidth in the nearest future :-/

I remember you do these things using script last time I saw ^_^

ok , after some digging, this isn't scripting task.

[vlsi]

Just in case, I've added nullability annotations to pgjdbc.
Here's what it becomes: pgjdbc/pgjdbc#1814

An interesting note is that I had to add several annotations for java.sql types (e.g. ResultSet, CallableStatement, Preparedstatement, ResultSet, etc)

[michaelmior]

@vlsi I was curious to take a look at this and I started to tackle some more necessary annotations on my own checkerframework branch. So far I see there are no @NonNull annotations in the codebase. Has this simply not been addressed yet or is there a reason for avoiding these?

[vlsi]

So far I see there are no @nonnull annotations in the codebase

The suggested behavior is non-null by default (it is default for checkerframework), so I have not added it yet.

It should be enough to add DefaultQualifier to the topmost package as follows: https://github.com/pgjdbc/pgjdbc/blob/aa190cce2c27ac5f5d4fa33f5d156f8bf04cbe0c/pgjdbc/src/main/java/org/postgresql/package-info.java#L6-L13

[vlsi]

Just in case, linq4j:compileJava says there are only 46 errors left :)

[vlsi]

Ok, now linq4j passes the verification.

[vlsi]

As of now, 480 errors left in :core

[zinking]

@vlsi not sure why this is closed (is the code abonded ?). I think this will be useful and seems most agrees with this (JIRA).
I'd suggest break these things into small iterations and put them in gradually.

[vlsi]

PR moved to #2268