Page MenuHomePhabricator

Not allow multi class inheritance in eo
Closed, ResolvedPublic

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.Nov 23 2018, 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.Nov 30 2018, 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.Dec 5 2018, 10:21 AM

The revision D7585 makes use of the new eolian keyword requires, with this, the need for regular classes in mixins stays completly internal to eolian, seperated from inheritance information. Additionally it enforces that eolian classes are using the mixin correctly.

With this a mixin can simply be converted to an interface, which can be inherited from in bindings, as long as this structure is then mirrored into the inheritance information that is passed into efl_class_name (Which also applies to inheritance of interfaces etc.). With this applied We don't need to refactor any parts of efl, and the problems regarding mixins inheriting from regulars are resolved.

This means that there is only the abstract classes inheriting from regular objects.

@Jaehyun @felipealmeida @segfaultxavi can you verify that this solves the problems for C# ?

With 3ddd577fb0c67b1867c6664af42a89652134c73a beeing merged, means that we don't have a broken inheritance structure anymore. There is now everywhere only ONE regular or abstract + N interfaces + M mixins. P259 does not show any other multiclass inherits anymore. So the first part of this task seems to be fixed ? The other part of the task (abstract classes inheriting from regulars) still needs to be addressed.

@Jaehyun_Cho I am not understanding the problem you are proposing in https://phab.enlightenment.org/T7366#129285 completly.

I am wondering why the override functions are a problem for the bindings. The functions in the implements block are a few implementation details, but not all. There could still be a internal call to eo that calls efl_override, or composition could add another set of API to the implementations. So bindings should not really rely on the functions from the implements {} block, we should IMO rather add a API to eo to find out what the next level of inheritance is that should be called. @felipealmeida @Jaehyun_Cho how is that handled in the bindings / why is that needed ? :)

State of this ticket: Most abstract classes inheriting from regular classes are solved now. The only appearance left is Efl.Ui.Item.

@bu5hm4n I think he is mentioning that if a C# programmer implements a mixin, then the regular class will not appear in its class's hierarchy. However, the user should inherit from the regular class (or some derived class) since the mixin requires it.

@bu5hm4n I think he is mentioning that if a C# programmer implements a mixin, then the regular class will not appear in its class's hierarchy. However, the user should inherit from the regular class (or some derived class) since the mixin requires it.

Ah! That should be enforced right. The mixins are requiring this class, and cannot work without it. I think we can add a API to eolian to return you the required classes of a mixin. Would that help you ?

Maybe, or Eo could enforce this too.

I am not too sure if we want to do that in eo. It seems to be quite a perfmance implact to walk the whole list of parent classes to ensure the required classes are there, more over, the concept of requires in mixins do only exist in eolian,

OK, then a way to extract that information from Eolian would be necessary.

Doing this now.

Okay, after a discussion with @felipealmeida it appears that abstracts only inheriting from abstracts is not required anymore.

q66 added a comment.Jan 17 2019, 7:23 AM

Landed the Eolian-side enforcement in 1b7129cea0d90f76a21bd6b4e40d1528141e087f.

@felipealmeida so it seems to me, that the last missing piece, is the object checking in efl-mono, so each mixin has all the required classes. After that we can close this and we are done with it ?

q66 added a comment.May 9 2019, 5:40 AM

Any update on this? Can it be closed? As far as Eolian is concerned it's been done for a while

segfaultxavi closed this task as Resolved.Sep 3 2019, 12:37 PM
segfaultxavi claimed this task.

Yeah, been done for a while.

Still missing the check by C#

I didn't fully understand what is this missing check.
If it is needed so C# devs can implement EO mixins, I do not think that is very useful.
The way I see it, mixins are a mechanism we used to develop EFL, but they should not show to the C# developer in any way. The C# dev sees the EFL# classes and inherits from them if he wants, but why would he want to create his own mixins? Moreover, the mixin concept is alien in C#, isn't it?