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#: Enable nullable environment for GodotSharp #83117

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Oct 10, 2023

This PR showcases a GodotSharp environment where nullability is enabled. While initially conceived as a means to centralize changes which can be branched off into standalone PRs (eg: #82980, #82983), it also showcases the consequences of trying to retrofit the concept without first undergoing significant refactoring

The biggest culprit is GodotObject, and how it's uniquely a reference type that Godot allows for nullability. While this draft explores the idea of returning GodotObject fields as nullable, I'm not convinced that makes for the best approach, because it floods almost every existing method/field with an added nullability check. It might be worth considering an approach similar to Unity Objects, where there's operator overloads & behind-the-scenes null checks to better handle null in the context of that environment. Or it could even be as simple as throwing in a bunch of [AllowNull]/[MaybeNull] annotations in the generation for object files

While the majority of areas have warnings accounted for, there's still a handful that have them. This is mainly those that I'm not entirely sure yet how to handle, or areas that feel particularly sensitive to adjustment. It's also entirely possible that many of these warnings/cases simply wouldn't be a thing with future core additions (particularly, some means of knowing if an object return value can be null). Either way, this is provided as-is for discussion and so potential areas can be cherrypicked for implementation as needed

@Repiteo Repiteo requested a review from a team as a code owner October 10, 2023 17:54
@Repiteo Repiteo marked this pull request as draft October 10, 2023 17:54
@Repiteo Repiteo changed the title C#: GodotSharp nullable enabled [Draft] C#: GodotSharp nullable enabled Oct 10, 2023
@Repiteo Repiteo force-pushed the c#-godotsharp-nullable branch 3 times, most recently from b7f9e49 to 020bd83 Compare October 10, 2023 22:51
@Calinou Calinou added this to the 4.x milestone Oct 10, 2023
@Repiteo Repiteo force-pushed the c#-godotsharp-nullable branch 3 times, most recently from 21cf810 to a99183a Compare October 11, 2023 15:47
@Repiteo

This comment was marked as outdated.

@Repiteo Repiteo force-pushed the c#-godotsharp-nullable branch 5 times, most recently from 99d811d to 1f238f0 Compare October 14, 2023 20:30
@Repiteo

This comment was marked as outdated.

@Repiteo Repiteo force-pushed the c#-godotsharp-nullable branch 2 times, most recently from 5fb0769 to 3156ddd Compare October 17, 2023 16:28
@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 17, 2023

Walked back the above two posts; I won't be adding any new functions nor expanding nullability to Godot.SourceGenerators.Sample. Both are outside the original scope of this PR and id prefer to keep things focused on ensuring a nullable environment for GodotSharp and GodotSharpEditor

@Repiteo Repiteo force-pushed the c#-godotsharp-nullable branch 3 times, most recently from bf80c79 to f2f8da3 Compare December 1, 2023 16:26
@AThousandShips AThousandShips changed the title [Draft] C#: GodotSharp nullable enabled [Draft] [C#] Enable nullable environment for GodotSharp Dec 1, 2023
@Repiteo
Copy link
Contributor Author

Repiteo commented Dec 1, 2023

As this PR is now functionally-sound entirely in isolation, I'm comfortable removing the "draft" qualifier. While it can still serve as a means of seeing how individual components would be handled, this should be just as viable to merge as-is

There's still no real answer for how best to handle GodotObject being implicitly nullable for the overwhelming majority of cases, but upon further reflection that's not really in the scope of this PR. This was about getting the existing functionality into a nullable environment, and GodotObject just so happens to be the one and only reference type that's truly nullable, and implicitly so

@Repiteo Repiteo marked this pull request as ready for review December 1, 2023 17:48
@Repiteo Repiteo changed the title [Draft] [C#] Enable nullable environment for GodotSharp C#: Enable nullable environment for GodotSharp Dec 1, 2023
@dsnopek
Copy link
Contributor

dsnopek commented Dec 1, 2023

There's still no real answer for how best to handle GodotObject being implicitly nullable for the overwhelming majority of cases

I'm not familiar with the specifics of this PR, but this bit sounds like it might be related to this proposal that we were discussing at the GDExtension team meeting today: godotengine/godot-proposals#2241

@Repiteo
Copy link
Contributor Author

Repiteo commented Dec 1, 2023

That proposal would indeed be the solution! This PR introduces nullability in an environment that lacks those hints, so GodotObject is always treated as a nullable reference type (with very few already-guaranteed exceptions, like engine singletons), but all other reference types are never nullable (strings, StringName, NodePath, etc), just by nature of the current Godot implementations

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.

3 participants