Allow ctor parameters to exactly match property name #38959
Conversation
| @@ -36,14 +36,14 @@ internal sealed partial class JsonClassInfo | |||
|
|
|||
| // All of the serializable parameters on a POCO constructor keyed on parameter name. | |||
| // Only paramaters which bind to properties are cached. | |||
| public volatile Dictionary<string, JsonParameterInfo>? ParameterCache; | |||
| public Dictionary<string, JsonParameterInfo>? ParameterCache; | |||
There was a problem hiding this comment.
These don't need volatile since they are not accessed by other threads until initialization is complete.
|
Needed to remove usage of C# 9.0 'record' usage in tests due to CI failure in dotnet-runtime-perf leg: |
...braries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Exceptions.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
| Assert.Equal(5, objType.GetProperty("Prop").GetValue(newObj)); | ||
| } | ||
|
|
||
| /* Enable when C# 9.0 'records' feature works in CI |
There was a problem hiding this comment.
Can we file an issue to track this?
cc @safern do you know when we'll enable this in CI?
There was a problem hiding this comment.
It appears the performance CI leg for some reason is not using the same compiler\SDK as the other tests.
There was a problem hiding this comment.
@safern @DrewScoggins the "IsExternalInit" failures are occurring with the tests using the "record" type:
Net472
Serialization\ConstructorTests\ConstructorTests.Exceptions.cs(155,37): error CS0518: Predefined type 'System.Runtime.CompilerServices.IsExternalInit' is not defined or imported [F:\workspace\_work\1\s\src\libraries\System.Text.Json\tests\System.Text.Json.Tests.csproj]
##[error]Serialization\ConstructorTests\ConstructorTests.Exceptions.cs(155,37): error CS0518: (NETCORE_ENGINEERING_TELEMETRY=Build) Predefined type 'System.Runtime.CompilerServices.IsExternalInit' is not defined or imported
Serialization\ConstructorTests\ConstructorTests.Exceptions.cs(168,60): error CS0518: Predefined type 'System.Runtime.CompilerServices.IsExternalInit' is not defined or imported [F:\workspace\_work\1\s\src\libraries\System.Text.Json\tests\System.Text.Json.Tests.csproj]
##[error]Serialization\ConstructorTests\ConstructorTests.Exceptions.cs(168,60): error CS0518: (NETCORE_ENGINEERING_TELEMETRY=Build) Predefined type 'System.Runtime.CompilerServices.IsExternalInit' is not defined or imported
Serialization\ConstructorTests\ConstructorTests.Exceptions.cs(168,74): error CS0518: Predefined type 'System.Runtime.CompilerServices.IsExternalInit' is not defined or imported [F:\workspace\_work\1\s\src\libraries\System.Text.Json\tests\System.Text.Json.Tests.csproj]
##[error]Serialization\ConstructorTests\ConstructorTests.Exceptions.cs(168,74): error CS0518: (NETCORE_ENGINEERING_TELEMETRY=Build) Predefined type 'System.Runtime.CompilerServices.IsExternalInit' is not defined or imported
There was a problem hiding this comment.
It seems like IsExternalInit (Which the record keyword is translated to use that) doesn't exist in net472, so we should probably make these tests just be included on NetCoreAppCurrent.
cc: @jaredpar @stephentoub are there any plans on supporting record in downlevel TFMs? Maybe by an autogenerated internal type by the compiler?
There was a problem hiding this comment.
we should probably make these tests just be included on NetCoreAppCurrent.
Do you mean #if NETCOREAPP?
There was a problem hiding this comment.
Yeah or splitting the test file into two test files and include the netcoreapp one whenever TargetFramework == $(NetCoreAppCurrent)
src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj
Outdated
Show resolved
Hide resolved
| @@ -251,10 +337,11 @@ public Parameterized_ClassWithUnicodeProperty(int a\u0467) | |||
| } | |||
|
|
|||
| [Fact] | |||
| [ActiveIssue("JsonElement needs to support Path")] | |||
There was a problem hiding this comment.
This was previously fixed -- the issue wasn't with JsonElement but in logic setting the current property.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
|
CI looks good except several Jobs (performance- and installer-related) are queued and not running even though ~4 hours in CI. |
ghost
locked as resolved and limited conversation to collaborators
Supports the new C# "record" type as well as C# anonymous types.
Fixes #38539