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

Use disallow instantiation flag for classes without the __new__ method #4568

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ahlinc
Copy link
Contributor

@ahlinc ahlinc commented Sep 19, 2024

Starting from Py 3.10 it's possible to prohibit instantiation by using Py_TPFLAGS_DISALLOW_INSTANTIATION flag instead of using a stub method.
From the Python side it makes it clear if there is no __new__ attr on a class then it can't be instantiated.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks for this and sorry for the very slow review.

Overall I think this is a welcome simplification however the treat needs to be adjusted further to deal with the new variety among versions.

assert exc_info.value.args == (
"No constructor defined for ClassWithoutConstructor",
)
assert exc_info.value.args == (exc_message,)
Copy link
Member

Choose a reason for hiding this comment

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

This test will be run on all of Python 3.7 through 3.13, so I think the parameterized test is a good start but this will need to account for the variety between versions too.

pyclasses.ClassWithoutConstructor,
"cannot create 'builtins.ClassWithoutConstructor' instances",
),
(ClassWithoutConstructor, "No constructor defined for ClassWithoutConstructor"),
Copy link
Member

Choose a reason for hiding this comment

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

This type is defined just above; we can change the message to match the expected Rust one to simplify the test. (Then there's just the differences across versions.)

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.

2 participants