-
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
Split Reflection and SourceGen TypeInfos #67526
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,7 +41,31 @@ private static void RootReflectionSerializerDependencies() | |
} | ||
|
||
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] | ||
static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) => new JsonTypeInfo(type, options); | ||
static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) | ||
{ | ||
JsonTypeInfo.ValidateType(type, null, null, options); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: probably fine for the scope of this PR, but eventually I'd like to see this method moved out of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. most of the work this does is checking if the type is complete generic type so essentially I'd need to duplicate that logic elsewhere, we previously could have it as part of JsonTypeInfo because it wasn't generic but now we cannot instantiate it otherwise |
||
|
||
MethodInfo methodInfo = typeof(JsonSerializerOptions).GetMethod(nameof(CreateReflectionJsonTypeInfo), BindingFlags.NonPublic | BindingFlags.Instance)!; | ||
#if NETCOREAPP | ||
return (JsonTypeInfo)methodInfo.MakeGenericMethod(type).Invoke(options, BindingFlags.NonPublic | BindingFlags.DoNotWrapExceptions, null, null, null)!; | ||
#else | ||
try | ||
{ | ||
return (JsonTypeInfo)methodInfo.MakeGenericMethod(type).Invoke(options, null)!; | ||
} | ||
catch (TargetInvocationException ex) | ||
{ | ||
throw ex.InnerException!; | ||
krwq marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
#endif | ||
} | ||
} | ||
|
||
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] | ||
private JsonTypeInfo<T> CreateReflectionJsonTypeInfo<T>() | ||
{ | ||
// We do not use Activator.CreateInstance because it will wrap exception if constructor throws it | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use Activator.CreateInstance pretty liberally when constructing converters. Wouldn't this be a concern there as well? It looks like you're unwrapping There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More importantly, if this is throwing, isn't this an indication of an internal bug? Do we need to care about this exception being surfaced appropriately to users? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually that's incorrect comment, I forgot to remove it. I used it here because it makes debugging easier and also I don't need to pass options to Invoke when using instance member (one allocation less) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For throwin indicating internal bug:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the constructor is capable of surfacing exceptions relevant to the user, we might be able to work around the issue by making the constructor itself trivial (i.e. just create the instance with the T type) and then follow up with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've just quickly checked which tests are failing when unwrapping is commented out and seeing following types of failures we check for (84 tests failures total in System.Text.Json.Tests alone):
Given above I don't think it can really be delayed. Also note that this is happening only during construction of JsonTypeInfo which is a one time operation |
||
return new ReflectionJsonTypeInfo<T>(this); | ||
} | ||
|
||
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -600,21 +600,22 @@ internal void InitializeForReflectionSerializer() | |
private JsonTypeInfo GetJsonTypeInfoFromContextOrCreate(Type type) | ||
{ | ||
JsonTypeInfo? info = _serializerContext?.GetTypeInfo(type); | ||
if (info != null) | ||
if (info == null && IsInitializedForReflectionSerializer) | ||
{ | ||
return info; | ||
Debug.Assert( | ||
s_typeInfoCreationFunc != null, | ||
"Reflection-based JsonTypeInfo creator should be initialized if IsInitializedForReflectionSerializer is true."); | ||
info = s_typeInfoCreationFunc(type, this); | ||
} | ||
|
||
if (!IsInitializedForReflectionSerializer) | ||
if (info == null) | ||
{ | ||
ThrowHelper.ThrowNotSupportedException_NoMetadataForType(type); | ||
return null!; | ||
} | ||
|
||
Debug.Assert( | ||
s_typeInfoCreationFunc != null, | ||
"Reflection-based JsonTypeInfo creator should be initialized if IsInitializedForReflectionSerializer is true."); | ||
return s_typeInfoCreationFunc(type, this); | ||
info.EnsureConfigured(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes because serialization is also hit through some APIs which directly pass in JsonType info (as a side note this call is very cheap, it's marked with aggressive inlining and it's a simple bool flag check in case where it's already configured) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to rename the method to something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually in my original prototype I called it "EnsureLocked" but here it's not really locking anything yet. What about I rename it to "EnsureLocked" once it starts ensuring no one tries to modify it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm guessing though that it would eventually lock the public setters we'd be adding in the future? I think it's ok to introduce the name now even though it doesn't actually do exactly that since it signifies intent. Alternatively "EnsureInitialized" can be a generic enough description. My issue with "EnsureConfigured" is that it's not really configuring anything (that happens on construction), it's just building caches based on configuration that has already been specified. Again though, that's me nit-picking on internal method names. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about BuildCache/EnsureCacheBuilt. I get similar notion with EnsureInitialized as you're getting with EnsureConfigured There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
return info; | ||
} | ||
|
||
internal JsonDocumentOptions GetDocumentOptions() | ||
|
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.
Could this be moved to the JsonTypeInfo construction phase (to avoid making contention a concern?)
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.
No for 3 reasons:
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.
Got it, essentially it is accounting for instances not returned from the
JsonSerializerOptions
cache so cycles cannot be accounted for?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.
For the first part of the question - yes - this accounts for source gen JsonTypeInfos. Yes it is done so that cycles cannot occur (that's a bit impl specific to source gen) but also so that JsonTypeInfo can be edited before this is called.