Repository: lebrice/SimpleParsing

Issue:
Allow optional params to be checked with default=None, in addition to using the Optional type hint
**Is your feature request related to a problem? Please describe.**

It's common to have args in a dataclass that are optional but aren't explicitly typed as such via the `Optional` type hint. For example:

```python
from dataclasses import dataclass, field
from simple_parsing import ArgumentParser

@dataclass
class Args:
    x: int = field(default=None)

parser = ArgumentParser()
parser.add_arguments(Args, dest="args")
args = parser.parse_args().args
```

In this case, `Args.x` is clearly intended to be optional despite not having an `Optional` type hint. However, if you run this you'll get `error: the following arguments are required: -x/--x`. In order to run this, you have to change the line with `x` to be `x: Optional[int] = field(default=None)`. 

Of course, this is an easy change to make, but it's a little tedious, especially as many developers don't bother using `Optional` because it's kind of redundant with simply setting the default value to `None`. 

But even beyond that, it would be nice to use `simple_parsing` with, say, the Huggingface's `TrainingArguments` class from the `transformers` library. However, that's currently not possible because that class contains lines of code like [this one](https://github.com/huggingface/transformers/blob/main/src/transformers/training_args.py#L563), where you have optional args that don't use the `Optional` type hint. 

**Describe the solution you'd like**

Can we instead determine if an arg is optional by just checking if the default value inside the field is `None`? 

If needed, I can volunteer to help make this change!

Discussion:

Patch:
diff --git a/simple_parsing/wrappers/dataclass_wrapper.py b/simple_parsing/wrappers/dataclass_wrapper.py
index 86866a9..bcb4589 100644
--- a/simple_parsing/wrappers/dataclass_wrapper.py
+++ b/simple_parsing/wrappers/dataclass_wrapper.py
@@ -1,6 +1,6 @@
 import argparse
 import dataclasses
-from dataclasses import _MISSING_TYPE
+from dataclasses import MISSING
 from logging import getLogger
 from typing import Dict, List, Optional, Type, Union, cast
 
@@ -48,6 +48,7 @@ class DataclassWrapper(Wrapper[Dataclass]):
 
         for field in dataclasses.fields(self.dataclass):
             if not field.init or field.metadata.get("cmd", True) is False:
+                # Don't add arguments for these fields.
                 continue
             field_default = getattr(default, field.name, None)
             if isinstance(field.type, str):
@@ -71,7 +72,7 @@ class DataclassWrapper(Wrapper[Dataclass]):
                     f"supported yet. (container of a dataclass type)"
                 )
 
