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

[mypyc] Add LoadAddress primitive op for PySet_Type & PyFrozenSet_Type #13057

Merged
merged 2 commits into from
Jul 3, 2022

Conversation

ichard26
Copy link
Collaborator

@ichard26 ichard26 commented Jul 2, 2022

Description

This also fixes mypyc/mypyc#917.

RE above, the root issue is that mypyc didn't know builtins.set was a built-in name, so it guessed it comes from the module globals. This didn't blow up anything up somehow... until the dataclasses commit (1bcfc04) which made the __annotations__ logic for dataclasses try to better preserve the type annotations (previously they would be erased to builtins.type). This new logic would use load_type to load builtins.set (so it can be put in __annotations__) which went poorly as only types registered with load_address_op are considered built-ins.

Test Plan

I added a run test sub-case that tests a generic built-in type (i.e. set) is stored correctly in the dataclass' __annotations__.

@ichard26 ichard26 force-pushed the add-set-loadaddress-op branch from bfc6715 to 7811f08 Compare July 3, 2022 19:56
@ichard26 ichard26 changed the title [mypyc] Add LoadAddress primitive op for PySet_Set [mypyc] Add LoadAddress primitive op for PySet_Type & PyFrozenSet_Type Jul 3, 2022
This also fixes mypyc/mypyc#917

RE above, the root issue is that mypyc didn't know builtins.set was a
built-in name, so it guessed it comes from the module globals. This
didn't blow up anything up somehow... until the dataclasses
commit[^1] which made the `__annotations__` logic for dataclasses try to
better preserve the type annotations (previously they would be erased
to builtins.type). This new logic would use `load_type` to load
`builtins.set` (so it can be put in `__annotations__`) which went poorly
as only types registered with `load_address_op` are considered
built-ins.

[^1]: python@1bcfc04
@ichard26 ichard26 force-pushed the add-set-loadaddress-op branch from 7811f08 to e6882c3 Compare July 3, 2022 20:15
It's good enough and I can't be bothered to work support in for
AbstractSet in the full test suite.
@JelleZijlstra JelleZijlstra merged commit 86aefb1 into python:master Jul 3, 2022
@ichard26 ichard26 deleted the add-set-loadaddress-op branch July 3, 2022 21:26
Gobot1234 pushed a commit to Gobot1234/mypy that referenced this pull request Aug 12, 2022
python#13057)

This also fixes mypyc/mypyc#917

RE above, the root issue is that mypyc didn't know builtins.set was a
built-in name, so it guessed it comes from the module globals. This
didn't blow up anything up somehow... until the dataclasses
commit[^1] which made the `__annotations__` logic for dataclasses try to
better preserve the type annotations (previously they would be erased
to builtins.type). This new logic would use `load_type` to load
`builtins.set` (so it can be put in `__annotations__`) which went poorly
as only types registered with `load_address_op` are considered
built-ins.

[^1]: python@1bcfc04
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.

[Regression] field(default_factory=set) is miscompiled
2 participants