Skip to content
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.

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

Multiple inheritance #1927

Merged
merged 10 commits into from Nov 9, 2017
Merged

Multiple inheritance #1927

merged 10 commits into from Nov 9, 2017

Conversation

robertwb
Copy link
Contributor

No description provided.

@@ -4709,6 +4676,31 @@ def analyse_declarations(self, env):
else:
scope.implemented = 1

if len(self.bases.args) > 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you changed the indentation space in a couple of places (4->2). I would prefer sticking with the standard of 4 spaces everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not intentionally, thanks for catching.

type = entry.type
typeobj_cname = type.typeobj_cname
scope = type.scope
if scope: # could be None if there was an error
Copy link
Contributor

Choose a reason for hiding this comment

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

I know, the code was like this before, so this comment is really misplaced here (sorry), but now that this old code is touched anyway, let me say that it would be a good opportunity to clean up this kind of useless level of indentation. If the code handles an error case, it should just say so and bail out early, instead of using lengthy indented if-blocks without an else. I don't mind long functions at all, as long as their structure is simple. But indented conditional blocks increase the complexity because if I can't see where an if-block ends, I have to assume that there is an else-case (and mentally prepare for it). If there is no else-case after a long block, then the if-block probably shouldn't be there in the first place, but be replaced by a short error handling case of the reversed condition, or be moved into a separate method entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

code.error_goto(entry.pos)))
if heap_type_bases:
code.putln("#if PY_VERSION_HEX >= 0x03050000")
code.putln("%s.tp_flags &= ~Py_TPFLAGS_HEAPTYPE;" % typeobj_cname)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Eventually, we will have to switch to heap-types anyway to properly support PEP 489, submodules, reloading, etc. I understand that this doesn't have to happen right now, but you would probably agree that this flag unsetting seems like a fairly crude hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is an ugly hack (and one of my hesitations for the whole PR) but I couldn't figure out a way around it without making everything heap allocated (a much bigger change, though, as you said, we may need to go there).

from . import ExprNodes
self.type_init_args = ExprNodes.TupleNode(
self.pos,
args=[ExprNodes.StringNode(self.pos, value=self.class_name),
Copy link
Contributor

Choose a reason for hiding this comment

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

IdentifierStringNode is better here.

base_class_scope = None
elif not base_type.is_extension_type and \
not (base_type.is_builtin_type and base_type.objstruct_cname):
print (base_type, base_type.is_builtin_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

;)

base_type = base.analyse_as_type(env)
if base_type in (PyrexTypes.c_int_type, PyrexTypes.c_long_type, PyrexTypes.c_float_type):
# Use the Python rather than C variant of these types.
base_type = env.lookup(str(base_type)).type
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather call sign_and_name() here instead of relying on the output of str().

@scoder
Copy link
Contributor

scoder commented Oct 20, 2017

Nice feature!

We should be doing this for pure Python defined classe as well,
but don't have a way to inject it.
@robertwb robertwb changed the title WIP: Multiple inheritance Multiple inheritance Nov 9, 2017
@robertwb robertwb merged commit 2032632 into cython:master Nov 9, 2017
@scoder
Copy link
Contributor

scoder commented Nov 26, 2017

Hmm, I think this might have an impact on the __richcmp__() generation:

https://github.com/cython/cython/blob/0.27.3/Cython/Compiler/ModuleNode.py#L1807

We should now also look into the Python base classes for special methods, and (I think) also generally delegate missing special methods to a runtime lookup.

@robertwb
Copy link
Contributor Author

Good point, these special methods that delegate to other (sets of) special methods need more sophisticated handling. Seems this may also be an issue for Python classes that inherit from cdef classes with this kind of special method.

da-woods added a commit to da-woods/cython that referenced this pull request Mar 12, 2020
scoder pushed a commit that referenced this pull request Mar 21, 2020
@cvanelteren
Copy link

Are there any plans for multiple extension inheritance (cdef class)?

f

@da-woods
Copy link
Contributor

Are there any plans for multiple extension inheritance (cdef class)?

I wouldn't think so. It's a limitation of how Python handles extension types (i.e. any type defined as a C struct). Try class C(int, dict): pass for example.

@cvanelteren
Copy link

I am a bit unfamiliar with the lower level internals of python but that would also imply that multi-level inheritance is out for extensions written in cython? I currently this problem with a 'need' for a third layer of class inheritance. According to the docs this is fine for normal python objects but the not for extensions, I was wondering why.

@da-woods
Copy link
Contributor

da-woods commented Sep 19, 2020

I am a bit unfamiliar with the lower level internals of python but that would also imply that multi-level inheritance is out for extensions written in cython? I currently this problem with a 'need' for a third layer of class inheritance. According to the docs this is fine for normal python objects but the not for extensions, I was wondering why.

The basic problem for multiple inheritance is that an extension type gets translated into:

struct some_internal_struct {
   PyObject_HEAD;
   int some_detail; // just as an example of data that might be part of the type
};

In order to function properly you really need exactly one PyObject_HEAD since it's this that holds the details of the reference counting.

Single inheritance is fairly simple:

struct some_derived_type {
    some_internal_struct base;
    double other_contents_for_example;  // just as an example of data that might be part of the type
}

For multiple inheritance though it's impossible to include extension types in some_derived_type while preserving their layout and keeping only one PyObject_HEAD. This doesn't apply to normal Python objects because their attributes are mostly based around a single dict rather than C structures.


Multi-level inheritance is no problem and has worked for a long long time. You can have as many "layers" of inheritance as you like (so C inherits from B inherits from A); you just can't have C inherits from A and B.

struct a_third_level_derived_type {
    some_derived_type base;
    // more detials
};

This fulfils all the requirements: one PyObject_HEAD at the start of the struct.


However, if you have questions about a specific implementation problem then you should follow it up on the cython-users mailing list rather than this PR.

@scoder
Copy link
Contributor

scoder commented Sep 19, 2020

Just to clarify, you
a) can create a Python class that inherits from a single cdef class and one or more Python classes
b) can create a cdef class that inherits from a single cdef class and one or more Python classes (this is what this PR is about)
c) cannot create a cdef class that inherits from more than one cdef class

The third point only diverges slightly from Python's own behaviour. You cannot create a Python class that inherits from two builtin/extension types with different object layouts (e.g. class PyClass(list, tuple)), but you can create a Python class that inherits from two builtin/extension types that have the same object layouts (e.g. class PyClass(SyntaxError, ValueError)).

This restriction could be lifted, but it's probably less interesting than the change in this PR.

Note that Python classes are not "slow" or "bad" per se. If you think that you have a specific need for inheriting from more than one cdef class which cannot (more or less) easily be resolved by using (compiled) Python classes, then I would suggest opening a new feature ticket that describes your use case and the reasons why a mix of cdef and Python classes cannot work here.

If it's unclear for you how to handle a specific use case with the current feature set, then I would second @da-woods' suggestion of asking on the cython-users mailing list for help.

mozillazg pushed a commit to mozillazg/pypy that referenced this pull request Nov 2, 2022
See https://bugs.python.org/issue22079 and the solution in cython from 2017
cython/cython#1927 (review)
If we, like CPython turn on CYTHON_USE_TYPE_SPECS then the cython hack goes away
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants