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.

Sign up for GitHub

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

Jump to bottom

[mypyc] Fix direct __dict__ access on inner functions in new Python #16084

Merged
merged 3 commits into from
Oct 15, 2023
Merged

[mypyc] Fix direct __dict__ access on inner functions in new Python #16084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca0f5ea
[mypyc] Fix direct __dict__ access on inner functions in new Python
hauntsaninja Sep 10, 2023
0fb92e5
fix tests
hauntsaninja Sep 10, 2023
5933273
Merge branch 'master' into mypyc-fix-nested
hauntsaninja Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion mypyc/codegen/emitclass.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ def generate_class(cl: ClassIR, module: str, emitter: Emitter) -> None:
fields["tp_name"] = f'"{name}"'

generate_full = not cl.is_trait and not cl.builtin_base
needs_getseters = cl.needs_getseters or not cl.is_generated
needs_getseters = cl.needs_getseters or not cl.is_generated or cl.has_dict

if not cl.builtin_base:
fields["tp_new"] = new_name
Expand Down Expand Up @@ -886,6 +886,9 @@ def generate_getseters_table(cl: ClassIR, name: str, emitter: Emitter) -> None:
else:
emitter.emit_line("NULL, NULL, NULL},")

if cl.has_dict:
emitter.emit_line('{"__dict__", PyObject_GenericGetDict, PyObject_GenericSetDict},')

emitter.emit_line("{NULL} /* Sentinel */")
emitter.emit_line("};")

Expand Down
30 changes: 30 additions & 0 deletions mypyc/test-data/run-functions.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1256,3 +1256,33 @@ def foo(**kwargs: Unpack[Person]) -> None:
foo(name='Jennifer', age=38)
[out]
Jennifer

[case testNestedFunctionDunderDict312]
import sys

def foo() -> None:
def inner() -> str: return "bar"
print(inner.__dict__) # type: ignore[attr-defined]
Copy link
Member

Choose a reason for hiding this comment

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

There's a lot of type ignores in this test, do we really need that?

Copy link
Collaborator Author

@hauntsaninja hauntsaninja Sep 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yes, fixtures, also mypy genuinely won't understand inner.x

inner.__dict__.update({"x": 1}) # type: ignore[attr-defined]
print(inner.__dict__) # type: ignore[attr-defined]
print(inner.x) # type: ignore[attr-defined]

if sys.version_info >= (3, 12): # type: ignore
foo()
[out]
[out version>=3.12]
{}
{'x': 1}
1

[case testFunctoolsUpdateWrapper]
import functools

def bar() -> None:
def inner() -> str: return "bar"
functools.update_wrapper(inner, bar) # type: ignore
print(inner.__dict__) # type: ignore
Comment on lines +1265 to +1284
Copy link
Member

Choose a reason for hiding this comment

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

Here's a hacky way of getting rid of a bunch of type: ignores in the test file. The tests all still pass for me locally with this change (on py311 and py312):

Suggested change
print(inner.__dict__) # type: ignore[attr-defined]
inner.__dict__.update({"x": 1}) # type: ignore[attr-defined]
print(inner.__dict__) # type: ignore[attr-defined]
print(inner.x) # type: ignore[attr-defined]
if sys.version_info >= (3, 12): # type: ignore
foo()
[out]
[out version>=3.12]
{}
{'x': 1}
1
[case testFunctoolsUpdateWrapper]
import functools
def bar() -> None:
def inner() -> str: return "bar"
functools.update_wrapper(inner, bar) # type: ignore
print(inner.__dict__) # type: ignore
print(getattr(inner, "__dict__"))
getattr(inner, "__dict__").update({"x": 1})
print(getattr(inner, "__dict__"))
print(getattr(inner, "x"))
if sys.version_info >= (3, 12): # type: ignore
foo()
[out]
[out version>=3.12]
{}
{'x': 1}
1
[case testFunctoolsUpdateWrapper]
import functools
def bar() -> None:
def inner() -> str: return "bar"
functools.update_wrapper(inner, bar) # type: ignore
print(getattr(inner, "__dict__"))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's okay as is, I kinda feel obfuscation here makes it less clear to the reader what is being tested

Copy link
Member

Choose a reason for hiding this comment

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

yeah i didn't really like this either, was just throwing it out there as an option :)


bar()
[out]
{'__module__': 'native', '__name__': 'bar', '__qualname__': 'bar', '__doc__': None, '__wrapped__': <built-in function bar>}