Add typing to brain_dataclasses

[DanielNoord]

Steps

• Write a good description on what the PR does.

Description

This does not fix everything as I have left some of the non-trivial cases for a separate PR.

I have one question that needs to be answered before this can be merged.

Type of Changes

	Type
✓	🔨 Refactoring

Related Issue

[DanielNoord]

@cdce8p For some reason I can't seem to restrict the return type of _get_field_default and let mypy know about this.

I tried Union[Tuple[None, None], Tuple[str, NodeNG]] (and the necessary change to the method) as return type annotation, but then mypy doesn't recognise that if default_type == "default" then default_node is a NodeNG. I have tried other things such as working but a type alias but I couldn't get it to work. Searching for "mypy union return distinguish tuple constraints" etc also didn't return anything.

Do you have any good ideas or are these assert's the best solution here?

[tushar-deepsource]

Here's a neat way to do that:

result = _get_field_default(value)
if result != (None, None):
    default_type, default_node = result
    # Here mypy should know that default_type is a str, and default_node is a NodeNG

[tushar-deepsource]

You could also modify the return type of _get_field_value to return None instead of None, None. Might make it better?

[DanielNoord]

                result = _get_field_default(value)
                if result != (None, None):
                    default_type, default_node = result
                    if default_type == "default":
                        param_str += f" = {default_node.as_string()}"  # L202
                    elif default_type == "default_factory":
                        param_str += f" = {DEFAULT_FACTORY}"
                        assignment_str = (
                            f"self.{name} = {default_node.as_string()} "  # L206
                            f"if {name} is {DEFAULT_FACTORY} else {name}"
                        )

Still gives me:

astroid/brain/brain_dataclasses.py:202: error: Item "None" of "Optional[NodeNG]" has no attribute "as_string"  [union-attr]
astroid/brain/brain_dataclasses.py:206: error: Item "None" of "Optional[NodeNG]" has no attribute "as_string"  [union-attr]

Am I missing something?

[tushar-deepsource]

Did you change the return type for that function to Union[Tuple[None, None], Tuple[str, NodeNG]]?

[DanielNoord]

Yeah.

Diff:

diff --git a/astroid/brain/brain_dataclasses.py b/astroid/brain/brain_dataclasses.py
index b34f1aeff..d239806c5 100644
--- a/astroid/brain/brain_dataclasses.py
+++ b/astroid/brain/brain_dataclasses.py
@@ -7,7 +7,7 @@ Support both built-in dataclasses and pydantic.dataclasses. References:
 - https://docs.python.org/3/library/dataclasses.html
 - https://pydantic-docs.helpmanual.io/usage/dataclasses/
 """
-from typing import FrozenSet, Generator, List, Optional, Tuple
+from typing import FrozenSet, Generator, List, Optional, Tuple, Union
 
 from astroid import context, inference_tip
 from astroid.builder import parse
@@ -195,17 +195,18 @@ def _generate_dataclass_init(assigns: List[AnnAssign]) -> str:
             if isinstance(value, Call) and _looks_like_dataclass_field_call(
                 value, check_scope=False
             ):
-                default_type, default_node = _get_field_default(value)
-                if default_type == "default":
-                    assert default_node
-                    param_str += f" = {default_node.as_string()}"
-                elif default_type == "default_factory":
-                    assert default_node
-                    param_str += f" = {DEFAULT_FACTORY}"
-                    assignment_str = (
-                        f"self.{name} = {default_node.as_string()} "
-                        f"if {name} is {DEFAULT_FACTORY} else {name}"
-                    )
+                result = _get_field_default(value)
+                if result != (None, None):
+                    default_type, default_node = result
+                    if default_type == "default":
+                        param_str += f" = {default_node.as_string()}"
+                    elif default_type == "default_factory":
+                        param_str += f" = {DEFAULT_FACTORY}"
+                        assignment_str = (
+                            f"self.{name} = {default_node.as_string()} "
+                            f"if {name} is {DEFAULT_FACTORY} else {name}"
+                        )
+
             else:
                 param_str += f" = {value.as_string()}"
 
@@ -335,7 +336,9 @@ def _looks_like_dataclass_field_call(node: Call, check_scope: bool = True) -> bo
     return inferred.name == FIELD_NAME and inferred.root().name in DATACLASS_MODULES
 
 
-def _get_field_default(field_call: Call) -> Tuple[str, Optional[NodeNG]]:
+def _get_field_default(
+    field_call: Call,
+) -> Union[Tuple[None, None], Tuple[str, NodeNG]]:
     """Return a the default value of a field call, and the corresponding keyword argument name.
 
     field(default=...) results in the ... node
@@ -363,7 +366,7 @@ def _get_field_default(field_call: Call) -> Tuple[str, Optional[NodeNG]]:
         new_call.postinit(func=default_factory)
         return "default_factory", new_call
 
-    return "", None
+    return None, None
 
 
 def _is_class_var(node: Optional[NodeNG]) -> bool:

