-
Notifications
You must be signed in to change notification settings - Fork 15.4k
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
Add option to generate C# nullable annotations #6632
Comments
I'm not generally opposed to having this, but here are the challenges:
There are two separate areas:
|
The annotations are backwards compatible, so I don't think this should be an issue.
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. Parser property marked as non-null. Constructor parameter is non-null |
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. |
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. |
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. Parser property => NotNullAttribute. Constructor parameter => DisallowNullAttribute |
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. |
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 |
Ah you mean to avoid a switch? That sounds like a decent idea. |
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. |
I don't think we'd want to add another dependency to be able to reference the attributes. |
This seems like a fairly decent compromise in reference the the idea of not wishing to add another dependency: @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 |
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 ( 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. |
@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. |
We are also interested in this feature. We currently look into enabling nullables and being able to differentiate between 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. |
In the meantime, could we have the auto generated code insert |
@thefringeninja I think adding '#nullable disable' to the generated files would actually break compilation on older compilers (since they don't know what |
I'm not sure about .net framework, but all the dotnet core versions that don't support Even so, a flag to enable this would be more than sufficient. |
Not true, .NET Core 2.1 is supported until August 2021 and it doesn't support nullable reference types.
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 |
One idea is to add (if there is really no way to detect whether the compiler supports the
Would that be something that is helpful? (It would require the project to define the GOOGLE_PROTOBUF_DISABLE_NULLABLE_REFERENCES property). |
CC @JamesNK |
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. |
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. |
@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 I hope this gets some more attention with .NET 5. |
#7382 has seemingly been merged, just waiting for a new release. |
@SonnyX Are you sure that's related? I think that's only about optional-marked fields. @jtattermusch C#9 is around the corner. Any plans or a roadmap here? Is grpc/grpc-dotnet#606 the leading ticket on that? |
As of the important box in https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references#nullable-contexts this would need to explicitly set Did some more tests recently, since we plan to switch to .NET 6 with nullability soon. It's still a bit disappointing that this is missing, but at least, it's not generating any warnings. Pinging @jtattermusch and @JamesNK since this is related to grpc/grpc#20729 |
As of 3 weeks from now .NET 4.8 and earlier are all unsupported. .NET 5 will also be unsupported and all versions of .NET Core that don't support nullable types are already unsupported. If Google insists on supporting stuff that Microsoft themselves do not, please just add a positive flag that we can set in the csproj file that once set, generates classes with nullable types. If it doesn't have optional, then it isn't nullable. Full stop. If the positive flag isn't there, then emit the old stuff. This is a major source of bugs and presents a massive barrier to adoption. I simply won't allow code that isn't nullable aware into our code chain anymore specifically because of the runtime errors that allowing it makes. Given that Dart's approach to this topic and being absolutists about it, I think it's fair to approach C# the same way. If people don't want to use nullable types, then they can stick with a previous version. And since the demark is easy (.net Core 3.1 or .net 5) it's easy to exclude them from newer versions. |
I'd love to see nullable-support as well, but I don't think your statements regarding End of Support for older .NET versions are correct:
|
@cwe1ss .NET Core 3.1 supports nullables. .NET 5 supports nullables and is dead in 2 weeks. .NET 4.6.1 and earlier (with the notable exception of which 0% of people using 3.5 will ber using GRPC) is dead in the same 2 weeks and 4.6.2+ you can shim to make it work, and by all means keep using the old version, or as I suggested just have a flag for what gets outputted and make it a positive output and give guidance on how to put that flag in the csproj file. IF you wait for no one to be using something, then you'll never improve. We're at the point where the fringe that doesn't have support for nullable types is so small AND very unlikely to be using GRPC that it's safe to say that you could just go all in, but if Google is worried, then just add a flag like they did with optional in the first place but for .NET only and this time document how to pass it from the csproj properly. |
Still waiting for this |
I think the problem most people are interested in is avoiding assigning null to string fields. As far as I can see this could simply be done with the #if NETCOREAPP3_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER
[global::System.Diagnostics.CodeAnalysis.DisallowNullAttribute]
#endif
public string MyStringField
... This should be a simple, backwards compatible change that only affects code with nullable analysis enabled. |
I've made a PR that implements the solution proposed by @Falco20019, which is in my opinion the most reasonable and effective option (no breaking changes, backward compatible, and optional) |
A gRFC has been published to discuss: Discussion: |
Whoot whoot! |
Any news here? it would be great to have this feature! |
We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment. This issue is labeled |
Would love to see this feature as well. |
+1 |
ping |
This is still a work in progress - see #16240 |
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.
The text was updated successfully, but these errors were encountered: