Skip to content
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

Allow source generation for classes with ignored init-only props #69327

Closed
wants to merge 2 commits into from
Closed

Allow source generation for classes with ignored init-only props #69327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1bc342b
Allow source generation for init-only props with JsonIgnore
brantburnett May 13, 2022
d97c1ca
Add tests for overridden and shadowed init only properties
brantburnett May 18, 2022
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
3 changes: 2 additions & 1 deletion src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1067,7 +1067,8 @@ void CacheMemberHelper(Location memberLocation)
_implicitlyRegisteredTypes.Add(dataExtensionPropGenSpec);
}

if (!hasInitOnlyProperties && spec.CanUseSetter && spec.IsInitOnlySetter && !PropertyIsConstructorParameter(spec, paramGenSpecArray))
if (!hasInitOnlyProperties && spec.CanUseSetter && spec.IsInitOnlySetter &&
spec.DefaultIgnoreCondition != JsonIgnoreCondition.Always && !PropertyIsConstructorParameter(spec, paramGenSpecArray))
{
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(InitOnlyPropertyDeserializationNotSupported, memberLocation, new string[] { type.Name }));
hasInitOnlyProperties = true;
Expand Down
37 changes: 37 additions & 0 deletions src/libraries/System.Text.Json/tests/Common/PropertyVisibilityTests.InitOnly.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,30 @@ public virtual async Task NonPublicInitOnlySetter_With_JsonInclude(Type type)
Assert.Equal(@"{""MyInt"":1}", await Serializer.SerializeWrapper(obj));
}

