chore: add checkerframework for nullness verification by vlsi · Pull Request #1814 · pgjdbc/pgjdbc

vlsi · 2020-07-03T19:24:46Z

See https://checkerframework.org/

Let's see how it works. The intention is to have non-nullable types by default, and add @Nullable when the returned type is nullable.

String nonNullable;
@Nullable String nullable;

String[] // array and elements: non-nullable
String @Nullable [] // array: nullable, elements: non-nullable
@Nullable String[] // array: non-nullable, elements: nullable
@Nullable String @Nullable [] // array: nullable, elements: nullable

So far I'm not sure if we can use our own nullability annotations.

Dependencies

The added dependency checkerframework:checker-qual:3.5.0. It is used for annotations only, so the dependency can be excluded.
Note: jre6, jre7, and source release are produced with all the annotations commented out.

Issues

Checker Framework relies on the annotated JDK for the verifications, and there might be issues there.
For instance, BatchUpdateException is not annotated with @Nullable

typetools/jdk#62
typetools/jdk#63
typetools/jdk#64

In practice, we can overrule that by placing stub files to config/checkerframework/... folder, so we do not depend on CF releases.

https://youtrack.jetbrains.com/issue/IDEA-245323 -- IDEA produces false positives for nested arrays.

Findings

CheckerFramework works on a method by method basis, so it seems to produce lots of errors when the precondition is verified in another method.

For instance:

class PgResultSet {
  @Nullable Map<String, Object> updateValues = ...;

  @EnsuresNonNull("updateValues")
  private void checkUpdateable() throws SQLException {
    ...
    if (updateValues == null) {
      // allow every column to be updated without a rehash.
      updateValues = new HashMap<String, Object>((int) (fields.length / 0.75), 0.75f);
    }
  }

  protected void updateValue(@Positive int columnIndex, @Nullable Object value) throws SQLException {
    checkUpdateable();
    ...
    updateValues.put("column_name", value); // <-- checkerframework warns that updateValues can be null
  }

As far as I understand, EnsuresNonNull should tell CF that updateValues is non-null if checkUpdateable() completes successfully. However, it does not seem to work, and, on top of that, IDEA does not seem to recognize @EnsuresNonNull either.

The same goes for if (wasNull()) { return null; } ... getString(...) which often appears in getters.
We know getString can't return null, however, CF produces warnings.

Edge cases

pgjdbc/pgjdbc/src/main/java/org/postgresql/core/v3/replication/V3PGReplicationStream.java

Lines 67 to 75 in 9a0d2b1

    
           public ByteBuffer read() throws SQLException { 
        
             checkClose(); 
        
             ByteBuffer payload = null; 
        
             while (payload == null && copyDual.isActive()) { 
        
               payload = readInternal(true); 
        
             } 
        
             return payload;

read() Javadoc says the method never returns null, yet it would return null in case copyDual.isActive() becomes false.

Test code

It looks like verifying test code takes too much time, and the gain does not seem to be significant enough, so the test code is not verified.

vlsi · 2020-07-03T19:30:31Z

Just in case, the very first error is a true bug:

src/main/java/org/postgresql/ssl/SingleCertValidatingFactory.java:101: error: [dereference.of.nullable] dereference of possibly-null reference Thread.currentThread().getContextClassLoader()
            Thread.currentThread().getContextClassLoader().getResourceAsStream(path));
                                                        ^

getContextClassLoader indeed might return null, so the code might fail with NPE.

davecramer · 2020-07-03T19:53:44Z

cool!

codecov-commenter · 2020-07-03T20:09:15Z

Codecov Report

Merging #1814 into master will decrease coverage by 0.14%.
The diff coverage is 63.26%.

@@             Coverage Diff              @@
##             master    #1814      +/-   ##
============================================
- Coverage     68.73%   68.59%   -0.15%     
- Complexity     4223     4231       +8     
============================================
  Files           195      196       +1     
  Lines         17578    17740     +162     
  Branches       2892     2939      +47     
============================================
+ Hits          12083    12168      +85     
- Misses         4192     4219      +27     
- Partials       1303     1353      +50

sehrope · 2020-07-04T13:20:18Z

Very cool. Does this stick around as a runtime dependency or is it only test / build time?

sehrope · 2020-07-04T13:28:26Z

Just in case, the very first error is a true bug:

src/main/java/org/postgresql/ssl/SingleCertValidatingFactory.java:101: error: [dereference.of.nullable] dereference of possibly-null reference Thread.currentThread().getContextClassLoader()
            Thread.currentThread().getContextClassLoader().getResourceAsStream(path));
                                                        ^

getContextClassLoader indeed might return null, so the code might fail with NPE.

Nice catch. I doubt that'd happen in practice as you'd have to go out of your way to end up with a null value.

Still nice to fix it, what should the fallback be? Classloader of the class itself (this.getClass().getClassLoader())?

vlsi · 2020-07-04T13:31:38Z

There are reasons to keep the annotations in the published jars: other languages and tools would see the annotations and infer nullability of the parameters and the returned values. I know Kotlin rejects the code if you pass null to a Java parameter that is annotated as not null.

