Revert PR#696 (#705)

omegaconf/_utils.py

@@ -198,8 +198,13 @@ def _is_optional(obj: Any, key: Optional[Union[int, str]] = None) -> bool:
     if key is not None:
         assert isinstance(obj, Container)
         obj = obj._get_node(key)
-    assert isinstance(obj, Node)  # raises if key could not be found
-    return obj._is_optional()
+    if isinstance(obj, Node):
+        return obj._is_optional()
+    else:
+        # In case `obj` is not a Node, treat it as optional by default.
+        # This is used in `ListConfig.append` and `ListConfig.insert`
+        # where the appended/inserted value might or might not be a Node.
+        return True
 
 
 def _resolve_forward(type_: Type[Any], module: str) -> Type[Any]:

omegaconf/basecontainer.py

@@ -14,7 +14,6 @@
     _is_missing_literal,
     _is_missing_value,
     _is_none,
-    _is_optional,
     _resolve_optional,
     get_ref_type,
     get_structured_config_data,
@@ -513,7 +512,7 @@ def _set_item_impl(self, key: Any, value: Any) -> None:
         )
 
         def wrap(key: Any, val: Any) -> Node:
-            is_optional = _is_optional(self)
+            is_optional = True
             if not is_structured_config(val):
                 ref_type = self._metadata.element_type
             else:

omegaconf/listconfig.py

@@ -256,7 +256,7 @@ def append(self, item: Any) -> None:
                 ref_type=self.__dict__["_metadata"].element_type,
                 key=index,
                 value=item,
-                is_optional=_is_optional(self),
+                is_optional=_is_optional(item),
                 parent=self,
             )
             self.__dict__["_content"].append(node)
@@ -292,7 +292,7 @@ def insert(self, index: int, item: Any) -> None:
                     ref_type=self.__dict__["_metadata"].element_type,
                     key=index,
                     value=item,
-                    is_optional=_is_optional(self),
+                    is_optional=_is_optional(item),
                     parent=self,
                 )
                 self._validate_set(key=index, value=node)

tests/structured_conf/test_structured_config.py

@@ -800,7 +800,19 @@ def test_set_list_correct_type(self, module: Any) -> None:
         assert cfg.tuple == value
 
     @mark.parametrize(
-        "value", [1, True, "str", 3.1415, ["foo", True, 1.2], User(), [None]]
+        "value",
+        [
+            1,
+            True,
+            "str",
+            3.1415,
+            ["foo", True, 1.2],
+            User(),
+            param(
+                [None],
+                marks=mark.xfail,  # https://github.com/omry/omegaconf/issues/579
+            ),
+        ],
     )
     def test_assign_wrong_type_to_list(self, module: Any, value: Any) -> None:
         cfg = OmegaConf.structured(module.ListClass)
@@ -810,7 +822,19 @@ def test_assign_wrong_type_to_list(self, module: Any, value: Any) -> None:
             cfg.tuple = value
         assert cfg == OmegaConf.structured(module.ListClass)
 
-    @mark.parametrize("value", [None, True, "str", 3.1415, User()])
+    @mark.parametrize(
+        "value",
+        [
+            param(
+                None,
+                marks=mark.xfail,  # https://github.com/omry/omegaconf/issues/579
+            ),
+            True,
+            "str",
+            3.1415,
+            User(),
+        ],
+    )
     def test_insert_wrong_type_to_list(self, module: Any, value: Any) -> None:
         cfg = OmegaConf.structured(module.ListClass)
         with raises(ValidationError):
@@ -825,7 +849,10 @@ def test_insert_wrong_type_to_list(self, module: Any, value: Any) -> None:
             3.1415,
             ["foo", True, 1.2],
             {"foo": True},
-            {"foo": None},
+            param(
+                {"foo": None},
+                marks=mark.xfail,  # expected failure, https://github.com/omry/omegaconf/issues/579
+            ),
             User(age=1, name="foo"),
             {"user": User(age=1, name="foo")},
             ListConfig(content=[1, 2], ref_type=List[int], element_type=int),

tests/test_basic_ops_dict.py

@@ -846,12 +846,6 @@ def test_getitem_with_invalid_key() -> None:
         cfg.__getitem__(object())  # type: ignore
 
 
-def test_assign_none_to_non_optional() -> None:
-    cfg = DictConfig(content={}, element_type=str, is_optional=False)
-    with raises(ValidationError):
-        cfg["key"] = None
-
-
 def test_hasattr() -> None:
     cfg = OmegaConf.create({"foo": "bar"})
     OmegaConf.set_struct(cfg, True)

tests/test_basic_ops_list.py

@@ -335,15 +335,6 @@ def test_list_append() -> None:
             ),
             id="append_str_to_list[int]",
         ),
-        param(
-            ListConfig(content=[], element_type=int, is_optional=False),
-            None,
-            raises(
-                ValidationError,
-                match=re.escape("Non optional field cannot be assigned None"),
-            ),
-            id="append_None_to_list[int]",
-        ),
         param(
             ListConfig(content=[], element_type=Color),
             "foo",
@@ -397,12 +388,6 @@ def test_append_invalid_element_type(
         lc.append(element)
 
 
-def test_assign_none_to_non_optional() -> None:
-    cfg = ListConfig(content=["abc"], element_type=str, is_optional=False)
-    with raises(ValidationError):
-        cfg[0] = None
-
-
 @mark.parametrize(
     "lc,element,expected",
     [
@@ -488,14 +473,6 @@ def validate_list_keys(c: Any) -> None:
             None,
             ValidationError,
         ),
-        (
-            ListConfig(element_type=int, content=[], is_optional=False),
-            0,
-            None,
-            None,
-            None,
-            ValidationError,
-        ),
     ],
 )
 def test_insert(

tests/test_errors.py

@@ -414,19 +414,6 @@ def finalize(self, cfg: Any) -> None:
         ),
         id="DictConfig[str,Color]:setitem_bad_value",
     ),
-    param(
-        Expected(
-            create=lambda: DictConfig(
-                key_type=str, element_type=int, content={}, is_optional=False
-            ),
-            op=lambda cfg: cfg.__setitem__("foo", None),
-            exception_type=ValidationError,
-            msg="Non optional field cannot be assigned None",
-            full_key="foo",
-            key="foo",
-        ),
-        id="DictConfig[str,non-optional-int]:setitem_None_value",
-    ),
     param(
         Expected(
             create=lambda: OmegaConf.structured(User),