Support more collections in STJ source generator #55566
Conversation
ghost
added
the
|
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. |
|
Tagging subscribers to this area: @eiriktsarpalis, @layomia |
layomia
force-pushed
the
SrcGenCollections
branch
5 times, most recently
from
17ef405 to
f78d7b9
Compare
layomia
requested review from
eiriktsarpalis,
jozkee and
steveharter
as code owners
layomia
force-pushed
the
SrcGenCollections
branch
from
f78d7b9 to
ab75b8d
Compare
src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Specialized.Read.cs
Outdated
Show resolved
Hide resolved
...tem.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/CollectionTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/Reflection/TypeExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/Reflection/TypeExtensions.cs
Outdated
Show resolved
Hide resolved
steveharter
left a comment
steveharter
left a comment
There was a problem hiding this comment.
Misc feedback provided (none that can't be addressed after this PR)
Looks good pending green CI.
This new collection support is important to get in for Preview 7.
layomia
force-pushed
the
SrcGenCollections
branch
3 times, most recently
from
6ce1542 to
6c05c74
Compare
layomia
force-pushed
the
SrcGenCollections
branch
from
6c05c74 to
011c403
Compare
layomia
force-pushed
the
SrcGenCollections
branch
from
d408b89 to
01ffde9
Compare
|
FYI @eerhardt |
|
|
||
| if (converter.RequiresDynamicMemberAccessors) | ||
| { | ||
| converter.Initialize(Options, this); |
There was a problem hiding this comment.
does this account for the possibility that one converter instance is initialized by multiple JsonTypeInfo instances? I think that it's occassionally possible although not sure it applies to converters that override RequiresDynamicMemberAccessors.
There was a problem hiding this comment.
Hmm I think it could happen occasionally due to threading, but I think even in that case each JsonTypeInfo construction would create a separate converter instance, and the first one to complete would win, discarding the duplicates. Each converter instance would only be for a specific type, so there are no issues of creating invalid dynamic accessors.
| }; | ||
|
|
||
| string expectedJson = JsonSerializer.Serialize(source); | ||
| string expectedJson = await JsonSerializerWrapperForString.SerializeWrapper(source); |
There was a problem hiding this comment.
Since this method call is only used to generate the expected JSON string, we don't really need to exercise any async APIs here.
| }; | ||
|
|
||
| string expectedJson = JsonSerializer.Serialize(new { Data = source }); | ||
| string expectedJson = await JsonSerializerWrapperForString.SerializeWrapper(new { Data = source }); |
There was a problem hiding this comment.
Ditto for this and all the other expectedJson values in this file.
| using Xunit; | ||
|
|
||
| namespace System.Text.Json.Tests.Serialization | ||
| namespace System.Text.Json.Serialization.Tests |
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Xunit; | ||
| using Utf8MemoryStream = System.Text.Json.Tests.Serialization.CollectionTests.Utf8MemoryStream; |
There was a problem hiding this comment.
FWIW the System.Text.Json.Tests.Serialization namespace has crept up in quite a few other places as well.
|
Test failures are unrelated. Will address further feedback in a follow-up. |
...rc/System/Text/Json/Serialization/Converters/Collection/IEnumerableWithAddMethodConverter.cs
Show resolved
Hide resolved
4 participants
Fixes #53393. The collection trimming tests are not updated in this PR (#53437).