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

[JSON source gen 3/3] Add new methods to JsonSerializer and System.Net.Http.Json APIs that take type metadata #51528

Merged
2 commits merged into from
Apr 20, 2021

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Apr 19, 2021

Contributes to #45448.

PR 1 - #51149
PR 2 - #51300
PR 3 - this PR

This PR completes the initial implementation of the source generator needed for community feedback (preview 4) and for Blazor to adopt the new pattern. cc @CoffeeFlux, @eerhardt, @ericstj, @Anipik.

#51544 tracks the remaining S.N.H.J APIs we need to add.

cc @jozkee for System.Net.Http.Json changes.

@layomia layomia added this to the 6.0.0 milestone Apr 19, 2021
@layomia layomia self-assigned this Apr 19, 2021
@layomia layomia requested a review from jozkee as a code owner April 19, 2021 23:02
@ghost
Copy link

ghost commented Apr 19, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #45448.

PR 1 - #51149
PR 2 - #51300
PR 3 - this PR

Author: layomia
Assignees: layomia
Labels:

area-System.Text.Json

Milestone: 6.0.0

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

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.

@eiriktsarpalis eiriktsarpalis requested a review from a team April 20, 2021 16:38
Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

LGTM; some nits and a couple questions.

}
private static JsonPropertyInfo[] PersonPropInitFunc(JsonSerializerContext context)
{
JsonContext jsonContext = (JsonContext)context;
Copy link
Member

Choose a reason for hiding this comment

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

Can this cast ever fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should never, unless we have a bug (e.g. a weird cross-pollination of options instances being used). The serializer should only pass the same context instance that the type info instance was a property on.

I'll think about this some more. We could be more defensive in the gen'd code and throw InvalidOperationException instead of leak InvalidCastException.

To avoid any potential issues here entirely, we could make the method non-static and reference the context directly (instead of taking it as an argument), but I want to be sure of any perf implications first.

@ghost
Copy link

ghost commented Apr 20, 2021

Hello @layomia!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 24966ab into dotnet:main Apr 20, 2021
@layomia
Copy link
Contributor Author

layomia commented Apr 20, 2021

/backport to release/6.0-preview4

@github-actions
Copy link
Contributor

Started backporting to release/6.0-preview4: https://github.com/dotnet/runtime/actions/runs/768780136

@github-actions
Copy link
Contributor

@layomia backporting to release/6.0-preview4 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Add new methods to JsonSerializer and System.Net.Http.Json APIs that take type metadata
.git/rebase-apply/patch:2505: trailing whitespace.
    
.git/rebase-apply/patch:2538: trailing whitespace.
            
.git/rebase-apply/patch:2551: trailing whitespace.
            
.git/rebase-apply/patch:2826: trailing whitespace.
    
.git/rebase-apply/patch:2859: trailing whitespace.
            
warning: squelched 6 whitespace errors
warning: 11 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	src/libraries/System.Text.Json/ref/System.Text.Json.cs
M	src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs
M	src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Span.cs
M	src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs
M	src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs
M	src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.cs
M	src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.ByteArray.cs
M	src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs
M	src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.String.cs
M	src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Utf8JsonWriter.cs
M	src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
M	src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs
M	src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
A	src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/DeserializationWrapper.cs
A	src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.Options.cs
A	src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.cs
A	src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/SerializationWrapper.cs
A	src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csproj
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj
CONFLICT (rename/rename): Rename "src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.cs"->"src/libraries/System.Text.Json/tests/Serialization/MetadataServicesTests/MetadataServicesTests.cs" in branch "HEAD" rename "src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.cs"->"src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/MetadataTests.JsonMetadataServices.cs" in "Add new methods to JsonSerializer and System.Net.Http.Json APIs that take type metadata"
Auto-merging src/libraries/System.Text.Json/tests/Serialization/MetadataServicesTests/MetadataServicesTests.cs and src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/MetadataTests.JsonMetadataServices.cs, both renamed from src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.cs
CONFLICT (rename/rename): Rename "src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.Options.cs"->"src/libraries/System.Text.Json/tests/Serialization/MetadataServicesTests/MetadataServicesTests.Options.cs" in branch "HEAD" rename "src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.Options.cs"->"src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/MetadataTests.Options.cs" in "Add new methods to JsonSerializer and System.Net.Http.Json APIs that take type metadata"
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Utf8JsonWriter.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Utf8JsonWriter.cs
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.String.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.String.cs
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.ByteArray.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.ByteArray.cs
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.cs
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Span.cs
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs
Auto-merging src/libraries/System.Text.Json/ref/System.Text.Json.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/ref/System.Text.Json.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add new methods to JsonSerializer and System.Net.Http.Json APIs that take type metadata
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@ericstj
Copy link
Member

ericstj commented Apr 20, 2021

I think the backport is failing because the 2/3 PR hasn't been merged yet

@layomia
Copy link
Contributor Author

layomia commented Apr 20, 2021

/backport to release/6.0-preview4

@github-actions
Copy link
Contributor

Started backporting to release/6.0-preview4: https://github.com/dotnet/runtime/actions/runs/768827649

Copy link

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

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

Everything in 2/3 and 3/3 LGTM, aside from one nit.

@danmoseley
Copy link
Member

@tdykstra @carlossanlop I think it's great that docs folks are able to review the docs in the exact same PR that we have implementation and tests. That has to be so much more efficient long term.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants