Skip to content

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

Merged
eiriktsarpalis merged 12 commits into
dotnet:mainfrom
eiriktsarpalis:json-fsharp-support
Jul 15, 2021
Merged

Add json support for F# options, lists, sets, maps and records#55108
eiriktsarpalis merged 12 commits into
dotnet:mainfrom
eiriktsarpalis:json-fsharp-support

Conversation

@eiriktsarpalis

@eiriktsarpalis eiriktsarpalis commented Jul 2, 2021

Copy link
Copy Markdown
Member

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

ghost commented Jul 2, 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

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

jkotas commented Jul 2, 2021

Copy link
Copy Markdown
Member

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

@eiriktsarpalis

Copy link
Copy Markdown
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

jkotas commented Jul 3, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member Author

Misc user feedback collected from social media:

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

@En3Tho

En3Tho commented Jul 3, 2021

Copy link
Copy Markdown
Contributor

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

eiriktsarpalis commented Jul 3, 2021

Copy link
Copy Markdown
Member Author

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

En3Tho commented Jul 4, 2021

Copy link
Copy Markdown
Contributor

Nice. Thank you for clarification.

@dsyme

dsyme commented Jul 5, 2021

Copy link
Copy Markdown

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

@eiriktsarpalis

Copy link
Copy Markdown
Member Author

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

@eiriktsarpalis

Copy link
Copy Markdown
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.

Comment thread src/libraries/System.Text.Json/System.Text.Json.sln Outdated

@steveharter steveharter left a comment

Copy link
Copy Markdown
Contributor

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

eiriktsarpalis commented Jul 13, 2021

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Member

The trimming annotations LGTM.

@ghost

ghost commented Jul 15, 2021

Copy link
Copy Markdown

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