Nullability context and code refactor #11

AgentFire · 2023-11-14T15:50:19Z

Added #nullable-context:enable to main projects.
Added #lang-version:latest to main projects.
Updated NuGet packages on the main projects.

Refactoring:
Added braces to code blocks.
Converted to file-scope namespaces.
Added nullability modifiers where required
Added missing null checks
Other minor one-click IDE refactors to bring the code base to be more consistent.

Added #nullable-context:enable to main projects. Added #lang-version:latest to main projects. Updated NuGet packages on the main projects. Refactoring: Added braces to code blocks. Converted to file-scope namespaces. Added nullability modifiers where required Added missing null checks Other minor one-click IDE refactors to bring the code base to be more consistent.

PJB3005

Doing everything in a single commit like this is a no-go. I can't possibly review these changes.

Doing something like enabling nullability is good.

Completely restyling all the lines in the project has little benefit and is not worth the churn it creates. Even if it were a separate commit I still don't want to bother for various reasons.

AgentFire · 2023-11-14T19:56:04Z

Understandable, here is a downgraded version 😊

PJB3005

All in all there's still more stylistic changes than I'd prefer, but the biggest review task was still the nullables.

PJB3005 · 2023-11-14T21:45:38Z

Lidgren.Network/Lidgren.Network.csproj

+		<Nullable>enable</Nullable>
+		<LangVersion>latest</LangVersion>


This is indented wrong. Also don't set LangVersion to latest, set it to a fixed version.

PJB3005 · 2023-11-14T21:49:39Z

UnitTests/UnitTests.csproj

+		<Nullable>enable</Nullable>
+		<LangVersion>latest</LangVersion>


Same issues as other csproj.

PJB3005 · 2023-11-14T22:03:20Z

Lidgren.Network/NetBigInteger.cs

+			if (zVal == null || yAccum == null)
+			{
+				throw new InvalidOperationException("both zVal and yAccum should not be null");
+			}


I'd just make this an assert instead. It should mute nullability warnings.

It doesn't mute any warning unfortunately, so I'll add another helper method based on AgumentNullException.ThrowIfNull.

PJB3005 · 2023-11-14T22:29:40Z

Lidgren.Network/NetBuffer.cs

+		internal byte[] BufferData => m_data ?? throw new InvalidOperationException($"{nameof(m_data)} is null");
+
+		internal byte[]? m_data;
 		internal int m_bitLength;
 		internal int m_readPosition;

 		/// <summary>
 		/// Gets or sets the internal data buffer
 		/// </summary>
-		public byte[] Data
+		public byte[]? Data


As far as I can tell, m_data can only be null if the message is currently pooled. I'd just make Data throw in that case (as it's kind of an invalid state to be doing stuff with it, you shouldn't have a handle) so it doesn't need to be nullable.

PJB3005 · 2023-11-14T22:31:21Z

