uber / NullAway

A tool to help eliminate NullPointerExceptions (NPEs) in your Java code with low build-time overhead
MIT License
3.62k stars 290 forks source link

JSpecify: Handle @Nonnull elements in @Nullable content arrays #963

Closed armughan11 closed 2 months ago

armughan11 commented 3 months ago

Handles unexpected error cases in JSpecify mode where an index is asserted @Nonnull (fizz[x]!=null) but the array elements itself could be null (@Nullable String [] fizz).

Example

@Nullable String [] fizz ={"1"}
if (fizz[i] != null) {
    fizz[i].toString();
}

Current Behavior This throws an error due to dereference of fizz[i]

New Behavior There should be no error here unless it's outside the block. So only the latter dereference throws up an error in the below case.

@Nullable String [] fizz ={"1"}

if (fizz[i] != null) {
    fizz[i].toString();
}
fizz[i].toString();
armughan11 commented 3 months ago

@msridhar

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 86.25000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 85.90%. Comparing base (a4ce249) to head (5df5697).

Files Patch % Lines
.../com/uber/nullaway/dataflow/ArrayIndexElement.java 56.25% 4 Missing and 3 partials :warning:
...er/nullaway/dataflow/FieldOrMethodCallElement.java 78.94% 3 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #963 +/- ## ============================================ + Coverage 85.88% 85.90% +0.02% - Complexity 2051 2067 +16 ============================================ Files 82 83 +1 Lines 6806 6859 +53 Branches 1312 1323 +11 ============================================ + Hits 5845 5892 +47 - Misses 547 550 +3 - Partials 414 417 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

msridhar commented 3 months ago

We should add tests for the following cases:

627721565 commented 3 months ago

您好,邮件已经收到,查阅后会尽快给您回复!

msridhar commented 3 months ago

I realized this change might have a deeper impact than we were expecting. Right now, the creation of access path elements for array accesses is not gated on the flag for JSpecify mode, and passing the NullAway config into all relevant methods to do that would be kind of painful. But, I believe this means that code like this will pass NullAway now, when it didn't before this change:

if (arr[i].f != null) {
  arr[i].f.toString();
}

@armughan11 can you write a test to confirm this?

@lazaroclapp do you see any high-level issues with not reporting warnings for code like the above? It seems to be keeping in the spirit of NullAway, but it does potentially introduce new vectors of unsoundness, e.g.:

if (arr[i].f != null) {
  arr[j] = null; // overwrites a[i] if i == j
  arr[i].f.toString(); // still no warning from NullAway
}

Most programs don't code too much directly with arrays, but I wonder if we should do some more performance and sanity testing before landing this.

armughan11 commented 3 months ago

Just added the test and you're correct @msridhar. The test you mentioned works fine right now, but throws an error without our changes.

msridhar commented 2 months ago

Gonna go ahead and merge this; I think we can address any concerns in follow-up PRs