Skip to content
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

Add json support for F# options, lists, sets, maps and records #55108

Merged
merged 12 commits into from
Jul 15, 2021

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Jul 2, 2021

Contributes to #29812 by adding support for the following types:

  • F# options and struct options.
  • F# lists, sets and maps.
  • Fixes support for all F# record representations.

Note that the changes do not include support for discriminated unions, attempting to serialize a DU will now throw NotSupportedException. See #29812 (comment) for an explanation why we're doing this. We can revisit this approach in future releases of .NET

cc @dsyme @bartelink.

@ghost
Copy link

ghost commented Jul 2, 2021

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

Issue Details

Contributes to #29812. Note that the changes do not include support for discriminated unions since it requires infrastructural changes touching on polymorphism support (cf. #30083).

cc @dsyme @bartelink.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json

Milestone: 6.0.0

@jkotas
Copy link
Member

jkotas commented Jul 2, 2021

How is this going to work with trimming? Does this need an explicit public API to opt-in?

@eiriktsarpalis
Copy link
Member Author

How is this going to work with trimming?

The feature requires unreferenced code to function, so it can break when used with trimming. There is precedent for this, for example the converters for System.Collections.Immutable that intentionally don't take a static dependency to the types.

Does this need an explicit public API to opt-in?

Maybe, but I don't see why it shouldn't be the default. The current handling of F# types is broken one way or another.

@jkotas
Copy link
Member

jkotas commented Jul 3, 2021

Maybe, but I don't see why it shouldn't be the default.

It is not pay-for-play. Every Json serializer construction is going to pay to the FSharp checks now.

@eiriktsarpalis
Copy link
Member Author

Maybe, but I don't see why it shouldn't be the default.

It is not pay-for-play. Every Json serializer construction is going to pay to the FSharp checks now.

Of course, but that argument could be made for other rarely used supported types like IAsyncEnumerable and System.Collections.Immutable. We can measure the impact of adding F# checks to warmup time and make a call based on that, however my expectation is that users conscious of startup times will switch to source generators.

@eiriktsarpalis
Copy link
Member Author

Misc user feedback collected from social media:

  • Add support for ValueOption.
  • Consider recognizing discriminated unions and throw NotSupportedException.

@En3Tho
Copy link
Contributor

En3Tho commented Jul 3, 2021

Aren't records supported by default in net5? I've even forked fsharp.stj repo to use record (de) serialization out of the box.

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Jul 3, 2021

Aren't records supported by default in net5? I've even forked fsharp.stj repo to use record (de) serialization out of the box.

They generally happen to work, however struct record deserialization is currently broken. This change fixes that particular corner case and makes support official by adding tests for all record kinds.

@En3Tho
Copy link
Contributor

En3Tho commented Jul 4, 2021

Nice. Thank you for clarification.

@dsyme
Copy link

dsyme commented Jul 5, 2021

@eiriktsarpalis How about single case unions, list types, option types, are they covered?

@eiriktsarpalis
Copy link
Member Author

Hi @dsyme, options and lists are supported special cases but other DUs have not been added yet.

@eiriktsarpalis
Copy link
Member Author

FWIW I wrote a benchmark comparing cold start serialization for main and the PR branch: https://gist.github.com/eiriktsarpalis/ecb9259ee92940469d19989f81383dfd. Running statistical tests with 3% threshold seems to indicate no notable performance regressions.

Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

LGTM; however not familiar with F# so can't comment on the value add here without the support for "discriminated unions".

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Jul 13, 2021

Heads up: I just pushed a commit that makes discriminated unions explicitly throw NotSupportedException. Rationale for not supporting the type can be found here. We can revisit our approach in future releases. cc @Zaid-Ajaj @dsyme

@eerhardt
Copy link
Member

The trimming annotations LGTM.

@ghost
Copy link

ghost commented Jul 15, 2021

Hello @eiriktsarpalis!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

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.

User Story: Developers using JsonSerializer want it to handle common F# types well
10 participants