Optional-ness in reflection (#6097)

* Optional scalars in reflection

* fixed name collision

* Remove code duplicated by merge

Co-authored-by: Casper Neo <cneo@google.com>

include/flatbuffers/reflection_generated.h

@@ -511,7 +511,8 @@ struct Field FLATBUFFERS_FINAL_CLASS : private flatbuffers::Table {
     VT_REQUIRED = 18,
     VT_KEY = 20,
     VT_ATTRIBUTES = 22,
-    VT_DOCUMENTATION = 24
+    VT_DOCUMENTATION = 24,
+    VT_OPTIONAL = 26
   };
   const flatbuffers::String *name() const {
     return GetPointer<const flatbuffers::String *>(VT_NAME);
@@ -552,6 +553,9 @@ struct Field FLATBUFFERS_FINAL_CLASS : private flatbuffers::Table {
   const flatbuffers::Vector<flatbuffers::Offset<flatbuffers::String>> *documentation() const {
     return GetPointer<const flatbuffers::Vector<flatbuffers::Offset<flatbuffers::String>> *>(VT_DOCUMENTATION);
   }
+  bool optional() const {
+    return GetField<uint8_t>(VT_OPTIONAL, 0) != 0;
+  }
   bool Verify(flatbuffers::Verifier &verifier) const {
     return VerifyTableStart(verifier) &&
            VerifyOffsetRequired(verifier, VT_NAME) &&
@@ -571,6 +575,7 @@ struct Field FLATBUFFERS_FINAL_CLASS : private flatbuffers::Table {
            VerifyOffset(verifier, VT_DOCUMENTATION) &&
            verifier.VerifyVector(documentation()) &&
            verifier.VerifyVectorOfStrings(documentation()) &&
+           VerifyField<uint8_t>(verifier, VT_OPTIONAL) &&
            verifier.EndTable();
   }
 };
@@ -612,6 +617,9 @@ struct FieldBuilder {
   void add_documentation(flatbuffers::Offset<flatbuffers::Vector<flatbuffers::Offset<flatbuffers::String>>> documentation) {
     fbb_.AddOffset(Field::VT_DOCUMENTATION, documentation);
   }
+  void add_optional(bool optional) {
+    fbb_.AddElement<uint8_t>(Field::VT_OPTIONAL, static_cast<uint8_t>(optional), 0);
+  }
   explicit FieldBuilder(flatbuffers::FlatBufferBuilder &_fbb)
         : fbb_(_fbb) {
     start_ = fbb_.StartTable();
@@ -637,7 +645,8 @@ inline flatbuffers::Offset<Field> CreateField(
     bool required = false,
     bool key = false,
     flatbuffers::Offset<flatbuffers::Vector<flatbuffers::Offset<reflection::KeyValue>>> attributes = 0,
-    flatbuffers::Offset<flatbuffers::Vector<flatbuffers::Offset<flatbuffers::String>>> documentation = 0) {
+    flatbuffers::Offset<flatbuffers::Vector<flatbuffers::Offset<flatbuffers::String>>> documentation = 0,
+    bool optional = false) {
   FieldBuilder builder_(_fbb);
   builder_.add_default_real(default_real);
   builder_.add_default_integer(default_integer);
@@ -647,6 +656,7 @@ inline flatbuffers::Offset<Field> CreateField(
   builder_.add_name(name);
   builder_.add_offset(offset);
   builder_.add_id(id);
+  builder_.add_optional(optional);
   builder_.add_key(key);
   builder_.add_required(required);
   builder_.add_deprecated(deprecated);
@@ -665,7 +675,8 @@ inline flatbuffers::Offset<Field> CreateFieldDirect(
     bool required = false,
     bool key = false,
     std::vector<flatbuffers::Offset<reflection::KeyValue>> *attributes = nullptr,
-    const std::vector<flatbuffers::Offset<flatbuffers::String>> *documentation = nullptr) {
+    const std::vector<flatbuffers::Offset<flatbuffers::String>> *documentation = nullptr,
+    bool optional = false) {
   auto name__ = name ? _fbb.CreateString(name) : 0;
   auto attributes__ = attributes ? _fbb.CreateVectorOfSortedTables<reflection::KeyValue>(attributes) : 0;
   auto documentation__ = documentation ? _fbb.CreateVector<flatbuffers::Offset<flatbuffers::String>>(*documentation) : 0;
@@ -681,7 +692,8 @@ inline flatbuffers::Offset<Field> CreateFieldDirect(
       required,
       key,
       attributes__,
-      documentation__);
+      documentation__,
+      optional);
 }
 
 struct Object FLATBUFFERS_FINAL_CLASS : private flatbuffers::Table {

reflection/reflection.fbs

@@ -74,6 +74,7 @@ table Field {
     key:bool = false;
     attributes:[KeyValue];
     documentation:[string];
+    optional:bool = false;
 }
 
 table Object {  // Used for both tables and structs.

src/idl_gen_swift.cpp

@@ -503,7 +503,8 @@ class SwiftGenerator : public BaseGenerator {
     auto &create_func_header = *create_header;
     auto name = Name(field);
     auto type = GenType(field.value.type);
-    auto nullable_type = (field.optional ? type + "?" : type);
+    bool opt_scalar = field.optional && IsScalar(field.value.type.base_type);
+    auto nullable_type = opt_scalar ? type + "?" : type;
     code_.SetValue("VALUENAME", name);
     code_.SetValue("VALUETYPE", nullable_type);
     code_.SetValue("OFFSET", name);
@@ -590,9 +591,10 @@ class SwiftGenerator : public BaseGenerator {
     code_.SetValue("VALUETYPE", type);
     code_.SetValue("OFFSET", name);
     code_.SetValue("CONSTANT", field.value.constant);
-    std::string nullable = field.optional ? "nil" : "{{CONSTANT}}";
-    std::string optional = field.optional ? "?" : "";
-    auto const_string = "return o == 0 ? " + nullable + " : ";
+    bool opt_scalar = field.optional && IsScalar(field.value.type.base_type);
+    std::string def_Val = opt_scalar ? "nil" : "{{CONSTANT}}";
+    std::string optional = opt_scalar ? "?" : "";
+    auto const_string = "return o == 0 ? " + def_Val + " : ";
     GenComment(field.doc_comment);
     if (IsScalar(field.value.type.base_type) && !IsEnum(field.value.type) &&
         !IsBool(field.value.type.base_type)) {

src/idl_parser.cpp

@@ -743,19 +743,6 @@ CheckedError Parser::ParseField(StructDef &struct_def) {
       return Error(
           "default values currently only supported for scalars in tables");
   }
-
-  // Mark the optional scalars. Note that a side effect of ParseSingleValue is
-  // fixing field->value.constant to null.
-  if (IsScalar(type.base_type)) {
-    field->optional = (field->value.constant == "null");
-    if (field->optional && !SupportsOptionalScalars()) {
-      return Error(
-        "Optional scalars are not yet supported in at least one the of "
-        "the specified programming languages."
-      );
-    }
-  }
-
   // Append .0 if the value has not it (skip hex and scientific floats).
   // This suffix needed for generated C++ code.
   if (IsFloat(type.base_type)) {
@@ -845,6 +832,23 @@ CheckedError Parser::ParseField(StructDef &struct_def) {
   field->required = field->attributes.Lookup("required") != nullptr;
   if (field->required && (struct_def.fixed || IsScalar(type.base_type)))
     return Error("only non-scalar fields in tables may be 'required'");
+
+  // Mark the optional scalars. Note that a side effect of ParseSingleValue is
+  // fixing field->value.constant to null.
+  if (IsScalar(type.base_type)) {
+    field->optional = (field->value.constant == "null");
+    if (field->optional && !SupportsOptionalScalars()) {
+      return Error(
+        "Optional scalars are not yet supported in at least one the of "
+        "the specified programming languages."
+      );
+    }
+  } else {
+    // For nonscalars, only required fields are non-optional.
+    // At least until https://github.com/google/flatbuffers/issues/6053
+    field->optional = !field->required;
+  }
+
   field->key = field->attributes.Lookup("key") != nullptr;
   if (field->key) {
     if (struct_def.has_key) return Error("only one field may be set as 'key'");
@@ -3307,7 +3311,7 @@ Offset<reflection::Field> FieldDef::Serialize(FlatBufferBuilder *builder,
       IsInteger(value.type.base_type) ? StringToInt(value.constant.c_str()) : 0,
       // result may be platform-dependent if underlying is float (not double)
       IsFloat(value.type.base_type) ? d : 0.0, deprecated, required, key,
-      attr__, docs__);
+      attr__, docs__, optional);
   // TODO: value.constant is almost always "0", we could save quite a bit of
   // space by sharing it. Same for common values of value.type.
 }

tests/test.cpp

@@ -900,6 +900,7 @@ void ReflectionTest(uint8_t *flatbuf, size_t length) {
   TEST_EQ_STR(hp_field.name()->c_str(), "hp");
   TEST_EQ(hp_field.id(), 2);
   TEST_EQ(hp_field.type()->base_type(), reflection::Short);
+
   auto friendly_field_ptr = fields->LookupByKey("friendly");
   TEST_NOTNULL(friendly_field_ptr);
   TEST_NOTNULL(friendly_field_ptr->attributes());
@@ -913,6 +914,12 @@ void ReflectionTest(uint8_t *flatbuf, size_t length) {
   TEST_NOTNULL(pos_table_ptr);
   TEST_EQ_STR(pos_table_ptr->name()->c_str(), "MyGame.Example.Vec3");
 
+  // Test nullability of fields: hp is a 0-default scalar, pos is a struct =>
+  // optional, and name is a required string => not optional.
+  TEST_EQ(hp_field.optional(), false);
+  TEST_EQ(pos_field_ptr->optional(), true);
+  TEST_EQ(fields->LookupByKey("name")->optional(), false);
+
   // Now use it to dynamically access a buffer.
   auto &root = *flatbuffers::GetAnyRoot(flatbuf);