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.

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

Conversation

brantburnett
Copy link
Contributor

@brantburnett brantburnett commented May 13, 2022

  • Skip init-only prop warning during source generation if the property
    is always ignored

  • Add tests

Fixes #66003

* Skip init-only prop warning during source generation if the property
is always ignored

* Add tests

Fixes dotnet#66033
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 13, 2022
@ghost
Copy link

ghost commented May 13, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Skip init-only prop warning during source generation if the property
    is always ignored

  • Add tests

Fixes #66033

Author: brantburnett
Assignees: -
Labels:

area-System.Text.Json, community-contribution

Milestone: -

@dnfadmin
Copy link

dnfadmin commented May 13, 2022

CLA assistant check
All CLA requirements met.

@brantburnett brantburnett marked this pull request as ready for review May 15, 2022 12:59
@brantburnett brantburnett changed the title Allow source generation for init-only props with JsonIgnore Allow source generation for classes with ignored init-only props May 15, 2022


[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.

@@ -49,6 +49,18 @@ public virtual async Task NonPublicInitOnlySetter_With_JsonInclude(Type type)
Assert.Equal(@"{""MyInt"":1}", await Serializer.SerializeWrapper(obj));
}

[Theory]
[InlineData(typeof(ClassWithPublic_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.

@jeffhandley
Copy link
Member

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.

Echoing @eiriktsarpalis's comment, we have some broader issues that we need to address with JsonIgnoreAttribute before we can proceed with this; that work will carry over into .NET 8. Thanks for working up the PR nonetheless, @brantburnett; it'll serve as a reference when we revisit this.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 27, 2022
@brantburnett brantburnett deleted the jsonignore-initonlyprops branch July 21, 2023 17:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Text.Json source generation should accept init-only properties with JsonIgnoreCondition.Always
6 participants