uber / NullAway / #985
93%

DEFAULT BRANCH: master
Ran
Jobs 1
Files 73
Run time 4s
Badge
Embed ▾
README BADGES
x

If you need to use a raster PNG badge, change the '.svg' to '.png' in the link

Markdown

Textile

RDoc

HTML

Rst
#985

push

github-actions

web-flow 
Fix LibraryModels recording of dataflow nullness for Map APs  (#685)

We had a long-standing bug in our recording of the nullness of access paths inside `LibraryModelsHandler`.

Namely, as part of `accessPathsAtIndexes`, we were calling `getAccessPathForNodeNoMapGet`, rather
than `getAccessPathForNodeWithMapGet`. Among other things, `accessPathsAtIndexes` is used to calculate
which arguments for a method like `Preconditions.checkNotNull([expr])` are marked as being non-null after
the method's execution (this extends to all methods in `failIfNullParameters` and similar logic applies to other
types of library models). Due to our use of `getAccessPathForNodeNoMapGet`, code like the following:

```
private void foo(Map<String,Object> m) {
    Preconditions.checkNotNull(m.get(\"foo\"));
    m.get(\"foo\").toString(); // FP!
  }
}
```

Would result in a false positive, where `m.get(\"foo\").toString()` shows up as a null-dereference, despite the
preceding `Preconditions.checkNotNull` call.

This despite the following working without issue:

```
private void foo(@Nullable Object o) {
    Preconditions.checkNotNull(o);
    o.toString(); // Worked before and still works!
  }
}
```

This PR fixes this bug.

The core fix is simply replacing the `getAccessPathForNodeNoMapGet` with `getAccessPathForNodeWithMapGet`.

However, to achieve this, we must thread `VisitorState` all the way to the 
`LibraryModelsHandler.accessPathsAtIndexes(...)` method, which requires changing the signature of the
`onDataflowVisitMethodInvocation(...)` handler extension point. This makes the bulk of the code changes
in this PR.

Finally, we include a few test cases for unboxing, which are an artifact of how we discovered and then triagged
this bug.

(Edit for posterity: thanks @msridhar for finding the root cause here)

5050 of 5446 relevant lines covered (92.73%)

0.93 hits per line

Uncovered Existing Lines

Lines Coverage ∆ File
1
96.15
 0.0% ../nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java
1
98.25
 0.03% ../nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java
1
91.67
 0.0% ../nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java
3
94.74
 -1.58% ../nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java
5
96.03
 0.0% ../nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java
6
91.55
 0.0% ../nullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullHandler.java
7
89.38
 -0.03% ../nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java
7
83.05
 0.0% ../nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java
20
81.58
 0.0% ../nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java
Jobs
ID Job ID Ran Files Coverage
1 #985.1 73
92.73
Source Files on build #985