Page MenuHomePhabricator

Force all apis to be implemented
Open, HighPublic

Description

We were discussing on irc how a caller of a api should no if a api is implemented or not, since in efl a function still may be unimplemented even if it isa interface of the interface that defined that function. We came to the conclusion that it would be a good idea to force every function to be implemented.

Compile time

Solve it at compile time, every function has to be mentioned in one implements section in the inheritance tree. Eolian should error when a function is not implemented in a regular class.

If someone wants to use composite to build up logic in a object, then he could specify that in eolian in the implements section with something like implements { Foo.Bar @composite } where Foo.Bar is a class, eo then enforces somehow that the objects are composited at construction time, (dont know exactly how to solve that exactly)

Runtime

@jpeg your turn! :)

bu5hm4n created this task.Jul 12 2017, 3:37 AM
bu5hm4n added a project: efl.
jpeg added a comment.Jul 12 2017, 7:59 PM

I agree with the objective, as I want a good API in C++/C# and similar strongly typed languages, including auto-completion.

Runtime checks: why

The reason why I would advocate for runtime checks instead are:

  • I'm not convinced we can make a nice API where all APIs are actually implemented. I also think that we may be using objects internally for which we don't need or want a full implementation.
  • There would be a painful transition period where we have to implement all APIs somehow just for compilation to work. Right now a lot of APIs are not implemented.
  • EO is very dynamic, it's not limited to Eolian or even C. Composition, overrides, etc... are all valid ways to implement an API and are only runtime (@composite would just be a hack to prevent a compile error)

Runtime checks: how

After finalize we could run some check in eo.c to verify that all APIs in the vtable point to an implementation function. Then print out the list of unimplemented APIs. This should probably be limited to only once per class. I would do this in eo_debug mode only. Alternatively we can write a tool for clouseau or command line that would walk all objects and inspect their vtables in a running process.

I understand this may not be a strong enough requirement for some people :)

Checking the vtables at "finalize" stage isn't good enough as we're allowed to composite_attach/detach beyond "finalize" (like changing "modes" of a functionality of an object).
As for overrides, I believe we can (and should) restrict it to pre-finalize stages.

To solve the composite issue, we might want to find another solution to document that "object Efl.Foo implements Efl.IBar via composition".
There is no other issue other than documenting, so I consider this solvable.

Simplest solution will be to look for a @composite reference in the class's .eo file. We may actually want this for objects that can change "modes" via some method.
We can then refer in the method's documentation to that compositing interface.

An example:

class Efl.Foo (Efl.Object) {
   methods {
      @property mode {
         [[Enables or disables a mode that...
           @composite Efl.IBar
         ]]
       ...
       }
   ...
   }
}

@jpeg but where is the big difference to do it at runtime like you did up there? In the end EVERY user will see this, a dev that just uses elm_test may not, since not each and every object is used there. At compile time you will have the problem in front of you in about ~2 min. when the eolian stage is done, at runtime you have a way longer workflow ..., meanwhile, do you have numbers of how many functions are not implemented?

Also to the internal objects: we could also have some kind of @internal at the class definition, so that the checks are off for those objects, and the obejcts .h files can only be accessed with #define THIS_IS_INTERNAL_I_AM_DOING_IT_WRONG which would only be enabled in the efl tree. Or we just use @empty all over the place in our internal api, (btw. there could also be @error which will print a std error when the func is called)

@herdsman not that sure why you would put those implementation details in the documentation, but i would just leave it in the implements {...} section, since every single implementation detail is safed there.

@herdsman not that sure why you would put those implementation details in the documentation, but i would just leave it in the implements {...} section, since every single implementation detail is safed there.

You can't even do that. You can't specify a function that is supported by some composition in the implements { ... } block.
If you do that, you are creating a symbol, which doesn't even exist (nor it should).

Take the following Efl.Foo.bar method of an interface Efl.Foo:
Now, examine the calls for efl_foo_bar(obj), where obj is some Efl.Baz class:

First, we do:

composite_attach(obj, foo_obj); // foo_obj is some class that implements Efl.Foo

We call to efl_foo_bar(obj) and the foo_obj's function is called.

Now, what happens when we add the following to efl_baz.eo file:

implements {
   Efl.Foo.bar;
}

The call will be from the obj itself (AND you now have to define _efl_baz_efl_foo_bar(...)).

Ok, then instead, lets add @empty:

implements {
   @empty Efl.Foo.bar;
}

Now every call to efl_foo_bar on obj will be a NOP.

Composition has nothing to do with implementing an interface.
It's a nice bonus to have, yes.
That's why it should be only a documentation thing.

jpeg added a comment.Jul 13 2017, 6:52 PM

@jpeg but where is the big difference to do it at runtime like you did up there? In the end EVERY user will see this

Thus eo_debug. This is not for users, and this would have a performance impact if inside EO.

a dev that just uses elm_test may not, since not each and every object is used there

Well, they should. Really. But that's another question entirely. :)

At compile time you will have the problem in front of you in about ~2 min.

And a very painful transition period where we have to implement or mark all unimplemented APIs as "empty".

@herdsman not that sure why you would put those implementation details in the documentation, but i would just leave it in the implements {...} section, since every single implementation detail is safed there.

You can't even do that. You can't specify a function that is supported by some composition in the implements { ... } block.
[...]

We could add a composed {} block. Or a @compose: Foo.Bar; feature inside implements {}.

[...]

Ok, then instead, lets add @empty:

implements {
   @empty Efl.Foo.bar;
}

Now every call to efl_foo_bar on obj will be a NOP.

And to me this @empty tag (or even the composed option) is just a workaround the fact that it is extremely hard and uncomfortable to try to implement all our APIs by design of EO.

