When reporting error on an initializer, also give line numbers for uninitialized fields

[chengniansun]

In java.util.AbstractMap, the field "values" should be annotated with @MonotonicNonNull. However, there is no way to annotate it. The best way is to use a library model, I guess.

https://github.com/openjdk-mirror/jdk7u-jdk/blob/master/src/share/classes/java/util/AbstractMap.java#L300

[msridhar]

What kind of error are you seeing? Don't know why you would see an error if you didn't mark `java.util` as annotated

…

On Wed, Dec 27, 2017, 4:15 PM chengniansun ***@***.***> wrote: In java.util.AbstractMap, the field "values" should be annotated with @monotonicnonnull. However, there is no way to annotate it. The best way is to use a library model, I guess. https://github.com/openjdk-mirror/jdk7u-jdk/blob/master/src/share/classes/java/util/AbstractMap.java#L300 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#95>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALyUUdnd-Y251YBb0aKqGOpR-o6dzv4ks5tEt22gaJpZM4RN5xn> .

[chengniansun]

https://github.com/google/guava/blob/master/guava/src/com/google/common/collect/MapMakerInternalMap.java#L160

com/google/common/collect/MapMakerInternalMap.java:160: warning: [NullAway] initializer method does not guarantee @NonNul l fields keySet, values, entrySet are initialized along all control-flow paths (remember to check for exceptions or early returns).

I suppose that the three fields are inherited from AbstractMap.

[msridhar]

Interesting! I'd say this a bug. By default we should assume that inherited fields from unannotated classes are initialized correctly. Will look into a fix. Thanks for the report!

…

On Wed, Dec 27, 2017, 4:30 PM chengniansun ***@***.***> wrote: https://github.com/google/guava/blob/master/guava/src/com/google/common/collect/MapMakerInternalMap.java#L160 com/google/common/collect/MapMakerInternalMap.java:160: warning: [NullAway] initializer method does not guarantee @nonnul l fields keySet, values, entrySet are initialized along all control-flow paths (remember to check for exceptions or early returns). I suppose that the three fields are inherited from AbstractMap. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#95 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALyUXm7DoJxIQE6YvhY3-lucz0mZMb9ks5tEuEwgaJpZM4RN5xn> .

[msridhar]

Actually looks like those fields are declared within MapMakerInternalMap:

keySet

values

entrySet

I still think our handling of fields inherited from abstract classes is buggy 😄 But that doesn't seem to be the problem here.

[msridhar]

I take back the comment about abstract classes. I think they should be treated just like regular classes: if you declare a non-null field in an abstract class, you should also declare a constructor / initializer that sets it. Relying on sub-classes to do this work seems fragile.

@chengniansun I'm going to close this one, pls re-open if I got something wrong

[chengniansun]

Oh, I am sorry for the false alarm. I found these three fields in this class, but thought they belonged to the internal class WeakKeyDummyValueSegment (I connected to my machine remotely and did not use an IDE).

I think the curry way that NullAway handles inheritance is correct, so nothing needs to be done with this bug report.

But, as an enhancement, it would help if NullAway can print the line numbers of the fields that are not initialized. Then I can easily identify these three fields.

[msridhar]

But, as an enhancement, it would help if NullAway can print the line numbers of the fields that are not initialized. Then I can easily identify these three fields.

Yeah, it's a bit tricky; should we report the error on the constructor / initializer missing the initialization, or on the fields? I agree that giving line numbers of the fields in the former case could be useful. Hopefully in most cases the IDE will solve the problem.

[chengniansun]

Currently, the locations of the initializers are reported. It will be helpful if there are locations after each field in the following example.

com/google/common/collect/MapMakerInternalMap.java:160: warning: [NullAway] initializer method does not guarantee @nonnul l fields keySet (LOCATION HERE), values (LOCATION HERE), entrySet (LOCATION HERE) are initialized along all control-flow paths (remember to check for exceptions or early returns).

This is definitely not an urgent or import feature, but worth consideration. :)

[msridhar]

I've re-titled the issue and marked it as low priority, though we will try to get to it as I don't think it should be a hard fix. I've also marked it as a good first bug to fix for someone looking to learn the NullAway code and contribute.

[chengniansun]

Thank you, Manu.

[swayamraina]

Hi! I am interested in this enhancement.
Can you please guide me how to build the code and how should I test my changes.

[lazaroclapp]

@swayamraina Sure!

For building, .gradlew build on the repository root should build everything.

For testing, feel free to add a few unit tests in nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java (see e.g. tryWithResourcesSupport for a simple example). Note that you can use these special comments // BUG: Diagnostic contains: to check what is on the error message reported by error prone.

I suspect most changes to fix this issue will be in NullAway.java and fairly close to where the original initialization error messages are created.

[swayamraina]

I am building it against a non-android java project.
It shows the following message with build failure

SDK location not found. Define location with sdk.dir in the local.properties file or with an ANDROID_HOME environment variable.

[rahilvora]

If someone is not looking into this can I contribute to this? Can someone please guide me with the requirement.

[msridhar]

@swayamraina sorry for the slow response. I'm not sure if NullAway can be built without the Android SDK installed, as we have a module that is a sample app. Can you open a separate issue on this and we can investigate?

@rahilvora what is unclear about the requirement? Hopefully this comment makes it clear what we are looking for.