[corefoundation] Add nullability to (generated and manual) bindings by tj-devel709 · Pull Request #15090 · dotnet/macios

tj-devel709 · 2022-05-20T22:38:43Z

This PR aims to bring nullability changes to CoreFoundation.
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-05-20T22:42:11Z

 		public bool Transform (ref CFRange range, string transform, bool reverse)
 		{
-			var t = NSString.CreateNative (transform);
+			var t = CreateNative (transform);


Context, NSString.CreateNative (...) is obsolete and the recommendation was to use CFString.CreateNative (...)

tj-devel709 · 2022-05-20T22:42:20Z

 		public bool Transform (string transform, bool reverse)
 		{
-			var t = NSString.CreateNative (transform);
+			var t = CreateNative (transform);


Context, NSString.CreateNative (...) is obsolete and the recommendation was to use CFString.CreateNative (...)

I love the fact that you are giving context in your changes, appreciate it.

rolfbjarne · 2022-05-23T14:46:31Z


 		public IntPtr Handle {
-			get { return data.Handle; }
+			get { return data?.Handle ?? IntPtr.Zero; }


Suggested change

get { return data?.Handle ?? IntPtr.Zero; }

get { return data.GetHandle (); }

I tried to use GetHandle () here but it was not supported.. I'll double check why!

Oop was just missing the namespace :)

rolfbjarne · 2022-05-23T14:48:56Z

-				if (name is null)
-					throw new ArgumentNullException (nameof (name), "When using the Darwin Notification Center, the value passed must not be null");
+			if (darwinnc is not null && darwinnc.Handle == Handle && name is null){
+				ObjCRuntime.ThrowHelper.ThrowArgumentNullException ($"{nameof (name)}: When using the Darwin Notification Center, the value passed must not be null");


I think you have to add another ThrowArgumentNullException overload that takes the message in addition to the parameter name.

@rolfbjarne There is not one in the ObjCRuntime.ThrowHelper class, but I could easily create one!

Yep, adding a new method is the way to go :)

rolfbjarne · 2022-05-23T14:51:21Z

 				}

-				throw new ArgumentException ("unsupported type: " + value.GetType (), "value");
+				if (value is not null)


This looks wrong, I think there's a missing return; statement after line 127, where it already handles the value is null case. Looks like the nullability attributes caught a bug here :)

I believe you are correct.

rolfbjarne · 2022-05-23T14:54:02Z

+			if (left as object is null)
+				return right as object is null;


You don't need the as object part:

Suggested change

if (left as object is null)

return right as object is null;

if (left is null)

return right is null;

rolfbjarne · 2022-05-23T14:54:16Z

+			if (left as object is null)
+				return right as object is not null;


Suggested change

if (left as object is null)

return right as object is not null;

if (left is null)

return right is not null;

mandel-macaque

After all rolfs comments have been addressed.

mandel-macaque · 2022-05-23T20:17:04Z

 		public bool Transform (string transform, bool reverse)
 		{
-			var t = NSString.CreateNative (transform);
+			var t = CreateNative (transform);


I love the fact that you are giving context in your changes, appreciate it.

mandel-macaque · 2022-05-23T20:20:42Z

 				}

-				throw new ArgumentException ("unsupported type: " + value.GetType (), "value");
+				if (value is not null)


I believe you are correct.

mandel-macaque · 2022-05-23T20:25:26Z

+				string? username = proxy?.Username;
+				string? password = proxy?.Password;
+				string? hostname = proxy?.HostName;
+				int? port = proxy?.Port;


var here would have make you type less :)

tj-devel709 · 2022-05-31T16:22:02Z

@rolfbjarne Changes addressed!

rolfbjarne · 2022-05-31T18:14:50Z

-		public NSString Domain {get; private set;}
-		public string FailureReason {get; private set;}
-		public string RecoverySuggestion {get; private set;}
+		public nint? Code {get; private set;}


You can't make this nullable, because it's a struct and that would be a breaking change.

