Support JsonConverterFactory with src-gen #58398

steveharter · 2021-08-30T21:24:43Z

Addresses #58267

Expected to be ported to 6.0.

Source gen example for value types (indentation issues included):
note: usage of GetConverterFromFactory()

   internal partial class MetadataAndSerializationContext
    {
        private global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverterFactory>? _StructWithCustomConverterFactory;
        public global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverterFactory> StructWithCustomConverterFactory
        {
            get
            {
                if (_StructWithCustomConverterFactory == null)
                {
                    global::System.Text.Json.Serialization.JsonConverter? customConverter;
                        if (Options.Converters.Count > 0 && (customConverter = GetRuntimeProvidedCustomConverter(typeof(global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverterFactory))) != null)
                        {
                            _StructWithCustomConverterFactory = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateValueInfo<global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverterFactory>(Options, customConverter);
                        }
                        else
                        {
                            global::System.Text.Json.Serialization.JsonConverter converter = this.GetConverterFromFactory(typeof(global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverterFactory), new global::System.Text.Json.SourceGeneration.Tests.CustomConverterFactory());
                                global::System.Type typeToConvert = typeof(global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverterFactory);
                                    if (!converter.CanConvert(typeToConvert))
                                    {
                                        global::System.Type? underlyingType = global::System.Nullable.GetUnderlyingType(typeToConvert);
                                        if (underlyingType != null && converter.CanConvert(underlyingType))
                                        {
                                            // Allow nullable handling to forward to the underlying type's converter.
                                            converter = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.GetNullableConverter<global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverterFactory>(this.StructWithCustomConverterFactory)!;
                                            converter = ((global::System.Text.Json.Serialization.JsonConverterFactory)converter).CreateConverter(typeToConvert, Options)!;
                                        }
                                        else
                                        {
                                            throw new global::System.InvalidOperationException($"The converter '{converter.GetType()}' is not compatible with the type '{typeToConvert}'.");
                                        }
                                    }
                                _StructWithCustomConverterFactory = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateValueInfo<global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverterFactory> (Options, converter); 
                        }
                }
        
                return _StructWithCustomConverterFactory;
            }
        }
    }

and for reference types:

    internal partial class MetadataAndSerializationContext
    {
        private global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory>? _ClassWithCustomConverterFactory;
        public global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory> ClassWithCustomConverterFactory
        {
            get
            {
                if (_ClassWithCustomConverterFactory == null)
                {
                    global::System.Text.Json.Serialization.JsonConverter? customConverter;
                        if (Options.Converters.Count > 0 && (customConverter = GetRuntimeProvidedCustomConverter(typeof(global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory))) != null)
                        {
                            _ClassWithCustomConverterFactory = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateValueInfo<global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory>(Options, customConverter);
                        }
                        else
                        {
                            global::System.Text.Json.Serialization.JsonConverter converter = this.GetConverterFromFactory(typeof(global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory), new global::System.Text.Json.SourceGeneration.Tests.CustomConverterFactory());
                                global::System.Type typeToConvert = typeof(global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory);
                                    if (!converter.CanConvert(typeToConvert))
                                    {
                                        throw new global::System.InvalidOperationException($"The converter '{converter.GetType()}' is not compatible with the type '{typeToConvert}'.");
                                    }
                                _ClassWithCustomConverterFactory = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateValueInfo<global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory> (Options, converter); 
                        }
                }
        
                return _ClassWithCustomConverterFactory;
            }
        }
    }

