Allow polymorphism with fine::ResourcePtr

[brodeuralexis]

This feature should introduce no breaking change to the existing API, while allowing resources to be subclasses and behave as such.

This requires a change to how ResourceWrapper works, but since it is an implementation detail, this should not be obvious to users of the library, causing no expected breaking changes.

The biggest downside of this feature is a slight increase in unused padding inside the ResourceWrapper class. With our now approach, we now add on average alignof(std::max_align_t) - alignof(T) unused bytes per resource.

[brodeuralexis]

Should we have a make_polymorphic_resource<Base, Concrete>(...) function instead, and have make_resource<T>(...) as a alias for make_polymorphic_resource<T, T>(...) ?

[jonatanklosko]

Hey @brodeuralexis!

I think if we want to make it polymorphic, ideally we should keep it close the the std smart pointers (and regular pointers), so similarly to:

std::shared_ptr<Base> ptr = std::make_shared<Derived>();

we would have:

fine::ResourcePtr<AbstractRes> res = fine::make_resource<ConcreteRes>();

My understanding is that we would need a constructor with std::is_convertible_v<U*, T*> check. The issue is that fine::ResourcePtr<T> does not store T* directly, it stores fine::ResourceWrapper<T>*.

Perhaps we could kill ResourceWrapper if we manage the memory alignment without that struct. So fine::ResourcePtr<T> would store T*, then when allocating the resource we would allocate memory space for both initialized flag and T.

I was trying to avoid messing with alignment manually, but you do it in the PR also, so perhaps there is no way to avoid it either way.

What do you think?

[jonatanklosko]

Gah, I guess we need to know AbstractRes upfront, because it's the one with FINE_RESOURCE.

[jonatanklosko]

Also, we allow for defining optional callbacks on the resource class, currently we only have one: void destructor(ErlNifEnv *env).

So if they define it on ConcreteRes and do not define as virtual method on AbstractRes, it wouldn't be called. So it could also be a footgun.

---

Do you have a use case where you actually needed a polymorphic resource like this? For example, I think you could do this:

struct MyResource {
  std::unique_ptr<Abstract> my_object;
}
FINE_RESOURCE(MyResource);

This way we move the polymorphism one level down.

I actually think it's good to keep the resource structs simple.

[brodeuralexis]

My goal with this feature was to remove the need for an allocation using std::unique_ptr.

So if they define it on ConcreteRes and do not define as virtual method on AbstractRes, it wouldn't be called. So it could also be a footgun.

We could also argue that the same footgun will happen if ~Abstract is not virtual.

We could introduce fine::Resource with a virtual destructor(ErlNifEnv*) method, and require fine::Registration::register_resource for T where std::is_base_of_v<Resource, T>. This would remove the need for has_destructor, since now all resources will have the destructor method and provide a place to add callbacks if needed with potential default behaviour.

Although this will prevent registering NIF resources on classes the NIF code doesn't control, we could introduce a separate register_resource to deal with such a case.

I was trying to avoid messing with alignment manually, but you do it in the PR also, so perhaps there is no way to avoid it either way.

enif_alloc ensures that memory is aligned for alignof(max_align_t), but I must also ensure that T starts correctly aligned at alignof(max_align_t) even if alignof(T) <= alignof(max_align_t). At the end, the current approach minimizes the use of pointer arithmetic at the cost of using a union.

I actually think it's good to keep the resource structs simple.

This is a feature I believe will make writing more convenient, as it would encourage NIF resources to be more than data containers. I would also be more in line with shared_ptr.

I think if we want to make it polymorphic, ideally we should keep it close the the std smart pointers (and regular pointers) [...]

I stumbled upon the, I need the base resource type for the ErlNifResourceType*`, problem too. I would have to do some tests to see if there is a way to recursively traverse base classes using template metaprogramming.

Otherwise, we could introduce fine::ResourcePtr<T>::make_polymorphic<U, Args...>(Args&&...), and have fine::make_resource<T, Args..>(Args&&...) stay the same. This would make it even more explicit (and more of an advanced API), as well as make the base and concrete templated types more distinguishable.

[brodeuralexis]

As an aside, I have the feeling that nifpp was also trying to get this to work:
https://github.com/saleyn/nifpp/blob/main/nifpp.h#L747-L751

But while they have the dynamic cast working, their resource creation is not polymorphic.

[jonatanklosko]

We could introduce fine::Resource with a virtual destructor(ErlNifEnv*) method, and require fine::Registration::register_resource for T where std::is_base_of_v<Resource, T>.

I did that initially, but I ended up no liking it, because it limits resources only to user-defined classes. And yes, we could have both, but I really think we should have only one abstraction.

I would have to do some tests to see if there is a way to recursively traverse base classes using template metaprogramming.

It's likely doable, though the problem is that someone may also use a derived class as a resource (they may not even know it's derived) and in that case we don't want to find the base one.

I don't like the divergence from std pointers, but yeah, passing both classes explicitly may be the only reasonable way.

[brodeuralexis]

Yeah, I've had no luck with trying to get base classes through templates without some kind of reflection.

If we include this, I would go for a fine::ResourcePtr<Base>::make_polymorphic<Concrete, Args...>(Args&&...) function. This makes the base and concrete type easily identifiable. fine::make_resource<T, Args...>(Args&&...)'s behaviour would be left unchanged.

This would obviously be a pretty advanced/complex feature for fine. While I believe its inclusion to be worthwhile, I will defer to your opinion, since, as you correctly pointed out, a layer of indirection can always be added through a std::unique_ptr inside the resource to solve the problem at the cost of an extra allocation.

[jonatanklosko]

Yeah, I would rather not expand the API for this purpose, so for now I would not do it. Thank you for discussing this through!