-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
Annotate System.Text.Json for nullable #528
Conversation
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonProperty.cs
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
Show resolved
Hide resolved
src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTestData.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonNamingPolicy.cs
Outdated
Show resolved
Hide resolved
...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
48/125 files reviewed (JsonDocument, JsonElement, JsonNode, and related types).
Here's some feedback so far.
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.Parse.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.Parse.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.StackRowStack.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNull.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNode.Traversal.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/JsonException.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderException.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Collections/Generic/StackExtensions.netstandard.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Collections/Generic/StackExtensions.netstandard.cs
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
Outdated
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
Outdated
Show resolved
Hide resolved
4ff8989
to
462b755
Compare
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs
Outdated
Show resolved
Hide resolved
…ion/JsonSerializer.Read.Stream.cs Remove not needed attribute
Failed CI legs doesn't look like caused from this PR changes, gonna merge this |
Yay! |
Awesome! Thanks for your patience and effort in getting this merged, @buyaa-n. I appreciate you taking your time with it and getting the details right. Nullability annotations of a complex library like JSON hopefully help explore and refine the guidelines. |
Thanks for your delicate review, learnt a lot. Have fun with nullability aware coding 😄 |
Moving comment from #114 (comment) (since that PR was closed). 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 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);
} |
Recreating PR as old one #114 no more accessible, gone with old private fork
CC @ahsonkhan @steveharter @stephentoub @safern