Skip to content

dataclasses.Field.default and .default_factory can be _MISSING_TYPE - #5900

Closed
altendky wants to merge 3 commits into
python:masterfrom
altendky:patch-3
Closed

altendky wants to merge 3 commits into
python:masterfrom
altendky:patch-3

Conversation

@altendky

@altendky altendky commented Aug 9, 2021

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions

This comment has been minimized.

@srittau srittau left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but please use the new union syntax (i.e. Foo | Bar).

Comment thread stdlib/dataclasses.pyi Outdated
@hauntsaninja

hauntsaninja commented Aug 9, 2021 •

Copy link
Copy Markdown
Collaborator

I considered doing this earlier, but didn't because I think this will be slightly annoying, since the idiomatic way to narrow is if x is dataclasses.MISSING, which type checkers will not understand. mypy primer has an example, you can see that this re-introduces a type error in pydantic (that was previously fixed by #5718).

There probably isn't a good solution here, but just wanted to call it out.

@JelleZijlstra

Copy link
Copy Markdown
Member

Yes, I had the same concern as Shantanu. I think PEP 661 (https://www.python.org/dev/peps/pep-0661/) would provide a cleaner solution.

@srittau

srittau commented Aug 9, 2021

Copy link
Copy Markdown
Collaborator

Maybe @altendky has a use case that requires this change? We can always change over to PEP 661 when that gets accepted and is available for use. That said, I have no opinion, what solution is best.

@Akuli

Akuli commented Aug 9, 2021 •

Copy link
Copy Markdown
Collaborator

We could define _MISSING_TYPE as an enum containing a single member, named _MISSING_TYPE.MISSING for example, and then use Literal[_MISSING_TYPE.MISSING] instead of _MISSING_TYPE. Mypy seems to support is for enums, and unlike most other things, enums can go inside a Literal.

Edit: PEP 661 mentions this as a rejected idea, but unlike most of their other rejected ideas, it actually works and it was apparently rejected only because nobody likes it.

@altendky

altendky commented Aug 9, 2021

Copy link
Copy Markdown
Contributor Author

Welp, of course the time I don't chat about a change first it ends up deserving discussion. :] Here's the code that made me take a look at this.

https://github.com/python-desert/desert/blob/483734f89528fef86c0f4bbd38b4ba3473acae9d/src/desert/_make.py#L324

        if field.default_factory != dataclasses.MISSING:  # type: ignore[misc]
error: Non-overlapping equality check (left operand type: "Callable[[], object]", right operand type: "_MISSING_TYPE")  [comparison-overlap]

Here's a minimal example. Note that the first error seems to be python/mypy#10750 and I was already avoiding it with # type: ignore[misc].

https://mypy-play.net/?mypy=latest&python=3.10&flags=strict&gist=f66bb887158648e99b97c042165dd684

import dataclasses


@dataclasses.dataclass
class C:
    f: int = 0


for field in dataclasses.fields(C):
    if field.default_factory != dataclasses.MISSING:
        pass
main.py:10: error: Attribute function "default_factory" with type "Callable[[], _T]" does not accept self argument
main.py:10: error: Non-overlapping equality check (left operand type: "Callable[[], Any]", right operand type: "_MISSING_TYPE")
Found 2 errors in 1 file (checked 1 source file)

I just tried is and is not and they both seemed to result in the same error.

Regardless, I'll be adding an ignore "for now" since of course this wouldn't be released anyways. If there's another more proper solution and this isn't a useful interrim change, I can handle that.

Thanks to all for the interest and effort here.

@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pydantic (https://github.com/samuelcolvin/pydantic.git)
- pydantic/dataclasses.py:174: error: unused "type: ignore" comment

@srittau srittau left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me, but I'll defer judgement to @hauntsaninja and @JelleZijlstra.

@JelleZijlstra

Copy link
Copy Markdown
Member

I'd prefer to stick with the current stub until PEP 661 gives us better tools to deal with this situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants