InitHandle should check whether a type with the same class name has already been registered

This would add a little bit of runtime cost at registration, but should be helpful for catching programming errors, especially with automatic registration, since there will be no central point where possible duplication become visible.

InitHandle should panic when a duplicate name is registered, because that is always a programming error. This should not constitute a breaking change, by the same reasoning.

Tracking registration state can also help us improve the runtime error when users try to Instance::new a type that they forgot to register.

[jacobsky]

@toasteater If this issue is still something that needs to be addressed (I didn't see anything that looked like this in InitHandle), I'd be happy to look into this issue. Please find my initial investigation/proposed solution below.

After taking a look at the InitHandle struct and impl, it looks like this shouldn't be too difficult to implement by using a HashSet<std::any::TypeId> and just a include a check/insertion of the type at the beginning of the InitHandle::add_maybe_tool_class<C>() function.

For the native class there can be a check in either Instance::new() or Instance::maybe_emplace() that checks that the type id is already registered or not.

After doing some more digging into NativeClass and comparing it to InitHandle, it looks like it will be necessary to include the information as a global value to store the registration.

This leads me to a couple of questions that I'd like to ask before tackling this issue in earnest.

1. What is the preferred way of handling the a necessary global value for this project? Using parking_lot::Once or would it be fine to use a static mut HashTable?
2. As a follow-up, I assume that there will need to be changes made to ensure that the registry is cleared when godot_gdnative_terminate!() is called. As I am less that familiar for this, do you have any information about where this change would need to be made?
3. As this will add a small performance hit, should this only be conditionally compiled into debug builds? Or it is fine to let it stay in all builds?
4. Anything else that I missed?

Sorry to bother with all of the above details, but this seems like an interesting place to work on, so I'd be happy to take a deeper look at this at work on a PR.

I think @Bromeon said that he was going to work on this but it has been a while since I heard about this from him. I'm not sure if he is still planning to do this.

there can be a check in either Instance::new() or Instance::maybe_emplace() that checks that the type id is already registered or not.

This should make error reporting a little bit better, although the current error message when a type isn't registered already hints at the possibility. I'd consider this optional. I wonder if we'd still need global state if you omit this check? Seems to me that it might suffice to just put the hashset in InitHandle in that case? It will have to lose Copy and Clone, but I don't think that's a huge loss if we manage to avoid the global state. It might not have been a good idea to give InitHandle Copy in the first place, anyway. It gives away too much information about the implementation, and doesn't make a lot of sense semantically either.

What is the preferred way of handling the a necessary global value for this project?

I'd prefer something like Lazy from once_cell than using parking_lot::Once or even static mut directly. You might want to put a parking_lot::Mutex inside the OnceCell or Lazy.

there will need to be changes made to ensure that the registry is cleared... do you have any information about where this change would need to be made?

Cleanup is done in gdnative_core::private::cleanup_internal_state.

As this will add a small performance hit, should this only be conditionally compiled into debug builds? Or it is fine to let it stay in all builds?

I think this should be fine in all builds, since the runtime cost is mostly paid during initialization, and it's unlikely for someone to be registering 1000s of types, where such an overhead might even start to matter.

Anything else that I missed?

I think your plan should work well enough!

[jacobsky]

@toasteater Thank you for all of the additional explanation.

If we omit the check in NativeClass then I can confirm that there would be no need for this to contain global state. My first look included putting the HashSet directly in the InitHandle, so that change would be pretty simple if doing the check in NativeScript is not desired.

@Bromeon as previously mentioned above, were you already looking into this, or should I give it a shot? If so, do you have any additional feedback on the proposed plan before I dive in? I'll adapt my future changes accordingly.

[Bromeon]

It was not this exact issue, but deduplication of registered properties that I wanted to handle. I have the code lying around and can probably finish it this week. But I don't think it affects the way this issue would be handled, so you might as well start if you prefer 🙂

[jacobsky]

@Bromeon thank you for letting me know. I'll go ahead and get started.

For now, I think I'll try starting with the implementation that uses a global registry with the initialization to allow native_class_registry to check during Instance::new() and during InitHandle::add_maybe_tool_class
If handling the global registry proves too unwieldy then I'll go ahead and fallback to adding the HashSet directly to the InitHandle.

Thank you for all of the help!

[jacobsky]

It took a surprisingly short amount of time to get an initial version of this going, but I did find that it looks like there is no need to implement this on the NativeClass to ensure that it has been registered.

If a class is not registered, it seems that you would get an error in "modules/gdnative/nativescript/nativescript.cpp", so from some initial testing it seems like going for the global registry doesn't actually help us detect classes that aren't added as godot isn't able to instantiate them anyways.

I realized that this doesn't apply when classes are instanced from rust. This still allows for a better panic error.

I'll also look into changing the InitHandle, since it's quite a bit cleaner and doesn't require any additional dependencies. I will also note that doing this does break a the API pretty heavily as it cannot derive copy and will require that most of the init methods take a borrow rather than a move for the InitHandle type.