Lidgren.Network/NetClient.cs

 		{
 			lock (m_connections)
 			{
 				if (m_connections.Count > 0)
 				{
-					LogWarning("Connect attempt failed; Already connected");
-					return null;
+					throw new InvalidOperationException("Connect attempt failed; Already connected");


Not sure it's a good idea to change behavior like this.

PJB3005 · 2023-11-15T00:19:19Z

Lidgren.Network/NetUtility.cs

+		public static string MakeCommaDelimitedList<T>(IReadOnlyList<T> list)
 		{
 			var cnt = list.Count;
 			StringBuilder bdr = new StringBuilder(cnt * 5); // educated guess
-			for(int i=0;i<cnt;i++)
+			for (int i = 0; i < cnt; i++)
 			{
-				bdr.Append(list[i].ToString());
+				bdr.Append(list[i]?.ToString());


Annotate the function with T : notnull instead, don't change the behavior.

PJB3005 · 2023-11-15T00:20:13Z

Lidgren.Network/Platform/PlatformWin32.cs

 		{
-			var defaultAddress = ProbeDefaultRouteAddress();
+			IPAddress? defaultAddress = ProbeDefaultRouteAddress();


This change shouldn't be necessary.

PJB3005 · 2023-11-15T00:36:22Z

Lidgren.Network/Platform/PlatformWin32.cs

@@ -71,7 +72,7 @@ private static IPAddress ProbeDefaultRouteAddress()
 				// This basically gets us the network interface address "that goes to the router".
 				using var socket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
 				socket.Connect(new IPAddress(new byte[] { 1, 1, 1, 1 }), 12345);
-				return ((IPEndPoint)socket.LocalEndPoint).Address;
+				return (socket.LocalEndPoint as IPEndPoint)?.Address;


LocalEndPoint is expected to be set by the connect call, this should be an assert if anything at all.

PJB3005 · 2023-11-15T00:37:12Z

UnitTests/NetUtilityTests.cs

@@ -51,7 +51,7 @@ public void TestResolveEndPointBasic()
            var addr = NetUtility.Resolve("example.com", 55555);

            Assert.That(addr, Is.Not.Null);
-            Assert.That(addr.Port, Is.EqualTo(55555));
+            Assert.That(addr?.Port, Is.EqualTo(55555));


This doesn't make sense, the address is null checked right above.

PJB3005 · 2023-11-15T00:37:15Z

UnitTests/NetUtilityTests.cs

@@ -88,7 +88,7 @@ public async Task TestResolveEndPointAsyncBasic()
            var addr = await NetUtility.ResolveAsync("example.com", 55555);

            Assert.That(addr, Is.Not.Null);
-            Assert.That(addr.Port, Is.EqualTo(55555));
+            Assert.That(addr?.Port, Is.EqualTo(55555));


AgentFire · 2023-11-18T00:33:11Z

I believe I've worked out all your comments.

AgentFire · 2023-11-20T18:13:35Z

@PJB3005 I believe there is a workflow button to be pressed?..

PJB3005

I'll just fix the last couple issues myself.

PJB3005 · 2023-12-11T23:15:52Z

Lidgren.Network/NetException.cs

+		/// <summary>Returns the passed <paramref name="argument"/>, but throws an <see cref="NetException"/> if the <paramref name="argument"/> is null.</summary>
+		/// <param name="argument">The reference type argument to validate as non-null.</param>
+		/// <param name="paramName">The name of the parameter with which <paramref name="argument"/> corresponds.</param>
+#if !DEBUG
+		[MethodImpl(MethodImplOptions.AggressiveInlining)]
+		public static T ThrowIfNull<T>(T argument, string? paramName = null)
+		{
+			return argument;
+		}
+#else
+#if NET5_0_OR_GREATER
+		public static T ThrowIfNull<T>([NotNull] T? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null) where T : class
+#else
+		public static T ThrowIfNull<T>([NotNull] T? argument, string? paramName = null) where T : class
+#endif
+		{
+			return argument ?? throw new NetException($"{paramName ?? "argument"} is null");
+		}


I don't like these methods for a few reasons:

They should be internal, not public.

They're used interchangeably as asserts and ArgumentNullExceptions, which is confusing.

PJB3005 · 2023-12-11T23:18:36Z

Lidgren.Network/NetPeer.Discovery.cs

@@ -19,7 +19,7 @@ public void DiscoverLocalPeers(int serverPort)
 			um.m_messageType = NetMessageType.Discovery;
 			Interlocked.Increment(ref um.m_recyclingCount);

-			m_unsentUnconnectedMessages.Enqueue(new NetTuple<NetEndPoint, NetOutgoingMessage>(new NetEndPoint(NetUtility.GetBroadcastAddress() ?? throw new InvalidOperationException(), serverPort), um));
+			m_unsentUnconnectedMessages.Enqueue(new NetTuple<NetEndPoint, NetOutgoingMessage>(new NetEndPoint(NetException.ThrowIfNull(NetUtility.GetBroadcastAddress()), serverPort), um));


This isn't great because it doesn't further explain why the exception happened.

PJB3005 · 2023-12-11T23:34:50Z

Lidgren.Network/NetQueue.cs

+		public T?[] ToArray()
 		{
 			m_lock.EnterReadLock();
 			try
 			{
-				T[] retval = new T[m_size];
+				T?[] retval = new T[m_size];


It shouldn't be possible for the queue to contain null entries within the actual valid range, so these shouldn't be nullable.

Remove NetException.ThrowIfNull in favor of NetException.Assert, ArgumentNullException and InvalidOperationException. NetException.Assert now has [DoesNotReturnIf(false)] on its bool parameter. This makes the C# compiler understand it for NRT analysis.

PJB3005 · 2023-12-12T00:59:31Z

Because you made this PR from your wizards branch I can't push to it, I made a PR to your fork: AgentFire#1

Address review comments.

AgentFire · 2023-12-12T09:06:02Z

Merged. A nice touch with the DoesNotReturnIf.

PJB3005 requested changes Nov 14, 2023

View reviewed changes

Most of the one-click refactorings were undone.

3fa0f70

AgentFire marked this pull request as draft November 14, 2023 19:47

BufferData prop renamed

dc8eb58

AgentFire marked this pull request as ready for review November 14, 2023 19:51

AgentFire requested a review from PJB3005 November 14, 2023 22:26

PJB3005 requested changes Nov 15, 2023

View reviewed changes

Review changes

61eeb0c

AgentFire requested a review from PJB3005 November 18, 2023 00:33

minor directive fix

27e6ece

PJB3005 requested changes Dec 11, 2023

View reviewed changes

PJB3005 added 2 commits December 12, 2023 01:31

Address review comments.

50cdac2

Remove NetException.ThrowIfNull in favor of NetException.Assert, ArgumentNullException and InvalidOperationException. NetException.Assert now has [DoesNotReturnIf(false)] on its bool parameter. This makes the C# compiler understand it for NRT analysis.

Release notes

e2d39f1

Merge pull request #1 from space-wizards/23-12-12-agentfire-nullability

7d9d5fc

Address review comments.

PJB3005 merged commit e2a5762 into space-wizards:wizards Dec 12, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nullability context and code refactor #11

Nullability context and code refactor #11

AgentFire commented Nov 14, 2023

PJB3005 left a comment

AgentFire commented Nov 14, 2023

PJB3005 left a comment

PJB3005 Nov 14, 2023

PJB3005 Nov 14, 2023

PJB3005 Nov 14, 2023

AgentFire Nov 15, 2023

PJB3005 Nov 14, 2023

PJB3005 Nov 14, 2023

PJB3005 Nov 15, 2023

PJB3005 Nov 15, 2023

PJB3005 Nov 15, 2023

PJB3005 Nov 15, 2023

PJB3005 Nov 15, 2023

AgentFire commented Nov 18, 2023

AgentFire commented Nov 20, 2023

PJB3005 left a comment

PJB3005 Dec 11, 2023

PJB3005 Dec 11, 2023

PJB3005 Dec 11, 2023

PJB3005 commented Dec 12, 2023

AgentFire commented Dec 12, 2023

		<Nullable>enable</Nullable>
		<LangVersion>latest</LangVersion>

Nullability context and code refactor #11

Nullability context and code refactor #11

Conversation

AgentFire commented Nov 14, 2023

PJB3005 left a comment

Choose a reason for hiding this comment

AgentFire commented Nov 14, 2023

PJB3005 left a 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

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

Choose a reason for hiding this comment

AgentFire commented Nov 18, 2023

AgentFire commented Nov 20, 2023

PJB3005 left a 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

PJB3005 commented Dec 12, 2023

AgentFire commented Dec 12, 2023