Annotate System.Text.Json for nullable #114

buyaa-n · 2019-11-18T23:07:14Z

Contributes to https://github.com/dotnet/corefx/issues/40623

src/libraries/System.Text.Json/ref/System.Text.Json.cs

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNode.cs

buyaa-n · 2019-11-19T23:14:56Z

...xt.Json/src/System/Text/Json/Serialization/Converters/DefaultImmutableDictionaryConverter.cs


-            JsonClassInfo elementClassInfo = state.Current.JsonPropertyInfo.ElementClassInfo;
+            JsonClassInfo elementClassInfo = state.Current.JsonPropertyInfo.ElementClassInfo!;


These properties state.Current.JsonPropertyInfo and state.Current.JsonPropertyInfo.ElementClassInfo are nullable but always set in case of Dictionary or Enumerable type, so just banged them

buyaa-n · 2019-11-19T23:21:24Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ImmutableCollectionCreator.cs

            foreach (DictionaryEntry item in sourceDictionary)
+#pragma warning restore 8605


Didn't found better way to suppress Unboxing a possible null value warning CC @stephentoub @safern

buyaa-n · 2019-11-19T23:40:42Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoCommon.cs

        {
-            Debug.Assert((_elementPropertyInfo ?? ElementClassInfo.PolicyProperty) != null);
-            (_elementPropertyInfo ?? ElementClassInfo.PolicyProperty).AddObjectToParentEnumerable(addMethodDelegate, value);
+            Debug.Assert((_elementPropertyInfo ?? ElementClassInfo?.PolicyProperty) != null);


ElementClassInfo is nullable

src/libraries/System.Text.Json/src/System.Text.Json.csproj

stephentoub · 2019-11-21T02:31:22Z

src/libraries/System.Text.Json/ref/System.Text.Json.cs

+#pragma warning disable 8609
+        public override string? Message { get { throw null; } }
+#pragma warning restore 8609
+        public string? Path { get { throw null; } }


Ditto; should we make the path in the ctor be non-nullable, and then make this non-nullable, too?

@ahsonkhan, @steveharter, can you help review this?

should we make the path in the ctor be non-nullable, and then make this non-nullable, too?

Changing the (string) and (string, Exception) ctors to assign "" instead of null?

So am gonna change it non null and set empty string wherever it setting null. There is also assert checking it for null but seems useless will, delete it

src/libraries/System.Text.Json/ref/System.Text.Json.cs

stephentoub · 2019-11-21T02:39:19Z

src/libraries/System.Text.Json/ref/System.Text.Json.cs

+        public static System.Threading.Tasks.ValueTask<TValue> DeserializeAsync<TValue>(System.IO.Stream utf8Json, System.Text.Json.JsonSerializerOptions? options = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
+        public static TValue Deserialize<TValue>(System.ReadOnlySpan<byte> utf8Json, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
+        public static TValue Deserialize<TValue>(string json, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
+        public static TValue Deserialize<TValue>(ref System.Text.Json.Utf8JsonReader reader, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }


@steveharter, can (and in what situations) might Deserialize return null? I'm wondering if we need to annotate these Deserialize methods with [return: MaybeNull].

Yes, Deserialize can return null. If the JSON payload has the "null" value and we are deserializing into a nullable reference type (like string, object, int?, etc.).

Assert.Null(JsonSerializer.Deserialize<string>("null")); Assert.Null(JsonSerializer.Deserialize<object>("null")); Assert.Null(JsonSerializer.Deserialize<int?>("null")); Assert.Null(JsonSerializer.Deserialize<SimpleTestClass>("null"));

I'm wondering if we need to annotate these Deserialize methods with [return: MaybeNull].

Yes we should.

I am not super familiar with the nullable annotation attributes. Do they only apply to return/out values of APIs?

I am not super familiar with the nullable annotation attributes. Do they only apply to return/out values of APIs?

Some attributes for return/out, some for input/set. [MaybeNull] apply to return/out for generic type.

Yes, Deserialize can return null. If the JSON payload has the "null" value and we are deserializing into a nullable reference type (like string, object, int?, etc.).

We shoudl use [return : MaybeNull] for above method if the method could return null for non nullable ref types. FYI non nullable ref types are <string>, <object> nullable ref types are <string?>, <object?>. Your test below has issue for nullability enabled case:

Assert.Null(JsonSerializer.Deserialize<string>("null")); Assert.Null(JsonSerializer.Deserialize<object>("null"));

When nullable enabled you cannot pass null implicitly for type parameter, null allowed if you use <string?> instead, above test would look like:

Assert.Null(JsonSerializer.Deserialize<string?>("null")); Assert.Null(JsonSerializer.Deserialize<object?>("null"));

So the question is if the deserializer could return null in any non null value is passed for ref type, if so we should annotate it with [return: MaybeNull].

src/libraries/System.Text.Json/ref/System.Text.Json.cs

src/libraries/System.Text.Json/src/System/Text/Json/BitStack.cs

ahsonkhan · 2019-11-22T06:19:02Z

src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs

@@ -106,7 +107,7 @@ public bool GetBoolean()
        /// </exception>
        public byte[] GetBytesFromBase64()
        {
-            if (!TryGetBytesFromBase64(out byte[] value))
+            if (!TryGetBytesFromBase64(out byte[]? value))


Question: Is that how we usually annotate the try methods?

The annotation makes you think that the out parameter can be null (since we have [NotNullWhen(true)]) but we throw in that case. The value that we return can never be null which we are conveying by the method signature, so I see why that would make sense.

I am asking because if method is doing a bunch of complex work, then having value appear to contain null value even after the throw, could be confusing.

For example:

public byte[] GetBytesFromBase64() { if (!TryGetBytesFromBase64(out byte[]? value)) { throw ThrowHelper.GetFormatException(DataType.Base64String); } byte[]? temp = value; // Why is this allowed? if (temp == null) { throw new Exception("We can't get here, compiler please complain!"); } return value; }

Question: Is that how we usually annotate the try methods?

Yes it is. as TryXXX methods annotated to receive nullable out (sometimes ref) parameters calling routines should send nullable argument as that value can be set null in case of unsuccessful load.

As you mentioned value will not be null after throw, not sure why you think compiler still assume value can be null, as you can see we did not banged returning value because compiler evaluated that it will not be null when TryGetBytesFromBase64(...) returned true.

ahsonkhan · 2019-11-22T06:28:19Z

src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs

@@ -511,7 +511,7 @@ public bool ValueTextEquals(ReadOnlySpan<char> text)
                return false;


This file looks good.

src/libraries/System.Text.Json/ref/System.Text.Json.cs

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

ahsonkhan · 2019-12-03T05:33:14Z

src/libraries/System.Text.Json/ref/System.Text.Json.cs

@@ -583,7 +598,7 @@ public partial struct JsonWriterOptions
        public bool TrySkip() { throw null; }
        public bool ValueTextEquals(System.ReadOnlySpan<byte> utf8Text) { throw null; }
        public bool ValueTextEquals(System.ReadOnlySpan<char> text) { throw null; }
-        public bool ValueTextEquals(string text) { throw null; }
+        public bool ValueTextEquals(string? text) { throw null; }
    }
    public sealed partial class Utf8JsonWriter : System.IAsyncDisposable, System.IDisposable


Changes to the Utf8JsonWriter and related types look good.

src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.MetadataDb.cs

src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.Parse.cs

src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.StackRowStack.cs

src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs

src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs

src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonProperty.cs

src/libraries/System.Text.Json/ref/System.Text.Json.cs

bartonjs · 2019-12-03T16:10:59Z

src/libraries/System.Text.Json/src/System/Text/Json/JsonException.cs

@@ -14,7 +14,7 @@ namespace System.Text.Json
    public class JsonException : Exception
    {
        // Allow the message to mutate to avoid re-throwing and losing the StackTrace to an inner exception.
-        private string _message;
+        internal string? _message;


Why is the nullablility flow changing this from private to internal?

That is kind of a long story. Exception.Message getter shouldn't be nullable (contract) so we changed the getter from return _message into return _message ?? base.Message;. But there is a logic in ThrowHelper set the message when Message is null set t https://github.com/dotnet/runtime/pull/114/files#diff-e586edfb9caf385bf493d49a97f197a7R217 which not setting correct message anymore after changing Message to non null. So instead of using Message property we changed it to use this field direclty ex._message; for which the field need to be internal

buyaa-n · 2019-12-04T22:25:47Z

Closing this PR as this one is not accessable anymore, new one created #528 and comments addressed

ahsonkhan · 2020-02-15T00:08:16Z

Highlighting the breaking change here (which became visible when enabling nullability analysis).

Deserializing into a char is now more strict to match user expectations. The payload must contain a single-character string for deserialization to succeed:

public class MyPoco
{
    public char Character { get; set; }
}

public static void TestDeserializeToChar()
{
    // Before (3.1): NullReferenceException
    // After (5.0): JsonException
    JsonSerializer.Deserialize<MyPoco>("{\"Character\": null}");

    // Before (3.1): IndexOutOfRangeException
    // After (5.0): JsonException
    JsonSerializer.Deserialize<MyPoco>("{\"Character\": \"\"}");

    // Before (3.1): Set Character to the first character - 'a'
    // After (5.0): JsonException
    JsonSerializer.Deserialize<MyPoco>("{\"Character\": \"abc\"}");
}

Passing in null for the Type parameter while serializing now correctly throws ArgumentNullException rather than returning "null" JSON:

public static void TestSerializeNullType()
{
    // Before (3.1): Returned a string with value "null"
    // After (5.0): ArgumentNullException: Value cannot be null. (Parameter 'type')
    JsonSerializer.Serialize(null, null);

    // Same with other Serialize{Async} overloads
    // Before (3.1): Returned a byte[] with value "null"
    // After (5.0): ArgumentNullException: Value cannot be null. (Parameter 'type')
    JsonSerializer.SerializeToUtf8Bytes(null, null);
}

NoneGiven · 2020-02-16T06:48:07Z

If we're making breaking changes to match user expectation, how about #1198?

ahsonkhan · 2020-02-20T04:27:35Z

Not all breaking changes are "equivalent" in terms of impact and expectation (changing the output from serialization which can be persisted has a higher bar than changing deserialization behavior). That said, the issue to fix the keyvaluepair issue is still open and targeting 5.0 (#1197), so I suspect it too will get fixed after proper due diligence/evaluating the impact :)

Discussion regarding that issue should be made there.

ahsonkhan · 2020-02-20T20:42:54Z

Since this PR wasn't merged, removing label and moving to the one that actually got merged:
#528 (comment)

- Add osx RuntimeIdentifiers - Make up RIDs - Fix build breaks - UseAppHost unconditionally - CMake cleanup Co-authored-by: Jan Kotas <jkotas@microsoft.com>

Annotate System.Text.Json for nullable

a649816

buyaa-n requested review from stephentoub, ahsonkhan, steveharter, layomia and safern November 18, 2019 23:07

Dotnet-GitSync-Bot added the area-System.Text.Json label Nov 18, 2019

buyaa-n added this to the 5.0 milestone Nov 18, 2019

buyaa-n added 3 commits November 18, 2019 17:34

Suppressing null warnings on non NetCoreApp targets

5015584

Suppress more error in other env

44d82fd

Annotate netstandard dependent file

3a7254b

buyaa-n commented Nov 19, 2019

View reviewed changes

src/libraries/System.Text.Json/ref/System.Text.Json.cs Outdated Show resolved Hide resolved

buyaa-n commented Nov 19, 2019

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNode.cs Outdated Show resolved Hide resolved

buyaa-n commented Nov 19, 2019

View reviewed changes

Some more update

6917d64

safern reviewed Nov 20, 2019

View reviewed changes

src/libraries/System.Text.Json/src/System.Text.Json.csproj Outdated Show resolved Hide resolved

buyaa-n added 2 commits November 20, 2019 09:52

Enebling nullability on file which fialing for other projects

150691f

Disabling nullable warning on netfx, netstandard

0ab2dd6

stephentoub reviewed Nov 21, 2019

View reviewed changes

src/libraries/System.Text.Json/ref/System.Text.Json.cs Show resolved Hide resolved

stephentoub reviewed Nov 21, 2019

View reviewed changes

src/libraries/System.Text.Json/ref/System.Text.Json.cs Show resolved Hide resolved

stephentoub reviewed Nov 21, 2019

View reviewed changes

src/libraries/System.Text.Json/ref/System.Text.Json.cs Show resolved Hide resolved

stephentoub reviewed Nov 21, 2019

View reviewed changes

src/libraries/System.Text.Json/ref/System.Text.Json.cs Show resolved Hide resolved

stephentoub reviewed Nov 21, 2019

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/BitStack.cs Show resolved Hide resolved

buyaa-n added 2 commits November 21, 2019 11:45

Addressing feedback

8459413

Banging Exception.Message instead of defaulting as tests failing

ea28014

ahsonkhan reviewed Nov 22, 2019

View reviewed changes

src/libraries/System.Text.Json/ref/System.Text.Json.cs Show resolved Hide resolved

Applying feedback

9d735b7

ahsonkhan reviewed Dec 2, 2019

View reviewed changes

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs Show resolved Hide resolved

ahsonkhan mentioned this pull request Dec 2, 2019

NullRefereceException when using a JsonConverter without a parameterless constructor #399

Closed