rolfbjarne · 2022-05-31T18:15:22Z

 	public class CFException : Exception {

-		public CFException (string description, NSString domain, nint code, string failureReason, string recoverySuggestion)
+		public CFException (string? description, NSString? domain, nint? code, string? failureReason, string? recoverySuggestion)


You can't make the code argument nullable, because it's a struct and it would be a breaking change.

tj-devel709 · 2022-06-02T16:06:13Z

@rolfbjarne Changes applied!

vs-mobiletools-engineering-service2 · 2022-06-02T17:44:58Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1036.Monterey
Hash: 24e648b914cc2adec8e05db055c2ce8653165be0

vs-mobiletools-engineering-service2 · 2022-06-02T17:45:57Z

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

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

Pipeline on Agent
Hash: 24e648b914cc2adec8e05db055c2ce8653165be0

vs-mobiletools-engineering-service2 · 2022-06-02T17:59:16Z

📋 [PR Build] API Diff 📋

API diff (for current PR)

ℹ️ API Diff (from PR only) (please review changes)

API diff: vsdrops gist

Xamarin

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

.NET

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

Xamarin vs .NET

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

iOS vs Mac Catalyst (.NET)

dotnet-macCatiOS vsdrops gist

API diff (vs stable)

✅ API Diff from stable

API diff: vsdrops gist

Xamarin

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

.NET

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

Xamarin vs .NET

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

iOS vs Mac Catalyst (.NET)

dotnet-macCatiOS vsdrops gist

Generator diff

✅ Generator Diff (no change)

Pipeline on Agent XAMBOT-1100.Monterey
Hash: 24e648b914cc2adec8e05db055c2ce8653165be0

vs-mobiletools-engineering-service2 · 2022-06-03T01:59:23Z

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Tests failed on VSTS: simulator tests iOS.

Test results

4 tests failed, 144 tests passed.

Failed tests

fsharp/watchOS 32-bits - simulator/Debug: Crashed
introspection/watchOS 32-bits - simulator/Debug: Crashed
dont link/watchOS 32-bits - simulator/Debug: Crashed
mono-native-compat/watchOS 32-bits - simulator/Debug: Crashed

Pipeline on Agent XAMBOT-1099.Monterey'
Merge 24e648b into 0307004

chamons · 2022-06-06T12:51:14Z

+				var username = proxy?.Username;
+				var password = proxy?.Password;
+				var hostname = proxy?.HostName;
+				var port = proxy?.Port;


I don't love the usage of var here where the right hand size doesn't make it obvious if Port is a int or a string for example, but I think it's fine enough.

tj-devel709 · 2022-06-06T14:56:41Z

Unrelated Test Failures: https://github.com/xamarin/maccore/issues/2558

tj_devel709 added 3 commits May 20, 2022 14:35

enable nullability

816fee8

throw better exceptions

4730f4b

use is null

aadb7d5

tj-devel709 added the not-notes-worthy Ignore for release notes label May 20, 2022

tj-devel709 added this to the Future milestone May 20, 2022

tj-devel709 requested a review from rolfbjarne as a code owner May 20, 2022 22:38

tj-devel709 commented May 20, 2022

View reviewed changes

use a gethandle ()

e940a59

This comment has been minimized.

Sign in to view

rolfbjarne requested changes May 23, 2022

View reviewed changes

mandel-macaque approved these changes May 23, 2022

View reviewed changes

tj_devel709 added 2 commits May 31, 2022 10:32

apply PR changes

d5e81f4

Merge remote-tracking branch 'xamarin/main' into Nullable-CoreFoundation

2387e51

rolfbjarne approved these changes May 31, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

rolfbjarne requested changes May 31, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

remove nullability from struct

24e648b

rolfbjarne approved these changes Jun 2, 2022

View reviewed changes

chamons approved these changes Jun 6, 2022

View reviewed changes

tj-devel709 merged commit 229dd2e into dotnet:main Jun 6, 2022

	get { return data?.Handle ?? IntPtr.Zero; }
	get { return data.GetHandle (); }

		if (left as object is null)
		return right as object is not null;

Conversation

tj-devel709 commented May 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mandel-macaque left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tj-devel709 commented May 31, 2022

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tj-devel709 commented Jun 2, 2022

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 2, 2022

📚 [PR Build] Artifacts 📚

Packages generated

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 2, 2022

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

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 2, 2022

📋 [PR Build] API Diff 📋

API diff (for current PR)

API diff (vs stable)

Generator diff

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 3, 2022

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Test results

Failed tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tj-devel709 commented Jun 6, 2022

Uh oh!

Reviewers