Support JsonExtensionData in STJ source-gen #58912
Conversation
layomia
requested review from
krwq,
eiriktsarpalis,
steveharter and
eerhardt
|
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. |
src/libraries/System.Text.Json/gen/Reflection/TypeExtensions.cs
Outdated
Show resolved
Hide resolved
|
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue DetailsFixes #58273. We should port this to 6.0.
|
src/libraries/System.Text.Json/tests/Common/ExtensionDataTests.cs
Outdated
Show resolved
Hide resolved
| @@ -141,4 +141,16 @@ | |||
| <data name="MultipleJsonConstructorAttributeTitle" xml:space="preserve"> | |||
| <value>Type has multiple constructors annotated with JsonConstructorAttribute.</value> | |||
| </data> | |||
| <data name="MultipleJsonExtensionDataAttributeFormat" xml:space="preserve"> | |||
| <value>Type '{0}' has multiple members annotated with 'JsonExtensionDataAttribute'.</value> | |||
There was a problem hiding this comment.
It's not consistent whether you quote time names or not. Two messages here have quotes and two do not. I'd say in general we don't quote unless possibly if it's an insertion. That seems to match src\Resources\Strings.resx as well (except one case)
| <value>Type '{0}' has multiple members annotated with 'JsonExtensionDataAttribute'.</value> | |
| <value>Type '{0}' has multiple members annotated with JsonExtensionDataAttribute.</value> |
There was a problem hiding this comment.
There's some others quoted in this file as well. Do what you will :)
There was a problem hiding this comment.
I thought that we also quote names if we don't want translation to convert them, e.g. type names.
There was a problem hiding this comment.
In these cases there is no way anyone would attempt to translate them as they're obviously not words. Maybe if it was Date but then there is a way to annotate the resource so they don't localize a part...
layomia
force-pushed
the
ExtensionData
branch
from
f4afd92
to
e6406f1
Compare
| @@ -58,6 +58,14 @@ internal class TypeGenerationSpec | |||
|
|
|||
| public TypeGenerationSpec? NullableUnderlyingTypeMetadata { get; private set; } | |||
|
|
|||
| /// <summary> | |||
| /// Supports deserialization of extension data dictionaries typed as I[ReadOnly]Dictionary<string, object/JsonElement>. | |||
| /// Specifies a concrete type to instanciate, which would be Dictionary<string, object/JsonElement>. | |||
There was a problem hiding this comment.
| /// Specifies a concrete type to instanciate, which would be Dictionary<string, object/JsonElement>. | |
| /// Specifies a concrete type to instantiate, which would be Dictionary<string, object/JsonElement>. |
| @@ -11,9 +11,6 @@ public abstract partial class ConstructorTests | |||
| { | |||
| [Fact] | |||
| [OuterLoop] | |||
| #if BUILDING_SOURCE_GENERATOR_TESTS | |||
| [ActiveIssue("Needs JsonExtensionData support.")] | |||
| #endif | |||
| public async Task MultipleThreadsLooping() | |||
There was a problem hiding this comment.
Nit: what is this test meant to be testing? If it's ensuring that the serialization infrastructure can cope with many serialization in parallel, that is generally handled by xunit's capacity to run multiple tests in parallel. If it's testing something else, it should be refelected in the test name.
There was a problem hiding this comment.
Yeah it's meant to ensure the serializer can cope with multiple serialization routines in parallel. In my experience it has caught a few issues in product changes, so I think it's beneficial to keep them. cc @steveharter
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1228674714 |
|
@eiriktsarpalis backporting to release/6.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Support JsonExtensionData in STJ source-gen
Using index info to reconstruct a base tree...
M src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
M src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs
M src/libraries/System.Text.Json/ref/System.Text.Json.cs
M src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Converters.cs
M src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.Exceptions.cs
M src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/System.Text.Json.SourceGeneration.Tests.csproj
M 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/System.Text.Json.Tests.csproj
Auto-merging src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/System.Text.Json.SourceGeneration.Tests.csproj
Auto-merging src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.Exceptions.cs
Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Converters.cs
Auto-merging src/libraries/System.Text.Json/ref/System.Text.Json.cs
Auto-merging src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs
Auto-merging src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.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 Support JsonExtensionData in STJ source-gen
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 128Please backport manually! |
|
@layomia looks like we need to get #58983 merged before we can start backporting this one. I expect the same will likely apply to #58993. cc @danmoseley |
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1230617504 |
ghost
locked as resolved and limited conversation to collaborators
Fixes #58273. We should port this to 6.0.