bpo-34284: Nonsensical exception message when calling `new` on non-instantiable objects #8576

VadimPushtaev · 2018-07-30T23:41:10Z

https://bugs.python.org/issue34284

pppery · 2018-07-30T23:44:11Z

Please don't combine patches for two issues in one PR

VadimPushtaev · 2018-07-30T23:52:12Z

Please don't combine patches for two issues in one PR

That was unintentional. Fixed.

pppery · 2018-07-30T23:45:45Z

Objects/typeobject.c

@@ -5885,12 +5885,24 @@ tp_new_wrapper(PyObject *self, PyObject *args, PyObject *kwds)
        staticbase = staticbase->tp_base;
    /* If staticbase is NULL now, it is a really weird type.
       In the spirit of backwards compatibility (?), just shut up. */
+    char extra[10] = "normal";


This seems to be dead code

pppery · 2018-07-30T23:56:50Z

Also, I disagree with this method of fixing it, as I explained on the BPO issue

eric-wieser · 2018-08-02T06:54:35Z

Lib/test/test_types.py

+            object.__new__(type(sys.flags))
+        message = 'object.__new__(sys.flags) is not safe'
+        self.assertEqual(str(cm.exception), message)
+


Can you add a test for tuple.__new__(type(sys.flags))?

That's easy, but I'm still not sure what is the right behavior here. I need time to read through bpo-31506 to comprehend the problem.

pppery · 2018-08-05T01:59:54Z

Discussion on the BPO issue seems to be suggesting a different method of fixing

pppery · 2018-08-08T18:42:41Z

I think that this patch makes the "if(type->tp_new == null)" check in type_call unnecessary

pppery · 2018-08-08T20:53:40Z

This also seems like it won't fix the bug for type(sys.flags), which has its own problem

VadimPushtaev · 2018-08-08T22:15:57Z

Thanks for your comments.

This also seems like it won't fix the bug for type(sys.flags), which has its own problem

Exactly, there is more than one reason why tp_new can be null. That's why we can't remove if(type->tp_new == null) yet.

I know that there is still work to do. I need to write some tests, check all places where tp_new is compared to null (there are numerous of them), repair sys.flags (and similar objects).

Now I just want to now that I'm following the right path.

pppery · 2018-08-11T03:48:17Z

Misc/NEWS.d/next/Core and Builtins/2018-08-08-20-40-46.bpo-34284.jFqKLF.rst

@@ -0,0 +1 @@
+NullNew for old extension types instead of just null


This NEWS entry could be worded clearer

VadimPushtaev · 2018-08-12T22:03:46Z

More changes:

News reworded.
sys.flags is fixed, as well as other tuples of sys
Bunch of necessary tests

VadimPushtaev · 2018-08-12T22:08:56Z

I'm also not sure what to do with all that tp_new == NULL. We can't guarantee there are no more types that are written the same way as sys.flags. Can we just fix what is initially reported in the issue and leave the rest alone?

pppery

