[gameplaykit] Add nullability to (generated and manual) bindings #14615

tj-devel709 · 2022-04-01T19:38:30Z

This PR aims to bring nullability changes to GameplayKit.
Following the steps here:

I am adding nullable enable to all manual files that are not "API_SOURCES" in src/frameworks.sources and making the required nullability changes
Changing all throw new ArgumentNullException ("object")); to ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (object)); for size saving optimization as well to mark that this framework contains nullability changes
Changing any == null or != null to is null and is not null

tj-devel709 · 2022-04-01T19:40:06Z

src/GameplayKit/GKBehavior.cs

@@ -18,7 +20,7 @@ public partial class GKBehavior {
 		}

 		public NSNumber this [GKGoal goal] {
-			get { return ObjectForKeyedSubscript (goal); }
+			get { return ObjectForKeyedSubscript (goal)!; }


From here: https://developer.apple.com/documentation/gameplaykit/gkbehavior/1388723-objectforkeyedsubscript?language=objc

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.

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?

@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

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.

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

mandel-macaque · 2022-04-01T20:52:22Z

src/GameplayKit/GKBehavior.cs

@@ -18,7 +20,7 @@ public partial class GKBehavior {
 		}

 		public NSNumber this [GKGoal goal] {
-			get { return ObjectForKeyedSubscript (goal); }
+			get { return ObjectForKeyedSubscript (goal)!; }


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?

vs-mobiletools-engineering-service2 · 2022-04-01T22:56:14Z

✅ Tests on macOS Mac Catalina (10.15) passed ✅

Tests passed

All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Merge 69eeeee into 180b561

vs-mobiletools-engineering-service2 · 2022-04-01T23:15:53Z

❌ Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Tests timed out

Pipeline on Agent
Merge 69eeeee into 180b561

dalexsoto

After Manuel's 👍

…GameplayKit

vs-mobiletools-engineering-service2 · 2022-04-11T21:43:55Z

🔥 Tests failed catastrophically on VSTS: simulator tests iOS (no summary found). 🔥

Result file D:\a\1\s\Reports\TestSummary-simulator\TestSummary.md not found.

Pipeline on Agent
Merge 249735d into 8ef3850

vs-mobiletools-engineering-service2 · 2022-04-11T21:50:46Z

✅ Tests on macOS Mac Catalina (10.15) passed ✅

Tests passed

All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Merge 249735d into 8ef3850

vs-mobiletools-engineering-service2 · 2022-04-11T22:18:49Z

❌ Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Tests timed out

Pipeline on Agent
Merge 249735d into 8ef3850

mandel-macaque · 2022-04-12T13:46:10Z

The errors are because the branch is behinds main and is using the main yaml. merging should fix the issue.

vs-mobiletools-engineering-service2 · 2022-04-12T15:31:27Z

✅ Tests on macOS Mac Catalina (10.15) passed ✅

Tests passed

All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Merge c26823c into 8b8cf07

vs-mobiletools-engineering-service2 · 2022-04-12T15:48:02Z

❌ Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Failed tests are:

introspection
linksdk
xammac_tests
monotouch-test

Pipeline on Agent
Merge c26823c into 8b8cf07

vs-mobiletools-engineering-service2 · 2022-04-27T21:17:49Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1105.Monterey'
Hash: 711b26f7e2f703dbb83a9d469b97ce1d01cd2271

vs-mobiletools-engineering-service2 · 2022-04-27T21:30:34Z

📋 [PR Build] API Diff 📋

API Current PR diff

✅ API Diff (from PR only) (no change)

View API diff

iOS vsdrops gist
macOS vsdrops gist
tvOS vsdrops gist
watchOS vsdrops gist

View dotnet API diff

dotnet-iOS vsdrops gist
dotnet-macOS vsdrops gist
dotnet-tvOS vsdrops gist
dotnet-MacCatalyst vsdrops gist

View dotnet legacy API diff

dotnet-legacy-iOS vsdrops gist
dotnet-legacy-macOS vsdrops gist
dotnet-legacy-tvOS vsdrops gist

View dotnet iOS-MacCatalayst API diff

dotnet-macCatiOS vsdrops gist

API diff

✅ API Diff from stable

View API diff

iOS vsdrops gist
macOS vsdrops gist
tvOS vsdrops gist
watchOS vsdrops gist

View dotnet API diff

dotnet-iOS vsdrops gist
dotnet-macOS vsdrops gist
dotnet-tvOS vsdrops gist
dotnet-MacCatalyst vsdrops gist

View dotnet legacy API diff

dotnet-legacy-iOS vsdrops gist
dotnet-legacy-macOS vsdrops gist
dotnet-legacy-tvOS vsdrops gist

View dotnet iOS-MacCatalayst API diff

dotnet-macCatiOS vsdrops gist

Generator diff

✅ Generator Diff (no change)

Pipeline on Agent XAMBOT-1107.Monterey'
Hash: 711b26f7e2f703dbb83a9d469b97ce1d01cd2271

vs-mobiletools-engineering-service2 · 2022-04-27T21:46:20Z

💻 [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
Hash: 711b26f7e2f703dbb83a9d469b97ce1d01cd2271

vs-mobiletools-engineering-service2 · 2022-04-27T21:48:44Z

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

✅ All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Hash: 711b26f7e2f703dbb83a9d469b97ce1d01cd2271

vs-mobiletools-engineering-service2 · 2022-04-28T05:14:22Z

✅ [CI Build] Tests passed on VSTS: simulator tests iOS. ✅

Tests passed on VSTS: simulator tests iOS.

🎉 All 148 tests passed 🎉

Pipeline on Agent XAMBOT-1107.Monterey'
Merge 711b26f into ffcfabd

tj_devel709 added 3 commits April 1, 2022 11:04

adding Nullability

243b689

throw better null exceptions

89f8375

use is null

be3d02f

tj-devel709 added the not-notes-worthy Ignore for release notes label Apr 1, 2022

tj-devel709 added this to the Future milestone Apr 1, 2022

tj-devel709 requested a review from dalexsoto as a code owner April 1, 2022 19:38

tj-devel709 commented Apr 1, 2022

View reviewed changes

Merge branch 'main' into Nullable-GameplayKit

69eeeee

mandel-macaque requested changes Apr 1, 2022

View reviewed changes