Page MenuHomePhabricator

Not allow multi class inheritance in eo
Open, HighPublic

Description

Now, eo does not limit multi class inheritance. So some eo classes inherit from more than one class.

As you know, many programming languages does not allow multi class inheritance. (e.g. C#)

To handle eo's multi class inheritance, classes in C# bindings should have unusual structures.
(e.g. In a widget class, there is an interface which inherits from other interfaces and a class implements the interface.)

So how about not allowing multi class inheritance in eo? If so, we can easily do language bindings with more common class forms.

bu5hm4n added a subscriber: bu5hm4n.Aug 7 2018, 2:16 AM

Hello,

i would like to add here a bit more information and ask things.

EO object model

EO's multiclass inheritance is no real multiclass inheritance like C++

Lets say you have regular classes A, B, C, D each of them have functions and implementations.

Now we have:
class E (A, B, C, D) {
... }

What this causes is that E now has the API of A B C & D but ONLY the implementation of A. The implementations of B C & D will not be executed when theire API is called on a object of E.

The only way of getting the implementation of B C & D into the a object of A , is to move B C & D to be a mixin and not a class. However, a mixin can never be created as a standalone object, and its only purpose is to be inherited from it.

C# mixin inheritance

You could replicate something similar with C# using extension methods.
Which basically means, that every mixin will be a interface with a extension class, which provides the actual implementation.

I could find the following references:

felipealmeida added a comment.EditedAug 14 2018, 5:00 PM

I could use extension methods to add methods, but the problem is not methods, I must generate the same inheritance tree as well. So, for example, E must be convertible to A, B, C and D and be used as-if we had an A, B, C and D.
But since A, B, C and D are classes, and not interfaces or mixins, I can't create this inheritance-ship.

Mixins are already being generated as interfaces.

Hello @q66

I checked the cases of multiple class inheritance in efl.
In C# language bindings, the only problem case is that a class inherits from more than 1 classes in eo.
Because abstract class and mixin can be converted to an interface. (not a class)

If D6904 is submitted, then there is no case that a class inherits from more than 1 classes in eo.
What do you think about that eolian restricts the case that a class inherits from more than 1 classes in eo?

With help of pyolian, I could find these instances where we have abstract classes inheriting from regular classes somewhere in their inheritance chain:

(Abstract -> Regulars in inheritance tree)

  • Efl.Ui.List_Item -> Efl.Canvas.Group, Efl.Loop_Consumer, Efl.Ui.Layout.Object
  • Efl.Ui.Widget -> Efl.Canvas.Group, Efl.Loop_Consumer
  • Efl.Ui.Item -> Efl.Canvas.Group, Efl.Loop_Consumer, Efl.Ui.Layout.Object
  • Efl.Canvas.Object -> Efl.Loop_Consumer
  • Efl.Canvas.Animation_Group -> Efl.Canvas.Animation
  • Efl.Canvas.Image_Internal -> Efl.Loop_Consumer

Regulars with abstract child:

  • Efl.Canvas.Animation
  • Efl.Canvas.Group
  • Efl.Loop_Consumer
  • Efl.Ui.Layout.Object

This causes the C# interface chain for these abstracts to be 'polluted' with regular C# classes (for Eo regulars).

Maybe these could be made abstract?

Jaehyun_Cho added a comment.EditedSep 3 2018, 12:31 AM

@lauromoura

Thank you for the information. I haven't thought about that..

I think that we cannot convert all those classes to abstracts because for example Efl.Ui.Layout.Object instance may be widely used by itself.

Initially I tried to change class to abstract or create new abstract or class to resolve this issue.

However, in fact, this case is perfectly fine in all OOP languages...

So even if we remove this case, I think that probably we cannot forbid this case..

If I have better idea, then I will share with you.

Thank you

I think regular classes with abstract children are perfectly OK in EO. They are OK in C++, for sure.

The problem here is that some classes, like Efl.Canvas.Object inherit from more than one regular class:

abstract Efl.Canvas.Object (Efl.Object, Efl.Gfx.Entity, Efl.Gfx.Color, Efl.Gfx.Stack, Efl.Animator,
                            Efl.Input.Interface, Efl.Gfx.Size_Hint,                                
                            Efl.Gfx.Map, Efl.Loop_Consumer, Efl.Ui.Base, Efl.Canvas.Pointer)

Where Efl.Object and Efl.Loop_Consumer are regular classes, and this should not be allowed.

@q66 why does not Eolian warn of these cases?

q66 added a comment.Sep 4 2018, 4:27 AM

because it's regular multi-class inheritance and it's how eo was designed? i mean, that's what this ticket is trying to point out

i think we should not allow it, but that will need work

Woah, you mean that Eolian does actually allow multi-class inheritance? All the Eo tutorials are wrong then, I have been lied to all my life!

I thought that people did not understand how inheritance worked in Eo, and now I see that it was me. Sorry for the noise.

@lauromoura with that pyolian magic, can you list all classes inheriting from more than one class (not interface or mixin) ? In this way we can start evaluating how much work would require fixing that.

q66 added a comment.Sep 4 2018, 4:39 AM

@segfaultxavi eolian does exactly the same check at compile-time that eo does at runtime: https://git.enlightenment.org/core/efl.git/tree/src/lib/eolian/database_validate.c#n802

that is, the first inherit of a regular class must again be a regulat class, while the first inherit of e.g. an interface again has to be that - any further inherit is not checked and can be literally any type

that is what the problem is here

@lauromoura with that pyolian magic, can you list all classes inheriting from more than one class (not interface or mixin) ? In this way we can start evaluating how much work would require fixing that.

Classes with more than 1 regular/abstract direct ancestor

  • Class Efl.Ui.Popup_Part has 2 ancestors
    • Regular: ['Efl.Ui.Layout.Part']
    • Abstracts: ['Efl.Canvas.Object']
  • Class Efl.Canvas.Object has 2 ancestors
    • Regular: ['Efl.Loop_Consumer']
    • Abstracts: ['Efl.Object']
  • Class Efl.Ui.Image_Zoomable has 2 ancestors
    • Regular: ['Efl.Ui.Image']
    • Abstracts: ['Efl.Ui.Widget']
  • Class Elm.Combobox has 4 ancestors
    • Regular: ['Efl.Ui.Button', 'Elm.Entry', 'Elm.Genlist', 'Elm.Hover']
    • Abstracts: []
  • Class Efl.Canvas.Layout_Part_External has 2 ancestors
    • Regular: ['Efl.Canvas.Layout_Part']
    • Abstracts: ['Efl.Canvas.Object']
  • Class Efl.Ui.Textpath has 2 ancestors
    • Regular: ['Efl.Ui.Layout.Object']
    • Abstracts: ['Efl.Object']
  • Class Efl.Ui.Win has 2 ancestors
    • Regular: ['Efl.Config_Global']
    • Abstracts: ['Efl.Ui.Widget']
  • Class Evas.Canvas has 2 ancestors
    • Regular: ['Efl.Loop_Consumer']
    • Abstracts: ['Efl.Object']

Some of these inherit explicitly from Efl.Object despite already being a "grandchild" of it. For these, this explicit inherit could be removed.

I created a subtask T7366 to evaluate the work to fix the above classes. I have my plate full at the moment, but will pick up this task later if nobody has done it.

@Jaehyun_Cho is already working on part of the inheritance problem, so maybe @Jaehyun_Cho could work on this new parts as well? If not, could you let us know @Jaehyun_Cho ?
Regards,

As I commented in T7366, the multiple class inheritance from abstract and class reported by @lauromoura is removed by D6982 and D6984.

Jaehyun_Cho added a comment.EditedSep 10 2018, 4:59 AM

Now the multiple class inheritance from abstract and class reported by @lauromoura has been removed. (Class Elm.Combobox is legacy so it is not related to C# bindings)

Now I think we need to consider the case that mixin inherits from abstract or class.

Because if mixin inherits from abstract or class, then mixin cannot be converted into interface in C# bindings.

The following are the list.

mixin Efl.Ui.Focus.Layer -> abstract Efl.Ui.Widget
mixin Efl.Ui.Focus.Manager_Sub -> abstract Efl.Object
mixin Efl.Ui.Focus.Composition -> abstract Efl.Ui.Widget
mixin Efl.Access.Object -> abstract Efl.Object
mixin Efl.Ui.Widget_Focus_Manager -> abstract Efl.Ui.Widget
mixin Efl.Io.Closer_Fd -> abstract Efl.Object
mixin Efl.Input.Event -> abstract Efl.Object
mixin Efl.Canvas.Filter.Internal -> abstract Efl.Object
mixin Efl.Canvas.Surface -> abstract Efl.Canvas.Image_Internal
mixin Efl.Gfx.Map -> abstract Efl.Object

q66 moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Nov 14 2018, 5:03 AM
q66 added a comment.Fri, Nov 23, 5:08 AM

Initial work has been merged on T7459. I believe once that is done, we can restrict things in Eolian so that only interfaces/mixins are allowed in the "implements Foo, Bar" list, but that will need some .eo file refactoring...

q66 added a comment.Fri, Nov 30, 5:20 AM

That has for now optionally been done in T7365, once all the definitions are fixed, we can enable it unconditionally...

segfaultxavi triaged this task as High priority.Wed, Dec 5, 10:21 AM