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

@stevehartersteveharter commented Aug 17, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

@stevehartersteveharter added this to the 6.0.0 milestone Aug 17, 2021
@stevehartersteveharter self-assigned this Aug 17, 2021
@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

@stevehartersteveharter changed the title Support reference type custom convertersSupport reference type custom converters in source genAug 17, 2021
eiriktsarpalis
eiriktsarpalis reviewed Aug 18, 2021
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
metadataInitSource = $@"{JsonConverterTypeRef} converter = {typeMetadata.ConverterInstantiationLogic};
{TypeTypeRef} typeToConvert = typeof({typeCompilableName});
if (!converter.CanConvert(typeToConvert))
{{

@eiriktsarpaliseiriktsarpalisAug 18, 2021•
edited
Loading

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()}

?

@stevehartersteveharterAug 18, 2021

Copy link
Copy Markdown
ContributorAuthor

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 approved these changes Aug 18, 2021
View reviewed changes

@eiriktsarpaliseiriktsarpalis 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:mainAug 18, 2021
@steveharter
steveharter deleted the GenWithRefTypeConverter branch August 18, 2021 19:18
@steveharter

steveharter commented Aug 19, 2021

Copy link
Copy Markdown
ContributorAuthor

/backport to release/6.0

@github-actions

github-actionsBot commented Aug 19, 2021

Copy link
Copy Markdown
Contributor

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

@github-actionsgithub-actionsBot mentioned this pull request Aug 19, 2021
Closed
@steveharter

steveharter commented Aug 23, 2021

Copy link
Copy Markdown
ContributorAuthor

/backport to release/6.0-rc1

@github-actions

github-actionsBot commented Aug 23, 2021

Copy link
Copy Markdown
Contributor

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

@github-actionsgithub-actionsBot mentioned this pull request Aug 23, 2021
Merged
@ghostghost locked as resolved and limited conversation to collaborators Sep 22, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eiriktsarpaliseiriktsarpaliseiriktsarpalis approved these changes
@layomialayomiaAwaiting requested review from layomia

Assignees

@stevehartersteveharter

Labels

area-System.Text.Json

Projects

None yet

Milestone

6.0.0

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

@steveharter@eiriktsarpalis