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] Fix direct __dict__ access on inner functions in new Python #16084

Merged
merged 3 commits into from
Oct 15, 2023

Conversation

hauntsaninja
Copy link
Collaborator

Fixes #16077

@AlexWaygood
Copy link
Member

Nice! I can confirm that, with this patch, both snippets I pasted in #16077 work for me locally on Python 3.12 once they've been mypycified.

@AlexWaygood
Copy link
Member

Hmm, looks like the new test passes with py312 but not with any other Python version?

@hauntsaninja
Copy link
Collaborator Author

Hmm maybe we can just drop support for all these legacy Python versions?

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Sep 10, 2023

I opted for a simple "fix" for older Python versions, which is to test less. But I think we can make the .__dict__ access actually work with this PR if we just remove __dict__ from tp_members. All tests pass with that change, but it's possibly incorrect.

@hauntsaninja hauntsaninja marked this pull request as ready for review September 10, 2023 23:26

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

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

Comment on lines +1265 to +1284
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
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 :)

@ichard26 ichard26 self-requested a review September 11, 2023 14:40
@hauntsaninja hauntsaninja merged commit 940fceb into python:master Oct 15, 2023
12 checks passed
@hauntsaninja hauntsaninja deleted the mypyc-fix-nested branch October 15, 2023 19:12
@hauntsaninja
Copy link
Collaborator Author

Merging since this has been open for a bit and unblocks Python 3.12 mypyc in black, let me know if this change is actually incorrect!

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.

functools.update_wrapper and functools.wraps cannot be used in mypycified files on py312
3 participants