Skip to content

Fix fast-path support for ignoring nullable value types #96639

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
eiriktsarpalis merged 1 commit into from
Jan 9, 2024
Merged

Fix fast-path support for ignoring nullable value types #96639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Fix fast-path support for ignoring nullable value types
  • Loading branch information
@eiriktsarpalis
eiriktsarpalis committed Jan 8, 2024
commit 76dce87cb3e50331848b680067210fe4f57d86a0
2 changes: 1 addition & 1 deletion src/libraries/Common/src/SourceGenerators/TypeRef.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public TypeRef(ITypeSymbol type)
FullyQualifiedName = type.GetFullyQualifiedName();
IsValueType = type.IsValueType;
TypeKind = type.TypeKind;
SpecialType = type.SpecialType;
SpecialType = type.OriginalDefinition.SpecialType;
Copy link
Member Author

@eiriktsarpalis eiriktsarpalis Jan 8, 2024

Choose a reason for hiding this comment

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

The root cause of the issue is that only the generic type definition of Nullable<T> is reported as SpecialType.System_Nullable_T resulting in the CanBeNull property in line 36 of this file to report incorrect values.

}

public string Name { get; }
Expand Down
77 changes: 10 additions & 67 deletions src/libraries/System.Text.Json/tests/Common/PropertyVisibilityTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1322,10 +1322,6 @@ public async Task JsonIgnoreAttribute()
}