-            elif dataclasses.is_dataclass(field.type):
+            elif dataclasses.is_dataclass(field.type) and field.default is not None:
                 # handle a nested dataclass attribute
                 dataclass, name = field.type, field.name
                 child_wrapper = DataclassWrapper(
@@ -128,11 +129,9 @@ class DataclassWrapper(Wrapper[Dataclass]):
             elif wrapped_field.arg_options:
                 options = wrapped_field.arg_options
                 if argparse.SUPPRESS in self.defaults:
-                    options['default'] = argparse.SUPPRESS
+                    options["default"] = argparse.SUPPRESS
 
-                logger.debug(
-                    f"Arg options for field '{wrapped_field.name}': {options}"
-                )
+                logger.debug(f"Arg options for field '{wrapped_field.name}': {options}")
                 group.add_argument(*wrapped_field.option_strings, **options)
 
     def equivalent_argparse_code(self, leading="group") -> str:
@@ -186,16 +185,12 @@ class DataclassWrapper(Wrapper[Dataclass]):
         if self.parent.defaults:
             self._defaults = []
             for default in self.parent.defaults:
-                if default is None:
-                    default = None
-                elif default == argparse.SUPPRESS:
-                    default = argparse.SUPPRESS
-                else:
+                if default not in (None, argparse.SUPPRESS):
                     default = getattr(default, self.name)
                 self._defaults.append(default)
         else:
             default_field_value = utils.default_value(self._field)
-            if isinstance(default_field_value, _MISSING_TYPE):
+            if default_field_value is MISSING:
                 self._defaults = []
             else:
                 self._defaults = [default_field_value]
diff --git a/simple_parsing/wrappers/field_wrapper.py b/simple_parsing/wrappers/field_wrapper.py
index 393ee50..9a541e7 100644
--- a/simple_parsing/wrappers/field_wrapper.py
+++ b/simple_parsing/wrappers/field_wrapper.py
@@ -11,8 +11,6 @@ from simple_parsing.help_formatter import TEMPORARY_TOKEN
 
 from .. import docstring, utils
 from .field_metavar import get_metavar
-
-# from ..utils import Dataclass, DataclassType
 from .field_parsing import get_parsing_fn
 from .wrapper import Wrapper
 
@@ -212,18 +210,13 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
                     logger.info(f"Chosen subgroup for '{self.dest}':  '{value}'")
 
     def get_arg_options(self) -> Dict[str, Any]:
-        """Create the `parser.add_arguments` kwargs for this field."""
+        """Create the `parser.add_arguments` kwargs for this field.
+
+        TODO: Refactor this, following https://github.com/lebrice/SimpleParsing/issues/150
+        """
         if not self.field.init:
             # Don't add command-line arguments for fields that have `init=False`.
-            # TODO: See (https://github.com/lebrice/SimpleParsing/issues/20#issue-619198492)
             return {}
-        # TODO: Refactor this:
-        # 1. Create a `get_argparse_options_for_field` function
-        # 2. Use `get_argparse_options_for_annotation` below as part of that function
-        # 3. Update the dict returned from 1. with values set in the field() function
-        # 4. Update the dict from 3. with the values set by the DataclassWrapper, or
-        # when this field is reused. (are they ever modified externally?)
-        # 5. Return that dictionary.
         _arg_options: Dict[str, Any] = {}
 
         # Not sure why, but argparse doesn't allow using a different dest for a positional arg.
@@ -249,20 +242,28 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
             # We use the default 'metavar' generated by argparse.
             _arg_options.pop("metavar", None)
 
-        elif utils.is_optional(self.type):
+        elif utils.is_optional(self.type) or self.field.default is None:
             _arg_options["required"] = False
 
-            type_arguments = utils.get_args(self.type)
-            # NOTE: Optional[<something>] is always translated to
-            # Union[<something>, NoneType]
-            assert type_arguments
-            non_none_types = [t for t in type_arguments if t is not type(None)]  # noqa: E721
-            assert non_none_types
-
-            if len(non_none_types) == 1:
-                wrapped_type = non_none_types[0]
+            if utils.is_optional(self.type):
+                type_arguments = utils.get_args(self.type)
+                # NOTE: Optional[<something>] is always translated to
+                # Union[<something>, NoneType]
+                assert type_arguments
+                non_none_types = [t for t in type_arguments if t is not type(None)]  # noqa: E721
+                assert non_none_types
+                if len(non_none_types) == 1:
+                    wrapped_type = non_none_types[0]
+                else:
+                    # Construct the type annotation for the non-optional version of the type.
+                    wrapped_type = Union[tuple(non_none_types)]  # type: ignore
             else:
-                wrapped_type = Union[tuple(non_none_types)]
+                assert self.field.default is None
+                # If the default value is None, then type annotation is incorrect (i.e. not
+                # `Optional[T]` or `T | None`). We allow it, as discussed in issue #132:
+                # https://github.com/lebrice/SimpleParsing/issues/132, and treat the type
+                # annotation as the type of the field.
+                wrapped_type = self.type
 
             if utils.is_tuple(wrapped_type):
                 # TODO: ISSUE 42: better handle optional/nested tuples.
@@ -711,7 +712,7 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
 
             defaults = []
             for default_dataclass_instance in self.parent.defaults:
-                if default_dataclass_instance is None or default_dataclass_instance == argparse.SUPPRESS:
+                if default_dataclass_instance in (None, argparse.SUPPRESS):
                     default_value = default
                 elif isinstance(default_dataclass_instance, dict):
                     default_value = default_dataclass_instance.get(self.name, default)
@@ -741,31 +742,26 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
     def required(self) -> bool:
         if self._required is not None:
             return self._required
-
         if self.action_str.startswith("store_"):
             # all the store_* actions do not require a value.
-            self._required = False
-        elif self.is_optional:
-            self._required = False
-        elif self.parent.required:
+            return False
+        if self.is_optional:
+            return False
+        if self.parent.required:
             # if the parent dataclass is required, then this attribute is too.
             # TODO: does that make sense though?
-            self._required = True
-
-        elif self.nargs in {"?", "*"}:
-            self._required = False
-        elif self.nargs == "+":
-            self._required = True
-
-        elif self.default is None and argparse.SUPPRESS not in self.parent.defaults:
-            self._required = True
-        elif self.is_reused:
+            return True
+        if self.nargs in {"?", "*"}:
+            return False
+        if self.nargs == "+":
+            return True
+        if self.default is None and argparse.SUPPRESS not in self.parent.defaults:
+            return True
+        if self.is_reused:
             # if we're reusing this argument, the default value might be a list
             # of `MISSING` values.
-            self._required = any(v == dataclasses.MISSING for v in self.default)
-        else:
-            self._required = False
-        return self._required
+            return any(v == dataclasses.MISSING for v in self.default)
+        return False
 
     @required.setter
     def required(self, value: bool):
@@ -774,6 +770,7 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
     @property
     def type(self) -> Type[Any]:
         """Returns the wrapped field's type annotation."""
+        # TODO: Refactor this. Really ugly.
         if self._type is None:
             self._type = self.field.type
             if isinstance(self._type, str):
@@ -1020,143 +1017,3 @@ def only_keep_action_args(options: Dict[str, Any], action: Union[str, Any]) -> D
         logger.debug(f"Kept options: \t{kept_options.keys()}")
         logger.debug(f"Removed options: \t{deleted_options.keys()}")
     return kept_options
-
-
-def get_argparse_options_for_annotation(t: Type) -> Dict[str, Any]:
-    """Generates some of the argparse arguments for `parser.add_argument` based on the
-    given type annotation.
-
-    This is able to generate some, but not all the necessary options.
-
-    WIP: This is part of an attempt at refactoring things a little bit.
-    """
-    _arg_options: Dict[str, Any] = {}
-    _arg_options["metavar"] = get_metavar(t)
-
-    if utils.is_enum(t):
-        _arg_options["type"] = str
-        assert False, t.names
-        _arg_options["choices"] = t.names
-        if utils.is_list(t):
-            _arg_options["nargs"] = argparse.ZERO_OR_MORE
-        # We use the default 'metavar' generated by argparse.
-        _arg_options.pop("metavar", None)
-
-    elif utils.is_optional(t):
-        type_arguments = utils.get_args(t)
-        assert type_arguments
-        if len(type_arguments) != 2:
-            raise NotImplementedError(
-                "Can only handle `Optional[<something>]`, not "
-                "`Union[<something>, <something_else>, None]`"
-            )
-        # NOTE: Optional[<something>] is always translated to
-        # Union[<something>, NoneType]
-        wrapped_type: Type = type_arguments[0]
-
-        # Recurse!
-        options_for_wrapped_type = get_argparse_options_for_annotation(wrapped_type)
-        _arg_options.update(options_for_wrapped_type)
-        _arg_options["required"] = False
-        _arg_options.setdefault("nargs", "?")
-        # _arg_options.setdefault("default", None)
-
-        # if utils.is_tuple(wrapped_type):
-        #     # TODO: ISSUE 42: better handle optional/nested tuples.
-        #     # For now we just assume that the item types are 'simple'.
-
-        #     # IDEA: This could probably be a static method that takes in the type
-        #     # annotation, and uses a recursive call to fetch the arg options of the
-        #     # item type, and uses some of the entries from that dict to construct
-        #     # the arg options of the parent?
-        #     # NOTE: We'd have to return different value for the `type` argument
-        #     # depending on the nesting:
-        #     # Tuple[int, int] -> <class int>
-        #     # Tuple[int, str] -> <some autogenerated function>
-        #     # Tuple[Tuple[int, str], Tuple[int, str]] -> <some autogenerated
-        #     #   function *that doesn't just use `<class int> from above twice!
-        #     #   (Since we want to support passing --foo '(a, 1)' '(b, 4)'
-        #     # `)>
-        #     _arg_options["type"] = get_parsing_fn(wrapped_type)
-        #     _arg_options["nargs"] = utils.get_container_nargs(wrapped_type)
-
-        # elif utils.is_list(wrapped_type):
-        #     _arg_options["type"] = utils.get_argparse_type_for_container(
-        #         wrapped_type
-        #     )
-        #     _arg_options["nargs"] = "*"
-        #     # NOTE: Can't set 'const', since we'd get:
-        #     # ValueError: nargs must be '?' to supply const
-        #     # _arg_options["const"] = []
-        # else:
-        #     _arg_options["type"] = get_parsing_fn(wrapped_type)
-        #     # TODO: Should the 'nargs' really be '?' here?
-        #     _arg_options["nargs"] = "?"
-        #     # assert False, (wrapped_type, utils.is_tuple(wrapped_type))
-
-    elif utils.is_union(t):
-        logger.debug("Parsing a Union type!")
-        _arg_options["type"] = get_parsing_fn(t)
-
-    elif utils.is_enum(t):
-        # we actually parse enums as string, and convert them back to enums
-        # in the `postprocess` method.
-        assert False, t.names
-        _arg_options["choices"] = list(e.name for e in t)
-        _arg_options["type"] = str
-        # if the default value is an Enum, we convert it to a string.
-        # if self.default:
-        #     def enum_to_str(e):
-        #         return e.name if isinstance(e, Enum) else e
-
-        #     if self.is_reused:
-        #         _arg_options["default"] = [
-        #             enum_to_str(default) for default in self.default
-        #         ]
-        #     else:
-        #         _arg_options["default"] = enum_to_str(self.default)
-
-    elif utils.is_list(t):
-        logger.debug(f"Adding a List attribute '{t}")
-        _arg_options["nargs"] = "*"
-        item_type = utils.get_item_type(t)
-        _arg_options["type"] = get_parsing_fn(item_type)
-
-        # if self.is_reused:
-        #     # TODO: Only the 'single-level' lists (not reused) use the new
-        #     # `get_parsing_fn` function (for now).
-        #     type_fn = utils._parse_multiple_containers(self.type)
-        #     type_fn.__name__ = utils.get_type_name(self.type)
-        #     _arg_options["type"] = type_fn
-        # else:
-        # _arg_options["type"] = utils.get_argparse_type_for_container(self.type)
-
-    elif utils.is_tuple(t):
-        logger.debug(f"Adding a Tuple attribute with type {t}")
-        _arg_options["nargs"] = utils.get_container_nargs(t)
-        _arg_options["type"] = get_parsing_fn(t)
-        _arg_options["nargs"] = utils.get_container_nargs(t)
-
-        # if self.is_reused:
-        #     type_fn = utils._parse_multiple_containers(self.type)
-        #     type_fn.__name__ = utils.get_type_name(self.type)
-        #     _arg_options["type"] = type_fn
-
-    elif utils.is_bool(t):
-        _arg_options["type"] = utils.str2bool
-        _arg_options["type"].__name__ = "bool"
-        _arg_options["nargs"] = "?"
-
-    else:
-        # "Plain" / simple argument.
-        # For the metavar, use a custom passed value, if present, else do
-        # not put any value in (which uses the usual value from argparse).
-        # _arg_options["metavar"] = get_metavar(t)
-        _arg_options["type"] = get_parsing_fn(t)
-
-    # if self.is_reused:
-    #     if self.required:
-    #         _arg_options["nargs"] = "+"
-    #     else:
-    #         _arg_options["nargs"] = "*"
-    return _arg_options


Test patch:
diff --git a/test/conftest.py b/test/conftest.py
index 193d8f5..a06ae2b 100644
--- a/test/conftest.py
+++ b/test/conftest.py
@@ -1,7 +1,7 @@
 import logging
 import sys
 from dataclasses import dataclass
-from typing import Any, ClassVar, List, Optional, Tuple, Type
+from typing import Any, ClassVar, List, NamedTuple, Optional, Tuple, Type
 
 import pytest
 
@@ -45,6 +45,12 @@ simple_arguments: List[Tuple[Type, Any, Any]] = [
 ]
 
 
+class SimpleAttributeTuple(NamedTuple):
+    field_type: Type
+    passed_value: str
+    expected_value: Any
+
+
 @pytest.fixture(params=simple_arguments)
 def simple_attribute(request):
     """Test fixture that produces an tuple of (type, passed value, expected value)"""
@@ -52,7 +58,9 @@ def simple_attribute(request):
     logging.debug(
         f"Attribute type: {some_type}, passed value: '{passed_value}', expected: '{expected_value}'"
     )
-    return request.param
+    return SimpleAttributeTuple(
+        field_type=some_type, passed_value=passed_value, expected_value=expected_value
+    )
 
 
 @pytest.fixture
diff --git a/test/test_custom_args.py b/test/test_custom_args.py
index 9303306..1d09201 100644
--- a/test/test_custom_args.py
+++ b/test/test_custom_args.py
@@ -100,10 +100,10 @@ def test_custom_nargs_plus():
 def test_custom_nargs_star():
     @dataclass
     class Foo(TestSetup):
-        some_int: int = field(type=int, nargs="*")
+        some_int: List[int] = field(default_factory=list, type=int, nargs="*")
 
     foo = Foo.setup("")
-    assert foo.some_int is None
+    assert foo.some_int == []
 
     foo = Foo.setup("--some_int")
     assert foo.some_int == []
diff --git a/test/test_fields.py b/test/test_fields.py
index 9056766..f29ab9d 100644
--- a/test/test_fields.py
+++ b/test/test_fields.py
@@ -1,8 +1,6 @@
 from dataclasses import dataclass
 
-from test.testutils import TestSetup
-
-from simple_parsing import ArgumentParser, field, ConflictResolution
+from simple_parsing import ArgumentParser, ConflictResolution, field
 
 
 def test_cmd_false_doesnt_create_conflicts():
@@ -29,11 +27,10 @@ def test_cmd_false_doesnt_create_conflicts():
     assert b == B(batch_size=32)
 
 
-from typing import Type, Dict, Tuple, List, Optional
-from simple_parsing.wrappers.field_wrapper import get_argparse_options_for_annotation
-import pytest
-
 from enum import Enum
+from typing import Dict, List, Optional, Tuple, Type
+
+import pytest
 
 
 class Color(Enum):
@@ -43,11 +40,13 @@ class Color(Enum):
     orange: str = "ORANGE"
 
 
-from simple_parsing.wrappers.field_parsing import get_parsing_fn
-
 from simple_parsing.utils import str2bool
 from simple_parsing.wrappers.field_parsing import parse_enum
 
+
+@pytest.mark.xfail(
+    reason="Removed this function. TODO: see https://github.com/lebrice/SimpleParsing/issues/150."
+)
 @pytest.mark.parametrize(
     "annotation, expected_options",
     [
@@ -62,6 +61,23 @@ from simple_parsing.wrappers.field_parsing import parse_enum
     ],
 )
 def test_generated_options_from_annotation(annotation: Type, expected_options: Dict):
-    actual_options = get_argparse_options_for_annotation(annotation)
-    for option, expected_value in expected_options.items():
-        assert actual_options[option] == expected_value
+    raise NotImplementedError(
+        """
+        TODO: Would be a good idea to refactor the FieldWrapper class a bit. The args_dict (a dict
+        of all the argparse arguments for a given field, that get passed to parser.add_arguments
+        in the FieldWrapper) is currently created using a mix of three things (with increasing
+        priority):
+        - The type annotation
+        - The dataclass context (e.g. when adding an Optional[Dataclass] field on another
+          dataclass, or when using the `default` or `prefix` arguments to `parser.add_arguments`.
+        - The manual overrides (arguments of `parser.add_argument` passed to the `field` function)
+
+        These three are currently a bit mixed together in the `FieldWrapper` class. It would be
+        preferable to design a way for them to be cleanly separated.
+        """
+    )
+    # from simple_parsing.wrappers.field_wrapper import get_argparse_options_for_annotation
+
+    # actual_options = get_argparse_options_for_annotation(annotation)
+    # for option, expected_value in expected_options.items():
+    #     assert actual_options[option] == expected_value
diff --git a/test/test_issue_132.py b/test/test_issue_132.py
new file mode 100644
index 0000000..373f2b0
--- /dev/null
+++ b/test/test_issue_132.py
@@ -0,0 +1,55 @@
+""" Test for https://github.com/lebrice/SimpleParsing/issues/132 """
+from dataclasses import dataclass
+
+from simple_parsing import field
+
+from .conftest import SimpleAttributeTuple
+from .testutils import TestSetup
+
+
+def test_field_with_custom_required_arg_is_optional(simple_attribute: SimpleAttributeTuple):
+    """Test that the `field` function can be used as a work-around for issue 132.
+
+    When passing `required` (or any other of the usual arguments of `parser.add_argument`) to the
+    `field` function, they get saved into the `FieldWrapper`, and used to populate the args_dict
+    that gets passed to `parser.add_arguments(*field_wrapper.option_strings, **args_dict)`.
+
+    Therefore, using `required=False` as a custom argument is a work-around, while we fix this
+    issue.
+    """
+    some_type, passed_value, expected_value = simple_attribute
+
+    @dataclass
+    class Foo(TestSetup):
+        a: some_type = field(default=None, required=False)  # type: ignore
+
+    assert Foo.setup() == Foo(a=None)
+    assert Foo.setup(f"--a {passed_value}") == Foo(a=expected_value)
+
+
+def test_field_with_none_default_is_optional(simple_attribute: SimpleAttributeTuple):
+    """Test that when the default value is None, the argument is treated as optional."""
+    some_type, passed_value, expected_value = simple_attribute
+
+    @dataclass
+    class Foo(TestSetup):
+        a: some_type = None  # type: ignore
+
+    assert Foo.setup() == Foo(a=None)
+    assert Foo.setup(f"--a {passed_value}") == Foo(a=expected_value)
+
+
+def test_dataclass_field_with_none_default_is_optional(simple_attribute: SimpleAttributeTuple):
+    """Test that when the default value is None, the argument is treated as optional."""
+    some_type, passed_value, expected_value = simple_attribute
+
+    @dataclass
+    class Foo(TestSetup):
+        a: some_type  # type: ignore
+
+    @dataclass
+    class Bar(TestSetup):
+        foo: Foo = None  # type: ignore
+
+    assert Bar.setup() == Bar(foo=None)  # type: ignore
+    assert Bar.setup(f"--a {passed_value}") == Bar(foo=Foo(a=expected_value))