Skip to content
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

__total__ is assigned True for types with optional fields #113

Closed
rehno-lindeque opened this issue Oct 7, 2021 · 7 comments
Closed

__total__ is assigned True for types with optional fields #113

rehno-lindeque opened this issue Oct 7, 2021 · 7 comments
Labels
🐞 bug Something isn't working 👋 response needed Awaiting response from a reporter

Comments

@rehno-lindeque
Copy link

Describe the bug
It appears that __total__ is assigned True in cases where the TypedDict should not be total. This ends up confusing tools like typeguard which uses it for totality checking

(E.g. see https://github.com/agronholm/typeguard/blob/3f12d1fadb18aa34de6334010e833825b506164d/src/typeguard/__init__.py#L332-L353)

To Reproduce

>>> import mypy_boto3_ec2.type_defs as type_defs
>>> ec2types.SpotFleetRequestConfigDataTypeDef.__total__
True
>>> import inspect
>>> print(''.join(inspect.getsourcelines(type_defs.SpotFleetRequestConfigDataTypeDef)[0]))
class SpotFleetRequestConfigDataTypeDef(
    _RequiredSpotFleetRequestConfigDataTypeDef, _OptionalSpotFleetRequestConfigDataTypeDef
):
    pass

Additional context

$ python -VV
Python 3.8.11 (default, Jun 28 2021, 10:57:31) 
[GCC 10.3.0]
@rehno-lindeque rehno-lindeque added the 🐞 bug Something isn't working label Oct 7, 2021
@vemel
Copy link
Collaborator

vemel commented Oct 7, 2021

I see why. This is because of the way I build TypedDicts with mixed totality. As you see, this TypedDict has both optional and required fields: https://github.com/vemel/boto3_stubs_docs/blob/e4d1c97abceff6fc3e1bee0428d5f56d5581eee9/mypy_boto3_ec2/type_defs.md#spotfleetrequestconfigdatatypedef . So, totality for SpotFleetRequestConfigDataTypeDef is still True because of inheritance from _RequiredSpotFleetRequestConfigDataTypeDef

You can check if TypedDict has optional keys instead:

if SpotFleetRequestConfigDataTypeDef.__optional_keys__:
   print("Some fields are optional")
if not SpotFleetRequestConfigDataTypeDef.__required_keys__:
   print("All fields are optional")

Let me if this works for you

@vemel vemel added the 👋 response needed Awaiting response from a reporter label Oct 7, 2021
@rehno-lindeque
Copy link
Author

Thanks so much for the really fast response!

Unfortunately typeguard is a downstream dependency for me so it wouldn't be easy.

However, I did notice from mypy documentation that it's possible to override total in inherited classes when required and non-required items are mixed:

from mypy_boto3_ec2 import type_defs
from typing import TypedDict

print('RunInstancesRequestRequestTypeDef', type_defs.RunInstancesRequestRequestTypeDef.__total__)

class X(type_defs.RunInstancesRequestRequestTypeDef, total=False):
    pass

print('X', X.__total__)

class Base(TypedDict):
    pass

class Y(Base, type_defs.RunInstancesRequestRequestTypeDef):
    pass

print('Y', Y.__total__)

class Z(Base, type_defs.RunInstancesRequestRequestTypeDef, total=False):
    pass

print('Z', Z.__total__)

Results:

RunInstancesRequestRequestTypeDef True
X False
Y True
Z False

Could that be a way forward?

(It doesn't work as a quick work-around on my side unfortunately because of fields referencing other TypedDict types)

@rehno-lindeque
Copy link
Author

rehno-lindeque commented Oct 8, 2021

Note that typeguard does actually check for __required_keys__ with a comment that seems to imply that maybe it's present in python >= 3.9. However, I'm not sure where this comes from (perhaps it's related to PEP 655 ?):

https://github.com/agronholm/typeguard/blob/3f12d1fadb18aa34de6334010e833825b506164d/src/typeguard/__init__.py#L334-L335

Never-the-less, my feeling is that the transitivity of total with inheritance is somewhat broken. (PEP 655 being the corrected semantics)

@rehno-lindeque
Copy link
Author

(It would also be difficult to require python 3.9 for this particular project I'm afraid - I'm just a contributor to it.)

@vemel
Copy link
Collaborator

vemel commented Oct 8, 2021

You can use TypedDict from https://pypi.org/project/typing-extensions/ , it is the same as in Python 3.9+, so __required_keys__ should be accessible.

@rehno-lindeque
Copy link
Author

rehno-lindeque commented Oct 12, 2021

Oh, I see!

Would it make sense to change the headers so that typing is imported (in place of typing_extensions) only when sys.version_info >= (3, 9)? At the moment it appears to be sys.version_info >= (3, 8)

E.g. in
https://github.com/vemel/mypy_boto3_builder/blob/bc763ba83b78c798d2984e9e3fbcec3814045ab2/mypy_boto3_builder/boto3_stubs_static/resources/model.pyi#L7-L10

I'm not sure how else to swap out typing.TypedDict for typing_extensions.TypedDict:

>>> from typing_extensions import TypedDict
class X(TypedDict): pass
>>> hasattr(X, '__required_keys__')
True

>>> import typing
>>> class Y(typing.TypedDict): pass
>>> hasattr(Y, '__required_keys__')
False

>>> import mypy_boto3_ec2.type_defs as type_defs
>>> hasattr(type_defs.SpotFleetRequestConfigDataTypeDef, '__required_keys__')
False

Thanks again!

@rehno-lindeque
Copy link
Author

I realized that ideally I really would want to have available both total and non-total versions of the dictionaries for my use case, but then the only elegant solution seems to be python/mypy#4128 which mypy can't support right now.

(So perhaps this issue ought to be closed for now.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 👋 response needed Awaiting response from a reporter
Projects
None yet
Development

No branches or pull requests

2 participants