Skip to content

Fix up some nullability annotations to remove unnecessary null-forgiving operations (!) #32186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ahsonkhan merged 7 commits into from
Feb 14, 2020
Merged

Fix up some nullability annotations to remove unnecessary null-forgiving operations (!) #32186

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8308a9b
Fix up some nullability annotations to remove unnecessary null-forgiving
ahsonkhan Feb 12, 2020
2d65d64
Revert change to TryGetOrAddReferenceOnSerialize since it will never
ahsonkhan Feb 12, 2020
7ab2968
Remove unused using directive.
ahsonkhan Feb 12, 2020
c19c05a
Adding NotNull on the JsonConverter<T>.Write and MaybeNullWhen on the
ahsonkhan Feb 12, 2020
1ec16e4
Address feedback - DisallowNull instead of NotNull and remove
ahsonkhan Feb 13, 2020
c6ef742
Add back Debug.Asserts because I like them :)
ahsonkhan Feb 13, 2020
c0aa571
Merge branch 'master' of https://github.com/dotnet/runtime into FixNu…
ahsonkhan Feb 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libraries/System.Text.Json/ref/System.Text.Json.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,7 @@ public abstract partial class JsonConverter<T> : System.Text.Json.Serialization.
protected internal JsonConverter() { }
public override bool CanConvert(System.Type typeToConvert) { throw null; }
public abstract T Read(ref System.Text.Json.Utf8JsonReader reader, System.Type typeToConvert, System.Text.Json.JsonSerializerOptions options);
public abstract void Write(System.Text.Json.Utf8JsonWriter writer, T value, System.Text.Json.JsonSerializerOptions options);
public abstract void Write(System.Text.Json.Utf8JsonWriter writer, [System.Diagnostics.CodeAnalysis.NotNull] T value, System.Text.Json.JsonSerializerOptions options);
Copy link
Copy Markdown
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotNull doesn't make sense here. Did you mean DisallowNull? And just so I understand, it's invalid to write nulls but read can return nulls?

Copy link
Copy Markdown
Contributor Author

@ahsonkhan ahsonkhan Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And just so I understand, it's invalid to write nulls but read can return nulls?

Yes. Write won't receive null but Read can return null.

The intent is the JsonSerializer will never pass in null T values to the JsonConverter<T>.Write method and hence the implementer of that method doesn't need to do a null check on it.

