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

RejectNullConverter PoC attempt #112

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

RejectNullConverter PoC attempt #112

wants to merge 3 commits into from

Conversation

bartelink
Copy link
Collaborator

@bartelink bartelink commented Jan 24, 2024

In the spirit of #87 this attempts to guard against the read path producing null instances of FSharpList<'T> and FSharpSet<'T>.

However, while the docs suggest that this general approach should work, I've run aground with a NullReferenceException that seems similar to dotnet/runtime#86483 when inspected in the debugger (elementTypeInfo is null)

System.NullReferenceException
Object reference not set to an instance of an object.
   at System.Text.Json.Serialization.JsonCollectionConverter`2.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, TCollection& value)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value, Boolean& isPopulatedValue)
   at System.Text.Json.Serialization.JsonResumableConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)
   at FsCodec.SystemTextJson.RejectNullConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)

@bartelink bartelink changed the title RejectNullConverter PoC atttempt RejectNullConverter PoC attempt Jan 24, 2024
@bartelink bartelink force-pushed the reject-prims branch 2 times, most recently from 0412abf to eb6e417 Compare January 24, 2024 01:12
@bartelink
Copy link
Collaborator Author

bartelink commented Jan 24, 2024

@eiriktsarpalis sorrynotsorry for the atting but I'm out of ideas on how to achieve this via other routes

edited: While the NRE will be a blocker for the overall thing to work (and I'd be interested to see if it is quick fix for someone that knows the code), further research shows me that the converter is not called for missing fields so this general approach is probably a dead end for now)

NOTE It builds fine with the V8 SDK - the tools in this repo are still on V6 as this approach is the first thing that triggers in a need for V7+ features (JsonSerializerOptions.Default) are relevant for

#endif

#if false // I guess TypeShape is doing a reasaonable thing not propagating
// PROBLEM: TypeShape.Generic.exists does not call the predicate if the list or set is `null`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My original workaround that came to mind was to walk the properties after the fact with TypeShape's Generic App, but I see I need to drop a level from that (as nulls are not passed to the predicate, and I want to do a generic check in any case, so disregard this piece for now...

@eiriktsarpalis
Copy link
Contributor

Seems related to dotnet/runtime#50205. It's a quirk in some of the built-in converters that cannot be composed outside of their created context.

@bartelink bartelink force-pushed the reject-prims branch 2 times, most recently from 8ca802c to 5919191 Compare January 24, 2024 10:20
@bartelink bartelink force-pushed the cleanups branch 3 times, most recently from 4f6eb9e to 600cfa6 Compare January 27, 2024 01:11
Base automatically changed from cleanups to master January 27, 2024 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants