Partial support for --ts-flat-files and --gen-all (google#7446)

* Partial support for --ts-flat-files and --gen-all

* Add generated code changes

* remove some debugging code left over

* missed grpc files

grpc/examples/ts/greeter/src/models/hello-reply.ts

@@ -7,7 +7,7 @@ import * as flatbuffers from 'flatbuffers';
 export class HelloReply {
   bb: flatbuffers.ByteBuffer|null = null;
   bb_pos = 0;
-__init(i:number, bb:flatbuffers.ByteBuffer):HelloReply {
+  __init(i:number, bb:flatbuffers.ByteBuffer):HelloReply {
   this.bb_pos = i;
   this.bb = bb;
   return this;

grpc/examples/ts/greeter/src/models/hello-request.ts

@@ -7,7 +7,7 @@ import * as flatbuffers from 'flatbuffers';
 export class HelloRequest {
   bb: flatbuffers.ByteBuffer|null = null;
   bb_pos = 0;
-__init(i:number, bb:flatbuffers.ByteBuffer):HelloRequest {
+  __init(i:number, bb:flatbuffers.ByteBuffer):HelloRequest {
   this.bb_pos = i;
   this.bb = bb;
   return this;

src/flatc.cpp

@@ -643,10 +643,6 @@ int FlatCompiler::Compile(int argc, const char **argv) {
         "well.");
   }
 
-  if (opts.ts_flat_file && opts.generate_all) {
-    Error("Combining --ts-flat-file and --gen-all is not supported.");
-  }
-
   flatbuffers::Parser conform_parser;
   if (!conform_to_schema.empty()) {
     std::string contents;

src/idl_gen_ts.cpp

@@ -38,7 +38,7 @@ struct ImportDefinition {
 };
 
 enum AnnotationType { kParam = 0, kType = 1, kReturns = 2 };
-}
+}  // namespace
 
 namespace ts {
 // Iterate through all definitions we haven't generate code for (enums, structs,
@@ -111,10 +111,6 @@ class TsGenerator : public BaseGenerator {
     for (auto kw = keywords; *kw; kw++) keywords_.insert(*kw);
   }
   bool generate() {
-    if (parser_.opts.ts_flat_file && parser_.opts.generate_all) {
-      // Not implemented; warning message should have been emitted by flatc.
-      return false;
-    }
     generateEnums();
     generateStructs();
     generateEntry();
@@ -227,39 +223,45 @@ class TsGenerator : public BaseGenerator {
 
   // Generate code for a single entry point module.
   void generateEntry() {
-    std::string code;
+    std::string code =
+        "// " + std::string(FlatBuffersGeneratedWarning()) + "\n\n";
     if (parser_.opts.ts_flat_file) {
       if (import_flatbuffers_lib_) {
         code += "import * as flatbuffers from 'flatbuffers';\n";
+        code += "\n";
       }
-      for (const auto &it : flat_file_import_declarations_) {
-        // Note that we do end up generating an import for ourselves, which
-        // should generally be harmless.
-        // TODO: Make it so we don't generate a self-import; this will also
-        // require modifying AddImport to ensure that we don't use
-        // namespace-prefixed names anywhere...
-        std::string file = it.first;
-        if (file.empty()) { continue; }
-        std::string noext = flatbuffers::StripExtension(file);
-        std::string basename = flatbuffers::StripPath(noext);
-        std::string include_file = GeneratedFileName(
-            parser_.opts.include_prefix,
-            parser_.opts.keep_prefix ? noext : basename, parser_.opts);
-        // TODO: what is the right behavior when different include flags are
-        // specified here? Should we always be adding the "./" for a relative
-        // path or turn it off if --include-prefix is specified, or something
-        // else?
-        std::string include_name =
-            "./" + flatbuffers::StripExtension(include_file);
-        code += "import {";
-        for (const auto &pair : it.second) {
-          code += EscapeKeyword(pair.first) + " as " +
-                  EscapeKeyword(pair.second) + ", ";
+      // Only include import statements when not generating all.
+      if (!parser_.opts.generate_all) {
+        for (const auto &it : flat_file_import_declarations_) {
+          // Note that we do end up generating an import for ourselves, which
+          // should generally be harmless.
+          // TODO: Make it so we don't generate a self-import; this will also
+          // require modifying AddImport to ensure that we don't use
+          // namespace-prefixed names anywhere...
+          std::string file = it.first;
+          if (file.empty()) { continue; }
+          std::string noext = flatbuffers::StripExtension(file);
+          std::string basename = flatbuffers::StripPath(noext);
+          std::string include_file = GeneratedFileName(
+              parser_.opts.include_prefix,
+              parser_.opts.keep_prefix ? noext : basename, parser_.opts);
+          // TODO: what is the right behavior when different include flags are
+          // specified here? Should we always be adding the "./" for a relative
+          // path or turn it off if --include-prefix is specified, or something
+          // else?
+          std::string include_name =
+              "./" + flatbuffers::StripExtension(include_file);
+          code += "import {";
+          for (const auto &pair : it.second) {
+            code += EscapeKeyword(pair.first) + " as " +
+                    EscapeKeyword(pair.second) + ", ";
+          }
+          code.resize(code.size() - 2);
+          code += "} from '" + include_name + "';\n";
         }
-        code.resize(code.size() - 2);
-        code += "} from '" + include_name + "';\n";
+        code += "\n";
       }
-      code += "\n\n";
+
       code += flat_file_;
       const std::string filename =
           GeneratedFileName(path_, file_name_, parser_.opts);
@@ -305,7 +307,9 @@ class TsGenerator : public BaseGenerator {
     if (reverse) return;  // FIXME.
     std::string &code = *code_ptr;
     GenDocComment(enum_def.doc_comment, code_ptr);
-    code += "export enum " + EscapeKeyword(enum_def.name) + "{\n";
+    code += "export enum ";
+    // TODO(7445): figure out if the export needs a namespace for ts-flat-files
+    code += EscapeKeyword(enum_def.name) + " {\n";
     for (auto it = enum_def.Vals().begin(); it != enum_def.Vals().end(); ++it) {
       auto &ev = **it;
       if (!ev.doc_comment.empty()) {
@@ -659,19 +663,14 @@ class TsGenerator : public BaseGenerator {
       // above, but since we force all non-self-imports to use namespace-based
       // names in flat file generation, it's fine).
       if (dependent.file == dependency.file) {
-        // Should already be caught elsewhere, but if we ever try to get flat
-        // file generation and --gen-all working concurrently, then we'll need
-        // to update this import logic.
-        FLATBUFFERS_ASSERT(!parser_.opts.generate_all);
         long_import_name = import_name;
       } else {
         long_import_name = ns + import_name;
-        std::string file = dependency.declaration_file == nullptr
-                               ? dependency.file
-                               : dependency.declaration_file->substr(2);
-        file = RelativeToRootPath(StripFileName(AbsolutePath(dependent.file)),
-                                  dependency.file)
-                   .substr(2);
+        std::string file =
+            RelativeToRootPath(StripFileName(AbsolutePath(dependent.file)),
+                               dependency.file)
+                // Strip the leading //
+                .substr(2);
         flat_file_import_declarations_[file][import_name] = long_import_name;
         if (parser_.opts.generate_object_based_api) {
           flat_file_import_declarations_[file][import_name + "T"] =
@@ -753,19 +752,14 @@ class TsGenerator : public BaseGenerator {
       // above, but since we force all non-self-imports to use namespace-based
       // names in flat file generation, it's fine).
       if (dependent.file == dependency.file) {
-        // Should already be caught elsewhere, but if we ever try to get flat
-        // file generation and --gen-all working concurrently, then we'll need
-        // to update this import logic.
-        FLATBUFFERS_ASSERT(!parser_.opts.generate_all);
         long_import_name = import_name;
       } else {
         long_import_name = ns + import_name;
-        std::string file = dependency.declaration_file == nullptr
-                               ? dependency.file
-                               : dependency.declaration_file->substr(2);
-        file = RelativeToRootPath(StripFileName(AbsolutePath(dependent.file)),
-                                  dependency.file)
-                   .substr(2);
+        std::string file =
+            RelativeToRootPath(StripFileName(AbsolutePath(dependent.file)),
+                               dependency.file)
+                // Strip the leading //
+                .substr(2);
         flat_file_import_declarations_[file][import_name] = long_import_name;
       }
     }
@@ -1347,15 +1341,17 @@ class TsGenerator : public BaseGenerator {
     // Emit constructor
     object_name = EscapeKeyword(struct_def.name);
     GenDocComment(struct_def.doc_comment, code_ptr);
-    code += "export class " + object_name;
+    code += "export class ";
+    // TODO(7445): figure out if the export needs a namespace for ts-flat-files
+    code += object_name;
     code += " {\n";
     code += "  bb: flatbuffers.ByteBuffer|null = null;\n";
     code += "  bb_pos = 0;\n";
 
     // Generate the __init method that sets the field in a pre-existing
     // accessor object. This is to allow object reuse.
     code +=
-        "__init(i:number, bb:flatbuffers.ByteBuffer):" + object_name + " {\n";
+        "  __init(i:number, bb:flatbuffers.ByteBuffer):" + object_name + " {\n";
     code += "  this.bb_pos = i;\n";
     code += "  this.bb = bb;\n";
     code += "  return this;\n";

tests/flatc/bar/baz/baz.fbs

@@ -1,3 +1,6 @@
-table Baz {
-  a:int;
+enum Baz : short {
+  None = 0,
+  Red,
+  Green,
+  Blue,
 }

tests/flatc/flatc_test.py

@@ -77,8 +77,22 @@ def assert_file_contains(file, needles):
     return file
 
 
-def assert_file_and_contents(file, needle, path=script_path, unlink=True):
+def assert_file_doesnt_contains(file, needles):
+    with open(file) as file:
+        contents = file.read()
+        for needle in [needles] if isinstance(needles, str) else needles:
+            assert needle not in contents, (
+                "Found unexpected '" + needle + "' in file: " + str(file)
+            )
+    return file
+
+
+def assert_file_and_contents(
+    file, needle, doesnt_contain=None, path=script_path, unlink=True
+):
     assert_file_contains(assert_file_exists(file, path), needle)
+    if doesnt_contain:
+        assert_file_doesnt_contains(assert_file_exists(file, path), doesnt_contain)
     if unlink:
         Path(path, file).unlink()
 
@@ -102,7 +116,7 @@ def run_all(*modules):
                 module_passing = module_passing + 1
             except Exception as e:
                 print(" [FAILED]: " + str(e))
-                failingmodule_failing = failingmodule_failing + 1
+                module_failing = module_failing + 1
         print(
             "{0}: {1} of {2} passsed".format(
                 module.__name__, module_passing, module_passing + module_failing