Repository: omry/omegaconf

Issue:
pop and del should throw in struct mode
This is a behavior change. DictConfig pop and del should not be permitted in struct mode (and in structured mode) because it's inconsistent with the the expected behavior of classes.

Discussion:

Patch:
diff --git a/news/225.removal.1 b/news/225.removal.1
new file mode 100644
index 0000000..e69de29
diff --git a/news/225.removal.2 b/news/225.removal.2
new file mode 100644
index 0000000..e69de29
diff --git a/omegaconf/base.py b/omegaconf/base.py
index e18302c..61f97ed 100644
--- a/omegaconf/base.py
+++ b/omegaconf/base.py
@@ -55,13 +55,14 @@ class Node(ABC):
         assert parent is None or isinstance(parent, Container)
         return parent
 
-    def _set_flag(self, flag: str, value: Optional[bool]) -> None:
+    def _set_flag(self, flag: str, value: Optional[bool]) -> "Node":
         assert value is None or isinstance(value, bool)
         if value is None:
             if flag in self._metadata.flags:
                 del self._metadata.flags[flag]
         else:
             self._metadata.flags[flag] = value
+        return self
 
     def _get_node_flag(self, flag: str) -> Optional[bool]:
         """
diff --git a/omegaconf/dictconfig.py b/omegaconf/dictconfig.py
index f90a894..181427e 100644
--- a/omegaconf/dictconfig.py
+++ b/omegaconf/dictconfig.py
@@ -31,6 +31,7 @@ from .basecontainer import DEFAULT_VALUE_MARKER, BaseContainer
 from .errors import (
     ConfigAttributeError,
     ConfigKeyError,
+    ConfigTypeError,
     KeyValidationError,
     MissingMandatoryValue,
     OmegaConfBaseException,
@@ -101,11 +102,14 @@ class DictConfig(BaseContainer, MutableMapping[str, Any]):
     def copy(self) -> "DictConfig":
         return copy.copy(self)
 
-    def _validate_get(self, key: Any, value: Any = None) -> None:
-        is_typed = self._metadata.object_type not in (Any, None,) and not is_dict(
+    def _is_typed(self) -> bool:
+        return self._metadata.object_type not in (Any, None) and not is_dict(
             self._metadata.object_type
         )
 
+    def _validate_get(self, key: Any, value: Any = None) -> None:
+        is_typed = self._is_typed()
+
         is_struct = self._get_flag("struct") is True
         if key not in self.__dict__["_content"]:
             if is_typed:
@@ -319,9 +323,26 @@ class DictConfig(BaseContainer, MutableMapping[str, Any]):
                 key=key,
                 value=None,
                 cause=ReadonlyConfigError(
-                    "Cannot delete item from read-only DictConfig"
+                    "DictConfig in read-only mode does not support deletion"
+                ),
+            )
+        if self._get_flag("struct"):
+            self._format_and_raise(
+                key=key,
+                value=None,
+                cause=ConfigTypeError(
+                    "DictConfig in struct mode does not support deletion"
                 ),
             )
+        if self._is_typed():
+            self._format_and_raise(
+                key=key,
+                value=None,
+                cause=ConfigTypeError(
+                    f"{type_str(self._metadata.object_type)} (DictConfig) does not support deletion"
+                ),
+            )
+
         del self.__dict__["_content"][key]
 
     def get(
@@ -362,15 +383,21 @@ class DictConfig(BaseContainer, MutableMapping[str, Any]):
 
     def pop(self, key: Union[str, Enum], default: Any = DEFAULT_VALUE_MARKER) -> Any:
         try:
-            key = self._validate_and_normalize_key(key)
             if self._get_flag("readonly"):
                 raise ReadonlyConfigError("Cannot pop from read-only node")
-
+            if self._get_flag("struct"):
+                raise ConfigTypeError("DictConfig in struct mode does not support pop")
+            if self._is_typed():
+                raise ConfigTypeError(
+                    f"{type_str(self._metadata.object_type)} (DictConfig) does not support pop"
+                )
+            key = self._validate_and_normalize_key(key)
             node = self._get_node(key=key, validate_access=False)
             if node is not None:
                 value = self._resolve_with_default(
                     key=key, value=node, default_value=default
                 )
+
                 del self[key]
                 return value
             else:


Test patch:
diff --git a/tests/test_basic_ops_dict.py b/tests/test_basic_ops_dict.py
index 6efd0b1..a4c61b6 100644
--- a/tests/test_basic_ops_dict.py
+++ b/tests/test_basic_ops_dict.py
@@ -15,7 +15,7 @@ from omegaconf import (
     _utils,
 )
 from omegaconf.basecontainer import BaseContainer
-from omegaconf.errors import KeyValidationError
+from omegaconf.errors import ConfigTypeError, KeyValidationError
 
 from . import (
     ConcretePlugin,
@@ -76,11 +76,6 @@ def test_nested_dict_mandatory_value() -> None:
         c.a.b
 
 
-def test_mandatory_with_default() -> None:
-    c = OmegaConf.create(dict(name="???"))
-    assert c.get("name", "default value") == "default value"
-
-
 def test_subscript_get() -> None:
     c = OmegaConf.create("a: b")
     assert isinstance(c, DictConfig)
@@ -139,6 +134,8 @@ def test_scientific_notation_float() -> None:
     assert 10e-3 == c.a
 
 
+@pytest.mark.parametrize("struct", [None, True, False])  # type: ignore
+@pytest.mark.parametrize("default_val", [4, True, False, None])  # type: ignore
 @pytest.mark.parametrize(  # type: ignore
     "d,select,key",
     [
@@ -148,6 +145,7 @@ def test_scientific_notation_float() -> None:
         ({"hello": "${foo}", "foo": "???"}, "", "hello"),
         ({"hello": None}, "", "hello"),
         ({"hello": "${foo}"}, "", "hello"),
+        ({"hello": "${foo}", "foo": "???"}, "", "hello"),
         ({"hello": DictConfig(is_optional=True, content=None)}, "", "hello"),
         ({"hello": DictConfig(content="???")}, "", "hello"),
         ({"hello": DictConfig(content="${foo}")}, "", "hello"),
@@ -155,10 +153,12 @@ def test_scientific_notation_float() -> None:
         ({"hello": ListConfig(content="???")}, "", "hello"),
     ],
 )
-def test_dict_get_with_default(d: Any, select: Any, key: Any) -> None:
+def test_dict_get_with_default(
+    d: Any, select: Any, key: Any, default_val: Any, struct: Any
+) -> None:
     c = OmegaConf.create(d).select(select)
-    assert isinstance(c, DictConfig)
-    assert c.get(key, 4) == 4
+    OmegaConf.set_struct(c, struct)
+    assert c.get(key, default_val) == default_val
 
 
 def test_map_expansion() -> None:
@@ -255,14 +255,6 @@ def test_iterate_dict_with_interpolation() -> None:
         i = i + 1
 
 
-@pytest.mark.parametrize(  # type: ignore
-    "struct",
-    [
-        pytest.param(None, id="struct_unspecified"),
-        pytest.param(False, id="struct_False"),
-        pytest.param(True, id="struct_True"),
-    ],
-)
 @pytest.mark.parametrize(  # type: ignore
     "cfg, key, default_, expected",
     [
@@ -298,11 +290,8 @@ def test_iterate_dict_with_interpolation() -> None:
         ),
     ],
 )
-def test_dict_pop(
-    cfg: Dict[Any, Any], key: Any, default_: Any, expected: Any, struct: Optional[bool]
-) -> None:
+def test_dict_pop(cfg: Dict[Any, Any], key: Any, default_: Any, expected: Any) -> None:
     c = OmegaConf.create(cfg)
-    OmegaConf.set_struct(c, struct)
 
     if default_ != "__NO_DEFAULT__":
         val = c.pop(key, default_)
@@ -313,6 +302,24 @@ def test_dict_pop(
     assert type(val) == type(expected)
 
 
+@pytest.mark.parametrize(  # type: ignore
+    "cfg",
+    [
+        (OmegaConf.create({"name": "Bond", "age": 7})._set_flag("struct", True)),
+        (OmegaConf.structured(User)),
+    ],
+)
+def test_dict_struct_mode_pop(cfg: Any) -> None:
+    with pytest.raises(ConfigTypeError):
+        cfg.pop("name")
+
+    with pytest.raises(ConfigTypeError):
+        cfg.pop("bar")
+
+    with pytest.raises(ConfigTypeError):
+        cfg.pop("bar", "not even with default")
+
+
 @pytest.mark.parametrize(  # type: ignore
     "cfg, key, expectation",
     [
@@ -377,14 +384,23 @@ def test_dict_config() -> None:
 
 
 def test_dict_delitem() -> None:
-    c = OmegaConf.create(dict(a=10, b=11))
-    assert c == dict(a=10, b=11)
+    src = {"a": 10, "b": 11}
+    c = OmegaConf.create(src)
+    assert c == src
     del c["a"]
-    assert c == dict(b=11)
+    assert c == {"b": 11}
     with pytest.raises(KeyError):
         del c["not_found"]
 
 
+def test_dict_struct_delitem() -> None:
+    src = {"a": 10, "b": 11}
+    c = OmegaConf.create(src)
+    OmegaConf.set_struct(c, True)
+    with pytest.raises(ConfigTypeError):
+        del c["a"]
+
+
 @pytest.mark.parametrize(  # type: ignore
     "d, expected", [({}, 0), ({"a": 10, "b": 11}, 2)],
 )
diff --git a/tests/test_errors.py b/tests/test_errors.py
index 2ada2b0..850638b 100644
--- a/tests/test_errors.py
+++ b/tests/test_errors.py
@@ -31,6 +31,7 @@ from . import (
     Plugin,
     StructuredWithMissing,
     UnionError,
+    User,
 )
 
 
@@ -449,12 +450,35 @@ params = [
             create=lambda: create_readonly({"foo": "bar"}),
             op=lambda cfg: cfg.__delitem__("foo"),
             exception_type=ReadonlyConfigError,
-            msg="Cannot delete item from read-only DictConfig",
+            msg="DictConfig in read-only mode does not support deletion",
             key="foo",
             child_node=lambda cfg: cfg.foo,
         ),
         id="dict,readonly:del",
     ),
+    pytest.param(
+        Expected(
+            create=lambda: create_struct({"foo": "bar"}),
+            op=lambda cfg: cfg.__delitem__("foo"),
+            exception_type=ConfigTypeError,
+            msg="DictConfig in struct mode does not support deletion",
+            key="foo",
+            child_node=lambda cfg: cfg.foo,
+        ),
+        id="dict,struct:del",
+    ),
+    pytest.param(
+        Expected(
+            create=lambda: OmegaConf.structured(User(name="bond")),
+            op=lambda cfg: cfg.__delitem__("name"),
+            exception_type=ConfigTypeError,
+            msg="User (DictConfig) does not support deletion",
+            object_type=User,
+            key="name",
+            child_node=lambda cfg: cfg.name,
+        ),
+        id="dict,structured:del",
+    ),
     ##############
     # ListConfig #
     ##############
@@ -578,6 +602,28 @@ params = [
         ),
         id="list:pop_invalid_key",
     ),
+    pytest.param(
+        Expected(
+            create=lambda: create_struct({"foo": "bar"}),
+            op=lambda cfg: cfg.pop("foo"),
+            exception_type=ConfigTypeError,
+            msg="DictConfig in struct mode does not support pop",
+            key="foo",
+            child_node=lambda cfg: cfg.foo,
+        ),
+        id="dict,struct:pop",
+    ),
+    pytest.param(
+        Expected(
+            create=lambda: OmegaConf.structured(ConcretePlugin),
+            op=lambda cfg: cfg.pop("name"),
+            exception_type=ConfigTypeError,
+            msg="ConcretePlugin (DictConfig) does not support pop",
+            key="name",
+            child_node=lambda cfg: cfg.name,
+        ),
+        id="dict,structured:pop",
+    ),
     pytest.param(
         Expected(
             create=lambda: ListConfig(content=None),