[coremidi] Add nullability to (generated and manual) bindings #15098

tj-devel709 · 2022-05-23T15:13:29Z

This PR aims to bring nullability changes to CoreMidi.
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-23T15:15:43Z

src/CoreMidi/MidiServices.cs

 			get {
 				return GetData (kMIDIPropertyConnectionUniqueID);
 			}
 			set {
+				if (value is null)
+					ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (value));


While I realize that I added a good amount of these and that SetData () throws an exception if the value arg is null, I thought it would be better to explicitly add the exceptions here and not use a !, but I am open to opinions!

Explicit over implicit, I like it.

tj-devel709 · 2022-05-23T15:21:16Z

src/CoreMidi/MidiThruConnectionParams.cs

 				connectionParams.NumSources = (uint)Sources.Length;
 				for (int i = 0; i < Sources.Length; i++)
-					connectionParams.Sources[i] = Sources[i];
+					connectionParams.Sources [i] = Sources [i];
 			}


I could possibly see this being a change in behavior since now it's possible that the connectionParams.NumSources could not be set if the connectionParams.Sources is null. I don't think that we want it to be set in that scenario anyways so either this is negligible, I can add a conditional around the for loop (but it looks messy), or I can just add an ! saying the connectionParams.Sources is not null.

Same thing with the connectionParams.NumDestinations a few lines below.

I don't think this is a breaking change:

If Sources.Length is 0, then if NumSources isn't assigned there's no difference (because 0 is the default value)

If Sources.Length > 0, then we'd run into a NullReferenceException in the loop.

So yes, there's a change in behavior, but from a NullReferenceException to no exception, which I think is fine in this scenario.

rolfbjarne · 2022-05-23T15:49:32Z

src/CoreMidi/MidiThruConnectionParams.cs

 				connectionParams.NumSources = (uint)Sources.Length;
 				for (int i = 0; i < Sources.Length; i++)
-					connectionParams.Sources[i] = Sources[i];
+					connectionParams.Sources [i] = Sources [i];
 			}


I don't think this is a breaking change:

If Sources.Length is 0, then if NumSources isn't assigned there's no difference (because 0 is the default value)

If Sources.Length > 0, then we'd run into a NullReferenceException in the loop.

So yes, there's a change in behavior, but from a NullReferenceException to no exception, which I think is fine in this scenario.

mandel-macaque · 2022-05-23T20:07:09Z

src/CoreMidi/MidiServices.cs

@@ -758,7 +761,7 @@ public class MidiPacket
 #if !COREBUILD
 		public long TimeStamp;
 		IntPtr byteptr;
-		byte [] bytes;
+		byte []? bytes;


Setting this to Array.Empty will remove a few things.

mandel-macaque · 2022-05-23T20:08:03Z

src/CoreMidi/MidiServices.cs

+					using (var args = new MidiPacketsEventArgs (packetList)) {
+						e (port, args);
+					}


For the future, you can use using without the {} since it is small.

mandel-macaque · 2022-05-23T20:11:01Z

src/CoreMidi/MidiServices.cs

 			get {
 				return GetData (kMIDIPropertyConnectionUniqueID);
 			}
 			set {
+				if (value is null)
+					ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (value));


Explicit over implicit, I like it.

vs-mobiletools-engineering-service2 · 2022-06-02T15:27:58Z

📋 [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-1033.Monterey'
Hash: a7d838cf5b1640f212cba6b3ccb508a53d060dcc

vs-mobiletools-engineering-service2 · 2022-06-02T15:31:08Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1103.Monterey
Hash: a7d838cf5b1640f212cba6b3ccb508a53d060dcc

vs-mobiletools-engineering-service2 · 2022-06-02T15:40:34Z

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

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

Pipeline on Agent
Hash: a7d838cf5b1640f212cba6b3ccb508a53d060dcc

vs-mobiletools-engineering-service2 · 2022-06-02T22:17:29Z

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

Tests failed on VSTS: simulator tests iOS.

Test results

1 tests failed, 147 tests passed.

Failed tests

xcframework-test/watchOS 32-bits - simulator/Debug: Crashed

Pipeline on Agent XAMBOT-1170.Monterey'
Merge a7d838c into 9822cbc

chamons · 2022-06-06T12:46:50Z

src/CoreMidi/MidiServices.cs

-			if (e != null) {
-				using (var args = new MidiPacketsEventArgs (packetList)) {
-					e (port, args);
+			if (gch.Target is MidiPort port) {


This is subtly different, as before a null gch.Target (or one of the wrong type) would throw, where this will silently no-op.

I think that is fine however.

Ah yes that is a good point!

chamons · 2022-06-06T12:47:19Z

src/CoreMidi/MidiServices.cs

+					using (var args = new MidiPacketsEventArgs (packetList))
+						e (port, args);


Suggested change

using (var args = new MidiPacketsEventArgs (packetList))

e (port, args);

using (var args = new MidiPacketsEventArgs (packetList)) {

e (port, args);

}

I think style wise we want braces for using blocks, but maybe I'm wrong?

Changed this block style as a request from Manuel

chamons · 2022-06-06T12:47:56Z

src/CoreMidi/MidiServices.cs

-			var e = port.MessageReceived;
-			if (e != null)
-				e (port, new MidiPacketsEventArgs (packetList));
+			if (gch.Target is MidiEndpoint port) {


Ditto comment above.

tj-devel709 · 2022-06-06T15:04:19Z

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

. (#16992) Closes #16979 The only `MidiPacket` constructor ever used internally is the one accepting an `IntPtr` argument. Therefore `bytes` will always have the default empty array value. Because of this, in the `Bytes` get accessor `bytes` is not null, but empty and indexing into `bytes` on line 848 throws the exception. The other option would have been to remove the empty array default value that was added in #15098, but the length check seemed like the safer, although maybe slightly less performant, option.

…ception. Fixes #16979. (#17022) Closes #16979 The only `MidiPacket` constructor ever used internally is the one accepting an `IntPtr` argument. Therefore `bytes` will always have the default empty array value. Because of this, in the `Bytes` get accessor `bytes` is not null, but empty and indexing into `bytes` on line 848 throws the exception. The other option would have been to remove the empty array default value that was added in #15098, but the length check seemed like the safer, although maybe slightly less performant, option. Backport of #16992 Co-authored-by: Reid Kirkpatrick <[email protected]>

Fixes #16979. (#17021) Closes #16979 The only `MidiPacket` constructor ever used internally is the one accepting an `IntPtr` argument. Therefore `bytes` will always have the default empty array value. Because of this, in the `Bytes` get accessor `bytes` is not null, but empty and indexing into `bytes` on line 848 throws the exception. The other option would have been to remove the empty array default value that was added in #15098, but the length check seemed like the safer, although maybe slightly less performant, option. Backport of #16992 Co-authored-by: Reid Kirkpatrick <[email protected]>

TJ Lambert added 3 commits May 23, 2022 09:27

enable nullability

41264fa

throw better exceptions

53e92b1

use is null and is not null

e110a6b

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

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

tj-devel709 commented May 23, 2022

View reviewed changes

remove extra spaces

6a81baa

tj-devel709 commented May 23, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

rolfbjarne approved these changes May 23, 2022

View reviewed changes

dalexsoto approved these changes May 23, 2022

View reviewed changes

mandel-macaque approved these changes May 23, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

stephen-hawley approved these changes May 25, 2022

View reviewed changes

TJ Lambert added 2 commits May 31, 2022 11:10

apply PR suggestions

a0fd096

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

2f613c2

This comment has been minimized.

Sign in to view

Merge branch 'main' into Nullable-CoreMidi

a7d838c

chamons approved these changes Jun 6, 2022

View reviewed changes

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

This was referenced Dec 7, 2022

MidiPacket.Bytes throws a System.IndexOutOfRangeException #16979

Closed

[CoreMidi] Fixed MidiPacket.Bytes IndexOutOfRangeException. Fixes #16979. #16992

Merged

This was referenced Dec 12, 2022

[xcode14.1] [CoreMidi] Fixed MidiPacket.Bytes IndexOutOfRangeException. Fixes #16979. #17021

Merged

[release/7.0.1xx] [CoreMidi] Fixed MidiPacket.Bytes IndexOutOfRangeException. Fixes #16979. #17022

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[coremidi] Add nullability to (generated and manual) bindings #15098

[coremidi] Add nullability to (generated and manual) bindings #15098

tj-devel709 commented May 23, 2022

tj-devel709 May 23, 2022

mandel-macaque May 23, 2022

tj-devel709 May 23, 2022

rolfbjarne May 23, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rolfbjarne May 23, 2022

mandel-macaque May 23, 2022

mandel-macaque May 23, 2022

mandel-macaque May 23, 2022

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.

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

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

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

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

Failed tests

chamons Jun 6, 2022

tj-devel709 Jun 6, 2022

chamons Jun 6, 2022

chamons Jun 6, 2022

tj-devel709 Jun 6, 2022

chamons Jun 6, 2022

tj-devel709 commented Jun 6, 2022

		using (var args = new MidiPacketsEventArgs (packetList))
		e (port, args);

[coremidi] Add nullability to (generated and manual) bindings #15098

[coremidi] Add nullability to (generated and manual) bindings #15098

Conversation

tj-devel709 commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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.

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

📋 [PR Build] API Diff 📋

API diff (for current PR)

API diff (vs stable)

Generator diff

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

📚 [PR Build] Artifacts 📚

Packages generated

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

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

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

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

Test results

Failed tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tj-devel709 commented Jun 6, 2022