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

[Regression] field(default_factory=set) is miscompiled #917

Closed
JelleZijlstra opened this issue Jan 10, 2022 · 2 comments · Fixed by python/mypy#13057
Closed

[Regression] field(default_factory=set) is miscompiled #917

JelleZijlstra opened this issue Jan 10, 2022 · 2 comments · Fixed by python/mypy#13057

Comments

@JelleZijlstra
Copy link
Collaborator

Given this code:

from dataclasses import dataclass, field
from typing import Set


@dataclass
class X:
    s: Set[str] = field(default_factory=set)

Compiling with mypyc setfield.py on 0.931 and then running import setfield fails with:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "setfield.py", line 7, in <module>
    s: Set[str] = field(default_factory=set)
KeyError: 'set'

On 0.920 it works as expected.

Inspecting the C and the IR (thanks @ichard26) shows that we're incorrectly looking up set in the module globals instead of the builtins.

L20:
    r59 = load_address PyType_Type
    r60 = test.globals :: static
    r61 = 'set'
    r62 = CPyDict_GetItem(r60, r61)
    if is_error(r62) goto L36 (error at <module>:7) else goto L21
@ichard26
Copy link
Collaborator

Bisected to python/mypy@1bcfc04 by the way.

@hauntsaninja
Copy link
Collaborator

cc @chadrik if you have time to take a look :-)

ichard26 added a commit to ichard26/mypy that referenced this issue Jul 2, 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).

[^1]: python@1bcfc04
ichard26 added a commit to ichard26/mypy that referenced this issue 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__`) whick went poorly
as only types registered with `load_address_op` are considered
built-ins.

[^1]: python@1bcfc04
ichard26 added a commit to ichard26/mypy that referenced this issue 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
JelleZijlstra pushed a commit to python/mypy that referenced this issue Jul 3, 2022
#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]: 1bcfc04
Gobot1234 pushed a commit to Gobot1234/mypy that referenced this issue 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 a pull request may close this issue.

3 participants