I am not sure yet which annotation library to choose (there are multiple), however, the switch can probably be made by a global search and replace.

There's a backfire from Java 1.7 though: it does not support type annotations, so we can't have Map<@Nullable String, String> there. I guess either we drop 1.7 support or we add "replace @Nullable with /*@Nullable*/" step there.

vlsi · 2020-07-04T13:33:51Z

@sehrope , I've improved classloader handling in 5937065#diff-b5476fa4ea51ca4548c14b11c8ec4179R100

sehrope · 2020-07-04T13:37:09Z

Good point on keeping them in. I took a peek at the license for this one, it's GPL with classpath exception so should be good right?

+1 for anything that gives us more reasons to drop 1.7 support. We have to drop it at some point, let's pick something and move on. It's been 5+ years that's it's EOL.

sehrope · 2020-07-04T13:44:16Z

@sehrope , I've improved classloader handling in 5937065#diff-b5476fa4ea51ca4548c14b11c8ec4179R100

LGTM. That looks like it could go in on it's own as it's own PR without the rest of the new annotations. I was going to suggest moving that logic into it's own method but the only other code reading a resource stream is in a test so not worth it.

vlsi · 2020-07-06T08:50:46Z

@sehrope , @bokken , @davecramer , do you have something to say regarding the changes like a688233 and d435f3b ?

