How should Cesil treat nullable reference types in client code?

[kevin-montrose]

This was posed in Overthinking CSV With Cesil: Adopting Nullable Reference Types

Basically, when Cesil is setting a member on a type it currently ignores any inviolability annotations that member might have. This means that Cesil can violate nullability annotations (ie. it can assign null to string, despite string lacking a ?).

However, not all (in fact, likely very little) C# code has opted into nullable reference types - which means if Cesil were to respect nullable annotations there are cases where Cesil would fail where a client's own code would succeed.

Concretely, I see three options for what Cesil could do:

1. Ignore nullable annotations, clients should perform their own null checks
2. Enforce nullable annotations as part of the DefaultTypeDescriber, if a client needs to disable it they can provider their own ITypeDescriber
3. Provide an Option to enable/disable nullable reference type enforcement a. This will move the logic out of ITypeDescribers, so client control via that method will no longer be available If this is the route to take, what should the value for Options.(Dynamic)Default be?

This is an Open Question, which means:

[A]s part of the sustainable open source experiment I detailed in the first post of this series, any commentary from a Tier 2 GitHub Sponsor will be addressed in a future comment or post. Feedback from non-sponsors will receive equal consideration, but may not be directly addressed.

[SmithPlatts]

Option 2 seems the most inline with where MS and the community are headed, and also gives clients the ability to override it.

[kevin-montrose]

@SmithPlatts Do you have examples where nullability annotations are being enforced at runtime? It'd be a helpful to inspect what others who have taken option 2 have done.

[SmithPlatts]

I'm so sorry Kevin, I never got the notification that you'd replied!

I do not have any examples, apologies. The ASP.NET 5 repo has heaps of nullable usage now, as they're planning to move the entire codebase over, but I'm not sure if they're enforcing anything at runtime.

NUnit seem to be reviewing a similar thing too.

[kevin-montrose]

No worries @SmithPlatts, no-one on earth has never missed a ping 😅.

I've started prototyping (still just a local branch, not functional enough to share) a variant on option 2, which is to place a bit on each DeserializableMember that indicates whether null is allowed to be produced by the paired Parser. The DefaultTypeDescriber will look at the various attributes the compiler uses to annotate reference nullability, but clients could still use a custom ITypeDescriber to change that behavior. There will be virtual methods on DefaultTypeDescriber to make it it easy to extend it and change just the treatment of nulls.

When the work approach is ready to share I'll post a note here.

[SmithPlatts]

That sounds like a good way to introduce the feature; I look forward to seeing what you architect here!

[kevin-montrose]

@SmithPlatts #27 represents the current stare of things. As I wrote up there, bits need redoing/rethinking. Still, making progress.

I think the biggest change from initial plans that will persist is that rather than putting nullability on the various *Members I’m putting zero, one, or two bits on each “wrapper” (Parser, Setter, etc.).

[SmithPlatts]

The three axis approach seems like it would be quite 'complete' in its coverage, but I can completely understand the complexities!

[kevin-montrose]

This has now shipped.