Make contexts more combinator friendly

[krwq]

• we don't need new API in JsonSerializeContext
• we do not need to create new instance of context inside of context.GetTypeInfo
• converters work more consistently
• you can customize child type info of contexts now if you combine (previously it was hardcoded)

[eiriktsarpalis]

If this method is "NotCached", why is it calling into GetOrAddJsonTypeInfoEx which does consult the cache? It basically means that a cache miss will result in multiple lookups for no clear reason.

[krwq]

NotCached refers to converter cache not JsonTypeInfo cache

[krwq]

I'll add a comment on top to make that clear

[krwq]

        /// <summary>
        /// Gets converter from type info without using converter cache.
        /// This method is used by converter cache. It may use type info cache.
        /// </summary>

[eiriktsarpalis]

Here's a question: do we need to keep the converter cache if we're moving the source truth for converters to metadata?

[krwq]

For regular scenario it's technically not needed but:

• GetConverter also needs to return answer from options and reflection even if type info is null (for compat)
• we previously cached instance which technically means we always got exactly same instance after first usage, if I removed the converter caching it will still be cached if resolver returned non-null though so that might be ok

I'm ok with removing that but we need to agree above is ok

[eiriktsarpalis]

I think it's ok to return a non-cached converter for GetConverter call not backed by JsonTypeInfo. Are there any cases in the hot path where it might result in unnecessary recalculations?

[krwq]

no, we're gonna cache everything in the hot paths; will remove converter cache

[eiriktsarpalis]

This seems wrong. Why would we throw this error if JsonSerializerOptions.Converters doesn't contain a compatible converter? As the name suggests the error is meant to be thrown if the built-in converters have not been rooted.

[krwq]

As I mentioned in other places, we don't throw here unless:

• rooting logic was never exercised
• we somehow ended up using resolver which source gen did not generate type info code for (that's what we also did before this change) - same applies if multiple contexts are combined

[eiriktsarpalis]

In the current behaviour we want to return the built-in converters (or throw an exception if they haven't been rooted yet). This seems like a breaking change?

[krwq]

it's not breaking change. If you call GetConverter directly it will root converter first and then call this method will behave as it was.

On the other hand if this is called when context is bound but converters are not rooted (it may be called implicitly) you will get an exception same way as you used to.

[eiriktsarpalis]

Calling it ResolverConverter over GetConverter might be better at emphasizing this is not caching

Suggested change

	private JsonConverter GetConverterFromOptionsOrReflectionConverter(Type typeToConvert)
	private JsonConverter ResolveConverter(Type typeToConvert)

[krwq]

this actually isn't resolving converter - this is technically speaking reflection converter logic. From user perspective the order is following:

• type info converter (i.e. serializer context)
• options
• reflection converter (if rooted, if GetConverter called explicitly then this is always rooted)

this method does the last two and it's used by: reflection json type info and as a fallback for GetConverter (the fallback is arguably wrong but since we already shipped like that this logic is preserved here)

[eiriktsarpalis]

this actually isn't resolving converter

Arguably it's resolving a converter based on the Converters list and the built-in stuff. My only concern is that we have accumulated too many JsonSerializer.GetConverter* method names and it might be in our interest to adopt a separate naming convention for the converter resolution logic that doesn't involve the cache in any way.

[krwq]

There are only two case which must not use cache and first one could also cause stack overflow: reflection/custom type info resolver which needs to get converter; calling GetConverter before options are locked (caching cannot happen because converters/type infos can still be changed). Everything else is should go through caching

[eiriktsarpalis]

This kind of lookup seems completely out of place compared to the other JsonMetadataServices converter components. I'm concerned it might be accidentally rooting reflection components in sourcegen. cc @layomia

[krwq]

it won't. The only place we do rooting is public GetConverter. This will lead to exception unless you called one of the rooting APIs. This is the reason I refactored GetConverter* methods so that we preserve this behavior

[krwq]

another thing to note here is that there are actually only 4 places we get inner converters:

• nullable
• collections
• enumerables
• objects

we do exactly same for all 3 others except it's more indirect

[eiriktsarpalis]

we do exactly same for all 3 others except it's more indirect

That's my point actually. The source generator is very intentionally making it so that metadata are composed explicitly rather than performing lookups via indirection to the metadata resolver. Why does nullable need to be different and does it imply that nested metadata resolution semantics diverge compared to, say, collections?

[krwq]

this indirection happens exactly once per type info, later it's cached forever. See this test case: https://github.com/krwq/runtime-1/pull/9/files#diff-9ca8a434d4ee452728961d1242d8680e5f4b72c8973a99b7ef59ecb005541025R131
this is not arguable what the expected behavior should be.

[eiriktsarpalis]

I'm not disputing that, it's more a question of why this specific type needing to be different. It seems weird that it does and introduces more accidental complexity. Isn't it possible to emulate prior art?

[krwq]

this is called from generated code and technically speaking we could call options.GetConverter but at the same time that currently roots reflection converters - if we do that that would break linker safety

[eiriktsarpalis]

Tests validating the new behavior?

[krwq]

I'll add them as part of Combine tests

[krwq]

I've added one larger test here https://github.com/krwq/runtime-1/pull/9/files#diff-9ca8a434d4ee452728961d1242d8680e5f4b72c8973a99b7ef59ecb005541025R131 exercising all trickier cases. Everything else should be covered by existing test cases

[krwq]

I've pushed a rebased version, will address the comment about removing converter cache as separate commit