Repository navigation
BUG: reject structured DTypes that contain StringDType fields - #32027
Conversation
| descr = PyDataType_SUBARRAY(descr)->base; | ||
| } | ||
| if (PyObject_IsInstance((PyObject *)descr, | ||
| (PyObject *)&PyArray_StringDType)) { |
There was a problem hiding this comment.
Thanks for the quick fix! Rather than this, can you check for it defining the "finalizer" slot? That is (in principle) more generic, even if string dtype is the only one right now.
(But maybe this is important for BytesDType :) for example).
Unfortunately people may be depending on the old, broken support. I'm not sure if closing these holes should trigger deprecation warnings?
Hmmmpf, yeah... I would prefer to just disable it and hope for the best, but can go either way.
Another thing that would make this simpler/nicer is if we also reject subarray dtype creation like this. Then we don't have to iterate bases at all here.
(Honestly, maybe you already do here, but then we don't need to iterate.)
(It's not like subarrays work, they only "work", because you can't actually attach them to an array in practice...)
seberg
added
the
62 - Python API
|
Not sure I have much to add, though in the end it would be nice to just support StringDType in structured arrays, just like we support object. |
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Thanks, for fixing this and converting to check for finalize_descr! looking I wonder if we could put this check to later to avoid duplicating, but with a helper it doesn't matter much.
Good to get this fixed!
PR summary
Currently we reject structured dtypes that contain StringDType in only one code path:
numpy/numpy/_core/src/multiarray/descriptor.c
Lines 498 to 502 in f67f65a
This rejects dtypes like
np.dtype([("a", StringDType())])but lets through dtypes like[("a", (T, 10))]or"T,i4".This PR closes those holes.
There's also a reference leak in the existing error path, which this fixes.
Open questions
Unfortunately people may be depending on the old, broken support. I'm not sure if closing these holes should trigger deprecation warnings?
Alternatively we could try to actually support StringDType subarrays, #32012 (comment) might have a path towards that.
AI Disclosure
I used an AI model to identify the code paths that need new errors. It also spotted the reference leak.