[gameplaykit] Add nullability to (generated and manual) bindings #14615
Conversation
src/GameplayKit/GKBehavior.cs
Outdated
| @@ -18,7 +20,7 @@ public partial class GKBehavior { | |||
| } | |||
|
|
|||
| public NSNumber this [GKGoal goal] { | |||
| get { return ObjectForKeyedSubscript (goal); } | |||
| get { return ObjectForKeyedSubscript (goal)!; } | |||
There was a problem hiding this comment.
Return Value The weight to be applied to the goal’s influence on an agent’s speed and direction, or 0.0 if the goal is not in the behavior.
There was a problem hiding this comment.
Why do we need the !? I'm looking at the bindings and we have:
NSNumber ObjectForKeyedSubscript (GKBehavior behavior);
There is no nullable value, is there?
There was a problem hiding this comment.
@mandel-macaque were you looking at this one inside GKCompositeBehavior https://github.com/xamarin/xamarin-macios/blob/main/src/gameplaykit.cs#L336?
This one refers to the ObjectForKeyedSubscript inside GKBehavior https://github.com/xamarin/xamarin-macios/blob/main/src/gameplaykit.cs#L227 which has a nullable return
There was a problem hiding this comment.
You pasted the documentation, as per the docs: "or 0.0 if the goal is not in the behavior." so it will always return a NSNumber and if the behaviour is not there, it will be 0.0. So, the indexer seems to be wrong:
- The binding says we get a nullable, the docs say otherwise, we should add a test for that.
- If we return a 0.0 in an indexer is wrong. Indexers should throw an exception if the GKBejaviour is not present.
We should get the value, check the result and decide if we have a null or not and throw and except and remove the ! which is wrong and makes no sense. The binding is wrong and therefore the nullability.
There was a problem hiding this comment.
Note for future TJ:
The tests show the return value is indeed null when the goal is not in the behavior, add that if this value returns null, throw an ArgumentOutOfRangeException, and file a radar with Apple that the APIs behavior is different than their implementation
There was a problem hiding this comment.
Note for future TJ:
The tests show the return value is indeed null when the goal is not in the behavior, add that if this value returns null, throw an ArgumentOutOfRangeException, and file a radar with Apple that the APIs behavior is different than their implementation
src/GameplayKit/GKBehavior.cs
Outdated
| @@ -18,7 +20,7 @@ public partial class GKBehavior { | |||
| } | |||
|
|
|||
| public NSNumber this [GKGoal goal] { | |||
| get { return ObjectForKeyedSubscript (goal); } | |||
| get { return ObjectForKeyedSubscript (goal)!; } | |||
There was a problem hiding this comment.
Why do we need the !? I'm looking at the bindings and we have:
NSNumber ObjectForKeyedSubscript (GKBehavior behavior);
There is no nullable value, is there?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🔥 Tests failed catastrophically on VSTS: simulator tests iOS (no summary found). 🔥Result file D:\a\1\s\Reports\TestSummary-simulator\TestSummary.md not found. |
|
The errors are because the branch is behinds main and is using the main yaml. merging should fix the issue. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
📋 [PR Build] API Diff 📋API Current PR diff✅ API Diff (from PR only) (no change) View dotnet API diffView dotnet legacy API diffAPI diff✅ API Diff from stable View dotnet API diffView dotnet legacy API diffGenerator diff✅ Generator Diff (no change) Pipeline on Agent XAMBOT-1107.Monterey' |
💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11.5) passed. Pipeline on Agent |
💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻✅ All tests on macOS Mac Catalina (10.15) passed. Pipeline on Agent |
This PR aims to bring nullability changes to GameplayKit.
Following the steps here:
nullable enableto all manual files that are not "API_SOURCES" in src/frameworks.sources and making the required nullability changesthrow new ArgumentNullException ("object"));toObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (object));for size saving optimization as well to mark that this framework contains nullability changes== nullor!= nulltois nullandis not null