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

C#: Make internal properties and property accessors public (but hidden) #90002

Merged

Conversation

raulsntos
Copy link
Member

Instead of making the accessors internal which can break binary compat, make them public but hide them with EB never so they don't show up in IntelliSense.

Also, do the same for properties with the PROPERTY_USAGE_INTERNAL flag. These properties were not meant to be exposed to scripting, but since they've been public before all we can do now is hide them.

@AThousandShips
Copy link
Member

AThousandShips commented Mar 29, 2024

I like this generally, but one potential compatibility issue:
Does it count as shadowing/blocking internal methods of parent classes in C#?

I.e. if I extend Animation and I decided to add a GetLength method to it, will this break now that Animation.GetLength goes from internal to public? Regardless of that being appropriate or not it's not a method anyone would know existed

Edit: To be clear I don't think it's a major issue, and we often add new methods and that might trigger the same effect, but we're exposing methods en-masse here and it's worth thinking about I think, especially since they're hidden they might cause confusion, depending on how it's handled in C# (I don't know enough of C#, it's been years since I worked with it, so it might just be fine to have shadowing etc.)

@@ -179,6 +187,14 @@ class BindingsGenerator {
*/
bool is_internal = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to update the comment on this, it speaks of setters and getters but are not internal any longer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this still applies, it refers to property getters and setters when the method starts with an underscore and are not virtual. For example: AnimationLibrary::_set_data.

@@ -3863,7 +3872,7 @@ bool BindingsGenerator::_populate_object_type_interfaces() {
// We only make internal an accessor method if it's in the same class as the property.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment as well

Instead of making the accessors `internal` which can break binary compat, make them `public` but hide them with EB never so they don't show up in IntelliSense.

Also, do the same for properties with the `PROPERTY_USAGE_INTERNAL` flag. These properties were not meant to be exposed to scripting, but since they've been public before all we can do now is hide them.
@raulsntos raulsntos force-pushed the dotnet/expose-property-accessors branch from 9cd722f to 7d08e87 Compare March 29, 2024 19:36
@raulsntos
Copy link
Member Author

I.e. if I extend Animation and I decided to add a GetLength method to it, will this break now that Animation.GetLength goes from internal to public?

It doesn't break binary compatibility if that's what you are asking. Your GetLength method shadows the inherited method and that will now give you a warning that you can suppress with the new modifier if shadowing was intended.

@AThousandShips
Copy link
Member

That answers my question that the warning isn't an issue as such, as opposed to virtual methods in C++ etc., good!

@akien-mga akien-mga merged commit 1a6e4ce into godotengine:master Apr 22, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@raulsntos raulsntos deleted the dotnet/expose-property-accessors branch April 22, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C# API] Possible type mismatch of property Node.AnchorsPreset
4 participants