Treat effectively final variables the same as static final fields when reasoning about "constant" function calls

[wimdeblauwe]

I am trying to add NullAway to my project and I have ran into what I think is a false positive.

I tried to make it as small as possible and this shows the problem:

import java.util.HashMap;
import java.util.Map;

public class Test {
    public static void main(String[] args) {
        String httpStatusValue = "200";
        Map<String, String> map = new HashMap<>();
        if (map.get(someOperation()) != null) {
            staticMethod(map.get(someOperation()));
        } else if (map.get(someOtherOperation(httpStatusValue)) != null) {
            staticMethod(map.get(someOtherOperation(httpStatusValue)));
        }
    }

    private static String someOperation() {
        return "test";
    }

    private static String someOtherOperation(String httpStatusValue) {
        return "test2";
    }

    private static void staticMethod(String t) {
        String trimmed = t.trim();
        System.out.println("trimmed = " + trimmed);
    }
}

Running NullAway on this code gives:

 error: [NullAway] passing @Nullable parameter 'map.get(someOtherOperation(httpStatusValue))' where @NonNull is required

Note that is only reports the problem for the someOtherOperation that has a parameter. The other method that has no parameter is ok.

I am using @NullMarked on the package that this class is in, and run with these settings -Xplugin:ErrorProne -Xep:NullAway:ERROR -XepOpt:NullAway:OnlyNullMarked.

[msridhar]

Hi @wimdeblauwe, thanks for the report. We have some documentation on our handling of maps here. A heuristic we have to implement in handling maps is determining when the argument to Map.get() is unchanged. Apparently, NullAway thinks that someOperation() will always return the same value, but not someOtherOperation(httpStatusValue), even though httpStatusValue in your example is effectively final. Could you test that if you put httpStatusValue in a static final field, the error goes away? I think it will, and probably we should handle effectively final locals and static final fields the same way.

[wimdeblauwe]

If I change the method someOtherOperation to this:

    private static String someOtherOperation(String httpStatusValue) {
        return TEST_2;
    }

With:

public static final String TEST_2 = "test2";

Then the error still remains.

If I do something like this (Which is closer to the actual code that I have):

    private static String someOtherOperation(String httpStatusValue) {
        return httpStatusValue.toUpperCase(Locale.ROOT);
    }

Then the error also still remains.

[msridhar]

Sorry what I meant was:

    static final String httpStatusValue = "200";
    public static void main(String[] args) {
        Map<String, String> map = new HashMap<>();
        if (map.get(someOperation()) != null) {
            staticMethod(map.get(someOperation()));
        } else if (map.get(someOtherOperation(httpStatusValue)) != null) {
            staticMethod(map.get(someOtherOperation(httpStatusValue)));
        }
    }

Also, FWIW, in all these cases NullAway's handling is a heuristic compromise. Really the code is potentially unsafe, since NullAway cannot prove that someOperation doesn't return a different value every time it is called (we deliberately avoid analyzing into a procedure call to remain fast). The safer thing to do is to store results in a local, if you can:

        var opResult = someOperation();
        if (map.get(opResult) != null) {
            staticMethod(map.get(opResult));
        }

This is guaranteed to be safe and also avoids one method call.

In any case, for consistency we should fix the original issue; we will work on it.