-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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 nullability for variant classes #82983
[C#] Enable nullability for variant classes #82983
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for helping annotate the codebase for nullability.
I feel like a lot of these cases shouldn't allow null
. It looks like you might have just added ?
to the parameters that are checked for nullability, but that doesn't necessarily mean a null value is acceptable.
I appreciate this being a separate PR, because I think it may take some time to decide the nullability of some of these parameters.
modules/mono/glue/GodotSharp/GodotSharp/Core/GodotObject.base.cs
Outdated
Show resolved
Hide resolved
Oh! Well then, that's a misconception on my end. In that case I would assume the null checks being for non-null types are for when they're called via a null-oblivious environment? |
Yes or when the user intentionally ignores the warning or uses the null forgiving operator (
So, nullable types can be used to document if the parameter allows |
d40d02d
to
c26f35e
Compare
Made some expansions to After this, I'll probably make a draft PR that just converts everything to nullable, and future PRs will simply piecemeal from that |
c26f35e
to
b7d9c4f
Compare
Added null checks to the typed collection constructors taking their untyped collection equivalents, as them not being there before seems to be an oversight ( |
modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs
Outdated
Show resolved
Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs
Outdated
Show resolved
Hide resolved
b7d9c4f
to
8e74445
Compare
1ea013c
to
1455224
Compare
1455224
to
63f14c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference types should implement IEquatable<T?>
, that means NodePath
should implement IEquatable<NodePath?>
but right now it implements IEquatable<NodePath>
(notice the missing ?
).
63f14c4
to
21b390a
Compare
6443844
to
4effd95
Compare
4effd95
to
08e4412
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thanks! |
Builds for the .NET version on the build server fail with these errors, which I believe are related to this PR:
And also with
which is related to #85975. Fix: #86373. |
Another minor update on the road to GodotSharp being nullable by default. In a similar vein to #82980, this focuses only on Variant types, but the class-equivalents instead. Still mostly small changes, but this is comparatively more involved than the structs, so it warrants its own PR