-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Enable property visibility tests in source-gen mode #54526
Conversation
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue Details
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
42feb2b
to
9305808
Compare
2077e0c
to
347577c
Compare
...libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs
Show resolved
Hide resolved
...libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs
Show resolved
Hide resolved
@@ -610,13 +609,16 @@ private string GenerateForObject(TypeGenerationSpec typeMetadata) | |||
sb.Append($@" | |||
{PropVarName}[{i}] = {JsonMetadataServicesTypeRef}.CreatePropertyInfo<{memberTypeCompilableName}>( | |||
options, | |||
isProperty: {memberMetadata.IsProperty.ToString().ToLowerInvariant()}, | |||
isProperty: {ToCSharpKeyword(memberMetadata.IsProperty)}, |
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.
This is now at 14 parameters. I wonder if we should just create the JsonPropertyInfo in one call (with minimal parameters) and then set the individual values.
Another option is to pass a struct by ref. This could be made binary forward-compatible (e.g. we only add fields) but would need to be discussed\designed.
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.
Agree with the feedback that this is getting too long. I like the struct-by-ref idea and will bring it up in design review.
|
||
if (!jsonPropertyInfo.SrcGen_IsPublic) | ||
{ | ||
if (hasJsonInclude) |
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.
Could this be detected during source generation instead of at runtime? (and avoid passing this info over). Perhaps a compile error would result (trying to access to private member)?
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.
Yes this a good idea, but makes testing a bit too difficult to tackle in this PR at this point. Switching from runtime exceptions to compile-time warnings means that test projects won't compile & we'd have to test using a different approach. Will tackle in a follow-up.
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.
Hm I think we might have to keep this functionality as-is, given that some of the things that cause property-name conflicts (e.g. naming policy) are specified at runtime only. I'll make sure.
8f5785b
to
91c74f0
Compare
91c74f0
to
7712b6d
Compare
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.
LGTM with later feedback after this PR to review the public API for source gen for alternate strategies.
Hello @layomia! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
FYI @romfir, thanks for logging #54385. It has been fixed in this PR, and you can try the fix using the latest build of |
@layomia thanks for the info, in the console app it now works, but json serializer stopped working at all with Blazor
|
@romfir thanks for the update. Can you please file a new issue for this? Any repro information, e.g. a simplified |
I've created #55568 |
JsonSerializer
functionality and theJsonSerializer
+ source gen programming model, to ensure functional parity between the two. This PR starts with thePropertyVisibilityTests
, but ultimately all the serializer tests will be shared.Currently contains changes from #54527, but will be cleaned up after that PR goes in.