Protobuf: Add option to generate C# nullable annotations

[YairHalberstadt]

Language : C#

Feature Request

C# 8.0 adds support for nullability annotations.

It would be helpful if the the generated protobuf objects added these annotations. This would provide warnings if people tried to dereference a possibly null value, or for example, tried to assign null to a string property of a protobuf object.

At first sight it looks like the rules for which types can be nullable and which can't should be quite simple.

I think the first stage would be annotating the Protobuf Nuget Packages.

Of course nullable annotaions are a compile error in earlier versions of C# than c# 8.0. Therefore this has to be off by default, and triggered by a command line option.

It would be helpful if this was added for GRPC as well.

[jtattermusch]

I'm not generally opposed to having this, but here are the challenges:

1. we would need a clear design - feel free to provide more thoughts here if you want
2. currently this is not a priority so there's no ETA and implementation would need to be contributed by someone from the community (but we'd need a design for this feature first).
3. C# 8 nullable annotations are a brand new C# 8 feature, but we definitely need to maintain the backward compatibility - which probably also means that it's going to take a while before this can actually be used.

There are two separate areas:

1. annotations in the Google.Protobuf runtime (=the nuget) - then concern is backward-compatibility of the APIs (you cannot have an "option" for enabling/disabling this so it would need to work for everyone - and it's going to take a very long time until everyone can use C# 8).
2. annotations in the generated code (you can have a codegen option to enable the default-off feature), the question is how useful these checks would be - it's generated code and most of it is just field accessors, so I'm not sure we'd gain much here (a more detailed design would help).

[YairHalberstadt]

annotations in the Google.Protobuf runtime (=the nuget) - then concern is backward-compatibility of the APIs (you cannot have an "option" for enabling/disabling this so it would need to work for everyone - and it's going to take a very long time until everyone can use C# 8).

The annotations are backwards compatible, so I don't think this should be an issue.

annotations in the generated code (you can have a codegen option to enable the default-off feature), the question is how useful these checks would be - it's generated code and most of it is just field accessors, so I'm not sure we'd gain much here (a more detailed design would help).

A very common mistake I make is to set a string to null. That would be the single most useful gain.

With GRPC specifically, I often by mistake return null from the server implementation. That would be another gain but of course that is for a different repository.

I don't know a huge amount about protoc but my initial design would be:

All string properties are marked non-null.
No change in struct properties.
All collection properties (repeated, or maps) are marked as non-null.
All other properties are marked as nullable.

Parser property marked as non-null.
Descriptor property marked as non-null.

Constructor parameter is non-null
Clone method returns non-null
Equals parameter is nullable
Mergefrom parameter is nullable
MergeFrom(CodedInputStream) parameter is non-null
WriteTo parameter is non-null

[ObsidianMinor]

Instead of using the normal C# 8 syntax perhaps we could make use of the code analysis attributes, that way the syntax works no matter what version of C# you're using. Of course that'd require that a separate package (like Microsoft.Bcl.Async) for everything under .NET Standard 2.1 but should that ever happen it could probably be done that way.

[YairHalberstadt]

@ObsidianMinor

I think that would be highly undesirable. C# 8.0 introduced nullability as a first class feature, and the amount of work that's been put in is phenomenal (probably more than any other feature in the history of C#). Even then it leaves a large number of holes.

Existing nullability attributes are extremely limited in what they can express, and enforce, and would fail to integrate well with C# 8.0. I think that it's better to make nullability work properly for a subset of users than to make it work badly for a larger subset.

[ObsidianMinor]

Were those attributes not specifically added for C# 8? dotnet/corefx#37826 says otherwise. They work with C# 8 flawlessly.

public string TypeUrl {
  get { return typeUrl_; }
  [global::System.Diagnostics.CodeAnalysis.DisallowNullAttribute]
  set {
    typeUrl_ = pb::ProtoPreconditions.CheckNotNull(value, "value");
  }
}

// .. somewhere far far away
#nullable enable

any.TypeUrl = null; // Cannot convert null literal to non-nullable reference type

All of the cases you mentioned work with these attributes.

String properties => DisallowNullAttribute.
All collection properties (repeated, or maps) => NotNullAttribute.
All other properties => MaybeNullAttribute.

Parser property => NotNullAttribute.
Descriptor property => NotNullAttribute.

Constructor parameter => DisallowNullAttribute
Clone method => NotNullAttribute
Equals parameter => AllowNullAttribute
MergeFrom parameter => AllowNullAttribute
MergeFrom(CodedInputStream) => DisallowNullAttribute
WriteTo => DisallowNullAttribute

[YairHalberstadt]

@ObsidianMinor

I see I misunderstood which attributes you were referring to. My apologies.

The C# 8 nullable annotations emit attributes themselves. The coreFX attributes are meant for when the behaviour is to complex to be described by these annotations.

In so far as pre C# 8 tooling understands the CoreFX attributes, I would expect it to understand the attributes emitted by the compiler. However I strongly doubt either would work well.

[ObsidianMinor]

I can probably get a prototype of this working pretty easily but it could only be pulled when and if dotnet/runtime#30801 happens.

Using these annotations would allow us to do this in a backwards compatible way. The behavior of nulls in generated code and the library is too complex to be done with standard ? and ! syntax. So rather than doing it using that syntax, we can express what we need using the attributes. This has the included bonus of working on other lang versions. Only downside is this requires the nuget package as shown above.

[YairHalberstadt]

Using these annotations would allow us to do this in a backwards compatible way.

Ah you mean to avoid a switch? That sounds like a decent idea.

[ObsidianMinor]

Alright I wrote a prototype (untested) on my fork on the csharp/nullability branch. As it turns out nullability attributes do not affect code analysis in function bodies as noted in dotnet/roslyn#36039 so depending on whether the attributes affect code analysis in the future we may or may not have to disable warnings on the file level which is a very simple addition. Again, this will only work if a separate nuget package is made for these attributes, it works on the prototype since I manually copy-pasted the attributes into Google.Protobuf.

[jtattermusch]

I don't think we'd want to add another dependency to be able to reference the attributes.

[LiamMorrow]

This seems like a fairly decent compromise in reference the the idea of not wishing to add another dependency:
https://www.nuget.org/packages/Nullable/
Essentially does what @ObsidianMinor did where the attributes get compiled into the application without having to copy them into the repo. Unless the preference is to not have any new dependencies, regardless of if they then cause a dependency downstream to consuming projects.

@ObsidianMinor , it mightn't even be necessary to have the nuget package, if those attributes which were copied in are made internal then there are no problems with conflicts and it will still allow us to build against older dotnet versions which are not null context aware

[LiamMorrow]

This is an interesting problem since it would be a breaking change (not so much binary breaking as source breaking). Consider a project with nullable enabled that has code like this:

message FooMessage {
  google.protobuf.StringValue bar = 1;
}

void DoSomething(FooMessage foo) 
{
    string extractedBar = foo.Bar;
    // do something with that string
}

Previously that would have emitted no warnings, but with a change like this the compiler will now say that we are possibly assigning null to a non nullable (extractedBar). Now clearly this is the desired behavior (this is the whole point of nullable reference types). The issue is when we look at this scenario in a project which has TreatWarningsAsErrrors enabled. When we update the tooling reference, our project will no longer compile. This would have a knock on effect with libraries that export their protobuf types (thinking gax etc.).

I'm strongly in favor of having the generator output nullable annotations, however this seems like it could have larger consequences than meets the eye, and thought it would be worth mentioning for discussion since an official proposal has not been done up.

[YairHalberstadt]

@LiamMorrow
That is true of every single library. The official line of the C# team is: nullable reference types will be disabled by default till .Net 5. If you enable them earlier, expect some churn till then whilst libraries switch NRTs on.

[LiamMorrow]

Very true @YairHalberstadt . I should have put a note I was mentioning it more of a justification to others (and my future self) for why it will likely be a while before this is turned on by default. I am still interested in the option of a flag to enable it for generation. Seems like it could be a good stopgap in the mean time.

[Falco20019]

We are also interested in this feature. We currently look into enabling nullables and being able to differentiate between string and google.protobuf.StringValue would be a great benefit for us.

I fully understand that this will take some more time, especially since most people won't enable it until it has a better adoption in libs. Just wanted to give it an upvote.

[thefringeninja]

In the meantime, could we have the auto generated code insert #nullable disable at the top of each generated file?

[jtattermusch]

@thefringeninja I think adding '#nullable disable' to the generated files would actually break compilation on older compilers (since they don't know what #nullable means) and that's completely unacceptable (unless you can somehow include #nullable conditionally).

[thefringeninja]

I'm not sure about .net framework, but all the dotnet core versions that don't support #nullable are already EOL.

Even so, a flag to enable this would be more than sufficient.

[jtattermusch]

I'm not sure about .net framework, but all the dotnet core versions that don't support #nullable are already EOL.

Not true, .NET Core 2.1 is supported until August 2021 and it doesn't support nullable reference types.
Even if it did, Google.Protobuf supports everything starting from net45 and we cannot just break the generated code for existing users.

Even so, a flag to enable this would be more than sufficient.

New protoc flags are pain to add, maintain, integrate and they also make using protoc complicated, so the we're pretty strict about not adding more flags unless they are absolutely necessary. If we introduced a flag for every feature we would have 1000 flags right now.

Under these restrictions, is there a way to add the #nullable disable conditionally so it doesn't break older compilers?

[jtattermusch]

One idea is to add (if there is really no way to detect whether the compiler supports the #nullable define)

#if GOOGLE_PROTOBUF_DISABLE_NULLABLE_REFERENCES
// For now Google.Protobuf doesn't have special support for nullable references,
// but this at least allow projects that already use the nullable reference feature from C# 8
// to include protobuf generated code more easily.
#nullable disable
#endif

Would that be something that is helpful? (It would require the project to define the GOOGLE_PROTOBUF_DISABLE_NULLABLE_REFERENCES property).

[jtattermusch]

CC @JamesNK

[YairHalberstadt]

@jtattermusch

Why not use that same technique, but to add nullable annotations in the correct places, guarded by an ifdef?

It would make the generated code much longer and uglier, but I don't know if that's a concern.

[jtattermusch]

@jtattermusch

Why not use that same technique, but to add nullable annotations in the correct places, guarded by an ifdef?

It would make the generated code much longer and uglier, but I don't know if that's a concern.

The difference is that what I'm proposing is a pretty trivial fix that could potentially unlock lot of users. What you're proposing is a significant time investments and it would need to be designed and tested carefully.

Besides that, much longer and uglier code is definitely a concern.

[mtrtm]

Did this thread end up dying? As a .Net developer, I was shocked to learn nullable wasn't supported. There were some arguments a year ago about c# 8, but we are about to be on 9. I would love if this could be implemented - nullable is an incredibly positive option to be able to turn on.

[Falco20019]

@jtattermusch We are also waiting and hoping for this as it's blocking us from enabling nullable annotations in our code base without getting lot's of errors or enabling nullability in each of our files manually. AFAIK there is sadly no way to disable nullability just for a folder (like obj). Only per file or per project. Due to the modular architecture of our code, we can not put the protos in a separate project as this would make our code base multiply heavily.

I hope this gets some more attention with .NET 5.