Skip to content

bpo-44232: Fix type_new() error reporting #26359

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

Merged
vstinner merged 1 commit into from
May 25, 2021
Merged

bpo-44232: Fix type_new() error reporting #26359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions Lib/test/test_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1334,6 +1334,24 @@ class N(type, metaclass=M):
N(5)
self.assertEqual(str(cm.exception), expected_message)

def test_metaclass_new_error(self):
# bpo-44232: The C function type_new() must properly report the
# exception when a metaclass constructor raises an exception and the
# winner class is not the metaclass.
class ModelBase(type):
def __new__(cls, name, bases, attrs):
super_new = super().__new__
new_class = super_new(cls, name, bases, {})
if name != "Model":
raise RuntimeWarning(f"{name=}")
return new_class

class Model(metaclass=ModelBase):
pass

with self.assertRaises(RuntimeWarning):
type("SouthPonies", (Model,), {})


class SimpleNamespaceTests(unittest.TestCase):

Expand Down
4 changes: 4 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2021-05-25-18-20-10.bpo-44232.DMcCCf.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Fix a regression in :func:`type` when a metaclass raises an exception. The C
function :c:func:`type_new` must properly report the exception when a metaclass
constructor raises an exception and the winner class is not the metaclass.

@Fidget-Spinner Fidget-Spinner May 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure if it's worth mentioning in the blurb that this bug only affected people using debug builds. Release build users shouldnt have noticed anything.

@pablogsal pablogsal May 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should mention it as people use debug build outside Cpython development quite a lot

@vstinner vstinner May 25, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't expect anyone to use a Python debug build until I saw https://bugs.python.org/issue44232

IMO it's worth it to document the bugfix since it impacts Django, but likely many more projects.

@Fidget-Spinner Fidget-Spinner May 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wow I need to stop sending messages late at night ;-). I'm really sorry this concern got lost in translation. I wasn't suggesting we get rid of the blurb. I was suggesting we add a line somewhere that says this only affects debug builds and not release ones. Previously I did that for another AssertionError crash in 3.9 - 3.10 because I was afraid people would read the blurb and think their Python is affected.

6a7fb9d

Anyways, I think it's fine if you dont want to. Thanks for fixing this :).

@vstinner vstinner May 26, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah ok. Well, the change is now merged, I'm fine with the merged NEWS entry :-)

Patch by Victor Stinner.
4 changes: 4 additions & 0 deletions Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3256,6 +3256,9 @@ type_new_get_bases(type_new_ctx *ctx, PyObject **type)
if (winner->tp_new != type_new) {
/* Pass it to the winner */
*type = winner->tp_new(winner, ctx->args, ctx->kwds);
if (*type == NULL) {
return -1;
}
return 1;
Comment on lines 3259 to 3262

@pablogsal pablogsal May 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Small nit, feel free to ignore

Suggested change
if (*type == NULL) {
return -1;
}
return 1;
return *type == NULL ? -1 : 1;

@vstinner vstinner May 25, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I prefer to "handle errors explicitly". Well, it's just a personal coding style preference.

}

Expand Down Expand Up @@ -3307,6 +3310,7 @@ type_new(PyTypeObject *metatype, PyObject *args, PyObject *kwds)
PyObject *type = NULL;
int res = type_new_get_bases(&ctx, &type);
if (res < 0) {
assert(PyErr_Occurred());
return NULL;
}
if (res == 1) {
Expand Down