The bug I initially reported would continue to appear for all types whose tp_new == NULL, which is why I suggested removing checks for that case: better to fail louder then let a cryptic error-handling bug go unnoticed. (Although I don't expect such a change to happen) Yes, this change could just happen, but that will still leave the potential for buggy types and make the code (in my opinion -- I expect to be disagreed with here) less clean. Also, the test you added testing sys.flags should probably also test the other types you fixed and live in test_sys

VadimPushtaev · 2018-09-03T13:18:07Z

Also, the test you added testing sys.flags should probably also test the other types you fixed and live in test_sys

Less ambitious tests for this were already there, so it makes perfect sense. Fixed.

VadimPushtaev · 2018-09-03T16:06:48Z

_curses_panel.panel aka curses.panel.panel is fixed as well.

VadimPushtaev · 2018-09-05T18:24:24Z

_tkinter fixed.

VadimPushtaev · 2018-09-05T18:47:30Z

Now we can remove that tp_new == NULL check. It can be done fairly simple:

--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -912,12 +912,10 @@ type_call(PyTypeObject *type, PyObject *args, PyObject *kwds)
 {
     PyObject *obj;

-    if (type->tp_new == NULL) {
-        PyErr_Format(PyExc_TypeError,
-                     "cannot create '%.100s' instances",
-                     type->tp_name);
-        return NULL;
-    }
+    /* tp_new cannot be NULL,
+       if you want to prevent class instantiation
+       use PyType_NullNew as tp_new instead */
+    assert(type->tp_new != NULL);

The Python test suite still passes with that change.

However, in that case, we should update docs as well. The current behavior is documented at least here:

What about custom user types? Is it OK to break C API between major releases? Sholdn't we depreceate it first?

I need advice.

pppery

Also, the title of this PR should probably be changed

Lib/tkinter/test/test_tkinter/test_internals.py

asvetlov

The PR adds a new public PyType_NullNew API.
I doubt if we need it, the problem can be solved by with static private functions

bedevere-bot · 2018-12-05T08:44:22Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

VadimPushtaev · 2018-12-05T10:51:47Z

@asvetlov, thanks for the review.

First, I'm still looking for an answer for #8576 (comment). Should we allow type->tp_new == NULL?

Second, if we don't expose PyType_NullNew as a public function, we don't allow third-party libraries to make classes with the undefined __new__. Is it OK?

asvetlov · 2018-12-05T12:27:48Z

Allowing tp_new == NULL is also a big change.
I don't say the change is not acceptable but we need a strong motivation for introducing it.

From my understanding all you need is:

static PyObject *
_curses_panel_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
{
    PyErr_string(PyExc_TypeError,
                 "'curses.panel.panel() is forbidden, please use curses.panel.new_panel(win)");
    return NULL;
}

The error text can be discussed and tuned but you've got my idea.

pppery · 2018-12-05T12:32:21Z

@asvetlov tp_new == NULL is what is currently allowed. This patch is proposing not allowing it

Why should trying to instantiate _curses_panel.panel produce a different error message than trying to instantiate any other currently tp_new == NULL type, like generator for example?

Not providing a PyType_NullNew function would have the effect of requiring each module that wants to create a heaptype that can't be instantiated by calling it to duplicate its code, a task that seems to have no benefit.

asvetlov · 2018-12-05T13:10:49Z

Sorry, I have to re-read the thread to get restore the PR context first.

VadimPushtaev requested a review from ericvsmith as a code owner July 30, 2018 23:41

the-knights-who-say-ni added the CLA signed label Jul 30, 2018

bedevere-bot added the awaiting review label Jul 30, 2018

VadimPushtaev force-pushed the type_error branch from 20180ab to 9125689 Compare July 30, 2018 23:48

pppery reviewed Jul 30, 2018

View reviewed changes

eric-wieser reviewed Aug 2, 2018

View reviewed changes

bpo-34284: NullNew for old extension types

06f3144

VadimPushtaev force-pushed the type_error branch from 9955e4d to 06f3144 Compare August 8, 2018 18:09

pppery reviewed Aug 11, 2018

View reviewed changes

VadimPushtaev force-pushed the type_error branch from c86689b to 2460e02 Compare August 12, 2018 22:02

bpo-34284: NullNew for tuples of sys

a64abba

VadimPushtaev force-pushed the type_error branch from 2460e02 to a64abba Compare August 12, 2018 22:18

pppery reviewed Aug 12, 2018

View reviewed changes

bpo-34284: Proper tests for tuples of sys

d018573

VadimPushtaev force-pushed the type_error branch from fc8857a to d018573 Compare September 3, 2018 13:54

bpo-34284: Null __new__ for curses.panel.panel

96cefbf

pppery suggested changes Sep 5, 2018

View reviewed changes

Lib/tkinter/test/test_tkinter/test_internals.py Outdated Show resolved Hide resolved

bedevere-bot added awaiting core review and removed awaiting review labels Sep 5, 2018

bpo-34284: Null __new__ for _tkinter

2302b69

VadimPushtaev force-pushed the type_error branch from f71475d to 2302b69 Compare September 5, 2018 22:14

VadimPushtaev changed the title ~~bpo-34284: Nonsensical exception message when calling __new__ on some sys objects~~ bpo-34284: Nonsensical exception message when calling __new__ on objects with tp_new == NULL Sep 5, 2018

VadimPushtaev changed the title ~~bpo-34284: Nonsensical exception message when calling __new__ on objects with tp_new == NULL~~ bpo-34284: Nonsensical exception message when calling __new__ on non-instantiable objects Sep 6, 2018

pppery approved these changes Dec 5, 2018

View reviewed changes

asvetlov requested changes Dec 5, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Dec 5, 2018

VadimPushtaev closed this Jan 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-34284: Nonsensical exception message when calling `new` on non-instantiable objects #8576

bpo-34284: Nonsensical exception message when calling `new` on non-instantiable objects #8576

VadimPushtaev commented Jul 30, 2018 •

edited

Loading

pppery commented Jul 30, 2018

VadimPushtaev commented Jul 30, 2018 •

edited

Loading

pppery Jul 30, 2018

pppery commented Jul 30, 2018

eric-wieser Aug 2, 2018

VadimPushtaev Aug 2, 2018 •

edited

Loading

pppery commented Aug 5, 2018

pppery commented Aug 8, 2018

pppery commented Aug 8, 2018

VadimPushtaev commented Aug 8, 2018

pppery Aug 11, 2018

VadimPushtaev commented Aug 12, 2018

VadimPushtaev commented Aug 12, 2018

pppery left a comment •

edited

Loading

VadimPushtaev commented Sep 3, 2018

VadimPushtaev commented Sep 3, 2018

VadimPushtaev commented Sep 5, 2018

VadimPushtaev commented Sep 5, 2018

pppery left a comment

asvetlov left a comment

bedevere-bot commented Dec 5, 2018

VadimPushtaev commented Dec 5, 2018

asvetlov commented Dec 5, 2018

pppery commented Dec 5, 2018

asvetlov commented Dec 5, 2018

		@@ -0,0 +1 @@
		NullNew for old extension types instead of just null

bpo-34284: Nonsensical exception message when calling __new__ on non-instantiable objects #8576

bpo-34284: Nonsensical exception message when calling __new__ on non-instantiable objects #8576

Conversation

VadimPushtaev commented Jul 30, 2018 • edited Loading

pppery commented Jul 30, 2018

VadimPushtaev commented Jul 30, 2018 • edited Loading

pppery Jul 30, 2018

Choose a reason for hiding this comment

pppery commented Jul 30, 2018

eric-wieser Aug 2, 2018

Choose a reason for hiding this comment

VadimPushtaev Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

pppery commented Aug 5, 2018

pppery commented Aug 8, 2018

pppery commented Aug 8, 2018

VadimPushtaev commented Aug 8, 2018

pppery Aug 11, 2018

Choose a reason for hiding this comment

VadimPushtaev commented Aug 12, 2018

VadimPushtaev commented Aug 12, 2018

pppery left a comment • edited Loading

Choose a reason for hiding this comment

VadimPushtaev commented Sep 3, 2018

VadimPushtaev commented Sep 3, 2018

VadimPushtaev commented Sep 5, 2018

VadimPushtaev commented Sep 5, 2018

pppery left a comment

Choose a reason for hiding this comment

asvetlov left a comment

Choose a reason for hiding this comment

bedevere-bot commented Dec 5, 2018

VadimPushtaev commented Dec 5, 2018

asvetlov commented Dec 5, 2018

pppery commented Dec 5, 2018

asvetlov commented Dec 5, 2018

bpo-34284: Nonsensical exception message when calling `new` on non-instantiable objects #8576

bpo-34284: Nonsensical exception message when calling `new` on non-instantiable objects #8576

VadimPushtaev commented Jul 30, 2018 •

edited

Loading

VadimPushtaev commented Jul 30, 2018 •

edited

Loading

VadimPushtaev Aug 2, 2018 •

edited

Loading

pppery left a comment •

edited

Loading