Add option to enable csharp nullable reference types #13218

ogxd · 2023-07-05T22:15:08Z

Related to:

Context

Nullable reference types introduce a nullability context. Currently, the C# code generated via protobuf does not allow null for reference types and will intentionally throw at runtime when a null is assigned to such field.
Because generated files are annotated with the // <auto-generated> attribute on top, nullable reference types won't work, even if NRT is explicitly enabled at the project level for instance.
The consequence of this is that there is no way to benefit from NRT static code analysis to know in advance where fields could be assigned with a null (and thus it would throw at runtime)

Solution

A first solution would be to allow removing the // <auto-generated> attribute, however, it has the unwanted side effects of enabling code analysis on the generated file itself, which we want to avoid.
Instead, it turns out that marking generated files with #nullable enable works.

The following PR is about this: Adding a CLI option to add #nullable enable on top of generated files.
The unit tests show that the code generated with that option now carries the nullability information so that we have a warning for any consuming code that may assign nulls (CS8601).

Compatibility

Files annotated with #nullable enable can still compile fine under versions of dotnet prior to 6.0, so it is a backward-compatible change.
Also, fields are still annotated as non-nullable, so I believe this feature does not break compatibility.

Todo

I haven't made any changes to the documentation yet as I would like feedback first.
CLA in progress.

jskeet · 2023-07-06T06:33:22Z

Thank you for all your work on this, but we're still discussing whether or not we want to include nullable reference types in code generation at all. Please see grpc/grpc#33507 where there is considerable discussion.
If you're happy to keep working on this PR knowing that the end result may be that we don't want to do anything, that's fine - but I won't be reviewing it in any detail until we've decided what we actually want to do.