They can certainly continue to write null JSON literals in the method itself or do whatever else. If an nullable object has the null value, the serializer writes null for you (and doesn't call the JsonConverter).

On the read side, the implementer should honor the nullability of the T. If T is nullable, the implementer of JsonConverter<T>.Read can certainly return null if they wish. If T is non-nullable (like string, or value type), it doesn't make sense for them to do so.

For types where null doesn't make sense (i.e. non-nullable valuetypes), the JsonSerializer can and will pass in a Utf8JsonReader with the null TokenType if the payload we are processing has the null JSON literal.
For types where null is allowed (nullable/non-nullable reference types), the JsonSerializer will not pass in a Utf8JsonReader in that state and will eagerly set the object to null up front, without calling the JsonConverter.

#nullable enable

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using Xunit;

namespace System.Text.Json.Serialization.Tests
{
    public static partial class PropertyNameTests
    {
        [Fact]
        public static void QuickTest()
        {
            var opts = new JsonSerializerOptions
            {
                Converters =
                {
                    new MyStringConverter(),
                    new MyNullableStringConverter(),
                    new MyNullableIntConverter(),
                    new MyIntConverter()
                }
            };

            string json = "{\"foo\": null, \"bar\": null, \"baz\": null,\"nullBaz\": null}";

            MyClass output = JsonSerializer.Deserialize<MyClass>(json, opts)!;
            Assert.Null(output.foo);
            Assert.Null(output.bar);
            Assert.Equal(0, output.baz);
            Assert.Null(output.nullBaz);
        }

        public class MyClass
        {
#pragma warning disable CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable.
            public string foo { get; set; }
#pragma warning restore CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable.

            public string? bar { get; set; }

            public int baz { get; set; }

            public int? nullBaz { get; set; }
        }

        public class MyStringConverter : JsonConverter<string>
        {
            public override string Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                Debug.Assert(reader.TokenType != JsonTokenType.Null);

                return reader.GetString()!;
            }

            public override void Write(Utf8JsonWriter writer, [DisallowNull] string value, JsonSerializerOptions options)
            {
                throw new NotImplementedException();
            }
        }

        public class MyNullableStringConverter : JsonConverter<string?>
        {
            public override string? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                Debug.Assert(reader.TokenType != JsonTokenType.Null);

                return reader.GetString();
            }

            public override void Write(Utf8JsonWriter writer, [DisallowNull] string? value, JsonSerializerOptions options)
            {
                throw new NotImplementedException();
            }
        }

        public class MyIntConverter : JsonConverter<int>
        {
            public override int Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                if (reader.TokenType == JsonTokenType.Null)
                    return default;
                return reader.GetInt32();
            }

            public override void Write(Utf8JsonWriter writer, [DisallowNull] int value, JsonSerializerOptions options)
            {
                throw new NotImplementedException();
            }
        }

        public class MyNullableIntConverter : JsonConverter<int?>
        {
            public override int? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                if (reader.TokenType == JsonTokenType.Null)
                    return null;
                return reader.GetInt32();
            }

            public override void Write(Utf8JsonWriter writer, [DisallowNull] int? value, JsonSerializerOptions options)
            {
                throw new NotImplementedException();
            }
        }
    }
}

}
[System.AttributeUsageAttribute(System.AttributeTargets.Property, AllowMultiple=false)]
public sealed partial class JsonExtensionDataAttribute : System.Text.Json.Serialization.JsonAttribute
Expand Down
3 changes: 2 additions & 1 deletion ...Text.Json/src/System/Text/Json/Serialization/Converters/JsonDictionaryDefaultConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