That being said, I am absolutely OK with writing tools in Eolian in order to easily track those APIs. What I am not in favor of is the build error. Right now this would slow down our development process a lot, which is bad considering how late we are on the interfaces work.

One thing is clear though: We need this information, one way or another (and I think both EO and Eolian can be used for that).

In T5719#91686, @jpeg wrote:

@jpeg but where is the big difference to do it at runtime like you did up there? In the end EVERY user will see this

Thus eo_debug. This is not for users, and this would have a performance impact if inside EO.

Not too sure about that, how many are running that? Not the two of us, or some other people, the guys that implement other things for example, there are people not even running make check, do you really think they are going to run elementary_test with eo_degug ?

a dev that just uses elm_test may not, since not each and every object is used there

Well, they should. Really. But that's another question entirely. :)

At compile time you will have the problem in front of you in about ~2 min.

And a very painful transition period where we have to implement or mark all unimplemented APIs as "empty".

@herdsman not that sure why you would put those implementation details in the documentation, but i would just leave it in the implements {...} section, since every single implementation detail is safed there.

You can't even do that. You can't specify a function that is supported by some composition in the implements { ... } block.
[...]

We could add a composed {} block. Or a @compose: Foo.Bar; feature inside implements {}.

Joup that would be something i would do.

[...]

Ok, then instead, lets add @empty:

implements {
   @empty Efl.Foo.bar;
}

Now every call to efl_foo_bar on obj will be a NOP.

And to me this @empty tag (or even the composed option) is just a workaround the fact that it is extremely hard and uncomfortable to try to implement all our APIs by design of EO.

That being said, I am absolutely OK with writing tools in Eolian in order to easily track those APIs. What I am not in favor of is the build error. Right now this would slow down our development process a lot, which is bad considering how late we are on the interfaces work.

One thing is clear though: We need this information, one way or another (and I think both EO and Eolian can be used for that).

Not sure about that, this just feels like a smooth "yeah we should" and in the end noone will have a eye on it, if we force it by having a tool that errors and blocks your build process we are forcing the people to have a eye on it, everyone that is just adding a patch, so less work for the imaginary guy that is having a look on it.

Also to the transition period, this would be just internal objects that would get the @error or @empty tags, for all other objects is basically fixing of missing api...

And here a few numbers:
https://gist.github.com/marcelhollerbach/54709216bf64e0bac7531c7dabe81694

result.txt is the file that includes all apis missed by eolian (its really not much!)
checks.c is the code implementing the checks, (i think we could add that to eolian)

What makes me wonder here alot is, elf.ui.check is not listet at all, but has ALOT of unimplemented functions ... wtf ?

Okay, i have updated it according to how eo works, i am still wondering about alot of apis that are not showing up on the check which have been discovered by closueau ... but well ... lets see

jpeg added a comment.Jul 16 2017, 6:14 PM

And here a few numbers:

https://gist.github.com/marcelhollerbach/54709216bf64e0bac7531c7dabe81694

result.txt is the file that includes all apis missed by eolian (its really not much!)
checks.c is the code implementing the checks, (i think we could add that to eolian)

Looks like mixins are not supported by your patch (see all the map APIs).
Please understand me well: I'm not in favor of a build error but I'm very much in favor of having either a tool or warnings directly from eolian. Anything automated helps for sure, as long as it's not massive noise.

i have updated it, i have updated the checks.c to check the way how eo is inheriting api and implementation.

But now i have something else. I think we have a eo bug!

Take efl.ui.button, follow the inherit chain, you will see efl.loop_user.loop.get is implemented as part of efl.canvas.object (a abstract).

(efl.ui.button->elm.layout->elm.widget->efl.canvas.group->efl.canvas.object)

calling efl_loop_get on efl.ui.button gives me:

ERR<14523>:eo lib/eo/eo.c:579 _efl_object_call_resolve() /home/marcel/git/clouseau/src/lib/clouseau_debug.c:464: unable
 to resolve regular api func 'efl_loop_get' in class 'Efl.Ui.Button'.

Eeeh wad?! I am currently not sure if i am overseeing something, but this is clearly a deep and hard bug in my opinion, does someone see something else?

jpeg added a comment.Jul 17 2017, 6:56 PM
ERR<14523>:eo lib/eo/eo.c:579 _efl_object_call_resolve() /home/marcel/git/clouseau/src/lib/clouseau_debug.c:464: unable
 to resolve regular api func 'efl_loop_get' in class 'Efl.Ui.Button'.

The call resolve code was missing the most important bits :) Ill assume you fixed it now.

Jep, up to date now!

You can now just check it yourself:
https://git.enlightenment.org/core/efl.git/log/?h=devs/bu5hm4n/eolian_warnings

There are now sadly also all the noneimplemented compose functions, will add that at some later point.

q66 added a comment.Jul 19 2017, 6:43 AM

I don't think eolian_gen is the right place to put this at all. It should go into the validator in the library. If you want me to implement this, I can...

Well, i dont really care if the checks.c is in eolian_gen or the library, the more important part there is to get the compose things out :)

bu5hm4n moved this task from Backlog to Eolian on the efl board.Sep 30 2017, 1:47 AM
bu5hm4n claimed this task.Jan 16 2018, 5:36 AM
bu5hm4n added a project: Restricted Project.Jun 11 2018, 3:00 AM
zmike edited projects, added Restricted Project; removed efl.Jun 11 2018, 6:54 AM
bu5hm4n edited projects, added efl: language bindings; removed Restricted Project.Jun 11 2018, 7:09 AM
zmike added a subscriber: zmike.

A #Goal ticket should not be set to a milestone.

bu5hm4n removed bu5hm4n as the assignee of this task.Fri, Dec 7, 5:11 AM