(In general we try to avoid adding language-specific command line options - when to use NRTs is one of the key design discussions here. It feels like a new option might be the only way to go as different consumers may want to generate code for the same proto differently, but it's not certain yet.)

ogxd · 2023-07-06T07:36:54Z

No problem, I understand.
I just noticed that my PR is actually about what @JamesNK just proposed yesterday here and here. As an user of protobuf for C# I confirm that simply having warnings in our code fulfills our most important need which is to avoid runtime null exceptions.

By the way, I used #nullable enable here, but #nullable enable annotations might be enough since we might not need nullable warning context within the generated files.

(I don't think I need to push anything else to that PR anyway until a decision has been taken, the implementation and the tests already shows whats this solution implies and its benefits)

tonydnewell · 2023-07-13T13:57:42Z

The code generation needs further changing to fully support handline NRT.
See grpc/grpc#33507 (comment)
e.g. allowing message fields to be set to null, Equals, MergeFrom, and removing redundant null checks elsewhere.

ogxd · 2023-07-13T21:59:52Z

The code generation needs further changing to fully support handline NRT.
See grpc/grpc#33507 (comment)
e.g. allowing message fields to be set to null, Equals, MergeFrom, and removing redundant null checks elsewhere.

I don't think it does. As the MS documentations says:

Nullable reference types aren't new class types, but rather annotations on existing reference types. The compiler uses those annotations to help you find potential null reference errors in your code.

This means that, given that the reference types are currently considered non-nullable, enabling NRT is about adding annotations so that a user knows that he might assign a null and trigger NREs.

What you describe with "allowing message fields to be set to null, Equals, MergeFrom, and removing redundant null checks elsewhere" is not about NRT but rather about allowing fields to be set to null. NRT annotations can be added on top, but again, it is different.

This PR is about adding NRT annotations so that users can consume this API while avoiding NREs, without any change to the generated code, unlike allowing fields to be set to null. Personally, I am not against also allowing reference fields to be set to null, but it is another topic (and more debatable).

tonydnewell · 2023-07-15T11:23:55Z

@ogxd Fields within a message that themselves are messages can be assigned null.
For example, proto file:

message Person {
  string name = 1;
  int32 id = 2;  // Unique ID number for this person.
  string email = 3;

  enum PhoneType {
    PHONE_TYPE_UNSPECIFIED = 0;
    PHONE_TYPE_MOBILE = 1;
    PHONE_TYPE_HOME = 2;
    PHONE_TYPE_WORK = 3;
  }

  message PhoneNumber {
    string number = 1;
    PhoneType type = 2;
  }

  PhoneNumber phone = 4; // this is a message and can be assigned null
}

with code:

Person person = new Person()
{
     Id = 1234,
     Name = "Fred",
     Email = "fred@example.com",
     Phone = null
 };

compiles today without your changes. Setting Phone to null effectively removes it from the message.

But with your changes:

Warning	CS8625	Cannot convert null literal to non-nullable reference type.

There is more involved than just adding #nullable enable to the generated code. Other changes to the generated code are necessary too.

ogxd · 2023-07-15T14:01:36Z

Yes, you're right, I thought they were treated the same as strings. But it means marking these fields/properties as nullable
with ? would be enough. Since this is still about adding annotations, the compiled code remains the same, so it is still a safe change, right?

We don't want NRT warnings within the generated code

ogxd · 2023-07-15T14:25:29Z

I've just updated the PR to use #nullable enable annotations instead of #nullable enable and to take into account remark from @tonydnewell about message fields. Generated code now also has nullable annotations for message fields (case covered by unit test)

Falco20019 · 2023-07-27T09:21:23Z

One special case that I think might have been missed (only gave it a quick glance) is google.protobuf.StringValue which then needs to be mapped onto string? instead of string. Since all other wrappers were already mapped onto nullables, they should be fine.

Falco20019 · 2023-07-27T09:23:04Z

src/google/protobuf/compiler/csharp/csharp_options.h

  bool serializable;
+  // Whether the generated files are annotated with "#nullable enable" to display warnings on potential null assignments. 


The comment still only talks about #nullable enable, which is not true anymore.

tonydnewell · 2023-07-27T12:48:38Z

I think there is a lot more work to be done. Adding #nullable enable annotations rather than #nullable enable hides a lot of potential problems.

As an experiment I took the code generated from unittest_well_known_types.proto and added #nullable enable and it produced 80 warnings.

tonydnewell · 2023-07-27T12:55:19Z

src/google/protobuf/compiler/csharp/csharp_generator.cc

@@ -79,6 +79,8 @@ bool Generator::Generate(const FileDescriptor* file,
      cli_options.internal_access = true;
    } else if (options[i].first == "serializable") {
      cli_options.serializable = true;
+    } else if (options[i].first == "enable_nullable_reference_types") {


My preference would be for a shorter option name. Microsoft just uses "nullable" (<Nullable> and #nullable) so maybe we should? Or "nullable_reference_types"? Whatever is chosen it would be good if both the protocol buffers compiler and gRPC plugin use the same name.

Given the requests that we've had for generating int?-backed properties for optional fields, I think just "nullable" could lead to confusion.

How about enable_nrt? I think NRT is a reasonably commonly-used abbrevation.

Indeed it is too long and can be confusing, enable_nrt seems more appropriate

ogxd · 2023-07-28T09:10:57Z

I think there is a lot more work to be done. Adding #nullable enable annotations rather than #nullable enable hides a lot of potential problems.

As an experiment I took the code generated from unittest_well_known_types.proto and added #nullable enable and it produced 80 warnings.

As the docs says about NRT, its purpose is to "minimize the likelihood that your code causes the runtime to throw System.NullReferenceException". I think the point here is that the autogenerated code is not "your" code (you don't edit it, you don't maintain it, it is simply an external API generated by an external tool). This is why I think there shouldn't be warnings from NRT within the generated code itself, and one of the reasons why #nullable enable annotations exists.

tonydnewell · 2023-07-31T07:53:51Z

I think there is a lot more work to be done. Adding #nullable enable annotations rather than #nullable enable hides a lot of potential problems.
As an experiment I took the code generated from unittest_well_known_types.proto and added #nullable enable and it produced 80 warnings.

As the docs says about NRT, its purpose is to "minimize the likelihood that your code causes the runtime to throw System.NullReferenceException". I think the point here is that the autogenerated code is not "your" code (you don't edit it, you don't maintain it, it is simply an external API generated by an external tool). This is why I think there shouldn't be warnings from NRT within the generated code itself, and one of the reasons why #nullable enable annotations exists.

Just my opinion (I'm not part of the protocol buffers team) but getting the generated code to be "clean" will probably help in not missing any corner cases, such as those arising with well-known types such as StringValue and BytesValue, and methods such as Equals.

JamesNK · 2023-07-31T08:24:08Z

What are the warnings? Nullable warnings shouldn't come from generated code.

I'm guessing the problem is Google.Protobuf itself isn't annotated. #nullable enable annotations could be a short term fix and a longer term fix is annotating Google.Protobuf (not a small task. I see someone attempted to do that here - #9801).

tonydnewell · 2023-07-31T09:08:44Z

What are the warnings? Nullable warnings shouldn't come from generated code.

I'm guessing the problem is Google.Protobuf itself isn't annotated. #nullable enable annotations could be a short term fix and a longer term fix is annotating Google.Protobuf (not a small task. I see someone attempted to do that here - #9801).

A lot of warnings about setting null in reflection methods and classes such as GeneratedClrTypeInfo - these could wait for Google.Protobuf to be updated, or the generated code could add the null forgiving operator.

Also warnings in constructors when fields contain null values, e.g. CS8618 Non-nullable field '_unknownFields' must contain a non-null value when exiting constructor. Consider declaring the field as nullable.

Also warnings in Equals - e.g. CS8604 Possible null reference argument for parameter 'other' in 'bool OneofWellKnownTypes.Equals(OneofWellKnownTypes other)'.

I know that we could use #nullable enable annotations but by using #nullable enable instead it might help spot the areas of the code that really need fixing.

JamesNK · 2023-07-31T12:17:12Z

It sounds like these nullable warnings are related to the generated code. In that case, generated code should be updated to fix the warnings. For example, allow things to be null where necessary. Or add the bang operator to suppress errors when there is no other choice.

I suggest putting a project with a copy of the generated code (copied from the temporary obj directory) in a GH repo. Then other people can see the errors you're talking about. Right now, we have no idea.

That would allow us focus to be on what generated code with nullable annotations should look like. Then once that is decided, update protoc generation to output it.

Also warnings in constructors when fields contain null values, e.g. CS8618 Non-nullable field '_unknownFields' must contain a non-null value when exiting constructor. Consider declaring the field as nullable.

_unknownFields field should be nullable. It can have a null value:

protobuf/csharp/src/Google.Protobuf.Test.TestProtos/UnittestWellKnownTypes.pb.cs

Lines 2250 to 2252 in a80daa2

    
           if (_unknownFields != null) { 
        
             hash ^= _unknownFields.GetHashCode(); 
        
           }

Also warnings in Equals - e.g. CS8604 Possible null reference argument for parameter 'other' in 'bool OneofWellKnownTypes.Equals(OneofWellKnownTypes other)'.

Messages implement IMessage<T>, which implements IEquatable<T>. Its Equals method accepts a nullable argument. The generated code needs to match:

public bool Equals(OneofWellKnownTypes? other)
{
    // ...
}

jskeet · 2023-08-04T12:42:24Z

Just to chime in, I agree that the generated code should build warning-free. It sounds like some more work is needed - but thanks for what you've already done, and I look forward to looking more closely when it's fully "done".
I suspect that when Google.Protobuf eventually becomes NRT-aware, we may need a second pass anyway - but that's okay.

ogxd · 2023-09-20T21:43:06Z

Thanks for the feedback! I just became dad on the 4th of August so my days were a little busy since but if the nullable topic hasn't addressed in parallel since I'll update the PR accordingly to your remarks when I have some time 😄

stebet · 2023-11-10T10:25:09Z

Thanks for the feedback! I just became dad on the 4th of August so my days were a little busy since but if the nullable topic hasn't addressed in parallel since I'll update the PR accordingly to your remarks when I have some time 😄

I'm very excited about getting this into the tooling so if there is anything I can do to help or speed things along, please let me know :)

tonydnewell · 2023-11-15T14:01:43Z

csharp/generate_protos.sh

+    --csharp_out=csharp/src/Google.Protobuf.Test.TestProtos \
+    --csharp_opt=file_extension=.pb.cs \
+    --csharp_opt=enable_nullable_reference_types \
+    --descriptor_set_out=csharp/src/Google.Protobuf.Test/testprotos.pb \


The overwriting of testprotos.pb here causes the tests in DescriptorDeclarationTest.cs to fail

tonydnewell · 2023-11-15T14:02:39Z

csharp/protos/unittest_nrt_proto3.proto

+  TestNrtNestedMessage message_field = 1;
+  string string_field = 2;
+  int32 int32_field = 3;
+}


Code generated for unittest_nrt_proto3.proto causes RefStructCompatibilityTest.cs tests to fail

tonydnewell · 2023-11-15T14:04:17Z

csharp/src/Google.Protobuf.Test/testprotos.pb

This version of the files causes tests in DescriptorDeclarationTest.cs to fail

tonydnewell · 2023-11-15T14:05:55Z

I've started looking at this in more detail to see if I can complete the work.
Currently there are some issues with the existing tests failing after some of the changes. See comments above.

github-actions · 2024-02-14T10:01:58Z

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

github-actions · 2024-02-29T10:02:00Z

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it.

This PR was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

ogxd requested a review from a team as a code owner July 5, 2023 22:15

ogxd requested review from jskeet and removed request for a team July 5, 2023 22:15

ogxd force-pushed the csharp-nrt-option branch from 57115ec to 22d4da3 Compare July 5, 2023 22:20

Add option to enable csharp nullable reference types

1e35b60

ogxd force-pushed the csharp-nrt-option branch from 22d4da3 to 1e35b60 Compare July 5, 2023 22:21

ogxd mentioned this pull request Jul 5, 2023

Add option to generate C# nullable annotations #6632

Open

Rename nullable message to nrt message

92bad8c

ogxd force-pushed the csharp-nrt-option branch from 10a904a to 92bad8c Compare July 5, 2023 22:39

fowles added the c# label Jul 7, 2023

ogxd added 2 commits July 15, 2023 16:23

Use #nullable enable annotations instead of #nullable enable

c526bc1

We don't want NRT warnings within the generated code

Add nullable annotation for message fiels and update test accordingly

d2d6007

This was referenced Jul 26, 2023

[C#] Add null reference types support to gRPC C# plugin grpc/grpc#33878

Open

[C#] Grpc.Tools changes to support nullable reference types grpc/grpc#33879

Draft

Investigate support for nullable reference types in generated code grpc/grpc-dotnet#606

Open

Falco20019 reviewed Jul 27, 2023

View reviewed changes

tonydnewell reviewed Jul 27, 2023

View reviewed changes

tonydnewell reviewed Nov 15, 2023

View reviewed changes

csharp/src/Google.Protobuf.Test/testprotos.pb

Copy link

Contributor

tonydnewell Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version of the files causes tests in DescriptorDeclarationTest.cs to fail

github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Feb 14, 2024

github-actions bot closed this Feb 29, 2024

tonydnewell mentioned this pull request Mar 20, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to enable csharp nullable reference types #13218

Add option to enable csharp nullable reference types #13218

ogxd commented Jul 5, 2023 •

edited

Loading

jskeet commented Jul 6, 2023

ogxd commented Jul 6, 2023 •

edited

Loading

tonydnewell commented Jul 13, 2023

ogxd commented Jul 13, 2023 •

edited

Loading

tonydnewell commented Jul 15, 2023

ogxd commented Jul 15, 2023 •

edited

Loading

ogxd commented Jul 15, 2023

Falco20019 commented Jul 27, 2023 •

edited

Loading

Falco20019 Jul 27, 2023

tonydnewell commented Jul 27, 2023

tonydnewell Jul 27, 2023 •

edited

Loading

jskeet Jul 27, 2023

ogxd Jul 28, 2023

ogxd commented Jul 28, 2023

tonydnewell commented Jul 31, 2023

JamesNK commented Jul 31, 2023 •

edited

Loading

tonydnewell commented Jul 31, 2023

JamesNK commented Jul 31, 2023 •

edited

Loading

jskeet commented Aug 4, 2023

ogxd commented Sep 20, 2023

stebet commented Nov 10, 2023

tonydnewell Nov 15, 2023

tonydnewell Nov 15, 2023

tonydnewell Nov 15, 2023

tonydnewell commented Nov 15, 2023

github-actions bot commented Feb 14, 2024

github-actions bot commented Feb 29, 2024

		bool serializable;
		// Whether the generated files are annotated with "#nullable enable" to display warnings on potential null assignments.

Add option to enable csharp nullable reference types #13218

Add option to enable csharp nullable reference types #13218

Conversation

ogxd commented Jul 5, 2023 • edited Loading

Context

Solution

Compatibility

Todo

jskeet commented Jul 6, 2023

ogxd commented Jul 6, 2023 • edited Loading

tonydnewell commented Jul 13, 2023

ogxd commented Jul 13, 2023 • edited Loading

tonydnewell commented Jul 15, 2023

ogxd commented Jul 15, 2023 • edited Loading

ogxd commented Jul 15, 2023

Falco20019 commented Jul 27, 2023 • edited Loading

Falco20019 Jul 27, 2023

Choose a reason for hiding this comment

tonydnewell commented Jul 27, 2023

tonydnewell Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

jskeet Jul 27, 2023

Choose a reason for hiding this comment

ogxd Jul 28, 2023

Choose a reason for hiding this comment

ogxd commented Jul 28, 2023

tonydnewell commented Jul 31, 2023

JamesNK commented Jul 31, 2023 • edited Loading

tonydnewell commented Jul 31, 2023

JamesNK commented Jul 31, 2023 • edited Loading

jskeet commented Aug 4, 2023

ogxd commented Sep 20, 2023

stebet commented Nov 10, 2023

tonydnewell Nov 15, 2023

Choose a reason for hiding this comment

tonydnewell Nov 15, 2023

Choose a reason for hiding this comment

tonydnewell Nov 15, 2023

Choose a reason for hiding this comment

tonydnewell commented Nov 15, 2023

github-actions bot commented Feb 14, 2024

github-actions bot commented Feb 29, 2024

ogxd commented Jul 5, 2023 •

edited

Loading

ogxd commented Jul 6, 2023 •

edited

Loading

ogxd commented Jul 13, 2023 •

edited

Loading

ogxd commented Jul 15, 2023 •

edited

Loading

Falco20019 commented Jul 27, 2023 •

edited

Loading

tonydnewell Jul 27, 2023 •

edited

Loading

JamesNK commented Jul 31, 2023 •

edited

Loading

JamesNK commented Jul 31, 2023 •

edited

Loading