and for properties with factories:

            properties[0] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.Text.Json.SourceGeneration.Tests.SampleEnum>(
...
                converter: jsonContext.GetConverterFromFactory<global::System.Text.Json.SourceGeneration.Tests.SampleEnum>(new global::System.Text.Json.Serialization.JsonStringEnumConverter()),

and the new helper methods (optionally generated)

        private global::System.Text.Json.Serialization.JsonConverter<T> GetConverterFromFactory<T>(global::System.Text.Json.Serialization.JsonConverterFactory factory)
        {
            return (global::System.Text.Json.Serialization.JsonConverter<T>) GetConverterFromFactory(typeof(T), factory);
        }

        private global::System.Text.Json.Serialization.JsonConverter GetConverterFromFactory(global::System.Type type, global::System.Text.Json.Serialization.JsonConverterFactory factory)
        {
            global::System.Text.Json.Serialization.JsonConverter? converter = factory.CreateConverter(type, Options);
            if (converter == null || converter is global::System.Text.Json.Serialization.JsonConverterFactory)
            {
                throw new global::System.InvalidOperationException($"The converter '{factory.GetType()}' cannot return null or a JsonConverterFactory instance.");
            }
             
            return converter;
        }

ghost · 2021-08-30T21:24:48Z

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Addresses #58267

Expected to be ported to 6.0.

Source gen example for value types (indentation issues included):

// <auto-generated/>
#nullable enable

namespace System.Text.Json.SourceGeneration.Tests
{
    internal partial class MetadataContext
    {
        private global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverter>? _StructWithCustomConverter;
        public global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverter> StructWithCustomConverter
        {
            get
            {
                if (_StructWithCustomConverter == null)
                {
                    global::System.Text.Json.Serialization.JsonConverter? customConverter;
                        if (Options.Converters.Count > 0 && (customConverter = GetRuntimeProvidedCustomConverter(typeof(global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverter))) != null)
                        {
                            _StructWithCustomConverter = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateValueInfo<global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverter>(Options, customConverter);
                        }
                        else
                        {
                            global::System.Text.Json.Serialization.JsonConverter converter = new global::System.Text.Json.SourceGeneration.Tests.CustomConverter_StructWithCustomConverter();
                                global::System.Type typeToConvert = typeof(global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverter);
                                    if (!converter.CanConvert(typeToConvert))
                                    {
                                        global::System.Type? underlyingType = global::System.Nullable.GetUnderlyingType(typeToConvert);
                                        if (underlyingType != null && converter.CanConvert(underlyingType))
                                        {
                                            // Allow nullable handling to forward to the underlying type's converter.
                                            converter = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.GetNullableConverter<global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverter>(this.StructWithCustomConverter)!;
                                        }
                                        else
                                        {
                                            throw new global::System.InvalidOperationException($"The converter '{converter.GetType()}' is not compatible with the type '{typeToConvert}'.");
                                        }
                                    }
                                else if (converter is global::System.Text.Json.Serialization.JsonConverterFactory factory)
                                {
                                    global::System.Text.Json.Serialization.JsonConverter? actualConverter = factory.CreateConverter(typeToConvert, Options);
                                    if (actualConverter == null || actualConverter is global::System.Text.Json.Serialization.JsonConverterFactory)
                                    {
                                        throw new global::System.InvalidOperationException($"JsonConverterFactory '{factory.GetType()}' cannot return a 'null' or 'JsonConverterFactory' value.");
                                    }
                                    converter = actualConverter;
                                }
                                _StructWithCustomConverter = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateValueInfo<global::System.Text.Json.SourceGeneration.Tests.StructWithCustomConverter> (Options, converter); 
                        }
                }
        
                return _StructWithCustomConverter;
            }
        }
    }
}

and for reference types:

// <auto-generated/>
#nullable enable

namespace System.Text.Json.SourceGeneration.Tests
{
    internal partial class MetadataContext
    {
        private global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory>? _ClassWithCustomConverterFactory;
        public global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory> ClassWithCustomConverterFactory
        {
            get
            {
                if (_ClassWithCustomConverterFactory == null)
                {
                    global::System.Text.Json.Serialization.JsonConverter? customConverter;
                        if (Options.Converters.Count > 0 && (customConverter = GetRuntimeProvidedCustomConverter(typeof(global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory))) != null)
                        {
                            _ClassWithCustomConverterFactory = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateValueInfo<global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory>(Options, customConverter);
                        }
                        else
                        {
                            global::System.Text.Json.Serialization.JsonConverter converter = new global::System.Text.Json.SourceGeneration.Tests.CustomConverterFactory();
                                global::System.Type typeToConvert = typeof(global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory);
                                    if (!converter.CanConvert(typeToConvert))
                                    {
                                        throw new global::System.InvalidOperationException($"The converter '{converter.GetType()}' is not compatible with the type '{typeToConvert}'.");
                                    }
                                else if (converter is global::System.Text.Json.Serialization.JsonConverterFactory factory)
                                {
                                    global::System.Text.Json.Serialization.JsonConverter? actualConverter = factory.CreateConverter(typeToConvert, Options);
                                    if (actualConverter == null || actualConverter is global::System.Text.Json.Serialization.JsonConverterFactory)
                                    {
                                        throw new global::System.InvalidOperationException($"JsonConverterFactory '{factory.GetType()}' cannot return a 'null' or 'JsonConverterFactory' value.");
                                    }
                                    converter = actualConverter;
                                }
                                _ClassWithCustomConverterFactory = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateValueInfo<global::System.Text.Json.SourceGeneration.Tests.ClassWithCustomConverterFactory> (Options, converter); 
                        }
                }
        
                return _ClassWithCustomConverterFactory;
            }
        }
    }
}

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Text.Json`
Milestone:	6.0.0

eiriktsarpalis · 2021-09-01T14:45:53Z

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs

-                    _{typeFriendlyName} = {JsonMetadataServicesTypeRef}.{GetCreateValueInfoMethodRef(typeCompilableName)}({OptionsInstanceVariableName}, converter);";
+                            else
+                            {{
+                                throw new {InvalidOperationExceptionTypeRef}($""The converter '{{converter.GetType()}}' is not compatible with the type '{{typeToConvert}}'."");


Not relevant to the scope of this PR, but here is another instance of non-localized error messages: #58292.

Since this is a run-time exception, not design-time, I think it should remain non-localized.

eiriktsarpalis · 2021-09-01T14:58:30Z

Looks good, but would it be possible to incorporate a fix for converter factories specified on the property level? See #58267 (comment) on the original issue.

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs

eiriktsarpalis · 2021-09-03T16:16:48Z

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs

-
-                    _{typeFriendlyName} = {JsonMetadataServicesTypeRef}.{GetCreateValueInfoMethodRef(typeCompilableName)}({OptionsInstanceVariableName}, converter);";
+                    metadataInitSource.Append($@"
+                        if (!converter.CanConvert(typeToConvert))


Where does this branch type test for converter factories?

It doesn't... The code before that that is shared between value types and reference types will call GetConverterFromFactory() for factories.

Also the error check for CanConvert was pre-existing from this PR. It applies to factory and non-factory cases although it may not be necessary for the factory case, as I'm not sure we double-check this in the serializer case (i.e. I don't think we verify the result from factory.CreateConverter()).

… types

eiriktsarpalis · 2021-09-03T17:23:56Z

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs

@@ -1092,6 +1178,20 @@ private static bool PropertyAccessorCanBeReferenced(MethodInfo? accessor)
                    return null;
                }

+                if (converterType.GetCompatibleBaseClass(JsonConverterFactoryFullName) != null)


Thanks, I was looking for this logic in the Emitter.cs diff. I know this predates your PR, but shouldn't this method live in Emitter.cs?

Ah, I'm assuming it's because we're embedding the generated initialization expression as a field in the property generation spec.

Yes the parser:emitter abstraction is broken here.

Ideally it should be moved to the emitter, perhaps in a later PR.

steveharter · 2021-09-03T19:49:42Z

/backport to release/6.0

github-actions · 2021-09-03T19:49:57Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1199230405

steveharter added the area-System.Text.Json label Aug 30, 2021

steveharter added this to the 6.0.0 milestone Aug 30, 2021

steveharter requested review from eiriktsarpalis and layomia as code owners August 30, 2021 21:24

steveharter self-assigned this Aug 30, 2021

eiriktsarpalis reviewed Sep 1, 2021

View reviewed changes

steveharter added 2 commits September 2, 2021 12:44

Support JsonConverterFactory with src-gen

f94d0ff

Support property factory converters

c6582d8

steveharter force-pushed the SrcGenFactory branch from d3eaf42 to c6582d8 Compare September 2, 2021 17:50

Fixup merge ordering; add tests

53e8e64

eiriktsarpalis reviewed Sep 3, 2021

View reviewed changes

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs Show resolved Hide resolved

eiriktsarpalis reviewed Sep 3, 2021

View reviewed changes

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs Show resolved Hide resolved

eiriktsarpalis reviewed Sep 3, 2021

View reviewed changes

Ensure context helper methods properly generated for implicit context…

1f2f755

… types

eiriktsarpalis reviewed Sep 3, 2021

View reviewed changes

eiriktsarpalis approved these changes Sep 3, 2021

View reviewed changes

steveharter merged commit 0becc99 into dotnet:main Sep 3, 2021

steveharter deleted the SrcGenFactory branch September 3, 2021 19:48

github-actions bot mentioned this pull request Sep 3, 2021

[release/6.0] Support JsonConverterFactory with src-gen #58652

Merged

ghost locked as resolved and limited conversation to collaborators Oct 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support JsonConverterFactory with src-gen #58398

Support JsonConverterFactory with src-gen #58398

steveharter commented Aug 30, 2021 •

edited

Loading

ghost commented Aug 30, 2021

eiriktsarpalis Sep 1, 2021

steveharter Sep 1, 2021

eiriktsarpalis commented Sep 1, 2021

eiriktsarpalis Sep 3, 2021

steveharter Sep 3, 2021

eiriktsarpalis Sep 3, 2021

eiriktsarpalis Sep 3, 2021 •

edited

Loading

steveharter Sep 3, 2021

layomia Sep 3, 2021

steveharter commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

Support JsonConverterFactory with src-gen #58398

Support JsonConverterFactory with src-gen #58398

Conversation

steveharter commented Aug 30, 2021 • edited Loading

ghost commented Aug 30, 2021

eiriktsarpalis Sep 1, 2021

Choose a reason for hiding this comment

steveharter Sep 1, 2021

Choose a reason for hiding this comment

eiriktsarpalis commented Sep 1, 2021

eiriktsarpalis Sep 3, 2021

Choose a reason for hiding this comment

steveharter Sep 3, 2021

Choose a reason for hiding this comment

eiriktsarpalis Sep 3, 2021

Choose a reason for hiding this comment

eiriktsarpalis Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

steveharter Sep 3, 2021

Choose a reason for hiding this comment

layomia Sep 3, 2021

Choose a reason for hiding this comment

steveharter commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

steveharter commented Aug 30, 2021 •

edited

Loading

eiriktsarpalis Sep 3, 2021 •

edited

Loading