get() / pop() return their default value when the key exists but is None

[odelalleau]

Describe the bug
The following code currently runs without error, showing that get() and pop() fall back to their default value when the key exists and is set to None:

cfg = OmegaConf.create({"x": None})
assert cfg.get("x", 0) == 0
assert cfg.pop("x", 0) == 0
assert cfg == {}  # `pop()` still removed the node even though it returned the default value!

This might actually be a (mostly) intended feature, but it is confusing because this is not how regular Python dictionaries work:

cfg = {"x": None}
assert cfg.get("x", 0) is None
assert cfg.pop("x", 0) is None
assert cfg == {}  # as expected

Expected behavior
I suggest to change it to match the behavior of Python dictionaries.

Since this is a potentially dangerous breaking change, it could be deprecated as follows in 2.1:

• Add a new argument allow_none: bool = False to get() and pop()
• If the key exists and has a None value, then if allow_none is True we treat is as any other valid value. If allow_none is False, we display a deprecation warning and fall back to the current behavior.

This will force users to specify allow_none=True if they ever call get() / pop() on keys with None values. This flag could then be deprecated in 2.2 (changing its default value to True) and removed in 2.3.

Thoughts?

[omry]

I think we should match the behavior of plain dict here.

We already have a handful of breaking changes that are matching the behavior of DictConfig with dict, some are very intrusive.
e.g:
In 2.0 and before accessing a non-existing key returns None,

OmegaConf.create().foo # None

In 2.1 this will result in ConfigAttributeError (and OmegaConf.create()["foo"] will result in ConfigKeyError).
This is a much bigger breaking change and there isn't a soft way to introduce it.

I am leaning toward changing this without a deprecation plan because this feels like a clear bug to me and I doubt many people are depending on this behavior.

[odelalleau]

I am leaning toward changing this without a deprecation plan because this feels like a clear bug to me and I doubt many people are depending on this behavior.

Ok that works too -- I'll prepare a PR for this