protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

C#: add option and code generation for null reference types

Open tonydnewell opened this issue 1 year ago • 5 comments

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 annotations to #nullable enable - but this requires further codegen changes
    • to annotate handling calling the Google.Protobuf.Reflection API that doesn't yet handle null reference types
    • to handle types such as string and bytes that use null to indicate presence

tonydnewell avatar Mar 20 '24 13:03 tonydnewell

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 null in 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 avatar Mar 27 '24 10:03 tonydnewell

@tonydnewell Sorry for not reviewing this for a while - is this now done and complete, ready for review and possible merging?

jskeet avatar Oct 11 '24 13:10 jskeet

@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 avatar Oct 14 '24 10:10 tonydnewell

@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!

jskeet avatar Oct 14 '24 10:10 jskeet

(It's actually much shorter than I'd have expected, so it might not be too hard to review after all...)

jskeet avatar Oct 14 '24 10:10 jskeet

@tonydnewell Is there anything I can do to help get traction on this?

Falco20019 avatar Jan 16 '25 13:01 Falco20019

I suspect the next step is mine - it's a matter of finding the time.

jskeet avatar Jan 16 '25 13:01 jskeet

@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 :)

Falco20019 avatar Feb 06 '25 07:02 Falco20019

@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.

JasonLunn avatar Feb 06 '25 20:02 JasonLunn

Marking this blocked until we make a decision on #20242

JasonLunn avatar Feb 06 '25 20:02 JasonLunn