JsonNull return false when comparing against null (dotnet#842)

* JsonNull return false when comparing against null I'ved modified JsonNulls comparison methods to return false when compared against null, the matching test have also been updated in change Fix dotnet#820 * Updated xml comments to reflect my changes I have updated the xml comments to reflect the changes that was made in JsonNull and removed left over comment in JsonNullTests * Applied suggested change Applied the suggested change to correct xml documentation Co-Authored-By: Ahson Khan <[email protected]> * Applied Suggested change Applied the suggested change to correct xml documentation Co-Authored-By: Ahson Khan <[email protected]>
zengande · Dec 18, 2019 · bdfbad5 · bdfbad5
1 parent fdcd30b
commit bdfbad5
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 17 deletions.
diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNull.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNull.cs
@@ -42,24 +42,44 @@ public JsonNull() { }
         ///   Compares other JSON null to the value of this instance.
         /// </summary>
         /// <param name="other">The JSON null to compare against.</param>
-        /// <returns><see langword="true"/></returns>
-        public bool Equals(JsonNull other) => true;
+        /// <returns>
+        ///    <see langword="true"/> if <paramref name="other"/> is not null,
+        ///    <see langword="false"/> otherwise.
+        /// </returns>
+        public bool Equals(JsonNull other) => !(other is null);
 
         /// <summary>
         ///   Compares values of two JSON nulls.
         /// </summary>
         /// <param name="left">The JSON null to compare.</param>
         /// <param name="right">The JSON null to compare.</param>
-        /// <returns><see langword="true"/></returns>
-        public static bool operator ==(JsonNull left, JsonNull right) => true;
+        /// <returns>
+        ///    <see langword="true"/> if both instances match,
+        ///    <see langword="false"/> otherwise.
+        /// </returns>
+        public static bool operator ==(JsonNull left, JsonNull right)
+        {
+            // Test "right" first to allow branch elimination when inlined for null checks (== null)
+            // so it can become a simple test
+            if (right is null)
+            {
+                // return true/false not the test result https://github.com/dotnet/coreclr/issues/914
+                return (left is null) ? true : false;
+            }
+
+            return right.Equals(left);
+        }
 
         /// <summary>
         ///   Compares values of two JSON nulls.
         /// </summary>
         /// <param name="left">The JSON null to compare.</param>
         /// <param name="right">The JSON null to compare.</param>
-        /// <returns><see langword="false"/></returns>
-        public static bool operator !=(JsonNull left, JsonNull right) => false;
+        /// <returns>
+        ///    <see langword="true"/> if both instances do not match,
+        ///    <see langword="false"/> otherwise.
+        /// </returns>
+        public static bool operator !=(JsonNull left, JsonNull right) => !(left == right);
 
         /// <summary>
         ///   Creates a new JSON null that is a copy of the current instance.

diff --git a/src/libraries/System.Text.Json/tests/JsonNullTests.cs b/src/libraries/System.Text.Json/tests/JsonNullTests.cs
@@ -45,17 +45,15 @@ public static void TestEquals()
             Assert.False(jsonNull1.Equals(new JsonString("null")));
             Assert.False(jsonNull1.Equals(new Exception()));
 
-            // Null comparisons behave this way because of implicit conversion from null to JsonNull:
-
-            Assert.True(jsonNull1.Equals(null));
-            Assert.True(jsonNull1 == null);
-            Assert.False(jsonNull1 != null);
+            Assert.False(jsonNull1.Equals(null));
+            Assert.False(jsonNull1 == null);
+            Assert.True(jsonNull1 != null);
 
             JsonNull jsonNullNull = null;
 
-            Assert.True(jsonNull1.Equals(jsonNullNull));
-            Assert.True(jsonNull1 == jsonNullNull);
-            Assert.False(jsonNull1 != jsonNullNull);
+            Assert.False(jsonNull1.Equals(jsonNullNull));
+            Assert.False(jsonNull1 == jsonNullNull);
+            Assert.True(jsonNull1 != jsonNullNull);
 
             JsonNull otherJsonNullNull = null;
             Assert.True(jsonNullNull == otherJsonNullNull);
@@ -64,7 +62,7 @@ public static void TestEquals()
 
             JsonNode jsonNodeNull = null;
             Assert.False(jsonNull1.Equals(jsonNodeNull));
-          
+
             JsonArray jsonArrayNull = null;
             Assert.False(jsonNull1.Equals(jsonArrayNull));
         }
@@ -75,10 +73,10 @@ public static void TestGetHashCode()
             var jsonNull = new JsonNull();
 
             Assert.Equal(jsonNull.GetHashCode(), new JsonNull().GetHashCode());
-            
+
             JsonNode jsonNodeNull = new JsonNull();
             Assert.Equal(jsonNull.GetHashCode(), jsonNodeNull.GetHashCode());
-            
+
             IEquatable<JsonNull> jsonNullIEquatable = new JsonNull();
             Assert.Equal(jsonNull.GetHashCode(), jsonNullIEquatable.GetHashCode());