[Fact]
#if BUILDING_SOURCE_GENERATOR_TESTS
// Needs support for more collections.
[ActiveIssue("https://github.com/dotnet/runtime/issues/53393")]
#endif
public async Task JsonIgnoreAttribute_UnsupportedCollection()
{
string json =
Expand Down Expand Up @@ -1771,10 +1767,6 @@ public async Task OverrideJsonIgnorePropertyUsingJsonPropertyNameReversed()
[Theory]
[InlineData(typeof(ClassWithProperty_IgnoreConditionAlways))]
[InlineData(typeof(ClassWithProperty_IgnoreConditionAlways_Ctor))]
#if BUILDING_SOURCE_GENERATOR_TESTS
// Need support for parameterized ctors.
[ActiveIssue("https://github.com/dotnet/runtime/issues/45448")]
#endif
public async Task JsonIgnoreConditionSetToAlwaysWorks(Type type)
{
string json = @"{""MyString"":""Random"",""MyDateTime"":""2020-03-23"",""MyInt"":4}";
Expand All @@ -1798,7 +1790,7 @@ public class ClassWithProperty_IgnoreConditionAlways
public int MyInt { get; set; }
}

private class ClassWithProperty_IgnoreConditionAlways_Ctor
public class ClassWithProperty_IgnoreConditionAlways_Ctor
{
public string MyString { get; set; }
[JsonIgnore(Condition = JsonIgnoreCondition.Always)]
Expand All @@ -1814,10 +1806,6 @@ public ClassWithProperty_IgnoreConditionAlways_Ctor(DateTime myDateTime, int myI

[Theory]
[MemberData(nameof(JsonIgnoreConditionWhenWritingDefault_ClassProperty_TestData))]
#if BUILDING_SOURCE_GENERATOR_TESTS
// Need support for parameterized ctors.
[ActiveIssue("https://github.com/dotnet/runtime/issues/45448")]
#endif
public async Task JsonIgnoreConditionWhenWritingDefault_ClassProperty(Type type, JsonSerializerOptions options)
{
// Property shouldn't be ignored if it isn't null.
Expand Down Expand Up @@ -1868,7 +1856,7 @@ public class ClassWithClassProperty_IgnoreConditionWhenWritingDefault
public int Int2 { get; set; }
}

private class ClassWithClassProperty_IgnoreConditionWhenWritingDefault_Ctor
public class ClassWithClassProperty_IgnoreConditionWhenWritingDefault_Ctor
{
public int Int1 { get; set; }
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]
Expand All @@ -1892,10 +1880,6 @@ public static IEnumerable<object[]> JsonIgnoreConditionWhenWritingDefault_ClassP

[Theory]
[MemberData(nameof(JsonIgnoreConditionWhenWritingDefault_StructProperty_TestData))]
#if BUILDING_SOURCE_GENERATOR_TESTS
// Need support for parameterized ctors.
[ActiveIssue("https://github.com/dotnet/runtime/issues/45448")]
#endif
public async Task JsonIgnoreConditionWhenWritingDefault_StructProperty(Type type, JsonSerializerOptions options)
{
// Property shouldn't be ignored if it isn't null.
Expand Down Expand Up @@ -1924,7 +1908,7 @@ public class ClassWithStructProperty_IgnoreConditionWhenWritingDefault
public int Int2 { get; set; }
}

private struct StructWithStructProperty_IgnoreConditionWhenWritingDefault_Ctor
public struct StructWithStructProperty_IgnoreConditionWhenWritingDefault_Ctor
{
public int Int1 { get; set; }
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]
Expand All @@ -1948,10 +1932,6 @@ public static IEnumerable<object[]> JsonIgnoreConditionWhenWritingDefault_Struct

[Theory]
[MemberData(nameof(JsonIgnoreConditionNever_TestData))]
#if BUILDING_SOURCE_GENERATOR_TESTS
// Need support for parameterized ctors.
[ActiveIssue("https://github.com/dotnet/runtime/issues/45448")]
#endif
public async Task JsonIgnoreConditionNever(Type type)
{
// Property should always be (de)serialized, even when null.
Expand Down Expand Up @@ -1983,10 +1963,6 @@ public async Task JsonIgnoreConditionNever(Type type)

[Theory]
[MemberData(nameof(JsonIgnoreConditionNever_TestData))]
#if BUILDING_SOURCE_GENERATOR_TESTS
// Need support for parameterized ctors.
[ActiveIssue("https://github.com/dotnet/runtime/issues/45448")]
#endif
public async Task JsonIgnoreConditionNever_IgnoreNullValues_True(Type type)
{
// Property should always be (de)serialized.
Expand Down Expand Up @@ -2060,9 +2036,6 @@ public async Task JsonIgnoreCondition_LastOneWins()
}

[Fact]
#if BUILDING_SOURCE_GENERATOR_TESTS
[ActiveIssue("https://github.com/dotnet/runtime/issues/53393")]
#endif
public async Task ClassWithComplexObjectsUsingIgnoreWhenWritingDefaultAttribute()
{
string json = @"{""Class"":{""MyInt16"":18}, ""Dictionary"":null}";
Expand Down Expand Up @@ -2091,9 +2064,6 @@ public class ClassUsingIgnoreWhenWritingDefaultAttribute
}

[Fact]
#if BUILDING_SOURCE_GENERATOR_TESTS
[ActiveIssue("https://github.com/dotnet/runtime/issues/53393")]
#endif
public async Task ClassWithComplexObjectUsingIgnoreNeverAttribute()
{
string json = @"{""Class"":null, ""Dictionary"":null}";
Expand Down Expand Up @@ -2375,10 +2345,6 @@ public async Task ValueType_Properties_NotIgnoredWhen_IgnoreNullValues_Active_Cl
}

[Fact]
#if BUILDING_SOURCE_GENERATOR_TESTS
// Need support for parameterized ctors.
[ActiveIssue("https://github.com/dotnet/runtime/issues/45448")]
#endif
public async Task ValueType_Properties_NotIgnoredWhen_IgnoreNullValues_Active_LargeStructTest()
{
var options = new JsonSerializerOptions { IgnoreNullValues = true };
Expand Down Expand Up @@ -2412,10 +2378,6 @@ public async Task ValueType_Properties_NotIgnoredWhen_IgnoreNullValues_Active_La
}

[Fact]
#if BUILDING_SOURCE_GENERATOR_TESTS
// Need support for parameterized ctors.
[ActiveIssue("https://github.com/dotnet/runtime/issues/45448")]
#endif
public async Task ValueType_Properties_NotIgnoredWhen_IgnoreNullValues_Active_SmallStructTest()
{
var options = new JsonSerializerOptions { IgnoreNullValues = true };
Expand Down Expand Up @@ -2478,7 +2440,7 @@ public LargeStructWithValueAndReferenceTypes(
}
}

private struct SmallStructWithValueAndReferenceTypes
public struct SmallStructWithValueAndReferenceTypes
{
public string MyString { get; }
public int MyInt { get; set; }
Expand All @@ -2502,10 +2464,6 @@ public SmallStructWithValueAndReferenceTypes(
public class PointClass { }

[Fact]
#if BUILDING_SOURCE_GENERATOR_TESTS
// Need support for parameterized ctors.
[ActiveIssue("https://github.com/dotnet/runtime/issues/45448")]
#endif
public async Task Ignore_WhenWritingNull_Globally()
{
var options = new JsonSerializerOptions
Expand Down Expand Up @@ -2579,17 +2537,8 @@ public class ClassWithThingsToIgnore
}

[Fact]
#if BUILDING_SOURCE_GENERATOR_TESTS
// Need support for parameterized ctors.
[ActiveIssue("https://github.com/dotnet/runtime/issues/45448")]
#endif
public async Task Ignore_WhenWritingNull_PerProperty()
{
var options = new JsonSerializerOptions
{
IncludeFields = true
};

string json = @"{
""MyPointClass2_IgnoredWhenWritingNull"":{},
""MyString1_IgnoredWhenWritingNull"":""Default"",
Expand All @@ -2604,7 +2553,7 @@ public async Task Ignore_WhenWritingNull_PerProperty()
}";

// All members should correspond to JSON contents, as ignore doesn't apply to deserialization.
ClassWithThingsToIgnore_PerProperty obj = await Serializer.DeserializeWrapper<ClassWithThingsToIgnore_PerProperty>(json, options);
ClassWithThingsToIgnore_PerProperty obj = await Serializer.DeserializeWrapper<ClassWithThingsToIgnore_PerProperty>(json);
Assert.NotNull(obj.MyPointClass2_IgnoredWhenWritingNull);
Assert.Equal("Default", obj.MyString1_IgnoredWhenWritingNull);
Assert.Null(obj.MyNullableBool1_IgnoredWhenWritingNull);
Expand All @@ -2628,7 +2577,7 @@ public async Task Ignore_WhenWritingNull_PerProperty()
""MyNullableBool2_IgnoredWhenWritingNull"":true,
""MyPointStruct1"":{""X"":0,""Y"":0}
}";
JsonTestHelper.AssertJsonEqual(expectedJson, await Serializer.SerializeWrapper(obj, options));
JsonTestHelper.AssertJsonEqual(expectedJson, await Serializer.SerializeWrapper(obj));
}

public class ClassWithThingsToIgnore_PerProperty
Expand All @@ -2639,22 +2588,24 @@ public class ClassWithThingsToIgnore_PerProperty
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public string MyString2_IgnoredWhenWritingNull;

[JsonInclude]
public int MyInt1;

public int MyInt2 { get; set; }

[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public bool? MyNullableBool1_IgnoredWhenWritingNull { get; set; }

[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
[JsonInclude, JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public bool? MyNullableBool2_IgnoredWhenWritingNull;

[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
[JsonInclude, JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public PointClass MyPointClass1_IgnoredWhenWritingNull;

[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public PointClass MyPointClass2_IgnoredWhenWritingNull { get; set; }

[JsonInclude]
public Point_2D_Struct_WithAttribute MyPointStruct1;

public Point_2D_Struct_WithAttribute MyPointStruct2 { get; set; }
Expand Down Expand Up @@ -2748,10 +2699,6 @@ public MyClassWithValueType() { }
}

[Fact]
#if BUILDING_SOURCE_GENERATOR_TESTS
// Needs bug fixes to custom converter handling.
[ActiveIssue("https://github.com/dotnet/runtime/issues/45448")]
#endif
public async Task JsonIgnoreCondition_WhenWritingDefault_OnValueTypeWithCustomConverter()
{
var obj = new MyClassWithValueType();
Expand Down Expand Up @@ -2799,10 +2746,6 @@ public async Task JsonIgnoreCondition_ConverterCalledOnDeserialize()
}

[Fact]
#if BUILDING_SOURCE_GENERATOR_TESTS
// Needs bug fixes to custom converter handling.
[ActiveIssue("https://github.com/dotnet/runtime/issues/45448")]
#endif
public async Task JsonIgnoreCondition_WhenWritingNull_OnValueTypeWithCustomConverter()
{
string json;
Expand Down
37 changes: 37 additions & 0 deletions ...on/tests/System.Text.Json.SourceGeneration.Tests/Serialization/PropertyVisibilityTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,14 @@ public override async Task ClassWithIgnoredAndPrivateMembers_DoesNotIncludeIgnor
[JsonSerializable(typeof(DictionaryWithPrivateKeyAndValueType))][JsonSerializable(typeof(ClassWithIgnoredAndPrivateMembers))]
[JsonSerializable(typeof(ClassWithInternalJsonIncludeProperties))]
[JsonSerializable(typeof(ClassWithIgnoredAndPrivateMembers))]
[JsonSerializable(typeof(ClassUsingIgnoreWhenWritingDefaultAttribute))]
[JsonSerializable(typeof(ClassUsingIgnoreNeverAttribute))]
[JsonSerializable(typeof(ClassWithIgnoredUnsupportedDictionary))]
[JsonSerializable(typeof(ClassWithProperty_IgnoreConditionAlways_Ctor))]
[JsonSerializable(typeof(ClassWithClassProperty_IgnoreConditionWhenWritingDefault_Ctor))]
[JsonSerializable(typeof(StructWithStructProperty_IgnoreConditionWhenWritingDefault_Ctor))]
[JsonSerializable(typeof(SmallStructWithValueAndReferenceTypes))]
[JsonSerializable(typeof(WrapperForClassWithIgnoredUnsupportedDictionary))]
[JsonSerializable(typeof(Class1))]
[JsonSerializable(typeof(Class2))]
[JsonSerializable(typeof(NamespaceBase.Class1), TypeInfoPropertyName = "Class1FromNamespaceBase")]
Expand Down Expand Up @@ -427,6 +435,27 @@ public void PublicContextAndJsonSerializerOptions()
Assert.Equal(obj.MaxDepth, deserialized.MaxDepth);
}

[Fact]
public void PocoWithNullableProperties_IgnoresNullValuesWithGlobalSetting()
{
// Regression test for https://github.com/dotnet/runtime/issues/96404
var value = new PocoWithNullableProperties();
string json = JsonSerializer.Serialize(value, DefaultContextWithGlobalIgnoreSetting.Default.PocoWithNullableProperties);
Assert.Equal("{}", json);
}

class PocoWithNullableProperties
{
public string? NullableRefType { get; set; }
public int? NullableValueType { get; set; }
}

[JsonSourceGenerationOptions(
GenerationMode = JsonSourceGenerationMode.Default,
DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull)]
[JsonSerializable(typeof(PocoWithNullableProperties))]
partial class DefaultContextWithGlobalIgnoreSetting : JsonSerializerContext;

[JsonSerializable(typeof(ClassWithNewSlotField))]
[JsonSerializable(typeof(int))]
[JsonSerializable(typeof(object))]
Expand Down Expand Up @@ -578,6 +607,14 @@ public void PublicContextAndJsonSerializerOptions()
[JsonSerializable(typeof(DictionaryWithPrivateKeyAndValueType))]
[JsonSerializable(typeof(ClassWithInternalJsonIncludeProperties))]
[JsonSerializable(typeof(ClassWithIgnoredAndPrivateMembers))]
[JsonSerializable(typeof(ClassUsingIgnoreWhenWritingDefaultAttribute))]
[JsonSerializable(typeof(ClassUsingIgnoreNeverAttribute))]
[JsonSerializable(typeof(ClassWithIgnoredUnsupportedDictionary))]
[JsonSerializable(typeof(ClassWithProperty_IgnoreConditionAlways_Ctor))]
[JsonSerializable(typeof(ClassWithClassProperty_IgnoreConditionWhenWritingDefault_Ctor))]
[JsonSerializable(typeof(StructWithStructProperty_IgnoreConditionWhenWritingDefault_Ctor))]
[JsonSerializable(typeof(SmallStructWithValueAndReferenceTypes))]
[JsonSerializable(typeof(WrapperForClassWithIgnoredUnsupportedDictionary))]
[JsonSerializable(typeof(Class1))]
[JsonSerializable(typeof(Class2))]
[JsonSerializable(typeof(NamespaceBase.Class1), TypeInfoPropertyName = "Class1FromNamespaceBase")]
Expand Down