[Theory]
[InlineData(typeof(ClassWithPublic_InitOnlyProperty_WithJsonIgnoreProperty))]
[InlineData(typeof(ClassWithPublicShadowed_InitOnlyProperty_WithJsonIgnoreProperty))]
[InlineData(typeof(ClassWithPublicOverridden_InitOnlyProperty_WithJsonIgnoreProperty))]
public async Task PublicInitOnlySetter_With_JsonIgnore(Type type)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add tests for these polymorphic scenarios (https://dotnetfiddle.net/caqjtK), here and in the unit tests? Ensure that no warnings are emitted for ignored properties.

using System;
using System.Text.Json;
using System.Text.Json.Serialization;
					
public class Program
{
	public static void Main()
	{
		string json = JsonSerializer.Serialize(new Derived1());
		Console.WriteLine(json);
		
		json = JsonSerializer.Serialize(new Derived2());
		Console.WriteLine(json);
	}
	
	public class Base1
	{
		public int MyInt { get; init; } = 3;
	}
	
	public class Derived1 : Base1
	{
		[JsonIgnore]
		public new int MyInt { get; init; } = 4;
	}
	
	public class Base2
	{
		public virtual int MyInt { get; init; } = 3;
	}
	
	public class Derived2 : Base2
	{
		[JsonIgnore]
		public override int MyInt { get; init; } = 4;
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you also add tests for these polymorphic scenarios (https://dotnetfiddle.net/caqjtK), here and in the unit tests? Ensure that no warnings are emitted for ignored properties.

I've added the tests, but I'm running into trouble with the shadowed property scenario. That scenario fails at runtime during deserialization with "Deserialization of init-only properties is currently not supported in source generation mode."

Looking at it briefly, this feels like an issue with shadowed properties in general. PropertyIsOverridenAndIgnored doesn't appear to take shadowing into account. However, I'm unsure what the design for polymorphism is supposed to do with this scenario. Is this a separate issue related to polymorphism and ignored fields we should fix first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@layomia Did you have any thoughts on how polymorphism, shadowed properties, and JsonIgnore should be working together? Is this a separate bug or working as designed?

Copy link
Member

Choose a reason for hiding this comment

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

There's a bunch of reliability issues concerning the semantics of JsonIgnoreAttribute, see #59675, #50078, #66900, #60082. I think we might want to consider ironing these out before merging this PR. This might likely happen after .NET 7.

{
// Public and ignored init-only property setter ignored.
object obj = await Serializer.DeserializeWrapper(@"{""MyInt"":1}", type);
Assert.Equal(0, (int)type.GetProperty("MyInt").GetValue(obj));

// Public getter also ignored for serialization.
Assert.Equal(@"{}", await Serializer.SerializeWrapper(obj, type));
}

public class ClassWithInitOnlyProperty
{
public int MyInt { get; init; }
}

public class ClassWithVirtualInitOnlyProperty
{
public virtual int MyInt { get; init; }
}

public struct StructWithInitOnlyProperty
{
public int MyInt { get; init; }
Expand Down Expand Up @@ -91,5 +110,23 @@ public class Class_PropertyWith_ProtectedInitOnlySetter_WithAttribute
[JsonInclude]
public int MyInt { get; protected init; }
}

public class ClassWithPublic_InitOnlyProperty_WithJsonIgnoreProperty
{
[JsonIgnore]
public int MyInt { get; init; }
}

public class ClassWithPublicShadowed_InitOnlyProperty_WithJsonIgnoreProperty : ClassWithInitOnlyProperty
{
[JsonIgnore]
public new int MyInt { get; init; }
}

public class ClassWithPublicOverridden_InitOnlyProperty_WithJsonIgnoreProperty : ClassWithVirtualInitOnlyProperty
{
[JsonIgnore]
public override int MyInt { get; init; }
}
}
}
6 changes: 6 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 @@ -222,6 +222,9 @@ public override async Task HonorJsonPropertyName_PrivateSetter()
[JsonSerializable(typeof(ClassWithPrivate_InitOnlyProperty_WithJsonIncludeProperty))]
[JsonSerializable(typeof(ClassWithInternal_InitOnlyProperty_WithJsonIncludeProperty))]
[JsonSerializable(typeof(ClassWithProtected_InitOnlyProperty_WithJsonIncludeProperty))]
[JsonSerializable(typeof(ClassWithPublic_InitOnlyProperty_WithJsonIgnoreProperty))]
[JsonSerializable(typeof(ClassWithPublicShadowed_InitOnlyProperty_WithJsonIgnoreProperty))]
[JsonSerializable(typeof(ClassWithPublicOverridden_InitOnlyProperty_WithJsonIgnoreProperty))]
[JsonSerializable(typeof(ClassWithPublicProperty))]
[JsonSerializable(typeof(ClassInheritedWithPropertyNamingConflictWhichThrows))]
[JsonSerializable(typeof(StructWithOverride))]
Expand Down Expand Up @@ -397,6 +400,9 @@ public override async Task JsonIgnoreCondition_WhenWritingNull_OnValueType_Fail_
[JsonSerializable(typeof(ClassWithPrivate_InitOnlyProperty_WithJsonIncludeProperty))]
[JsonSerializable(typeof(ClassWithInternal_InitOnlyProperty_WithJsonIncludeProperty))]
[JsonSerializable(typeof(ClassWithProtected_InitOnlyProperty_WithJsonIncludeProperty))]
[JsonSerializable(typeof(ClassWithPublic_InitOnlyProperty_WithJsonIgnoreProperty))]
[JsonSerializable(typeof(ClassWithPublicShadowed_InitOnlyProperty_WithJsonIgnoreProperty))]
[JsonSerializable(typeof(ClassWithPublicOverridden_InitOnlyProperty_WithJsonIgnoreProperty))]
[JsonSerializable(typeof(ClassWithPublicProperty))]
[JsonSerializable(typeof(ClassInheritedWithPropertyNamingConflictWhichThrows))]
[JsonSerializable(typeof(StructWithOverride))]
Expand Down
102 changes: 101 additions & 1 deletion .../System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/CompilationHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,106 @@ public partial class MyJsonContext : JsonSerializerContext

return CreateCompilation(source);
}


public static Compilation CreateCompilationWithIgnoredInitOnlyProperties()
{
string source = @"
using System;
using System.Text.Json.Serialization;

namespace HelloWorld
{
public class MyClass
{
public MyClass()
{
}

public int Value { get; set; }
[JsonIgnore] public string Ignored { get; init; }
}

[JsonSerializable(typeof(MyClass))]
public partial class MyJsonContext : JsonSerializerContext
{
}
}";

return CreateCompilation(source);
}

public static Compilation CreateCompilationWithOverriddenIgnoredInitOnlyProperties()
{
string source = @"
using System;
using System.Text.Json.Serialization;

namespace HelloWorld
{
public class Base
{
public Base()
{
}

public int Value { get; set; }
public virtual string InitOnly { get; init; }
}

public class Derived
{
public Base()
{
}

[JsonIgnore] public override string InitOnly { get; init; }
}

[JsonSerializable(typeof(Derived))]
public partial class MyJsonContext : JsonSerializerContext
{
}
}";

return CreateCompilation(source);
}

public static Compilation CreateCompilationWithShadowedIgnoredInitOnlyProperties()
{
string source = @"
using System;
using System.Text.Json.Serialization;

namespace HelloWorld
{
public class Base
{
public Base()
{
}

public int Value { get; set; }
public string InitOnly { get; init; }
}

public class Derived
{
public Base()
{
}

[JsonIgnore] public new string InitOnly { get; init; }
}

[JsonSerializable(typeof(Derived))]
public partial class MyJsonContext : JsonSerializerContext
{
}
}";

return CreateCompilation(source);
}

public static Compilation CreateCompilationWithMixedInitOnlyProperties()
{
string source = @"
Expand All @@ -352,6 +451,7 @@ public MyClass(int value)
}

public int Value { get; init; }
[JsonIgnore] public string Ignored { get; init; }
public string Orphaned { get; init; }
}

Expand Down
41 changes: 40 additions & 1 deletion ...tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorDiagnosticsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,46 @@ public void DoNotWarnOnClassesWithConstructorInitOnlyProperties()
CompilationHelper.CheckDiagnosticMessages(DiagnosticSeverity.Warning, generatorDiags, Array.Empty<(Location, string)>());
CompilationHelper.CheckDiagnosticMessages(DiagnosticSeverity.Error, generatorDiags, Array.Empty<(Location, string)>());
}


[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/58770", TestPlatforms.Browser)]
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the issue with browser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly am unsure, I copied this from all the other tests in the file.

public void DoNotWarnOnClassesWithIgnoredInitOnlyProperties()
{
Compilation compilation = CompilationHelper.CreateCompilationWithIgnoredInitOnlyProperties();
JsonSourceGenerator generator = new JsonSourceGenerator();
CompilationHelper.RunGenerators(compilation, out var generatorDiags, generator);

CompilationHelper.CheckDiagnosticMessages(DiagnosticSeverity.Info, generatorDiags, Array.Empty<(Location, string)>());
CompilationHelper.CheckDiagnosticMessages(DiagnosticSeverity.Warning, generatorDiags, Array.Empty<(Location, string)>());
CompilationHelper.CheckDiagnosticMessages(DiagnosticSeverity.Error, generatorDiags, Array.Empty<(Location, string)>());
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/58770", TestPlatforms.Browser)]
public void DoNotWarnOnClassesWithOverriddenIgnoredInitOnlyProperties()
{
Compilation compilation = CompilationHelper.CreateCompilationWithOverriddenIgnoredInitOnlyProperties();
JsonSourceGenerator generator = new JsonSourceGenerator();
CompilationHelper.RunGenerators(compilation, out var generatorDiags, generator);

CompilationHelper.CheckDiagnosticMessages(DiagnosticSeverity.Info, generatorDiags, Array.Empty<(Location, string)>());
CompilationHelper.CheckDiagnosticMessages(DiagnosticSeverity.Warning, generatorDiags, Array.Empty<(Location, string)>());
CompilationHelper.CheckDiagnosticMessages(DiagnosticSeverity.Error, generatorDiags, Array.Empty<(Location, string)>());
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/58770", TestPlatforms.Browser)]
public void DoNotWarnOnClassesWithShawdowedIgnoredInitOnlyProperties()
{
Compilation compilation = CompilationHelper.CreateCompilationWithShadowedIgnoredInitOnlyProperties();
JsonSourceGenerator generator = new JsonSourceGenerator();
CompilationHelper.RunGenerators(compilation, out var generatorDiags, generator);

CompilationHelper.CheckDiagnosticMessages(DiagnosticSeverity.Info, generatorDiags, Array.Empty<(Location, string)>());
CompilationHelper.CheckDiagnosticMessages(DiagnosticSeverity.Warning, generatorDiags, Array.Empty<(Location, string)>());
CompilationHelper.CheckDiagnosticMessages(DiagnosticSeverity.Error, generatorDiags, Array.Empty<(Location, string)>());
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/58770", TestPlatforms.Browser)]
public void WarnOnClassesWithMixedInitOnlyProperties()
Expand Down