namespace System.Text.Json.Serialization.Converters
{
Expand Down Expand Up @@ -66,7 +67,7 @@ internal sealed override bool OnTryRead(
Type typeToConvert,
JsonSerializerOptions options,
ref ReadStack state,
out TCollection value)
[MaybeNullWhen(false)] out TCollection value)
{
bool shouldReadPreservedReferences = options.ReferenceHandling.ShouldReadPreservedReferences();

Expand Down
3 changes: 2 additions & 1 deletion ...ext.Json/src/System/Text/Json/Serialization/Converters/JsonIEnumerableDefaultConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

namespace System.Text.Json.Serialization.Converters
{
Expand Down Expand Up @@ -36,7 +37,7 @@ internal override bool OnTryRead(
Type typeToConvert,
JsonSerializerOptions options,
ref ReadStack state,
out TCollection value)
[MaybeNullWhen(false)] out TCollection value)
Copy link
Copy Markdown
Contributor Author

@ahsonkhan ahsonkhan Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this, once #32090 is merged, we can remove the ! in classes like:
JsonIEnumerableDefaultConverter.cs
JsonDictionaryDefaultConverter.cs
etc.

Everywhere we do:
value = default!;

{
bool shouldReadPreservedReferences = options.ReferenceHandling.ShouldReadPreservedReferences();

Expand Down
6 changes: 3 additions & 3 deletions ...ries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonIListConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
namespace System.Text.Json.Serialization.Converters
{
/// Converter for <cref>System.Collections.IList</cref>.
internal sealed class JsonIListConverter<TCollection> : JsonIEnumerableDefaultConverter<TCollection, object>
internal sealed class JsonIListConverter<TCollection> : JsonIEnumerableDefaultConverter<TCollection, object?>
where TCollection : IList
{
protected override void Add(object? value, ref ReadStack state)
Expand Down Expand Up @@ -65,7 +65,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value,
enumerator = state.Current.CollectionEnumerator;
}

JsonConverter<object> converter = GetElementConverter(ref state);
JsonConverter<object?> converter = GetElementConverter(ref state);
do
{
if (ShouldFlush(writer, ref state))
Expand All @@ -76,7 +76,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value,

object? element = enumerator.Current;

if (!converter.TryWrite(writer, element!, options, ref state))
if (!converter.TryWrite(writer, element, options, ref state))
{
state.Current.CollectionEnumerator = enumerator;
return false;
Expand Down
3 changes: 2 additions & 1 deletion ...tem.Text.Json/src/System/Text/Json/Serialization/Converters/JsonObjectDefaultConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

namespace System.Text.Json.Serialization.Converters
{
Expand All @@ -11,7 +12,7 @@ namespace System.Text.Json.Serialization.Converters
/// </summary>
internal sealed class JsonObjectDefaultConverter<T> : JsonObjectConverter<T>
{
internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out T value)
internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, [MaybeNullWhen(false)] out T value)
Copy link
Copy Markdown
Contributor

@steveharter steveharter Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may become protected (instead of just internal) and I don't think we want to say this shouldn't be null.

Also what's the difference between DisallowNull and [MaybeNullWhen(false)]?

Copy link
Copy Markdown
Contributor Author

@ahsonkhan ahsonkhan Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DisallowNull is for inputs, MaybeNullWhen(false) is for outputs. In this case this is stating that regardless of the nullability of the generic, the OnTryRead might set it to null when it returns false.

Even if we make it protected (and expose it), the nullability is showcasing the intent of the API. We are doing T value = default! in a bunch of places.

We can revisit the annotation if the implementation/design changes as part of exposing it. This change makes sense to me as it accurately represents what the API is doing so the caller knows what to do with the out parameter.

{
bool shouldReadPreservedReferences = options.ReferenceHandling.ShouldReadPreservedReferences();
object obj;
Expand Down
8 changes: 3 additions & 5 deletions ...libraries/System.Text.Json/src/System/Text/Json/Serialization/DefaultReferenceResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,14 @@ public bool AddReferenceOnDeserialize(string referenceId, object value)
/// <returns></returns>
public bool TryGetOrAddReferenceOnSerialize(object value, out string referenceId)
{
if (!_objectToReferenceIdMap!.TryGetValue(value, out referenceId!))
bool result = _objectToReferenceIdMap!.TryGetValue(value, out referenceId!);
if (!result)
{
_referenceCount++;
referenceId = _referenceCount.ToString();
_objectToReferenceIdMap.Add(value, referenceId);

return false;
}

return true;
return result;
}

/// <summary>
Expand Down
3 changes: 2 additions & 1 deletion src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

namespace System.Text.Json.Serialization
{
Expand Down Expand Up @@ -392,6 +393,6 @@ internal void VerifyWrite(int originalDepth, Utf8JsonWriter writer)
/// <param name="writer">The <see cref="Utf8JsonWriter"/> to write to.</param>
/// <param name="value">The value to convert.</param>
/// <param name="options">The <see cref="JsonSerializerOptions"/> being used.</param>
public abstract void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options);
public abstract void Write(Utf8JsonWriter writer, [NotNull] T value, JsonSerializerOptions options);
Copy link
Copy Markdown
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DisallowNull?

}
}
62 changes: 29 additions & 33 deletions ...ystem.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,42 +14,32 @@ public static partial class JsonSerializer
internal static readonly JsonEncodedText s_metadataRef = JsonEncodedText.Encode("$ref", encoder: null);
internal static readonly JsonEncodedText s_metadataValues = JsonEncodedText.Encode("$values", encoder: null);

internal static MetadataPropertyName GetResolvedReferenceHandling(
JsonConverter converter,
object value,
ref WriteStack state,
out string? referenceId)
{
if (!converter.CanHaveIdMetadata || converter.TypeToConvert.IsValueType)
{
referenceId = default;
return MetadataPropertyName.NoMetadata;
}

if (state.ReferenceResolver.TryGetOrAddReferenceOnSerialize(value, out referenceId))
{
return MetadataPropertyName.Ref;
}

return MetadataPropertyName.Id;
}

internal static MetadataPropertyName WriteReferenceForObject(
JsonConverter jsonConverter,
object currentValue,
ref WriteStack state,
Utf8JsonWriter writer)
{
MetadataPropertyName metadataToWrite = GetResolvedReferenceHandling(jsonConverter, currentValue, ref state, out string? referenceId);
MetadataPropertyName metadataToWrite;
Copy link
Copy Markdown
Contributor

@steveharter steveharter Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the motivation here only to remove the ! in referenceId! or also other reasons (perf?)

Copy link
Copy Markdown
Contributor Author

@ahsonkhan ahsonkhan Feb 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that and perf (avoiding unnecessary/duplicate checks on the enum).


if (metadataToWrite == MetadataPropertyName.Ref)
// If the jsonConverter supports immutable dictionaries or value types, don't write any metadata
if (!jsonConverter.CanHaveIdMetadata || jsonConverter.TypeToConvert.IsValueType)
{
metadataToWrite = MetadataPropertyName.NoMetadata;
}
else if (state.ReferenceResolver.TryGetOrAddReferenceOnSerialize(currentValue, out string referenceId))
{
writer.WriteString(s_metadataRef, referenceId!);
Debug.Assert(referenceId != null);
writer.WriteString(s_metadataRef, referenceId);
writer.WriteEndObject();
metadataToWrite = MetadataPropertyName.Ref;
}
else if (metadataToWrite == MetadataPropertyName.Id)
else
{
writer.WriteString(s_metadataId, referenceId!);
// TryGetOrAddReferenceOnSerialize is guaranteed to not return null.
Copy link
Copy Markdown
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment necessary? The purpose of nullable reference types is you shouldn't need a comment like this, given the signature says "out string" rather than "out string?"

Copy link
Copy Markdown
Contributor Author

@ahsonkhan ahsonkhan Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it along with the Debug.Assert to make it explicit that referenceId shouldn't be null even if TryGetOrAddReferenceOnSerialize accidentally set referenceId to null!. Otherwise, it's a bug in the implementation detail.

I am fine removing the comment though. Should I remove the Debug.Assert as well?

Copy link
Copy Markdown
Member

@stephentoub stephentoub Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I remove the Debug.Assert as well?

The comment seemed superfluous. I'm fine with the Debug.Assert if you think it adds value.

Debug.Assert(referenceId != null);
writer.WriteString(s_metadataId, referenceId);
metadataToWrite = MetadataPropertyName.Id;
}

return metadataToWrite;
Expand All @@ -61,24 +51,30 @@ internal static MetadataPropertyName WriteReferenceForCollection(
ref WriteStack state,
Utf8JsonWriter writer)
{
MetadataPropertyName metadataToWrite = GetResolvedReferenceHandling(jsonConverter, currentValue, ref state, out string? referenceId);
MetadataPropertyName metadataToWrite;

if (metadataToWrite == MetadataPropertyName.NoMetadata)
// If the jsonConverter supports immutable enumerables or value type collections, don't write any metadata
if (!jsonConverter.CanHaveIdMetadata || jsonConverter.TypeToConvert.IsValueType)
{
writer.WriteStartArray();
metadataToWrite = MetadataPropertyName.NoMetadata;
}
else if (metadataToWrite == MetadataPropertyName.Id)
else if (state.ReferenceResolver.TryGetOrAddReferenceOnSerialize(currentValue, out string referenceId))
{
Debug.Assert(referenceId != null);
writer.WriteStartObject();
writer.WriteString(s_metadataId, referenceId!);
writer.WriteStartArray(s_metadataValues);
writer.WriteString(s_metadataRef, referenceId);
writer.WriteEndObject();
metadataToWrite = MetadataPropertyName.Ref;
}
else
{
Debug.Assert(metadataToWrite == MetadataPropertyName.Ref);
// TryGetOrAddReferenceOnSerialize is guaranteed to not return null.
Debug.Assert(referenceId != null);
writer.WriteStartObject();
writer.WriteString(s_metadataRef, referenceId!);
writer.WriteEndObject();
writer.WriteString(s_metadataId, referenceId);
writer.WriteStartArray(s_metadataValues);
metadataToWrite = MetadataPropertyName.Id;
}

return metadataToWrite;
Expand Down