[DanielNoord]

The following diff works, but it feels weird to need to do an additional assignment to help mypy infer the type:
It should be able to link the None to the None and str to NodeNG.

diff --git a/astroid/brain/brain_dataclasses.py b/astroid/brain/brain_dataclasses.py
index b34f1aeff..648fdb827 100644
--- a/astroid/brain/brain_dataclasses.py
+++ b/astroid/brain/brain_dataclasses.py
@@ -7,7 +7,7 @@ Support both built-in dataclasses and pydantic.dataclasses. References:
 - https://docs.python.org/3/library/dataclasses.html
 - https://pydantic-docs.helpmanual.io/usage/dataclasses/
 """
-from typing import FrozenSet, Generator, List, Optional, Tuple
+from typing import FrozenSet, Generator, List, Optional, Tuple, Union
 
 from astroid import context, inference_tip
 from astroid.builder import parse
@@ -195,17 +195,17 @@ def _generate_dataclass_init(assigns: List[AnnAssign]) -> str:
             if isinstance(value, Call) and _looks_like_dataclass_field_call(
                 value, check_scope=False
             ):
-                default_type, default_node = _get_field_default(value)
-                if default_type == "default":
-                    assert default_node
-                    param_str += f" = {default_node.as_string()}"
-                elif default_type == "default_factory":
-                    assert default_node
-                    param_str += f" = {DEFAULT_FACTORY}"
-                    assignment_str = (
-                        f"self.{name} = {default_node.as_string()} "
-                        f"if {name} is {DEFAULT_FACTORY} else {name}"
-                    )
+                result = _get_field_default(value)
+                if result:
+                    default_type, default_node = result
+                    if default_type == "default":
+                        param_str += f" = {default_node.as_string()}"
+                    elif default_type == "default_factory":
+                        param_str += f" = {DEFAULT_FACTORY}"
+                        assignment_str = (
+                            f"self.{name} = {default_node.as_string()} "
+                            f"if {name} is {DEFAULT_FACTORY} else {name}"
+                        )
             else:
                 param_str += f" = {value.as_string()}"
 
@@ -335,7 +335,9 @@ def _looks_like_dataclass_field_call(node: Call, check_scope: bool = True) -> bo
     return inferred.name == FIELD_NAME and inferred.root().name in DATACLASS_MODULES
 
 
-def _get_field_default(field_call: Call) -> Tuple[str, Optional[NodeNG]]:
+def _get_field_default(
+    field_call: Call,
+) -> Optional[Tuple[str, NodeNG]]:
     """Return a the default value of a field call, and the corresponding keyword argument name.
 
     field(default=...) results in the ... node
@@ -363,7 +365,7 @@ def _get_field_default(field_call: Call) -> Tuple[str, Optional[NodeNG]]:
         new_call.postinit(func=default_factory)
         return "default_factory", new_call
 
-    return "", None
+    return None
 
 
 def _is_class_var(node: Optional[NodeNG]) -> bool:

[tushar-deepsource]

Yeah. Possibly mypy bug.

But I prefer this solution, as we were essentially returning two sentinels in the previous case.

+1 from me on return None

[Pierre-Sassoulas]

👍 Thank you @tushar-deepsource for the thorough review !

[cdce8p]

I still plan to get to it, sorry for the delay.

[Pierre-Sassoulas]

No worry, there's still the issue with python 3.10 to fix before merging anyway. I don't know what could be happening, maybe a deprecation in setuptools/distutils ?

[DanielNoord]

@cdce8p No rush! We're all still volunteers.

If I can be so bold, could I ask you to put pylint-dev/pylint#5221 on the top of your pylint/astroid to-do list? We recently got a new issue about it and it blocks pylint-dev/pylint#3704 which would be good to clean up from our open PR list. As I said, no rush, just a possible rearrangement of todo's 😄

[cdce8p]

As you pointed out in the comment, this will always be true.
I haven't used it much so far, but something like this might work.

class _DataclassClassDef(ClassDef):
    decorators: Decorators

def _check_generate_dataclass_init(node: _DataclassClassDef) -> bool:
    ...

It might even make sense to guard _DataclassClassDef behind if TYPE_CHECKING, just to be safe that it isn't used at runtime.

Usually I would recommend isinstance calls but with the astroid brains those already happen in the predicate functions, is_decorated_with_dataclass in this case.

[DanielNoord]

I'm wondering if we should make an instance for dataclass like I did for NamedTuple in #1306.
That would also allow setting decorators. For now, I have removed the assert until we find a more permanent solution.

[cdce8p]

I'm not sure yet if a separate instance would provide much benefit. Let's leave this as is then until I had time to look at #1306 and we decided how to proceed.

[cdce8p]

I'm not sure yet if a separate instance would provide much benefit. Let's leave this as is then until I had time to look at #1306 and we decided how to proceed.

[cdce8p]

Looks good! One last comment, can be merged afterwards IMO.
Next up pylint-dev/pylint#5221