Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

altendky
Copy link
Contributor

@altendky altendky commented Aug 9, 2021

No description provided.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@srittau srittau left a comment

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).

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Aug 9, 2021

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
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
Copy link
Collaborator

srittau commented Aug 9, 2021

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
Copy link
Collaborator

Akuli commented Aug 9, 2021

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
Copy link
Contributor Author

altendky commented Aug 9, 2021

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
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

Copy link
Collaborator

@srittau srittau left a comment

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
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