Skip to content

Support reference type custom converters in source gen#57592

Merged
steveharter merged 1 commit into
dotnet:mainfrom
steveharter:GenWithRefTypeConverter
Aug 18, 2021
Merged

Support reference type custom converters in source gen#57592
steveharter merged 1 commit into
dotnet:mainfrom
steveharter:GenWithRefTypeConverter

Conversation

@steveharter

@steveharter steveharter commented Aug 17, 2021

Copy link
Copy Markdown
Contributor

@ghost

ghost commented Aug 17, 2021

Copy link
Copy Markdown

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

Issue Details

Fixes #56864

Author: steveharter
Assignees: steveharter
Labels:

area-System.Text.Json

Milestone: 6.0.0

@steveharter steveharter changed the title Support reference type custom converters Support reference type custom converters in source gen Aug 17, 2021
metadataInitSource = $@"{JsonConverterTypeRef} converter = {typeMetadata.ConverterInstantiationLogic};
{TypeTypeRef} typeToConvert = typeof({typeCompilableName});
if (!converter.CanConvert(typeToConvert))
{{

@eiriktsarpalis eiriktsarpalis Aug 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: given that there is a some amount of duplication between the ref type and value type generated code, is there any way we could splice in the nullable check, i.e some thing like

if (!converter.CanConvert(typeToConvert))
{{
     {!typeof(T).IsValueType ? "" : EmitNullableTypeHandling()}

?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a fair suggestion, and I'll leave it up to @layomia to change it if desired. However, in this case I think it is more readable\understandable to have separate ifs since the amount of emit duplication is only 5 LOC or so.

@eiriktsarpalis eiriktsarpalis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@steveharter steveharter merged commit 97d8130 into dotnet:main Aug 18, 2021
@steveharter steveharter deleted the GenWithRefTypeConverter branch August 18, 2021 19:18
@steveharter

Copy link
Copy Markdown
Contributor Author

/backport to release/6.0

@github-actions

Copy link
Copy Markdown
Contributor

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

@steveharter

Copy link
Copy Markdown
Contributor Author

/backport to release/6.0-rc1

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0-rc1: https://github.com/dotnet/runtime/actions/runs/1159663823

@ghost ghost locked as resolved and limited conversation to collaborators Sep 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Text.Json source generator does not handle reference types with design time custom converter properly

2 participants