-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[mypyc] Transform object.__new__ inside __new__ #19866
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
Conversation
you can add ie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! A few more minor things.
mypyc/irbuild/statement.py
Outdated
if isinstance(lvalue, (TupleExpr, ListExpr)): | ||
lvalues += lvalue.items | ||
else: | ||
lvalues.append(lvalue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be a nested expression, e.g. [a, [b, c]] = ...
, so this needs to be recursive.
mypyc/irbuild/statement.py
Outdated
# Disallowed because it could break the transformation of object.__new__ calls | ||
# inside __new__ methods. | ||
builder.error( | ||
f"Assignment to argument {cls_arg} in __new__ method unsupported", stmt.line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use double quotes around Python names for consistency, i.e. '... "{cls_arg}" in "__new__" method ...'
.
not_raised = RaisesException(1) | ||
assert not_raised.val == 1 | ||
|
||
with assertRaises(TypeError, "object.__new__(str) is not safe, use str.__new__()"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we test anywhere that happens if there is str.__new__(cls)
or str.__new__(str)
or str.__new__(int)
? Add run tests (or irbuild if they generate compile errors) for these if they don't exist (both within __new__
and outside).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, added the tests. turned out that we still need to check if translate_object_new
was called for object.__new__()
specifically because str.__new__()
also triggered the transformation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good now!
#19739 introduced support for compiling
__new__
methods in native classes. Native classes differ internally from regular python types which results inTypeError
s being raised whenobject.__new__(cls)
is called whencls
is a native class.To avoid making this call,
super().__new__(cls)
is transformed into a call to an internal setup function. I forgot to replicate this for calls to equivalentobject.__new__(cls)
calls so this PR fixes that. This introduced a regression because before my changes,__new__
methods withobject.__new__(cls)
were effectively ignored at runtime, and after my changes they started raisingTypeError
s.Note that these calls are left as-is outside of
__new__
methods so it's still possible to trigger theTypeError
but that is not a regression as this was the case before. For example this code:results in
TypeError: object.__new__(Test) is not safe, use Test.__new__()
. This differs from interpreted python but the error message is actually correct in that usingTest.__new__(Test)
instead works.