Model Map.getOrDefault

[msridhar]

We model the method so that the nullability of the return matches the nullability of the second argument. We also add an explicit model of ImmutableMap.getOrDefault, to override the annotations on that method for the case where AcknowledgeRestrictiveAnnotations is set.

Fixes #723

[coveralls]

Pull Request Test Coverage Report for Build #1056

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 4 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.01%) to 93.044%

---

Files with Coverage Reduction	New Missed Lines	%
../nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java	1	95.24%
../nullaway/src/main/java/com/uber/nullaway/LibraryModels.java	3	88.0%

Totals
Change from base Build #1055:	-0.01%
Covered Lines:	5297
Relevant Lines:	5693

---

💛 - Coveralls

[msridhar]

Ideally this would be inherited always and we wouldn't need to list it explicitly here. Unfortunately, for the combination of an inherited model and AcknowledgeRestrictiveAnnotations this does not seem to happen. Rather than trying to fix that subtle corner case, I decided to just list the method explicitly.

[lazaroclapp]

I think, in retrospect, that it makes some amount of sense that the resolution order is (for B extends A):

• Explicit library models for overriding method B.foo(Object o)
• Restrictive annotations in the jar for overriding method B.foo(Object o)
• Explicit library models for overridden method A.foo(Object o)
• Restrictive annotations in the jar for overridden method A.foo(Object o)

We just want to make sure that library models for a given method override near everything else for that exact method.

That said, this case is still weird, because I doubt ImmutableMap has @Contract(...) and we are already assuming all arguments and the return are @Nullable in the more general case, no? I guess my model is a bit flawed here in that adding a library model of NULL_IMPLIES_NULL_PARAMETERS is not actually the same as annotating with @Contract(...). For example, I would have expected us to also need to assert that Map::get can return @Nullable explicitly in a different library model, but adding the method here actually implies that, whereas adding an @Contract wouldn't have.

Either way, happy with this explicit fix for now.

[msridhar]

That said, this case is still weird, because I doubt ImmutableMap has @Contract(...) and we are already assuming all arguments and the return are @Nullable in the more general case, no? I guess my model is a bit flawed here in that adding a library model of NULL_IMPLIES_NULL_PARAMETERS is not actually the same as annotating with @Contract(...). For example, I would have expected us to also need to assert that Map::get can return @Nullable explicitly in a different library model, but adding the method here actually implies that, whereas adding an @Contract wouldn't have.

I think the issue here was that the documentation for com.uber.nullaway.LibraryModels#nullImpliesNullParameters was unclear. I tried to clarify here: https://github.com/uber/NullAway/pull/724/files#diff-a42b334be0392157034a866655e5e0596680448baad4ed205d09858d061b3a95R87 Does that help?

[lazaroclapp]

Appreciate the updated documentation! But, in the spirit of full transparency, I hadn't RTFM, I was just going with what I remember nullImpliesNullParameters meaning, that my mistaken recollections matched our mistaken docs is purely coincidental 😅

[msridhar]

Note that this change could lead to new warnings being reported; before, we treated Map.getOrDefault as unannotated, which meant it had a @NonNull return. Now, its return will be @Nullable in cases when the second parameter is `@Nullable, leading to newly-reported warnings.

[lazaroclapp]

Very minor nits about naming in the test cases, and a long but ultimately no-op digression. Generally makes sense to me!

[lazaroclapp]

I think, in retrospect, that it makes some amount of sense that the resolution order is (for B extends A):

• Explicit library models for overriding method B.foo(Object o)
• Restrictive annotations in the jar for overriding method B.foo(Object o)
• Explicit library models for overridden method A.foo(Object o)
• Restrictive annotations in the jar for overridden method A.foo(Object o)

We just want to make sure that library models for a given method override near everything else for that exact method.

That said, this case is still weird, because I doubt ImmutableMap has @Contract(...) and we are already assuming all arguments and the return are @Nullable in the more general case, no? I guess my model is a bit flawed here in that adding a library model of NULL_IMPLIES_NULL_PARAMETERS is not actually the same as annotating with @Contract(...). For example, I would have expected us to also need to assert that Map::get can return @Nullable explicitly in a different library model, but adding the method here actually implies that, whereas adding an @Contract wouldn't have.

Either way, happy with this explicit fix for now.

[lazaroclapp]

Minor nit, but could we call the first string nonNullString or similar? If not, let's call it s. I know it's perfectly legal, but lower-case "string" just seems like a typo for a fraction of a second every time I read over this line:

m.getOrDefault(\"key\", string).toString();

[msridhar]

Fixed in c27f3e3

[XN137]

thanks for picking this up!

[XN137]

nit: when i originally copied the example from somewhere there was 3 space indentation on the 1st level, but maybe we want to use 2 instead everywhere

[msridhar]

Thanks! Fixed in 9793733

[lazaroclapp]

LGTM!

Feel free to land. As discussed, we are having this miss the train for NullAway 0.10.9 (which should be up soon and was cut already) on purpose, given the potential for many new reports on codebases using Map::getOrDefault, but we should cut a new release with this change (and a few others) soon.

[lazaroclapp]

Appreciate the updated documentation! But, in the spirit of full transparency, I hadn't RTFM, I was just going with what I remember nullImpliesNullParameters meaning, that my mistaken recollections matched our mistaken docs is purely coincidental 😅