Model java.util.Map.remove(Object) return value as @Nullable, similar to Map.get()

[magicghostVu]

Description

NullAway currently warns for unsafe dereference of Map.get(...), since Map.get(...) may return null when no mapping exists.

However, java.util.Map.remove(Object key) has a very similar return contract: it returns the previous value associated with the key, or null if there was no mapping.

Currently, NullAway appears not to warn when dereferencing the result of Map.remove(...), which can lead to a false negative.

Minimal repro

package com.example;

import java.util.Map;

class Test {
  void removeShouldBeNullable(Map<String, Object> map) {
    map.remove("x").toString(); // Expected NullAway warning, but no warning currently
  }

  void getIsAlreadyNullable(Map<String, Object> map) {
    map.get("x").toString(); // NullAway warns
  }
}

[codingkiddo]

Hi, I would like to work on this.

My understanding is that java.util.Map.remove(Object) should be added to NullAway's nullable library return models, similar to Map.get(Object), since it can return null when no mapping exists.

I plan to add the library model entry and a regression test showing that dereferencing map.remove(key) reports a NullAway warning.

Please let me know if this approach sounds okay.

[magicghostVu]

yes, thank you, you guys are very supportive. <3

[codingkiddo]

yes, thank you, you guys are very supportive. <3

Thanks! I’ll take a look and try to put up a small focused PR for this.

My plan is to add Map.remove(Object) to the nullable library return models, similar to Map.get(Object), and include a regression test showing that dereferencing map.remove(key) reports a NullAway warning.

[subhramit]

yes, thank you, you guys are very supportive. <3

Thanks! I’ll take a look and try to put up a small focused PR for this.

My plan is to add Map.remove(Object) to the nullable library return models, similar to Map.get(Object), and include a regression test showing that dereferencing map.remove(key) reports a NullAway warning.

Hey @codingkiddo, are you still working on this?

[codingkiddo]

yes, thank you, you guys are very supportive. <3

Thanks! I’ll take a look and try to put up a small focused PR for this.
My plan is to add Map.remove(Object) to the nullable library return models, similar to Map.get(Object), and include a regression test showing that dereferencing map.remove(key) reports a NullAway warning.

Hey @codingkiddo, are you still working on this?

Yes, I’m still working on this. I have made the change locally and added a regression test for Map.remove(Object) returning nullable.

I’m running the focused tests/format checks now, and I’ll open a small PR shortly once they pass. Thanks for checking!

[codingkiddo]

Opened the PR here: #1623

It adds Map.remove(Object) to the nullable library return models and includes a regression test for dereferencing map.remove(key).