Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add protections against registering classes that didn't use GDCLASS() #1279

Merged
merged 1 commit into from
Oct 22, 2023

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Oct 21, 2023

This adds the same protections against registering classes that didn't use GDCLASS(), as were added to the engine in PR godotengine/godot#81020

This same problem affects godot-cpp, but to a much lesser degree. I was able to reproduce the issue by making a class heirarchy like:

class Example : public Object {
    GDCLASS(Example, Object);

protected:
    static void _bind_methods() { };
};

class ExampleChild : public Example {
};

With out this PR, registering ExampleChild was allowed to go ahead, which meant really registering Example again under a different name. However, with this PR, it gives the compiler error Class not declared properly, please use GDCLASS., just like in the engine.

@dsnopek dsnopek added the bug This has been identified as a bug label Oct 21, 2023
@dsnopek dsnopek added this to the 4.x milestone Oct 21, 2023
@dsnopek dsnopek requested a review from a team as a code owner October 21, 2023 22:59
@AThousandShips
Copy link
Member

AThousandShips commented Oct 22, 2023

What happens if you register a custom class with a built-in parent? The check for typename T::self_type should not be a valid type, should this also be added to the GDEXTENSION_CLASS_ALIAS and Wrapped like it is int the engine (With Object)?

I.e. add typedef Wrapped self_type; to Wrapped, and add the define to the other macro, just for completeness (or just to Wrapped, that would ensure there's a self_type to check against)

@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 22, 2023

What happens if you register a custom class with a built-in parent?

Well, you'd get other compiler errors even without the static_assert():

/home/dsnopek/Sync/Projects/default/godot-cpp/include/godot_cpp/core/class_db.hpp:196:41: error: 'has_get_property_list' is not a member of 'ExampleNoGDCLASS'
  196 |                 T::has_get_property_list() ? T::get_property_list_bind : nullptr, // GDExtensionClassGetPropertyList get_property_list_func;
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~^~
/home/dsnopek/Sync/Projects/default/godot-cpp/include/godot_cpp/core/class_db.hpp:205:20: error: 'create' is not a member of 'ExampleNoGDCLASS'
  205 |                 T::create, // GDExtensionClassCreateInstance create_instance_func; /* this one is mandatory */
      |                    ^~~~~~
/home/dsnopek/Sync/Projects/default/godot-cpp/include/godot_cpp/core/class_db.hpp:206:20: error: 'free' is not a member of 'ExampleNoGDCLASS'
  206 |                 T::free, // GDExtensionClassFreeInstance free_instance_func; /* this one is mandatory */
      |                    ^~~~
/home/dsnopek/Sync/Projects/default/godot-cpp/include/godot_cpp/core/class_db.hpp:207:20: error: 'recreate' is not a member of 'ExampleNoGDCLASS'
  207 |                 T::recreate, // GDExtensionClassRecreateInstance recreate_instance_func;

Because native classes with GDEXTENSION_CLASS() (as opposed to GDCLASS()) are missing a number of other things that are needed to register a class.

However, I suppose it'd be nice to have the static_assert() message in there too, because it's more helpful? Or, we could even add stub implementations of the missing methods to GDEXTENSION_CLASS() so there's only the helpful message?

@AThousandShips
Copy link
Member

AThousandShips commented Oct 22, 2023

I'd say that sounds good! At minimum adding the definition to Wrapped to get a clean error message, and see if that resolves the rest, as in catches the error and stops there, it will give you something descriptive you can solve otherwise it gets cryptic

@dsnopek dsnopek force-pushed the gdclass-protections branch from 002987c to a61cdc8 Compare October 22, 2023 13:44
@AThousandShips
Copy link
Member

AThousandShips commented Oct 22, 2023

Just for completeness I'd add:

typedef Wrapped self_type;

To wrapped, though extending wrapped directly shouldn't happen it'd ensure the check doesn't break (and has parity with the engine)

@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 22, 2023

I added the stub methods as well as self_type on native classes, and so in my latest push, you'll get the same helpful error (rather than the cryptic ones I posted above) if you try to register a class declared like:

class ExampleNoGDCLASS : public Object {
};

@dsnopek dsnopek merged commit c82f2a3 into godotengine:master Oct 22, 2023
12 checks passed
@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 23, 2023

Cherry-picked for 4.1 in PR #1281

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants