Improved .proto conversion.

- Allowed enums to be declared before use.
- Generalized parsing of `required`.
- Reworked escaping of namespaces.
- Escaping field names that are C++ keywords.
- Many smaller fixes.

Change-Id: Ie580de7b70dc208f676f4f71bb0d061808648b8d
Tested: on Linux.

include/flatbuffers/idl.h

@@ -199,7 +199,8 @@ template<typename T> class SymbolTable {
 
 // A name space, as set in the schema.
 struct Namespace {
-  std::vector<std::string> components;
+  Namespace() : from_table(0) {}
+
 
   // Given a (potentally unqualified) name, return the "fully qualified" name
   // which has a full namespaced descriptor.
@@ -207,12 +208,15 @@ struct Namespace {
   // the current namespace has.
   std::string GetFullyQualifiedName(const std::string &name,
                                     size_t max_components = 1000) const;
+
+  std::vector<std::string> components;
+  size_t from_table;  // Part of the namespace corresponds to a message/table.
 };
 
 // Base class for all definition types (fields, structs_, enums_).
 struct Definition {
   Definition() : generated(false), defined_namespace(nullptr),
-                 serialized_location(0), index(-1) {}
+                 serialized_location(0), index(-1), refcount(1) {}
 
   flatbuffers::Offset<flatbuffers::Vector<flatbuffers::Offset<
     reflection::KeyValue>>>
@@ -229,6 +233,7 @@ struct Definition {
   // For use with Serialize()
   uoffset_t serialized_location;
   int index;  // Inside the vector it is stored.
+  int refcount;
 };
 
 struct FieldDef : public Definition {
@@ -271,12 +276,15 @@ struct StructDef : public Definition {
                                        const Parser &parser) const;
 
   SymbolTable<FieldDef> fields;
+
   bool fixed;       // If it's struct, not a table.
   bool predecl;     // If it's used before it was defined.
   bool sortbysize;  // Whether fields come in the declaration or size order.
   bool has_key;     // It has a key field.
   size_t minalign;  // What the whole object needs to be aligned to.
   size_t bytesize;  // Size if fixed.
+
+  std::unique_ptr<std::string> original_location;
 };
 
 inline bool IsStruct(const Type &type) {
@@ -361,7 +369,6 @@ struct IDLOptions {
   bool generate_all;
   bool skip_unexpected_fields_in_json;
   bool generate_name_strings;
-  bool escape_proto_identifiers;
   bool generate_object_based_api;
   std::string cpp_object_api_pointer_type;
   std::string cpp_object_api_string_type;
@@ -417,7 +424,6 @@ struct IDLOptions {
       generate_all(false),
       skip_unexpected_fields_in_json(false),
       generate_name_strings(false),
-      escape_proto_identifiers(false),
       generate_object_based_api(false),
       cpp_object_api_pointer_type("std::unique_ptr"),
       object_suffix("T"),
@@ -568,7 +574,11 @@ class Parser : public ParserState {
 
   FLATBUFFERS_CHECKED_ERROR CheckInRange(int64_t val, int64_t min, int64_t max);
 
+  StructDef *LookupStruct(const std::string &id) const;
+
 private:
+  void Message(const std::string &msg);
+  void Warning(const std::string &msg);
   FLATBUFFERS_CHECKED_ERROR Error(const std::string &msg);
   FLATBUFFERS_CHECKED_ERROR ParseHexNum(int nibbles, uint64_t *val);
   FLATBUFFERS_CHECKED_ERROR Next();

src/flatc.cpp

@@ -85,7 +85,6 @@ std::string FlatCompiler::GetUsageString(const char* program_name) const {
       "  --gen-mutable      Generate accessors that can mutate buffers in-place.\n"
       "  --gen-onefile      Generate single output file for C#.\n"
       "  --gen-name-strings Generate type name functions for C++.\n"
-      "  --escape-proto-ids Disable appending '_' in namespaces names.\n"
       "  --gen-object-api   Generate an additional object-based API.\n"
       "  --cpp-ptr-type T   Set object API pointer type (default std::unique_ptr)\n"
       "  --cpp-str-type T   Set object API string type (default std::string)\n"
@@ -228,8 +227,6 @@ int FlatCompiler::Compile(int argc, const char** argv) {
         binary_files_from = filenames.size();
       } else if(arg == "--proto") {
         opts.proto_mode = true;
-      } else if(arg == "--escape-proto-ids") {
-        opts.escape_proto_identifiers = true;
       } else if(arg == "--schema") {
         schema_binary = true;
       } else if(arg == "-M") {

src/idl_gen_fbs.cpp

@@ -23,18 +23,20 @@
 
 namespace flatbuffers {
 
-static std::string GenType(const Type &type) {
+static std::string GenType(const Type &type, bool underlying = false) {
   switch (type.base_type) {
     case BASE_TYPE_STRUCT:
       return type.struct_def->defined_namespace->GetFullyQualifiedName(
                type.struct_def->name);
-    case BASE_TYPE_UNION:
-      return type.enum_def->defined_namespace->GetFullyQualifiedName(
-               type.enum_def->name);
     case BASE_TYPE_VECTOR:
         return "[" + GenType(type.VectorType()) + "]";
     default:
-        return kTypeNames[type.base_type];
+        if (type.enum_def && !underlying) {
+          return type.enum_def->defined_namespace->GetFullyQualifiedName(
+                   type.enum_def->name);
+        } else {
+          return kTypeNames[type.base_type];
+        }
   }
 }
 
@@ -54,14 +56,13 @@ static void GenNameSpace(const Namespace &name_space, std::string *_schema,
 
 // Generate a flatbuffer schema from the Parser's internal representation.
 std::string GenerateFBS(const Parser &parser, const std::string &file_name) {
- // Proto namespaces may clash with table names, so we have to prefix all:
-  if (!parser.opts.escape_proto_identifiers) {
-    for (auto it = parser.namespaces_.begin(); it != parser.namespaces_.end();
-         ++it) {
-      for (auto comp = (*it)->components.begin(); comp != (*it)->components.end();
-           ++comp) {
-        (*comp) = "_" + (*comp);
-      }
+  // Proto namespaces may clash with table names, escape the ones that were
+  // generated from a table:
+  for (auto it = parser.namespaces_.begin(); it != parser.namespaces_.end();
+       ++it) {
+    auto &ns = **it;
+    for (size_t i = 0; i < ns.from_table; i++) {
+      ns.components[ns.components.size() - 1 - i] += "_";
     }
   }
 
@@ -90,7 +91,7 @@ std::string GenerateFBS(const Parser &parser, const std::string &file_name) {
     GenNameSpace(*enum_def.defined_namespace, &schema, &last_namespace);
     GenComment(enum_def.doc_comment, &schema, nullptr);
     schema += "enum " + enum_def.name + " : ";
-    schema += GenType(enum_def.underlying_type) + " {\n";
+    schema += GenType(enum_def.underlying_type, true) + " {\n";
     for (auto it = enum_def.vals.vec.begin();
          it != enum_def.vals.vec.end(); ++it) {
       auto &ev = **it;
@@ -109,11 +110,13 @@ std::string GenerateFBS(const Parser &parser, const std::string &file_name) {
     for (auto field_it = struct_def.fields.vec.begin();
              field_it != struct_def.fields.vec.end(); ++field_it) {
       auto &field = **field_it;
-      GenComment(field.doc_comment, &schema, nullptr, "  ");
-      schema += "  " + field.name + ":" + GenType(field.value.type);
-      if (field.value.constant != "0") schema += " = " + field.value.constant;
-      if (field.required) schema += " (required)";
-      schema += ";\n";
+      if (field.value.type.base_type != BASE_TYPE_UTYPE) {
+        GenComment(field.doc_comment, &schema, nullptr, "  ");
+        schema += "  " + field.name + ":" + GenType(field.value.type);
+        if (field.value.constant != "0") schema += " = " + field.value.constant;
+        if (field.required) schema += " (required)";
+        schema += ";\n";
+      }
     }
     schema += "}\n\n";
   }

src/idl_parser.cpp

@@ -75,7 +75,7 @@ static bool ValidateUTF8(const std::string &str) {
   return true;
 }
 
-CheckedError Parser::Error(const std::string &msg) {
+void Parser::Message(const std::string &msg) {
   error_ = file_being_parsed_.length() ? AbsolutePath(file_being_parsed_) : "";
   #ifdef _WIN32
     error_ += "(" + NumToString(line_) + ")";  // MSVC alike
@@ -83,7 +83,15 @@ CheckedError Parser::Error(const std::string &msg) {
     if (file_being_parsed_.length()) error_ += ":";
     error_ += NumToString(line_) + ":0";  // gcc alike
   #endif
-  error_ += ": error: " + msg;
+  error_ += ": " + msg;
+}
+
+void Parser::Warning(const std::string &msg) {
+  Message("warning: " + msg);
+}
+
+CheckedError Parser::Error(const std::string &msg) {
+  Message("error: " + msg);
   return CheckedError(true);
 }
 
@@ -454,6 +462,12 @@ EnumDef *Parser::LookupEnum(const std::string &id) {
   return nullptr;
 }
 
+StructDef *Parser::LookupStruct(const std::string &id) const {
+  auto sd = structs_.Lookup(id);
+  if (sd) sd->refcount++;
+  return sd;
+}
+
 CheckedError Parser::ParseTypeIdent(Type &type) {
   std::string id = attribute_;
   EXPECT(kTokenIdentifier);
@@ -557,7 +571,7 @@ CheckedError Parser::AddField(StructDef &struct_def, const std::string &name,
 CheckedError Parser::ParseField(StructDef &struct_def) {
   std::string name = attribute_;
 
-  if (structs_.Lookup(name))
+  if (LookupStruct(name))
     return Error("field name can not be the same as table/struct name");
 
   std::vector<std::string> dc = doc_comment_;
@@ -610,7 +624,7 @@ CheckedError Parser::ParseField(StructDef &struct_def) {
       !type.enum_def->attributes.Lookup("bit_flags") &&
       !type.enum_def->ReverseLookup(static_cast<int>(
                          StringToInt(field->value.constant.c_str()))))
-    return Error("enum " + type.enum_def->name +
+    Warning("enum " + type.enum_def->name +
           " does not have a declaration for this field\'s default of " +
           field->value.constant);
 
@@ -682,7 +696,7 @@ CheckedError Parser::ParseField(StructDef &struct_def) {
     // Keep a pointer to StructDef in FieldDef to simplify re-use later
     auto nested_qualified_name =
         current_namespace_->GetFullyQualifiedName(nested->constant);
-    field->nested_flatbuffer = structs_.Lookup(nested_qualified_name);
+    field->nested_flatbuffer = LookupStruct(nested_qualified_name);
   }
 
   if (field->attributes.Lookup("flexbuffer")) {
@@ -1326,7 +1340,7 @@ StructDef *Parser::LookupCreateStruct(const std::string &name,
                                       bool create_if_new, bool definition) {
   std::string qualified_name = current_namespace_->GetFullyQualifiedName(name);
   // See if it exists pre-declared by an unqualified use.
-  auto struct_def = structs_.Lookup(name);
+  auto struct_def = LookupStruct(name);
   if (struct_def && struct_def->predecl) {
     if (definition) {
       // Make sure it has the current namespace, and is registered under its
@@ -1337,7 +1351,7 @@ StructDef *Parser::LookupCreateStruct(const std::string &name,
     return struct_def;
   }
   // See if it exists pre-declared by an qualified use.
-  struct_def = structs_.Lookup(qualified_name);
+  struct_def = LookupStruct(qualified_name);
   if (struct_def && struct_def->predecl) {
     if (definition) {
       // Make sure it has the current namespace.
@@ -1349,7 +1363,7 @@ StructDef *Parser::LookupCreateStruct(const std::string &name,
     // Search thru parent namespaces.
     for (size_t components = current_namespace_->components.size();
          components && !struct_def; components--) {
-      struct_def = structs_.Lookup(
+      struct_def = LookupStruct(
           current_namespace_->GetFullyQualifiedName(name, components - 1));
     }
   }
@@ -1363,12 +1377,13 @@ StructDef *Parser::LookupCreateStruct(const std::string &name,
       // Not a definition.
       // Rather than failing, we create a "pre declared" StructDef, due to
       // circular references, and check for errors at the end of parsing.
-      // It is defined in the root namespace, since we don't know what the
+      // It is defined in the current namespace, as the best guess what the
       // final namespace will be.
-      // TODO: maybe safer to use special namespace?
       structs_.Add(name, struct_def);
       struct_def->name = name;
-      struct_def->defined_namespace = empty_namespace_;
+      struct_def->defined_namespace = current_namespace_;
+      struct_def->original_location.reset(new std::string(file_being_parsed_ +
+                                                     ":" + NumToString(line_)));
     }
   }
   return struct_def;
@@ -1655,9 +1670,9 @@ CheckedError Parser::ParseService() {
 }
 
 bool Parser::SetRootType(const char *name) {
-  root_struct_def_ = structs_.Lookup(name);
+  root_struct_def_ = LookupStruct(name);
   if (!root_struct_def_)
-    root_struct_def_ = structs_.Lookup(
+    root_struct_def_ = LookupStruct(
                          current_namespace_->GetFullyQualifiedName(name));
   return root_struct_def_ != nullptr;
 }
@@ -1735,6 +1750,7 @@ CheckedError Parser::ParseProtoDecl() {
       *ns = *current_namespace_;
       // But with current message name.
       ns->components.push_back(name);
+      ns->from_table++;
       parent_namespace = current_namespace_;
       current_namespace_ = UniqueNamespace(ns);
     }
@@ -1797,9 +1813,8 @@ CheckedError Parser::ParseProtoFields(StructDef *struct_def, bool isextend,
       EXPECT(';');
     } else if (IsIdent("reserved")) {  // Skip these.
       NEXT();
-      EXPECT(kTokenIntegerConstant);
-      while (Is(',')) { NEXT(); EXPECT(kTokenIntegerConstant); }
-      EXPECT(';');
+      while (!Is(';')) { NEXT(); }  // A variety of formats, just skip.
+      NEXT();
     } else {
       std::vector<std::string> field_comment = doc_comment_;
       // Parse the qualifier.
@@ -1837,6 +1852,13 @@ CheckedError Parser::ParseProtoFields(StructDef *struct_def, bool isextend,
       if (repeated) {
         type.element = type.base_type;
         type.base_type = BASE_TYPE_VECTOR;
+        if (type.element == BASE_TYPE_VECTOR) {
+          // We have a vector or vectors, which FlatBuffers doesn't support.
+          // For now make it a vector of string (since the source is likely
+          // "repeated bytes").
+          // TODO(wvo): A better solution would be to wrap this in a table.
+          type.element = BASE_TYPE_STRING;
+        }
       }
       std::string name = attribute_;
       EXPECT(kTokenIdentifier);
@@ -2088,11 +2110,63 @@ CheckedError Parser::ParseRoot(const char *source, const char **include_paths,
   ECHECK(DoParse(source, include_paths, source_filename, nullptr));
 
   // Check that all types were defined.
-  for (auto it = structs_.vec.begin(); it != structs_.vec.end(); ++it) {
-    if ((*it)->predecl) {
-      return Error("type referenced but not defined (check namespace): " +
-                   (*it)->name);
+  for (auto it = structs_.vec.begin(); it != structs_.vec.end(); ) {
+    auto &struct_def = **it;
+    if (struct_def.predecl) {
+      if (opts.proto_mode) {
+        // Protos allow enums to be used before declaration, so check if that
+        // is the case here.
+        EnumDef *enum_def = nullptr;
+        for (size_t components = struct_def.defined_namespace->
+                                   components.size() + 1;
+             components && !enum_def; components--) {
+          auto qualified_name = struct_def.defined_namespace->
+                                  GetFullyQualifiedName(struct_def.name,
+                                                        components - 1);
+          enum_def = LookupEnum(qualified_name);
+        }
+        if (enum_def) {
+          // This is pretty slow, but a simple solution for now.
+          auto initial_count = struct_def.refcount;
+          for (auto struct_it = structs_.vec.begin();
+                    struct_it != structs_.vec.end();
+                    ++struct_it) {
+            auto &sd = **struct_it;
+            for (auto field_it = sd.fields.vec.begin();
+                      field_it != sd.fields.vec.end();
+                      ++field_it) {
+              auto &field = **field_it;
+              if (field.value.type.struct_def == &struct_def) {
+                field.value.type.struct_def = nullptr;
+                field.value.type.enum_def = enum_def;
+                auto &bt = field.value.type.base_type == BASE_TYPE_VECTOR
+                           ? field.value.type.element
+                           : field.value.type.base_type;
+                assert(bt == BASE_TYPE_STRUCT);
+                bt = enum_def->underlying_type.base_type;
+                struct_def.refcount--;
+                enum_def->refcount++;
+              }
+            }
+          }
+          if (struct_def.refcount)
+            return Error("internal: " + NumToString(struct_def.refcount) + "/" +
+                         NumToString(initial_count) +
+                         " use(s) of pre-declaration enum not accounted for: "
+                         + enum_def->name);
+          structs_.dict.erase(structs_.dict.find(struct_def.name));
+          it = structs_.vec.erase(it);
+          delete &struct_def;
+          continue;  // Skip error.
+        }
+      }
+      auto err = "type referenced but not defined (check namespace): " +
+                 struct_def.name;
+      if (struct_def.original_location)
+        err += ", originally at: " + *struct_def.original_location;
+      return Error(err);
     }
+    ++it;
   }
 
   // This check has to happen here and not earlier, because only now do we
@@ -2433,7 +2507,7 @@ std::string Parser::ConformTo(const Parser &base) {
     auto &struct_def = **sit;
     auto qualified_name =
         struct_def.defined_namespace->GetFullyQualifiedName(struct_def.name);
-    auto struct_def_base = base.structs_.Lookup(qualified_name);
+    auto struct_def_base = base.LookupStruct(qualified_name);
     if (!struct_def_base) continue;
     for (auto fit = struct_def.fields.vec.begin();
              fit != struct_def.fields.vec.end(); ++fit) {

tests/prototest/test.golden

 // Generated from test.proto
 
-namespace _proto._test;
+namespace proto.test;
 
 /// Enum doc comment.
 enum ProtoEnum : int {
@@ -33,12 +33,12 @@ table ProtoMessage {
   /// lines
   l:string (required);
   m:[ubyte];
-  n:_proto._test._ProtoMessage.OtherMessage;
+  n:proto.test.ProtoMessage_.OtherMessage;
   o:[string];
-  z:_proto._test.ImportedMessage;
+  z:proto.test.ImportedMessage;
 }
 
-namespace _proto._test._ProtoMessage;
+namespace proto.test.ProtoMessage_;
 
 table OtherMessage {
   a:double;