C#: add option and code generation for null reference types
This replaces the closed PR https://github.com/protocolbuffers/protobuf/pull/13218
It adds an option for generating C# code that handles null reference types. See https://github.com/grpc/proposal/blob/master/L110-csharp-nullable-reference-types.md
Further work could be done to:
- add tests to check the generated code is as expected - there doesn't appear to be these kind of tests for other code generation
- change
#nullable enable annotationsto#nullable enable- but this requires further codegen changes- to annotate handling calling the
Google.Protobuf.ReflectionAPI that doesn't yet handle null reference types - to handle types such as
stringandbytesthat use null to indicate presence
- to annotate handling calling the
The code generated by this PR may be good enough - however to be able to change #nullable enable annotations to #nullable enable the following needs to be addressed:
-
Calling
Google.Protobuf.Reflection.GeneratedClrTypeInfo()GeneratedClrTypeInfo() doesn’t support NRT, thus need to add null forgiving operator in some calls: e.g.new pbr::GeneratedClrTypeInfo[] { !null, } -
Checking for
nullin message fields uses the field name for the check but the property name to get the value e.g. in the generated GetHashCode() this idiom is used:if (someMessge_ != null) hash ^= SomeMessage.GetHashCode();Similar in MergeFrom(). -
Optional message, string, and bytes fields need to be declared as nullable otherwise the constructor won't compile
-
oneof fields need to be declared nullable code accessing those fields needs to be annotated or errors suppressed, e.g.
public int OneInt { get { return HasOneInt ? (int) myOneofField_ : 0; } // Error CS8605 unboxing null value set { myOneofField_ = value; myOneofFieldCase_ = MyOneofFieldOneofCase.OneInt; } }
@tonydnewell Sorry for not reviewing this for a while - is this now done and complete, ready for review and possible merging?
@tonydnewell Sorry for not reviewing this for a while - is this now done and complete, ready for review and possible merging?
It is ready. However my contract has now ended so I'll only be able to fix any problems on an ad hoc basis.
@tonydnewell Sorry for not reviewing this for a while - is this now done and complete, ready for review and possible merging?
It is ready. However my contract has now ended so I'll only be able to fix any problems on an ad hoc basis.
Righto - if I spot anything, I could quite possibly fix it myself instead. Will attempt to find headspace to review this soon. Thanks!
(It's actually much shorter than I'd have expected, so it might not be too hard to review after all...)
@tonydnewell Is there anything I can do to help get traction on this?
I suspect the next step is mine - it's a matter of finding the time.
@JasonLunn I think we can close this one in favor of the extended one. If you want to keep it until the other is handled, I added close-keywords to my PR instead :)
@JasonLunn I think we can close this one in favor of the extended one. If you want to keep it until the other is handled, I added close-keywords to my PR instead :)
Yes let's keep this one open until the other merges.
Marking this blocked until we make a decision on #20242