[lifecycle/meta] illegal hooks API and rtt illegal hooks propagation

[jpeletier]

This PR introduces a mechanism for Flecs to signal to applications when a specific lifecycle hook is not allowed. This feature allows applications using reflection to determine, at runtime, whether a type is default-constructible, copy-constructible, movable, etc., by comparing the lifecycle hook to the newly provided _ILLEGAL constant in the API:

if (hooks.copy_ctor == ECS_COPY_CTOR_ILLEGAL) {
    // Don't call !!
}

Additionally, the PR ensures that runtime types automatically propagate this constant: if a type within a runtime struct or array has an illegal hook, the runtime type itself will inherit an illegal hook of the same type by default:

  struct UncopyableType {
    std::unique_ptr<int> value;
  };

  flecs::entity_t uncopyable = ecs.component<UncopyableType>();

  ecs.component<UncopyableType>().opaque(flecs::I32);  // ...

  // define a run-time struct type that has an uncopyable member:
  flecs::entity_t runtime_type = ecs.component()
      .member(uncopyable, "x")
      .member(flecs::I32, "y");

  assert(ecs_get_type_info(ecs, uncopyable)->hooks.copy == ECS_COPY_ILLEGAL);    // ok!

  // as a result, the runtime type is also uncopyable:
  assert(ecs_get_type_info(ecs, runtime_type)->hooks.copy == ECS_COPY_ILLEGAL);  // ok!

This also allows early detection of the impossibility to, for example, copy a complex run-time type. Instead of making an attempt and have it fail when the nested type that can't be copied is reached, it is possible to check in advance and avoid making the call at all.

[SanderMertens]

Seeing how this design works out in code, I think it might be easier if instead of having distinct functions we add a flags member to type_info which has flags for which hooks are valid. For example:

ti->flags |= ECS_CTOR_ILLEGAL;

This has a few benefits:

• We don't need to expose additional symbols for marker functions
• We don't need to duplicate code for different function types
• We can check if a type has illegal hooks in a single operation:

if (ti->flags & (ECS_CTOR_ILLEGAL|ECS_DTOR_ILLEGAL|ECS_COPY_ILLEGAL|ECS_MOVE_ILLEGAL) {
  // type has illegal hooks
}

An additional benefit of this is that we can use the same flags member in the future to allow for other type related feature checks.

[jpeletier]

I considered that approach but I was concerned with people having to make too many changes. What I am proposing here is most backward compatible. As of now, we already have an implicit marker function NULL, to indicate a default hook. The logic to check whether or not there is a hook defined (and then perform a default action, such as memcpy()) is everywhere.

Then, there are marker functions in the cpp module, used internally.

This PR just moved those useful marker functions in the cpp module to share across Flecs (first attempt) . However, due to a quirk on how exported functions work across DLLs in Windows (result), I could not just move the functions to Flec's API and be done. That is why I had to work around this and put the helper macros in place.

Here are my thoughts about this for your consideration:

1. Mainly, there is a lot of existing code within Flecs (and I assume in people's projects as well) that relies on checking ti->hook.xxx != NULL to see if a default action should be applied. That is, we are checking the function pointer itself already to see what it is and what to do.

2. Therefore, we would then have two flags per hook, one for illegal, another one to indicate default: according to the new logic, we should be doing ti->hook.flags & ECS_COPY_DEFAULT instead of ti->hook.copy != NULL before invoking the hook.

3. Care must be taken to keep the flags and actual hook function pointer in sync. If for example a type has an illegal copy-assign hook, do you set the actual hook to NULL and then set the flag (ti->hooks.flags |= ECS_COPY_ILLEGAL) ? or wouldn't you also set the hook to an aborting handler as well to make sure if somebody calls inadvertently we provide a human-readable error? So we would still have a special aborting handler, and now have the extra work of having to be declarative and set the flag.

4. Changes in the cpp module would be required to signal the flag instead of assigning a hook that aborts. In there, these hooks are created via template specialization, so the the cpp module would have to see somehow what hook is created and then declare the correct flags when registering the hooks.

We don't need to expose additional symbols for marker functions
We don't need to duplicate code for different function types

I agree with the above two benefits.

We can check if a type has illegal hooks in a single operation:

if (ti->flags & (ECS_CTOR_ILLEGAL|ECS_DTOR_ILLEGAL|ECS_COPY_ILLEGAL|ECS_MOVE_ILLEGAL) {
  // type has illegal hooks
}

That looks nice, however I am not sure how useful that is. It is normal that a particular type has some illegal hooks, like a missing move constructor or whatever, so the above quoted expression is probably going to be true most of the time.

In reality, this is how I actually use it, checking on a per-hook basis when necessary:

void Type::copy_assign(void *dst, const void *src, int32_t count) const {
  auto copy = info->hooks.copy;
  if (copy != NULL) {
    if (copy == ECS_COPY_ILLEGAL) {
      throw Exception("Type {} does not have a copy assign operator", name());
    }
    copy(dst, src, count, info);
  } else {
    memcpy(dst, src, count * info->size);
  }
}

Additionally, one would have to carry over ecs_type_info_t to check the flags. Not a problem in my case, but perhaps in other situations in case people have copied the hook function pointer.

My conclusion is: The code above would look a bit nicer with flags and the API would expose less clutter, I agree. However, I am not sure it outweighs the benefits of not having to touch a lot of code (cpp module, meta, etc.) and perhaps break people's applications.

[SanderMertens]

Can you expand on how it would break applications? The approach would still set callbacks that assert, but in addition it would also set the flags. I don't think there are any downsides to that approach.

and now have the extra work of having to be declarative and set the flag.

There is only one place in the code where that happens though, and that is where the C++ hooks are registered. If that turns out to be too big of a problem (for example, because other language bindings want the same behavior) it could be wrapped in functions:

ecs_type_info_set_illegal(world, ti, ECS_CTOR_ILLEGAL | ECS_DTOR_ILLEGAL);

That looks nice, however I am not sure how useful that is.

It's useful for the rtt_hooks code, where instead of doing one if-else check you have macro's and dedicated functions for each function type.

[jpeletier]

My concern is more on requiring users to set the flag they were not setting before. My concern is more for the "default" flag, not the "illegal" flag, because that would be kind of a new feature really.

Right now the "API" for default is to check if hook != NULL or not.

With the flags, will the API change to hooks.flags & ECS_CTOR_DEFAULT to determine if a hook is default? This is a big change throughout, that is what I mean. Plus you may need to carry over ecs_type_info_t to check the flags if you didn't copy it (perhaps the developer copied the hook function pointer only)

It's useful for the rtt_hooks code, where instead of doing one if-else check you have macro's and dedicated functions for each function type.

I agree, it would be cleaner. Again the current approach in this PR was to be more backward compatible.

I am available on Discord now. If you can, we can talk and find a solution quickly.
Thanks!
Javier

[SanderMertens]

My concern is more for the "default" flag, not the "illegal" flag

There won't be a default flag. Code will still do a != NULL check to see if a hook is registered.

With the flags, will the API change to hooks.flags & ECS_CTOR_DEFAULT to determine if a hook is default?

Nope, existing code remains as is: they'll just keep using (and calling) the callback.

tl;dr if code wants/needs to know whether the configured callback is illegal it can, but the existing behavior will still work, just as in the current state of the PR.

[jpeletier]

OK, I see now

please let me know if this action plan makes sense:

1.- move the illegal callbacks back to the cpp module
2.- create the new flags field in ecs_hooks_t.
3.- have cpp module declare if it has set illegal callbacks.
4.- adapt the rtt code to check and propagate the flag
5.- also propagate the flag among generated hooks, like when the copy ctor hook is generated out of ctor and copy assign.
6.- adapt tests

[jpeletier]

Actually there is another option, which is the illegal callbacks stay in the new place but they are private. When the illegal flag is detected, Flecs automatically sets the right illegal hook.

Cpp module then only has to set the flag and Flecs configures the illegal callback automatically.
This way people don't have to create their own aborting callbacks.

[SanderMertens]

I like that idea!

[jpeletier]

Done!

[jpeletier]

With the changes, the example now looks like this:

  struct UncopyableType {
    std::unique_ptr<int> value;
  };

  flecs::entity_t uncopyable = ecs.component<UncopyableType>();

  ecs.component<UncopyableType>().opaque(flecs::I32);  // ...

  flecs::entity_t runtime_type = ecs.component().member(uncopyable, "x");

  assert(ecs_get_type_info(ecs, uncopyable)->hooks.flags & ECS_COPY_ILLEGAL);    // ok!
  assert(ecs_get_type_info(ecs, runtime_type)->hooks.flags & ECS_COPY_ILLEGAL);  // ok!

[SanderMertens]

I don't think this is necessary since all functions that use it have a void return type. Not major enough to prevent the PR from landing, but probably something I'll end up removing unless it breaks CI.

As an aside, defines like this belong in include/private/api_defines.h.

[SanderMertens]

Style issue: { should be on new line (for future ref, not going to halt the PR on this).

[SanderMertens]

This functionality should be tested in isolation (in a future PR) since it's part of the core, in test/core/ComponentLifecycle