Special-case enum method calls

[ilevkivskyi]

Improves mypyc/mypyc#1121, this gives a bit above 1% on mypy self-check.

This only adds support for regular and overloaded methods without decorators (class/static methods and properties stay slow). When working on this I considered (and actually tried) four options:

• Make enums extension classes, then many methods will use fast calls ~automatically (we will just need to set a final flag). This just didn't work, in the sense no segfaults, but it looks like we don't call __prepare__(), or don't call it at the right moment. Or maybe I just didn't try hard enough. In general, for some reason this feels risky.
• Use existing CPyDefs for (non-extension) enum methods, but since they have an extra argument, __mypyc_self__, we can supply NULL there, since we know it is unused. This is actually easy and it works, but IMO it is ultra-ugly, so I decided to not do it.
• Write a separate CPyDef without __mypy_self__, use it for direct calls, and make existing callable classes CPyDefs one-line functions that simply call the first one. This is possible, but quite complicated, and I am not sure it is easy to generalize (e.g. on classmethods).
• Finally, the way I do this is to simply generate a second method, that is almost a copy of the original one. This involves a bit of code duplication (in C), but the benefit is that it is conceptually simple, and easily extendable. We can cover more special cases on as-needed basis.

[ilevkivskyi]

Btw, this same idea can be applied for:

• All non-extension classes explicitly marked as final
• All methods in non-final non-extension classes that are explicitly marked as final

I will however defer these cases, unless I will find that mypy itself will benefit from these.

[ilevkivskyi]

Oh interesting, there is another 3.9 test failure because there is no Self in typing, but it causes a segfault. I can reproduce this locally. Looks like a pre-existing bug (happens on master as well). Interestingly, this happens only with non-extension classes, simple repro

[case testSegfaultOnUndefined] from mypy_extensions import mypyc_attr from typing import NoWay # type: ignore @mypyc_attr(native_class=False) class Test: def next(self) -> NoWay: return 1 

Also, segfault happens after printing the traceback. I guess we add an incref too late for the method register (which only exists for non-extension classes) or for the class object register itself, or something similar. I will look tomorrow if there is a simple fix, if no simple fix, I will ignore this, as this is not related to my PR.

[ilevkivskyi]

I think I see the bug

146 char result = CPyDef___top_level__(); 147 if (result == 2) 148 goto fail; 149 Py_DECREF(modname); 150 return 0; 151 fail: 152 Py_CLEAR(CPyModule_native_internal); 153 Py_CLEAR(modname); 154 Py_CLEAR(CPyType_Test); 155 Py_CLEAR(CPyType_next_Test_obj); 

but CPyType_Test is only initialized within __top_level__() near the end (in a place we didn't reach yet because of an import error), so we may be decrefing NULL.

[ilevkivskyi]

It looks a bit trickier than that, in gdb I see segfault occurs on this line

PyMem_Free((char *)type->tp_doc); 

now here is a punishment for not writing docstrings :-)

[ilevkivskyi]

 /* A type's tp_doc is heap allocated, unlike the tp_doc slots * of most other objects. It's okay to cast it to char *. */ 

hm LOL doesn't look that way

99 .tp_doc = "next_Test_obj()\n--\n\n", 

[ilevkivskyi]

It looks like this was broken recently in #19307 cc @brianschubert

[ilevkivskyi]

Well, adding some conditional logic looks annoying, I guess we should simply skip adding .tp_doc for non-extension classes.

[ilevkivskyi]

Oh wait, this applies to extension classes as well. Then we should always allocate the docstring on the heap. @brandtbucher I hope you can fix this, segfault on an uncaught exception is a pretty bad behavior. TBH I still don't understand what exactly is going on, like why don't we have this problem if the program exits normally (i.e. without exception)? A minimal test to reproduce:

[case testDocExplosion] class Empty: pass raise 

generates

*** Exit status: -6 munmap_chunk(): invalid pointer 

[brianschubert]

Still catching up, but would something like this do the trick?

diff --git a/mypyc/lib-rt/misc_ops.c b/mypyc/lib-rt/misc_ops.c index 3787ea553..23981f9f3 100644 --- a/mypyc/lib-rt/misc_ops.c+++ b/mypyc/lib-rt/misc_ops.c@@ -300,6 +300,19 @@ PyObject *CPyType_FromTemplate(PyObject *template, Py_XDECREF(dummy_class); + if (template_->tp_doc){+ // Silently truncate the docstring if it contains a null byte+ Py_ssize_t size = strlen(template_->tp_doc) + 1;+ char *tp_doc = (char *)PyMem_Malloc(size);+ if (tp_doc == NULL){+ PyErr_NoMemory();+ goto error;+ }++ memcpy(tp_doc, template_->tp_doc, size);+ t->ht_type.tp_doc = tp_doc;+ }+ #if PY_MINOR_VERSION == 11 // This is a hack. Python 3.11 doesn't include good public APIs to work with managed // dicts, which are the default for heap types. So we try to opt-out until Python 3.12.

Following after https://github.com/python/cpython/blob/485b16b4f7b28cefdfb524c2869d473078e349bf/Objects/typeobject.c#L4530-L4540

That seems to resolve the seg fault

[ilevkivskyi]

Yeah, that should work, I think we can simply add that chunk to CPyType_FromTemplate.

[ilevkivskyi]

(this however still doesn't explain why we don't fail without exception, this means we may have an extra incref for class objects, but this is a much smaller problem)

[sterliakov]

I also explored your options 1 and 3, with the same result for 1 and "no, doesn't look right" feeling regarding 3. Such duplication looks quite reasonable here - after all, normally enums are small and do not account for significant portion of a codebase, so this extra size should be negligible.

I'll try to look deeper into this tonight, thanks!

[brandtbucher]

@brandtbucher I hope you can fix this, segfault on an uncaught exception is a pretty bad behavior.

I'm guessing this is a misspelling of "@brianschubert"? Otherwise, sorry to disappoint, but I'm not sure how I can help with this issue specifically. :)

[ilevkivskyi]

@brandtbucher Oops, this is some serious misspelling, sorry :-)

[JukkaL]

This part of the test case isn't compiled -- is this intentional? Maybe move these to the compiled part of the test case under some test_<...> function?

[ilevkivskyi]

This was kind of intentional, as I wanted to test both compiled and interpreted calls, but I realised I only added one call in the compiled path (see definition of C.foo). I will add more calls there (to the overloaded method one etc).

[ilevkivskyi]

I added more asserts to foo() itself (note I call C().foo() manually in the driver).