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.

Sign up for GitHub

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

Jump to bottom

Make KeyValuePair<,> support in JSON src-gen mode trim-safe #60072

Closed
layomia opened this issue Oct 6, 2021 · 1 comment · Fixed by #87637
Closed

Make KeyValuePair<,> support in JSON src-gen mode trim-safe #60072

layomia opened this issue Oct 6, 2021 · 1 comment · Fixed by #87637
Assignees
@layomia
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions source-generator Indicates an issue with a source generator feature
Milestone
8.0.0

Comments

@layomia
Copy link
Contributor

layomia commented Oct 6, 2021

From #60031 (comment):

Test failures indicate that in the reflection serializer, we are currently always rooting most of the implementation of the KeyValuePair type, while in src-gen mode, we always root the implementation whenever there is an type that should be deserialized with a parameterized ctor, even if it is not a KVP type.

[14:23:05] fail: [FAIL] System.Text.Json.Serialization.Tests.ArrayTests.ReadClassWithObjectList
[14:23:05] info: System.InvalidOperationException : Each parameter in the deserialization constructor on type 'System.Collections.Generic.KeyValuePair`2[System.String,System.String]' must bind to an object property or field on deserialization. Each parameter name must match with a property or field on the object. The match can be case-insensitive.
[14:23:05] info:    at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_ConstructorParameterIncompleteBinding(Type ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs:line 213
[14:23:05] info:    at System.Text.Json.Serialization.Converters.ObjectWithParameterizedConstructorConverter`1[[System.Collections.Generic.KeyValuePair`2[[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].OnTryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , KeyValuePair`2& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs:line 35
[14:23:05] info:    at System.Text.Json.Serialization.JsonConverter`1[[System.Collections.Generic.KeyValuePair`2[[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].TryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , KeyValuePair`2& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs:line 249
[14:23:05] info:    at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1[[System.Collections.Generic.KeyValuePair`2[[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ReadJsonAndSetMember(Object , ReadStack& , Utf8JsonReader& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs:line 391
[14:23:05] info:    at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1[[System.Text.Json.Serialization.Tests.SimpleTestClass, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].OnTryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , SimpleTestClass& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs:line 70
[14:23:05] info:    at System.Text.Json.Serialization.JsonConverter`1[[System.Text.Json.Serialization.Tests.SimpleTestClass, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].TryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , SimpleTestClass& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs:line 249
[14:23:05] info:    at System.Text.Json.Serialization.JsonCollectionConverter`2[[System.Collections.Generic.List`1[[System.Text.Json.Serialization.Tests.SimpleTestClass, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]], System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Text.Json.Serialization.Tests.SimpleTestClass, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].OnTryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , List`1& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonCollectionConverter.cs:line 91
[14:23:05] info:    at System.Text.Json.Serialization.JsonConverter`1[[System.Collections.Generic.List`1[[System.Text.Json.Serialization.Tests.SimpleTestClass, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]], System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].TryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , List`1& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs:line 249
[14:23:05] info:    at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1[[System.Collections.Generic.List`1[[System.Text.Json.Serialization.Tests.SimpleTestClass, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]], System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ReadJsonAndSetMember(Object , ReadStack& , Utf8JsonReader& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs:line 391
[14:23:05] info:    at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1[[System.Text.Json.Serialization.Tests.TestClassWithObjectList, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].OnTryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , TestClassWithObjectList& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs:line 70
[14:23:05] info:    at System.Text.Json.Serialization.JsonConverter`1[[System.Text.Json.Serialization.Tests.TestClassWithObjectList, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].TryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , TestClassWithObjectList& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs:line 249
[14:23:06] info:    at System.Text.Json.Serialization.JsonConverter`1[[System.Text.Json.Serialization.Tests.TestClassWithObjectList, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].ReadCore(Utf8JsonReader& , JsonSerializerOptions , ReadStack& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.ReadCore.cs:line 62
[14:23:06] info:    at System.Text.Json.JsonSerializer.ReadFromSpan[TestClassWithObjectList](ReadOnlySpan`1 , JsonTypeInfo , Nullable`1 ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Helpers.cs:line 43
[14:23:06] info:    at System.Text.Json.JsonSerializer.Deserialize[TestClassWithObjectList](ReadOnlySpan`1 , JsonSerializerOptions ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Span.cs:line 32
[14:23:06] info:    at System.Text.Json.Serialization.Tests.ArrayTests.ReadClassWithObjectList() in /_/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/Array.ReadTests.cs:line 271
[14:23:06] info:    at System.Reflection.RuntimeMethodInfo.Invoke(Object , BindingFlags , Binder , Object[] , CultureInfo ) in /_/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs:line 370

This is the LOC responsible:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/KeyValuePairConverter.cs

Line 14 in 3e79767

typeof(KeyValuePair<TKey, TValue>).GetConstructor(new[] { typeof(TKey), typeof(TValue) })!;

Although the rooting code never actually runs in the src-gen case & the rooted KVP code is rather small, for correctness, we should look into having a trim-safe way to provide KVP support in src-gen mode.
@layomia layomia added area-System.Text.Json source-generator Indicates an issue with a source generator feature labels Oct 6, 2021
@layomia layomia added this to the 7.0.0 milestone Oct 6, 2021
@layomia layomia self-assigned this Oct 6, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Oct 6, 2021
@ghost
Copy link

ghost commented Oct 6, 2021

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

From #60031 (comment):

Test failures indicate that in the reflection serializer, we are currently always rooting most of the implementation of the KeyValuePair type, while in src-gen mode, we always root the implementation whenever there is an type that should be deserialized with a parameterized ctor, even if it is not a KVP type.

[14:23:05] fail: [FAIL] System.Text.Json.Serialization.Tests.ArrayTests.ReadClassWithObjectList
[14:23:05] info: System.InvalidOperationException : Each parameter in the deserialization constructor on type 'System.Collections.Generic.KeyValuePair`2[System.String,System.String]' must bind to an object property or field on deserialization. Each parameter name must match with a property or field on the object. The match can be case-insensitive.
[14:23:05] info:    at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_ConstructorParameterIncompleteBinding(Type ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs:line 213
[14:23:05] info:    at System.Text.Json.Serialization.Converters.ObjectWithParameterizedConstructorConverter`1[[System.Collections.Generic.KeyValuePair`2[[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].OnTryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , KeyValuePair`2& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs:line 35
[14:23:05] info:    at System.Text.Json.Serialization.JsonConverter`1[[System.Collections.Generic.KeyValuePair`2[[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].TryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , KeyValuePair`2& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs:line 249
[14:23:05] info:    at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1[[System.Collections.Generic.KeyValuePair`2[[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ReadJsonAndSetMember(Object , ReadStack& , Utf8JsonReader& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs:line 391
[14:23:05] info:    at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1[[System.Text.Json.Serialization.Tests.SimpleTestClass, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].OnTryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , SimpleTestClass& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs:line 70
[14:23:05] info:    at System.Text.Json.Serialization.JsonConverter`1[[System.Text.Json.Serialization.Tests.SimpleTestClass, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].TryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , SimpleTestClass& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs:line 249
[14:23:05] info:    at System.Text.Json.Serialization.JsonCollectionConverter`2[[System.Collections.Generic.List`1[[System.Text.Json.Serialization.Tests.SimpleTestClass, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]], System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Text.Json.Serialization.Tests.SimpleTestClass, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].OnTryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , List`1& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonCollectionConverter.cs:line 91
[14:23:05] info:    at System.Text.Json.Serialization.JsonConverter`1[[System.Collections.Generic.List`1[[System.Text.Json.Serialization.Tests.SimpleTestClass, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]], System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].TryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , List`1& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs:line 249
[14:23:05] info:    at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1[[System.Collections.Generic.List`1[[System.Text.Json.Serialization.Tests.SimpleTestClass, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]], System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ReadJsonAndSetMember(Object , ReadStack& , Utf8JsonReader& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs:line 391
[14:23:05] info:    at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1[[System.Text.Json.Serialization.Tests.TestClassWithObjectList, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].OnTryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , TestClassWithObjectList& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs:line 70
[14:23:05] info:    at System.Text.Json.Serialization.JsonConverter`1[[System.Text.Json.Serialization.Tests.TestClassWithObjectList, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].TryRead(Utf8JsonReader& , Type , JsonSerializerOptions , ReadStack& , TestClassWithObjectList& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs:line 249
[14:23:06] info:    at System.Text.Json.Serialization.JsonConverter`1[[System.Text.Json.Serialization.Tests.TestClassWithObjectList, System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].ReadCore(Utf8JsonReader& , JsonSerializerOptions , ReadStack& ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.ReadCore.cs:line 62
[14:23:06] info:    at System.Text.Json.JsonSerializer.ReadFromSpan[TestClassWithObjectList](ReadOnlySpan`1 , JsonTypeInfo , Nullable`1 ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Helpers.cs:line 43
[14:23:06] info:    at System.Text.Json.JsonSerializer.Deserialize[TestClassWithObjectList](ReadOnlySpan`1 , JsonSerializerOptions ) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Span.cs:line 32
[14:23:06] info:    at System.Text.Json.Serialization.Tests.ArrayTests.ReadClassWithObjectList() in /_/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/Array.ReadTests.cs:line 271
[14:23:06] info:    at System.Reflection.RuntimeMethodInfo.Invoke(Object , BindingFlags , Binder , Object[] , CultureInfo ) in /_/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs:line 370

This is the LOC responsible:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/KeyValuePairConverter.cs

Line 14 in 3e79767

typeof(KeyValuePair<TKey, TValue>).GetConstructor(new[] { typeof(TKey), typeof(TValue) })!;

Although the rooting code never actually runs in the src-gen case & the rooted KVP code is rather small, for correctness, we should look into having a trim-safe way to provide KVP support in src-gen mode.

Author: layomia
Assignees: layomia
Labels:

area-System.Text.Json, source-generator
Milestone: 7.0.0

@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Oct 6, 2021
@eiriktsarpalis eiriktsarpalis added the enhancement Product code improvement that does NOT require public API changes/additions label Oct 14, 2021
@layomia layomia mentioned this issue Jan 22, 2022
Closed
16 tasks
@jeffhandley jeffhandley modified the milestones: 7.0.0, Future Apr 6, 2022
@layomia layomia mentioned this issue Oct 13, 2022
Closed
26 tasks
@eiriktsarpalis eiriktsarpalis mentioned this issue Jun 15, 2023
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 15, 2023
@teo-tsirpanis teo-tsirpanis modified the milestones: Future, 8.0.0 Jun 15, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 16, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@layomia layomia

Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions source-generator Indicates an issue with a source generator feature
Projects
None yet
Milestone
8.0.0
Development

Successfully merging a pull request may close this issue.

Remove KeyValuePairConverter. eiriktsarpalis/runtime
4 participants
@jeffhandley @eiriktsarpalis @layomia @teo-tsirpanis