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

Ensure JsonSerializer / source generator works well with required members #70945

Closed
Tracked by #63762
stephentoub opened this issue Jun 19, 2022 · 4 comments · Fixed by #72937
Closed
Tracked by #63762

Ensure JsonSerializer / source generator works well with required members #70945

stephentoub opened this issue Jun 19, 2022 · 4 comments · Fixed by #72937
Assignees
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Jun 19, 2022

The new required members feature was recently merged into the C# 11 compiler.
https://github.com/dotnet/csharplang/blob/main/proposals/required-members.md
dotnet/runtime is already being built with a version supporting it.

We should ensure JsonSerializer plays nicely with required members, in whatever way is deemed fit. What should happen, for example, if a type with required members is deserialized and those required members aren't in the payload being deserialized?

@ghost
Copy link

ghost commented Jun 19, 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

The new required members feature was recently merged into the C# 11 compiler.
https://github.com/dotnet/csharplang/blob/main/proposals/required-members.md

We should ensure JsonSerializer plays nicely with required members, in whatever way is deemed fit. What should happen, for example, if a type with required members is deserialized and those required members aren't in the payload being deserialized?

Author: stephentoub
Assignees: -
Labels:

area-System.Text.Json

Milestone: 7.0.0

@bjornen77
Copy link
Contributor

bjornen77 commented Jun 19, 2022

Related:
#29861

@eiriktsarpalis
Copy link
Member

This needs to be considered separately for reflection and source generated serialization:

  • The reflection case should be relatively straightforward, since it doesn't affect how properties are populated. Effectively it's implementing Implement a concept of "Required" properties #29861. Since there's now a language keyword, we can avoid exposing a dedicated attribute for now, but we still need to add an JsonPropertyInfo.IsRequired flag in the metadata layer.
  • Source generation is different, like init-only properties we would need to make changes to the metadata infrastructure. Very unlikely this would make the cut for .NET 7 (we already punted init-only property support). We probably need make changes so that the relevant compile-time diagnostics and runtime exceptions are thrown when the required keyword is encountered.

cc @krwq

@krwq
Copy link
Member

krwq commented Jul 21, 2022

Required members - compiler feature TL; DR

Full info: https://github.com/dotnet/csharplang/blob/main/proposals/required-members.md

  • Any field or property of a type can be marked with required keyword.
  • Marking a member will cause compiler to add RequiredMemberAttribute to a member
  • If any of the members of the type have required keyword compiler will also annotate type with RequiredMemberAttribute as an indicator that given class contains required members
  • Any member marked with required keyword must be set during initialization (or attribute construction but it's irrelevant here) - note: initializing in the ctor is irrelevant
  • If object is constructed using constructor which is marked with SetsRequiredMembersAttribute validation rules are skipped

Current behavior without any code changes

  • reflection deserialization ignores required keyword, deserialization works regardless if field/property is required and given member won't be set if they're not present in JSON
  • source gen doesn't compile unless ctor has SetsRequiredMembersAttribute

Issues of mapping compiler required with required JSON properties

Proposal #29861 is very simple:

  • if JSON contract has required property then it is required to be there during deserialization
  • removing required property from the contract removes the requirement

When we look at the compiler feature following problems occur (assuming JsonTypeInfo.CreateObject is not explicitly set by user):

  • if contract with fields were ignored (current default) and field is marked with required keyword then compiler contract is always violated, the JSON required contract would not be
  • similar situation to the one above is when user modified contract and removed field or property with required keyword
  • undefined behavior happens when user sets JsonTypeInfo.CreateObject since we do not know what fields are initialized - potentially we could assume user has set all required compiler fields/properties but user could have potentially re-assigned same CreateObject we gave him

Potential solution

Since 1:1 mapping compiler feature with #29861 seems rather unrealistic and tricky we can define following simplified behavior for JSON:

Reflection

  • We add JsonPropertyInfo.IsRequired property with semantics described in Implement a concept of "Required" properties #29861 except no attribute (attribute TBD if still needed)
  • For DefaultJsonTypeInfoResolver if ctor of the given type doesn't have SetsRequiredMembersAttribute we set JsonPropertyInfo.IsRequired when matching C# (and equivalent for other languages) property has required keyword - SetsRequiredMembersAttribute means none of the properties are required

Source gen

Currently we will need to throw.
Generated CreateObject currently won't compile if there are any required members and compiler doesn't allow to suppress CS9035, this will likely need to be solved along with init-only properties

@ghost ghost locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants