[C-Sharp] Inconsistent generated code for wrapper primitive types and new proto3 optional keyword

[betmix-matt]

As documented in:
https://developers.google.com/protocol-buffers/docs/reference/csharp-generated#wrapper_types

Fields that use the well known wrapper types (i.e. Int32Value, StringValue etc) generate as the primitive type but as a nullable field. I love this behavior. Most other languages did not choose this implementation which in my opinion is the simplest, most maintainable, and easy to use.

The new "optional" keyword introduced in 3.14 behaves differently where the generated property is the primitive type but not nullable and has generated "HasX" property and "ClearX" method.

So for example

syntax = "proto3";

package example;

import "google/protobuf/wrappers.proto";

message Example {
   google.protobuf.Int32Value wrapper_field = 1;
   optional int32 optional_field = 2;
}

Generates the following class (simplified for brevity and clarity):

namespace Example {
  public sealed partial class Example : pb::IMessage<Example> {
    public int? WrapperField { get; set; }
    public int OptionalField { get; set; }
    public bool HasOptionalField { get; }
    public void ClearOptionalField() { ... }
  }
}

There is no documentation in the C# generated code reference about the difference in behaviors (or really anything about the behavior of the "optional" keyword).

I would highly prefer that the optional keyword generated identical code to the wrapper types as their intent is identical.
Google API design guidelines even says that you should not use wrapper types anymore in favor of optional keyword: https://cloud.google.com/apis/design/design_patterns.md#optional-primitive-fields

I propose changing the behavior of the protoc plugin for dotnet to generate the same code for both methods of specifying optional primitive fields.

If for some reason we need to avoid the breaking change for the default behavior, there should be a plugin option to change the behavior so they do generate identical code (or at least code that is identical in its interface) however as the optional support in proto3 is very new, it's unlikely to break existing code.

[jtattermusch]

CC @jskeet @haberman @moserware

@jskeet and @haberman will probably know more about why we chose the approach we did for proto3 optional.

[jskeet]

If for some reason we need to avoid the breaking change for the default behavior

... which we do, to avoid breaking everyone who already uses it, including the Google Cloud client libraries. Note that support for optional in proto3 isn't really "very new" - it's been in the protoc for C# since April 2020.

It's worth noting that in the current design, adding optional to a field is not a breaking change in terms of the generated code. It adds the ability to detect whether or not something was explicitly set, but that's all. Using nullable types instead changes that completely. While it's been a long time since I implemented the C# support, I expect that was my reasoning for the design at the time... as well as it following what Java does, as the most similar language.

I would be strongly against changing the default behavior. I would be moderately against introducing a plugin option for this, as I expect it will make the plugin code significantly more complex, and have repercussions on the library support as well.

As I'm not on the protobuf team it's not my decision though.

[haberman]

I agree with @jskeet on all points. I would add that a primary goal of protobuf is to have consistent semantics across languages. One of those semantics is that optional fields return a default value when unset, not null.

For protos with syntax="proto2"; you can even set an explicit default value in the .proto file. If we returned null instead of the specified default, this would create an even larger behavior difference between languages.

A lot of users request nullable semantics, and many languages have types that represent this (eg. std::optional in C++, Option in Rust, etc). To bring this to protobuf, it would need to be a larger effort that targets all languages, not just some, in order to keep the semantics consistent across languages.

[betmix-matt]

To bring this to protobuf, it would need to be a larger effort that targets all languages, not just some, in order to keep the semantics consistent across languages.

So why was the optional keyword brought back into Proto3 and Google recommending using it in place of nullable types if it wasn't to do exactly what you're saying, i.e. a larger effort to target all languages to represent nulls consistently?

And yes agree there is a difference in the on the wire representation of the wrapper object vs the nullable field, but technically making a field go from nullable to not nullable or visa versa IS a breaking change or at least should be considered that from the point of view of your code base. I would want it if I changed that field that everywhere it used to compile would now need review with a compiler warning which is exactly what would happen in C# if you did that and the field actually became a nullable field. It doesn't break your code, but it will warn you that there might be an issue now.

However because you don't change anything about the property when you change it from non-optional, to optional in the current implementation, you negate that whole benefit.

You also argued my case in the last point.

A lot of users request nullable semantics, and many languages have types that represent this

The C# implementation of Protobuf DOES use nullable semantics because C# is a language that supports it. I'm just saying that the intent of an "optional" field is exactly that, a nullable type. So C# should use nullable types there. You're already doing it for the wrapper types! While I might agree that this could be considered a breaking change and should be gated behind some option, it should still be available. It's logically the correct answer even if it does cause differentiation between the proto2 and proto3 representations. There is already differences between them. If google recommends you don't use the well known wrapper types any more in exchange for using the new "optional" keyword, then if I convert my proto files from using wrapper types to optional keywords, they should generally function similarly in the generated code.

[mirhagk]

I would add that a primary goal of protobuf is to have consistent semantics across languages.

Unfortunately that ship has long sailed.

Protobuf.js for instance went with optional returning null: protobufjs/protobuf.js#1584, as did several other libraries. It's not surprising when google's docs literally say:

Protocol Buffers v3 (proto3) supports optional primitive fields, which are semantically equivalent to nullable types in many programming languages.

This is the page where google recommends getting rid of wrapper types, and when it says that most libraries can't even tell the difference, it's clear that factors into their motivation for their recommendation.

The fact that C# can tell the difference makes it unusual, but worse C# doesn't allow migrating from one to the other. If your API is using wrappers and you want to switch to optional, you not only have to worry about serialization changes (which are to be expected and can be handled by renaming proto fields) but you have to worry about rewriting all your code, removing any situation where you use an expression to create a protobuf.

It's incredibly odd to see the library massively changed it's mind on how to handle semantically equivalent things, especially when they new way is more awkward to use, doesn't play well with core language ideas, and comes at a time where the downsides of using null are massively decreased within C#.

I understand not wanting to introduce breaking changes, but that happened when optional support was added. Worse it's not a compile-time error in the case of optional string, it's a runtime error. You'll change wrapper values to optional, as google says you "must" do, and everything will seem to work until the day you start getting exceptions on production and discover the change in nullable semantics. Introducing a change here will be a breaking change for those of us who've already been burned by this, but it makes it so anyone who hasn't migrated yet will no longer come across this silent major change. And I think those of us who have already migrated our protos are willing to accept another breaking change when it means such a huge improvement in both type safety and ease of use.