NullAway dataflow analysis does not prune out infeasible paths

[agrieve]

In the context of this code review, I found:

// Complains that the contract is not satisfied

    @Contract("_, !null -> !null")
    public @Nullable Set<String> readStringSet(String key, @Nullable Set<String> defaultValue) {
        checkIsKeyInUse(key);
        Set<String> values = ContextUtils.getAppSharedPreferences().getStringSet(key, defaultValue);
        return (values != null) ? Collections.unmodifiableSet(values) : null;
    }

// Also complains:

    @Contract("_, !null -> !null")
    public @Nullable Set<String> readStringSet(String key, @Nullable Set<String> defaultValue) {
        checkIsKeyInUse(key);
        Set<String> values = ContextUtils.getAppSharedPreferences().getStringSet(key, defaultValue);
        if (values != null) {
            return Collections.unmodifiableSet(values);
        }
        return null;
    }

The error is:

../../base/android/java/src/org/chromium/base/shared_preferences/SharedPreferencesManager.java:206: warning: [NullAway] Method readStringSet has @Contract(_, !null -> !null), but this appears to be violated, as a @Nullable value may be returned when parameter defaultValue is non-null.
        return null;
        ^
    (see http://t.uber.com/nullaway )
1 warning

However, if I remove the unmodifiableSet part, it succeeds:

// No warnings:

    @Contract("_, !null -> !null")
    public @Nullable Set<String> readStringSet(String key, @Nullable Set<String> defaultValue) {
        checkIsKeyInUse(key);
        Set<String> values = ContextUtils.getAppSharedPreferences().getStringSet(key, defaultValue);
        return values;
    }

Note that values is determined to be non-null as a result of a nullImpliesNullParameters() library model here. I'm guessing the data flow analysis is not recognizing that if (values != null) is always true when defaultValue != null

[msridhar]

Sorry for the slow response. I think this is in a similar category to #98 and #1112. Here, I think the issue is that the dataflow analysis does not prune out infeasible paths. Like in the second example, if defaultValue is non-null, then the final return null statement becomes unreachable. Here is a simpler example:

    // this method can never return null
    public Object foo() {
        Object x = new Object();
        if (x != null) {
            return x;
        }
        return null; // NullAway reports a false positive here
    }

The Checker Framework Nullness Checker also reports a false positive for code like the above. We will try to look at this eventually (it might not be too hard to fix), but for now issues like #98 will be higher priority.

[agrieve]

Another example:

    @Contract("!null -> !null")
    public static @Nullable String bar(@Nullable String value) {
        if (value == null) return null; // warns about this
        return value;
    }