Big backwards compatibility problem with fused types
Hi devs, this is really bad: https://github.com/cython/cython/pull/1873 The problem is that cdef functions with fused types are expanded arbitrarily on use, and their Entries removed and re-appended to the list of cdef function entries. But that list also defines the order of the entries in the vtable! Currently, when a method with fused types is defined in a .pxd file, it is the implementation order in the .pyx (!) file that determines the order in the vtable. Even worse, for modules that cimport from that .pxd file, it is the order in which these methods are *called* that determines the corresponding vtable on the other side, as the entries are expanded at first use, i.e. at their first occurrence in the code, and appended to the current list of cdef functions at this (arbitrary) point. What this means is that there is no way to safely infer the vtable order of an extension type with fused methods from a .pxd file. Now, the correct way to handle this would have been to *replace* the original fused entry with the expanded list of specialised entries. But it's too late for that now. All existing modules out there would need to get recompiled if we changed their vtable order, at least if they use fused any types methods anywhere in their hierarchy. However, I think we can assume code that uses a different method order in the .pxd and .pyx files to be broken already, so a way out of this misery would be to make the .pxd order the one and only source, with the twist that we must keep the "delete and re-append" algorithm to keep the order that existing translated modules are using. Thus the proposal: - we switch to a scheme now that is only defined by the first declaration order, i.e. the order in the .pxd file if there is one. - we make sure all fused methods are expanded in that same order, but continue to follow all non-fused methods in the vtable. - we break all code now that uses a different order of fused methods in their pyx and pxd files Thoughts? Stefan
On Tue, Sep 19, 2017 at 10:48 AM, Stefan Behnel <stefan_ml@behnel.de> wrote:
Hi devs,
this is really bad:
https://github.com/cython/cython/pull/1873
The problem is that cdef functions with fused types are expanded arbitrarily on use, and their Entries removed and re-appended to the list of cdef function entries. But that list also defines the order of the entries in the vtable!
Currently, when a method with fused types is defined in a .pxd file, it is the implementation order in the .pyx (!) file that determines the order in the vtable. Even worse, for modules that cimport from that .pxd file, it is the order in which these methods are *called* that determines the corresponding vtable on the other side, as the entries are expanded at first use, i.e. at their first occurrence in the code, and appended to the current list of cdef functions at this (arbitrary) point.
What this means is that there is no way to safely infer the vtable order of an extension type with fused methods from a .pxd file.
Now, the correct way to handle this would have been to *replace* the original fused entry with the expanded list of specialised entries. But it's too late for that now. All existing modules out there would need to get recompiled if we changed their vtable order, at least if they use fused any types methods anywhere in their hierarchy.
However, I think we can assume code that uses a different method order in the .pxd and .pyx files to be broken already, so a way out of this misery would be to make the .pxd order the one and only source, with the twist that we must keep the "delete and re-append" algorithm to keep the order that existing translated modules are using.
Thus the proposal:
- we switch to a scheme now that is only defined by the first declaration order, i.e. the order in the .pxd file if there is one.
- we make sure all fused methods are expanded in that same order, but continue to follow all non-fused methods in the vtable.
- we break all code now that uses a different order of fused methods in their pyx and pxd files
Thoughts?
+1. We should let the .pxd file, as it is declared, be the source of truth. Code that doesn't already follow this is and does cross-module fused-cdef function calls is already broken.
Robert Bradshaw schrieb am 19.09.2017 um 22:55:
On Tue, Sep 19, 2017 at 10:48 AM, Stefan Behnel wrote:
this is really bad: https://github.com/cython/cython/pull/1873
The problem is that cdef functions with fused types are expanded arbitrarily on use, and their Entries removed and re-appended to the list of cdef function entries. But that list also defines the order of the entries in the vtable!
Currently, when a method with fused types is defined in a .pxd file, it is the implementation order in the .pyx (!) file that determines the order in the vtable. Even worse, for modules that cimport from that .pxd file, it is the order in which these methods are *called* that determines the corresponding vtable on the other side, as the entries are expanded at first use, i.e. at their first occurrence in the code, and appended to the current list of cdef functions at this (arbitrary) point.
What this means is that there is no way to safely infer the vtable order of an extension type with fused methods from a .pxd file.
Now, the correct way to handle this would have been to *replace* the original fused entry with the expanded list of specialised entries. But it's too late for that now. All existing modules out there would need to get recompiled if we changed their vtable order, at least if they use fused any types methods anywhere in their hierarchy.
However, I think we can assume code that uses a different method order in the .pxd and .pyx files to be broken already, so a way out of this misery would be to make the .pxd order the one and only source, with the twist that we must keep the "delete and re-append" algorithm to keep the order that existing translated modules are using.
Thus the proposal:
- we switch to a scheme now that is only defined by the first declaration order, i.e. the order in the .pxd file if there is one.
- we make sure all fused methods are expanded in that same order, but continue to follow all non-fused methods in the vtable.
- we break all code now that uses a different order of fused methods in their pyx and pxd files
Thoughts?
+1. We should let the .pxd file, as it is declared, be the source of truth. Code that doesn't already follow this is and does cross-module fused-cdef function calls is already broken.
This should fix it: https://github.com/cython/cython/commit/fcc3461e0077c86e30cc81a2fd78bce60f1b... There were also other problems that I could fix along the way with respect to inheritance from types with fused cdef methods, so I can hope that we're really fixing a rare case in practice. At least, it took more than 5 years after implementing fused types (in April 2012) to discover this bug, so it can't be the most widely used Cython feature out there... Stefan
participants (2)
-
Robert Bradshaw -
Stefan Behnel