Support proto3 optional fields in the C# generator

Most changes are:

- Introducing new helpers of SupportsPresenceApi and RequiresPresenceBit. This allows calling code to be a lot clearer about what it's interested in.
- Changing most previous IsProto2 calls to use one of the two new helper methods
- Avoiding treating synthetic oneofs as regular ones
- Some slight refactoring in csharp_primitive_field to avoid code duplication
- Comments explaining what we want when, so the next maintainer doesn't need to do the detective work I did!

This change deliberately doesn't modify the API surface of any
existing code. The only change to previously-generated C# should be
making presence bits more efficient in proto2.

Once proto3 optional fields are supported, we can consider further
changes to make the proto2 and proto3 generated API surface more
consistent (e.g. adding presence API for message fields and oneofs).

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

@@ -96,13 +96,13 @@ void FieldGeneratorBase::SetCommonFieldVariables(
   (*variables)["default_value"] = default_value();
   (*variables)["capitalized_type_name"] = capitalized_type_name();
   (*variables)["number"] = number();
-  if (has_default_value() && !IsProto2(descriptor_->file())) {
+  if (has_default_value() && !SupportsPresenceApi(descriptor_)) {
     (*variables)["name_def_message"] =
       (*variables)["name"] + "_ = " + (*variables)["default_value"];
   } else {
     (*variables)["name_def_message"] = (*variables)["name"] + "_";
   }
-  if (IsProto2(descriptor_->file())) {
+  if (SupportsPresenceApi(descriptor_)) {
     (*variables)["has_property_check"] = "Has" + (*variables)["property_name"];
     (*variables)["other_has_property_check"] = "other.Has" + (*variables)["property_name"];
     (*variables)["has_not_property_check"] = "!" + (*variables)["has_property_check"];
@@ -125,7 +125,7 @@ void FieldGeneratorBase::SetCommonFieldVariables(
 void FieldGeneratorBase::SetCommonOneofFieldVariables(
     std::map<string, string>* variables) {
   (*variables)["oneof_name"] = oneof_name();
-  if (IsProto2(descriptor_->file())) {
+  if (SupportsPresenceApi(descriptor_)) {
     (*variables)["has_property_check"] = "Has" + property_name();
   } else {
     (*variables)["has_property_check"] =

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

@@ -515,13 +515,13 @@ FieldGeneratorBase* CreateFieldGenerator(const FieldDescriptor* descriptor,
         }
       } else {
         if (IsWrapperType(descriptor)) {
-          if (descriptor->containing_oneof()) {
+          if (descriptor->real_containing_oneof()) {
             return new WrapperOneofFieldGenerator(descriptor, presenceIndex, options);
           } else {
             return new WrapperFieldGenerator(descriptor, presenceIndex, options);
           }
         } else {
-          if (descriptor->containing_oneof()) {
+          if (descriptor->real_containing_oneof()) {
             return new MessageOneofFieldGenerator(descriptor, presenceIndex, options);
           } else {
             return new MessageFieldGenerator(descriptor, presenceIndex, options);
@@ -532,7 +532,7 @@ FieldGeneratorBase* CreateFieldGenerator(const FieldDescriptor* descriptor,
       if (descriptor->is_repeated()) {
         return new RepeatedEnumFieldGenerator(descriptor, presenceIndex, options);
       } else {
-        if (descriptor->containing_oneof()) {
+        if (descriptor->real_containing_oneof()) {
           return new EnumOneofFieldGenerator(descriptor, presenceIndex, options);
         } else {
           return new EnumFieldGenerator(descriptor, presenceIndex, options);
@@ -542,7 +542,7 @@ FieldGeneratorBase* CreateFieldGenerator(const FieldDescriptor* descriptor,
       if (descriptor->is_repeated()) {
         return new RepeatedPrimitiveFieldGenerator(descriptor, presenceIndex, options);
       } else {
-        if (descriptor->containing_oneof()) {
+        if (descriptor->real_containing_oneof()) {
           return new PrimitiveOneofFieldGenerator(descriptor, presenceIndex, options);
         } else {
           return new PrimitiveFieldGenerator(descriptor, presenceIndex, options);

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

@@ -158,6 +158,34 @@ inline bool IsProto2(const FileDescriptor* descriptor) {
   return descriptor->syntax() == FileDescriptor::SYNTAX_PROTO2;
 }
 
+inline bool SupportsPresenceApi(const FieldDescriptor* descriptor) {
+  // We don't use descriptor->is_singular_with_presence() as C# has slightly
+  // different behavior to other languages.
+ 
+  if (IsProto2(descriptor->file())) {
+    // We generate Has/Clear for oneof fields in C#, as well as for messages.
+    // It's possible that we shouldn't, but stopping doing so would be a
+    // breaking change for proto2. Fortunately the decision is moot for
+    // onoeof in proto3: you can't use "optional" inside a oneof.
+    // Proto2: every singular field has presence. (Even fields in oneofs.)
+    return !descriptor->is_repeated();
+  } else {
+    // Proto3: only for explictly-optional fields that aren't messages.
+    // (Repeated fields can never be explicitly optional, so we don't need
+    // to check for that.) Currently we can't get at proto3_optional directly,
+    // but we can use has_optional_keyword() as a surrogate check.    
+    return descriptor->has_optional_keyword() &&
+      descriptor->type() != FieldDescriptor::TYPE_MESSAGE;
+  }
+}
+
+inline bool RequiresPresenceBit(const FieldDescriptor* descriptor) {
+  return SupportsPresenceApi(descriptor) &&
+    !IsNullable(descriptor) &&
+    !descriptor->is_extension() &&
+    !descriptor->real_containing_oneof();
+}
+
 }  // namespace csharp
 }  // namespace compiler
 }  // namespace protobuf

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

@@ -72,15 +72,13 @@ MessageGenerator::MessageGenerator(const Descriptor* descriptor,
   std::sort(fields_by_number_.begin(), fields_by_number_.end(),
             CompareFieldNumbers);
 
-  if (IsProto2(descriptor_->file())) {
-    int primitiveCount = 0;
-    for (int i = 0; i < descriptor_->field_count(); i++) {
-      const FieldDescriptor* field = descriptor_->field(i);
-      if (!IsNullable(field)) {
-        primitiveCount++;
-        if (has_bit_field_count_ == 0 || (primitiveCount % 32) == 0) {
-          has_bit_field_count_++;
-        }
+  int presence_bit_count = 0;
+  for (int i = 0; i < descriptor_->field_count(); i++) {
+    const FieldDescriptor* field = descriptor_->field(i);
+    if (RequiresPresenceBit(field)) {
+      presence_bit_count++;
+      if (has_bit_field_count_ == 0 || (presence_bit_count % 32) == 0) {
+        has_bit_field_count_++;
       }
     }
   }
@@ -222,11 +220,12 @@ void MessageGenerator::Generate(io::Printer* printer) {
     printer->Print("\n");
   }
 
-  // oneof properties
-  for (int i = 0; i < descriptor_->oneof_decl_count(); i++) {
-    vars["name"] = UnderscoresToCamelCase(descriptor_->oneof_decl(i)->name(), false);
-    vars["property_name"] = UnderscoresToCamelCase(descriptor_->oneof_decl(i)->name(), true);
-    vars["original_name"] = descriptor_->oneof_decl(i)->name();
+  // oneof properties (for real oneofs, which come before synthetic ones)
+  for (int i = 0; i < descriptor_->real_oneof_decl_count(); i++) {
+    const OneofDescriptor* oneof = descriptor_->oneof_decl(i);
+    vars["name"] = UnderscoresToCamelCase(oneof->name(), false);
+    vars["property_name"] = UnderscoresToCamelCase(oneof->name(), true);
+    vars["original_name"] = oneof->name();
     printer->Print(
       vars,
       "private object $name$_;\n"
@@ -234,8 +233,8 @@ void MessageGenerator::Generate(io::Printer* printer) {
       "public enum $property_name$OneofCase {\n");
     printer->Indent();
     printer->Print("None = 0,\n");
-    for (int j = 0; j < descriptor_->oneof_decl(i)->field_count(); j++) {
-      const FieldDescriptor* field = descriptor_->oneof_decl(i)->field(j);
+    for (int j = 0; j < oneof->field_count(); j++) {
+      const FieldDescriptor* field = oneof->field(j);
       printer->Print("$field_property_name$ = $index$,\n",
                      "field_property_name", GetPropertyName(field),
                      "index", StrCat(field->number()));
@@ -382,23 +381,24 @@ void MessageGenerator::GenerateCloningCode(io::Printer* printer) {
   for (int i = 0; i < has_bit_field_count_; i++) {
     printer->Print("_hasBits$i$ = other._hasBits$i$;\n", "i", StrCat(i));
   }
-  // Clone non-oneof fields first
+  // Clone non-oneof fields first (treating optional proto3 fields as non-oneof)
   for (int i = 0; i < descriptor_->field_count(); i++) {
-    if (!descriptor_->field(i)->containing_oneof()) {
-      std::unique_ptr<FieldGeneratorBase> generator(
-        CreateFieldGeneratorInternal(descriptor_->field(i)));
-      generator->GenerateCloningCode(printer);
+    const FieldDescriptor* field = descriptor_->field(i);
+    if (field->real_containing_oneof()) {
+      continue;
     }
-  }
-  // Clone just the right field for each oneof
-  for (int i = 0; i < descriptor_->oneof_decl_count(); ++i) {
-    vars["name"] = UnderscoresToCamelCase(descriptor_->oneof_decl(i)->name(), false);
-    vars["property_name"] = UnderscoresToCamelCase(
-        descriptor_->oneof_decl(i)->name(), true);
+    std::unique_ptr<FieldGeneratorBase> generator(CreateFieldGeneratorInternal(field));
+    generator->GenerateCloningCode(printer);
+  }
+  // Clone just the right field for each real oneof
+  for (int i = 0; i < descriptor_->real_oneof_decl_count(); ++i) {
+    const OneofDescriptor* oneof = descriptor_->oneof_decl(i);
+    vars["name"] = UnderscoresToCamelCase(oneof->name(), false);
+    vars["property_name"] = UnderscoresToCamelCase(oneof->name(), true);
     printer->Print(vars, "switch (other.$property_name$Case) {\n");
     printer->Indent();
-    for (int j = 0; j < descriptor_->oneof_decl(i)->field_count(); j++) {
-      const FieldDescriptor* field = descriptor_->oneof_decl(i)->field(j);
+    for (int j = 0; j < oneof->field_count(); j++) {
+      const FieldDescriptor* field = oneof->field(j);
       std::unique_ptr<FieldGeneratorBase> generator(CreateFieldGeneratorInternal(field));
       vars["field_property_name"] = GetPropertyName(field);
       printer->Print(
@@ -461,9 +461,9 @@ void MessageGenerator::GenerateFrameworkMethods(io::Printer* printer) {
             CreateFieldGeneratorInternal(descriptor_->field(i)));
         generator->WriteEquals(printer);
     }
-    for (int i = 0; i < descriptor_->oneof_decl_count(); i++) {
-        printer->Print("if ($property_name$Case != other.$property_name$Case) return false;\n",
-            "property_name", UnderscoresToCamelCase(descriptor_->oneof_decl(i)->name(), true));
+    for (int i = 0; i < descriptor_->real_oneof_decl_count(); i++) {
+      printer->Print("if ($property_name$Case != other.$property_name$Case) return false;\n",
+          "property_name", UnderscoresToCamelCase(descriptor_->oneof_decl(i)->name(), true));
     }
     if (has_extension_ranges_) {
       printer->Print(
@@ -488,9 +488,9 @@ void MessageGenerator::GenerateFrameworkMethods(io::Printer* printer) {
             CreateFieldGeneratorInternal(descriptor_->field(i)));
         generator->WriteHash(printer);
     }
-    for (int i = 0; i < descriptor_->oneof_decl_count(); i++) {
-        printer->Print("hash ^= (int) $name$Case_;\n",
-            "name", UnderscoresToCamelCase(descriptor_->oneof_decl(i)->name(), false));
+    for (int i = 0; i < descriptor_->real_oneof_decl_count(); i++) {
+      printer->Print("hash ^= (int) $name$Case_;\n",
+          "name", UnderscoresToCamelCase(descriptor_->oneof_decl(i)->name(), false));
     }
     if (has_extension_ranges_) {
       printer->Print(
@@ -589,22 +589,24 @@ void MessageGenerator::GenerateMergingMethods(io::Printer* printer) {
     "if (other == null) {\n"
     "  return;\n"
     "}\n");
-  // Merge non-oneof fields
+  // Merge non-oneof fields, treating optional proto3 fields as normal fields
   for (int i = 0; i < descriptor_->field_count(); i++) {
-    if (!descriptor_->field(i)->containing_oneof()) {
-      std::unique_ptr<FieldGeneratorBase> generator(
-          CreateFieldGeneratorInternal(descriptor_->field(i)));
-      generator->GenerateMergingCode(printer);
+    const FieldDescriptor* field = descriptor_->field(i);
+    if (field->real_containing_oneof()) {
+      continue;
     }
-  }
-  // Merge oneof fields
-  for (int i = 0; i < descriptor_->oneof_decl_count(); ++i) {
-    vars["name"] = UnderscoresToCamelCase(descriptor_->oneof_decl(i)->name(), false);
-    vars["property_name"] = UnderscoresToCamelCase(descriptor_->oneof_decl(i)->name(), true);
+    std::unique_ptr<FieldGeneratorBase> generator(CreateFieldGeneratorInternal(field));
+    generator->GenerateMergingCode(printer);
+  }
+  // Merge oneof fields (for non-synthetic oneofs)
+  for (int i = 0; i < descriptor_->real_oneof_decl_count(); ++i) {
+    const OneofDescriptor* oneof = descriptor_->oneof_decl(i);
+    vars["name"] = UnderscoresToCamelCase(oneof->name(), false);
+    vars["property_name"] = UnderscoresToCamelCase(oneof->name(), true);
     printer->Print(vars, "switch (other.$property_name$Case) {\n");
     printer->Indent();
-    for (int j = 0; j < descriptor_->oneof_decl(i)->field_count(); j++) {
-      const FieldDescriptor* field = descriptor_->oneof_decl(i)->field(j);
+    for (int j = 0; j < oneof->field_count(); j++) {
+      const FieldDescriptor* field = oneof->field(j);
       vars["field_property_name"] = GetPropertyName(field);
       printer->Print(
         vars,
@@ -698,8 +700,7 @@ void MessageGenerator::GenerateMergingMethods(io::Printer* printer) {
 
 // it's a waste of space to track presence for all values, so we only track them if they're not nullable
 int MessageGenerator::GetPresenceIndex(const FieldDescriptor* descriptor) {
-  if (IsNullable(descriptor) || !IsProto2(descriptor->file()) ||
-      descriptor->is_extension()) {
+  if (!RequiresPresenceBit(descriptor)) {
     return -1;
   }
 
@@ -709,7 +710,7 @@ int MessageGenerator::GetPresenceIndex(const FieldDescriptor* descriptor) {
     if (field == descriptor) {
       return index;
     }
-    if (!IsNullable(field)) {
+    if (RequiresPresenceBit(field)) {
       index++;
     }
   }

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

@@ -53,7 +53,7 @@ MessageFieldGenerator::MessageFieldGenerator(const FieldDescriptor* descriptor,
                                              int presenceIndex,
                                              const Options *options)
     : FieldGeneratorBase(descriptor, presenceIndex, options) {
-  if (!IsProto2(descriptor_->file())) {
+  if (!SupportsPresenceApi(descriptor_)) {
     variables_["has_property_check"] = name() + "_ != null";
     variables_["has_not_property_check"] = name() + "_ == null";
   }
@@ -77,7 +77,7 @@ void MessageFieldGenerator::GenerateMembers(io::Printer* printer) {
     "    $name$_ = value;\n"
     "  }\n"
     "}\n");
-  if (IsProto2(descriptor_->file())) {
+  if (SupportsPresenceApi(descriptor_)) {
     printer->Print(
       variables_,
       "/// <summary>Gets whether the $descriptor_name$ field is set</summary>\n");
@@ -228,7 +228,7 @@ void MessageOneofFieldGenerator::GenerateMembers(io::Printer* printer) {
     "    $oneof_name$Case_ = value == null ? $oneof_property_name$OneofCase.None : $oneof_property_name$OneofCase.$property_name$;\n"
     "  }\n"
     "}\n");
-  if (IsProto2(descriptor_->file())) {
+  if (SupportsPresenceApi(descriptor_)) {
     printer->Print(
       variables_,
       "/// <summary>Gets whether the \"$descriptor_name$\" field is set</summary>\n");

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

@@ -53,7 +53,7 @@ PrimitiveFieldGenerator::PrimitiveFieldGenerator(
   // TODO(jonskeet): Make this cleaner...
   is_value_type = descriptor->type() != FieldDescriptor::TYPE_STRING
       && descriptor->type() != FieldDescriptor::TYPE_BYTES;
-  if (!is_value_type && !IsProto2(descriptor_->file())) {
+  if (!is_value_type && !SupportsPresenceApi(descriptor_)) {
     variables_["has_property_check"] = variables_["property_name"] + ".Length != 0";
     variables_["other_has_property_check"] = "other." + variables_["property_name"] + ".Length != 0";
   }
@@ -63,42 +63,65 @@ PrimitiveFieldGenerator::~PrimitiveFieldGenerator() {
 }
 
 void PrimitiveFieldGenerator::GenerateMembers(io::Printer* printer) {
-  // TODO(jonskeet): Work out whether we want to prevent the fields from ever being
-  // null, or whether we just handle it, in the cases of bytes and string.
-  // (Basically, should null-handling code be in the getter or the setter?)
+  
+  // Note: in multiple places, this code assumes that all fields
+  // that support presence are either nullable, or use a presence field bit.
+  // Fields which are oneof members are not generated here; they're generated in PrimitiveOneofFieldGenerator below.
+  // Extensions are not generated here either.
+
+
+  // Proto2 allows different default values to be specified. These are retained
+  // via static fields. They don't particularly need to be, but we don't need
+  // to change that. In Proto3 the default value we don't generate these
+  // fields, just using the literal instead.
   if (IsProto2(descriptor_->file())) {
+    // Note: "private readonly static" isn't as idiomatic as
+    // "private static readonly", but changing this now would create a lot of
+    // churn in generated code with near-to-zero benefit.
     printer->Print(
       variables_,
       "private readonly static $type_name$ $property_name$DefaultValue = $default_value$;\n\n");
+    variables_["default_value_access"] =
+      variables_["property_name"] + "DefaultValue";
+  } else {
+    variables_["default_value_access"] = variables_["default_value"];
   }
 
+  // Declare the field itself.
   printer->Print(
     variables_,
     "private $type_name$ $name_def_message$;\n");
 
   WritePropertyDocComment(printer, descriptor_);
   AddPublicMemberAttributes(printer);
-  if (IsProto2(descriptor_->file())) {
-    if (presenceIndex_ == -1) {
+
+  // Most of the work is done in the property:
+  // Declare the property itself (the same for all options)
+  printer->Print(variables_, "$access_level$ $type_name$ $property_name$ {\n");
+
+  // Specify the "getter", which may need to check for a presence field.
+  if (SupportsPresenceApi(descriptor_)) {
+    if (IsNullable(descriptor_)) {
       printer->Print(
         variables_,
-        "$access_level$ $type_name$ $property_name$ {\n"
-        "  get { return $name$_ ?? $property_name$DefaultValue; }\n"
-        "  set {\n");
+        "  get { return $name$_ ?? $default_value_access$; }\n");
     } else {
       printer->Print(
         variables_,
-        "$access_level$ $type_name$ $property_name$ {\n"
-        "  get { if ($has_field_check$) { return $name$_; } else { return $property_name$DefaultValue; } }\n"
-        "  set {\n");
+        // Note: it's possible that this could be rewritten as a
+        // conditional ?: expression, but there's no significant benefit
+        // to changing it.
+        "  get { if ($has_field_check$) { return $name$_; } else { return $default_value_access$; } }\n");
     }
   } else {
     printer->Print(
       variables_,
-      "$access_level$ $type_name$ $property_name$ {\n"
-      "  get { return $name$_; }\n"
-      "  set {\n");
+      "  get { return $name$_; }\n");
   }
+
+  // Specify the "setter", which may need to set a field bit as well as the
+  // value.
+  printer->Print("  set {\n");
   if (presenceIndex_ != -1) {
     printer->Print(
       variables_,
@@ -116,8 +139,11 @@ void PrimitiveFieldGenerator::GenerateMembers(io::Printer* printer) {
   printer->Print(
     "  }\n"
     "}\n");
-  if (IsProto2(descriptor_->file())) {
-    printer->Print(variables_, "/// <summary>Gets whether the \"$descriptor_name$\" field is set</summary>\n");
+
+  // The "HasFoo" property, where required.
+  if (SupportsPresenceApi(descriptor_)) {
+    printer->Print(variables_,
+      "/// <summary>Gets whether the \"$descriptor_name$\" field is set</summary>\n");
     AddPublicMemberAttributes(printer);
     printer->Print(
       variables_,
@@ -133,8 +159,11 @@ void PrimitiveFieldGenerator::GenerateMembers(io::Printer* printer) {
         "$has_field_check$; }\n}\n");
     }
   }
-  if (IsProto2(descriptor_->file())) {
-    printer->Print(variables_, "/// <summary>Clears the value of the \"$descriptor_name$\" field</summary>\n");
+
+  // The "ClearFoo" method, where required.
+  if (SupportsPresenceApi(descriptor_)) {
+    printer->Print(variables_,
+      "/// <summary>Clears the value of the \"$descriptor_name$\" field</summary>\n");
     AddPublicMemberAttributes(printer);
     printer->Print(
       variables_,
@@ -270,7 +299,7 @@ void PrimitiveOneofFieldGenerator::GenerateMembers(io::Printer* printer) {
     "    $oneof_name$Case_ = $oneof_property_name$OneofCase.$property_name$;\n"
     "  }\n"
     "}\n");
-  if (IsProto2(descriptor_->file())) {
+  if (SupportsPresenceApi(descriptor_)) {
     printer->Print(
       variables_,
       "/// <summary>Gets whether the \"$descriptor_name$\" field is set</summary>\n");

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

@@ -81,7 +81,7 @@ void WrapperFieldGenerator::GenerateMembers(io::Printer* printer) {
     "    $name$_ = value;\n"
     "  }\n"
     "}\n\n");
-  if (IsProto2(descriptor_->file())) {
+  if (SupportsPresenceApi(descriptor_)) {
     printer->Print(
       variables_,
       "/// <summary>Gets whether the $descriptor_name$ field is set</summary>\n");
@@ -219,7 +219,7 @@ void WrapperOneofFieldGenerator::GenerateMembers(io::Printer* printer) {
     "    $oneof_name$Case_ = value == null ? $oneof_property_name$OneofCase.None : $oneof_property_name$OneofCase.$property_name$;\n"
     "  }\n"
     "}\n");
-  if (IsProto2(descriptor_->file())) {
+  if (SupportsPresenceApi(descriptor_)) {
     printer->Print(
       variables_,
       "/// <summary>Gets whether the \"$descriptor_name$\" field is set</summary>\n");