The change is like

  public void flush() throws IOException {
-   checkClosed();
+   LargeObject lo = checkClosed();
    try {
      if (bpos > 0) {
        lo.write(buf, 0, bpos);

It looks like LargeObject lo = checkClosed() can provide IDE and checker the information that lo can't be null in this context which sounds nice.

On the other hand, then lo = checkClosed() makes it a little bit harder to "find lo field usages".

Frankly speaking, I'm inclined to LargeObject lo = checkClosed() style as it makes it easier to see if lo is usable at all.

davecramer · 2020-07-07T13:02:20Z

@sehrope , @bokken , @davecramer , do you have something to say regarding the changes like a688233 and d435f3b ?

The change is like
  public void flush() throws IOException {
-   checkClosed();
+   LargeObject lo = checkClosed();
    try {
      if (bpos > 0) {
        lo.write(buf, 0, bpos);
It looks like LargeObject lo = checkClosed() can provide IDE and checker the information that lo can't be null in this context which sounds nice.

On the other hand, then lo = checkClosed() makes it a little bit harder to "find lo field usages".

Frankly speaking, I'm inclined to LargeObject lo = checkClosed() style as it makes it easier to see if lo is usable at all.

Looking at this I see no reason not to do it. Did you have specific concerns ?

sehrope · 2020-07-08T11:06:28Z

... do you have something to say regarding the changes like a688233

+1. Much cleaner.

... and d435f3b ?

I don't like this one as much, in particular the shading of the member variables.

Can instead make the lo member variable not-null (assigned only at constructor) and change checkClosed() to check the lo's own closed property? I don't think it's accessible but that's an easy fix to expose it. They're in the same package too so no need to expose it further than package level.

vlsi · 2020-07-08T11:18:30Z

Can instead make the lo member variable not-null

I thought behind the same lines as well, however, there are cases when nullifying might help to release resources.

bokken · 2020-07-08T22:15:17Z

It is definitely more of a style thing, but I kind of like the changes in d435f3b
I might suggest making the local LargeObject lo as final.

config/checkerframework/CallableStatement.astub

config/checkerframework/ResultSet.astub

vlsi · 2020-07-09T20:39:00Z

Ok, less than 100 errors left :)

96 errors
2 warnings

vlsi · 2020-07-10T08:18:04Z

Open question: is it legal to return null from org.postgresql.jdbc.PgArray#getArray() (it implements java.sql.Array#getArray() ) ?

Javadoc specifies:

   * @return an array in the Java programming language that contains
   * the ordered elements of the SQL <code>ARRAY</code> value
   * designated by this <code>Array</code> object

The current code has return null branch:

pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgArray.java

Lines 164 to 166 in 9a0d2b1

    
           if (fieldString == null) { 
        
             return null; 
        
           }

When receiving the array from DB, I expect that ResultSet.getArray() should return nullable Array rather than Array with a nullable contents.
So when receiving data from the DB it would be better to have non-nullable contents.

When sending data to the DB, the current API allows to pass null to Connection#createArrayOf(String typeName, Object[] elements) elements :-(
The current implementation treats conn.createArrayOf("float8", null) as if it were cast(null as float8[]).

Unfortunately, we have a single class PgArray only for doing both reads and writes, so we can't have nullable getArray and non-nullable getArray at the same time :(

Any thoughts?

vlsi · 2020-07-10T09:54:12Z

One more item: CopyManage#copyIn(String sql), CopyManage#copyOut(String sql), CopyManage#copyDual(String sql)

pgjdbc/pgjdbc/src/main/java/org/postgresql/copy/CopyManager.java

Lines 44 to 47 in 9a0d2b1

    
           public CopyIn copyIn(String sql) throws SQLException { 
        
             CopyOperation op = queryExecutor.startCopy(sql, connection.getAutoCommit()); 
        
             if (op == null || op instanceof CopyIn) { 
        
               return (CopyIn) op;

I wonder if the following should be converted to an error rather than a silent log:

pgjdbc/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java

Lines 1132 to 1135 in 9a0d2b1

    
           if ( processingCopyResults.compareAndSet(false,true) == false ) { 
        
             LOGGER.log(Level.INFO, "Ignoring request to process copy results, already processing"); 
        
             return null; 
        
           }

vlsi · 2020-07-11T12:14:02Z

AFAIK the PR is more-or-less ready. It builds (including jre6, jre7, and dependency-reduced-pom).

vlsi · 2020-07-11T20:16:43Z

I think the PR is ready for merge, and I'm inclined to merge it in 2-3 days.

See https://checkerframework.org

vlsi force-pushed the checkerframework branch from 9e92d03 to 6b59cb4 Compare July 3, 2020 19:27

vlsi force-pushed the checkerframework branch from 19fb133 to 43f46d4 Compare July 4, 2020 20:46

vlsi mentioned this pull request Jul 5, 2020

fix: avoid preparedStatement leak when using updateable ResultSet via insert/update/refreshRow #1815

Merged

vlsi force-pushed the checkerframework branch from 43f46d4 to a56b5e1 Compare July 5, 2020 09:32

bokken reviewed Jul 9, 2020

View reviewed changes

config/checkerframework/CallableStatement.astub Outdated Show resolved Hide resolved

bokken reviewed Jul 9, 2020

View reviewed changes

config/checkerframework/CallableStatement.astub Outdated Show resolved Hide resolved

bokken reviewed Jul 9, 2020

View reviewed changes

config/checkerframework/CallableStatement.astub Outdated Show resolved Hide resolved

bokken reviewed Jul 9, 2020

View reviewed changes

config/checkerframework/ResultSet.astub Outdated Show resolved Hide resolved

vlsi mentioned this pull request Jul 10, 2020

Allow different compilation tasks to have their own checkerframework configuration options kelloggm/checkerframework-gradle-plugin#110

Closed

vlsi force-pushed the checkerframework branch 4 times, most recently from 8b30082 to 683ab4c Compare July 11, 2020 07:23

vlsi force-pushed the checkerframework branch 3 times, most recently from f9abdd7 to 7be56cf Compare July 11, 2020 12:10

vlsi force-pushed the checkerframework branch 8 times, most recently from 28cf119 to 1231a79 Compare July 11, 2020 19:39

vlsi mentioned this pull request Jul 13, 2020

[CALCITE-4199] Add nullability annotations to the methods and fields, ensure consistency with checkerframework apache/calcite#1929

Closed

vlsi force-pushed the checkerframework branch 3 times, most recently from 5260475 to a514045 Compare July 18, 2020 13:36

vlsi added 3 commits July 18, 2020 19:35

chore: add checkerframework for nullness verification

79d080c

See https://checkerframework.org

doc: add null safety documentation to CONTRIBUTING.md

3852576

refactor: add nullness annotations

1ce9dd5

vlsi force-pushed the checkerframework branch from a514045 to 1ce9dd5 Compare July 18, 2020 16:35

vlsi merged commit 529e5dc into pgjdbc:master Jul 18, 2020

dcitron mentioned this pull request Aug 27, 2020

PgResultSet.getObject() throws java.lang.AssertionError: Misuse of castNonNull for null column values #1870

Closed

vlsi mentioned this pull request Jun 3, 2021

Split modules testng-team/testng#2564

Merged

15 tasks

	public ByteBuffer read() throws SQLException {
	checkClose();

	ByteBuffer payload = null;
	while (payload == null && copyDual.isActive()) {
	payload = readInternal(true);
	}

	return payload;

Conversation

vlsi commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependencies

Issues

Findings

Edge cases

Test code

Uh oh!

vlsi commented Jul 3, 2020

Uh oh!

davecramer commented Jul 3, 2020

Uh oh!

codecov-commenter commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sehrope commented Jul 4, 2020

Uh oh!

sehrope commented Jul 4, 2020

Uh oh!

vlsi commented Jul 4, 2020

Uh oh!

vlsi commented Jul 4, 2020

Uh oh!

sehrope commented Jul 4, 2020

Uh oh!

sehrope commented Jul 4, 2020

Uh oh!

vlsi commented Jul 6, 2020

Uh oh!

davecramer commented Jul 7, 2020

Uh oh!

sehrope commented Jul 8, 2020

Uh oh!

vlsi commented Jul 8, 2020

Uh oh!

bokken commented Jul 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vlsi commented Jul 9, 2020

Uh oh!

vlsi commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vlsi commented Jul 10, 2020

Uh oh!

vlsi commented Jul 11, 2020

Uh oh!

vlsi commented Jul 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vlsi commented Jul 3, 2020 •

edited

Loading

codecov-commenter commented Jul 3, 2020 •

edited

Loading

vlsi commented Jul 10, 2020 •

edited

Loading