-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Remove unnecessary check on polymorphic serialization #48464
Conversation
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue DetailsFollow-up to #46101 (comment) We hit this condition only if the JsonConverter is our internal converter for When we use runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs Lines 350 to 355 in 1905462
I suspect that this condition used to make sense before #40914 but now is pointless.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -363,26 +363,25 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions | |||
// For internal converter only: Handle polymorphic case and get the new converter. | |||
// Custom converter, even though polymorphic converter, get called for reading AND writing. | |||
JsonConverter jsonConverter = state.Current.InitializeReEntry(type, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a follow up, we could Debug.Assert
in InitializeReEntry
that type
is not == typeof(object)
.
* upstream/main: (83 commits) Fix a crash in llvm if the sreg of a setret is not set because the methods ends with a throw. (dotnet#49122) [macOS-arm64] Disable failing libraries tests (dotnet#49400) improve PriorityQueue documentation (dotnet#49392) [wasm] Fix debugger tests (dotnet#49206) [mono] Fix the emission of EnumEqualityComparer instances into the corlib AOT image. (dotnet#49402) jitutils M2M renaming reaction (dotnet#49430) WinHttpHandler: Read HTTP/2 trailing headers [RyuJIT] Make casthelpers cold for sealed classes (dotnet#49295) JIT: Non-void ThrowHelpers (dotnet#48589) Update package index for servicing (dotnet#49417) Remove unnecessary check on polymorphic serialization (dotnet#48464) Remove release build cron triggers from jitstress jobs (dotnet#49333) [main] Update dependencies from dotnet/arcade dotnet/llvm-project dotnet/runtime-assets (dotnet#49359) Implement AppleCryptoNative_X509GetRawData using SecCertificateCopyData [AndroidCrypto] Support a zero-length salt for HMACs. (dotnet#49384) Use managed implementation of pbkdf2 for Android's one-shot implementation. (dotnet#49314) Make 303 redirects do GET like Net Framework (dotnet#49095) Make sure event generation is incremental (dotnet#48903) Add amd and Surface arm64 perf runs (dotnet#49389) Enregister EH var that are single def (dotnet#47307) ...
Follow-up to #46101 (comment)
We hit this condition only if the JsonConverter is our internal converter for
object
(ObjectConverter
).When we use
InitializeReEntry
to get the converter for the runtime type, is it not possible that it returns the ObjectConverter. I would imagine that it would return a converter for a type other thanobject
given that we previously verified thattype != typeof(object)
.runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs
Lines 350 to 355 in 1905462
I suspect that this condition used to make sense before #40914 but now is pointless.