Update ruby_generator.cc to allow proto2 imports in proto3 (#9003)

* Update ruby_generator.cc to allow proto2 imports in proto3, with updated unit tests

* Update Makefile.am with new ruby_generated_code_proto2_import.proto

* Fix ruby_generator unit test to use temporary test directory for imported protos

* Add test for imported proto2 to ruby/tests

* Fix proto_path, restore to ../src/protoc, and fix/cleanup unit test.

* Rename Proto2TestMessage to TestImportedMessage for consistency, for ruby compiler tests

ruby/Rakefile

@@ -61,7 +61,7 @@ unless ENV['IN_DOCKER'] == 'true'
     output_file = proto_file.sub(/\.proto$/, "_pb.rb")
     genproto_output << output_file
     file output_file => proto_file do |file_task|
-      sh "#{protoc_command} -I../src -I. --ruby_out=. #{proto_file}"
+      sh "#{protoc_command} -I../src -I. -I./tests --ruby_out=.  #{proto_file}"
     end
   end
 end

ruby/tests/basic.rb

@@ -168,6 +168,17 @@ module BasicTest
       assert_equal nil, m.singular_msg
     end
 
+    def test_import_proto2
+      m = TestMessage.new
+      assert !m.has_optional_proto2_submessage?
+      m.optional_proto2_submessage = ::FooBar::Proto2::TestImportedMessage.new
+      assert m.has_optional_proto2_submessage?
+      assert TestMessage.descriptor.lookup('optional_proto2_submessage').has?(m)
+
+      m.clear_optional_proto2_submessage
+      assert !m.has_optional_proto2_submessage?
+    end
+
     def test_clear_repeated_fields
       m = TestMessage.new
 
@@ -487,6 +498,7 @@ module BasicTest
         :optional_int64=>0,
         :optional_msg=>nil,
         :optional_msg2=>nil,
+        :optional_proto2_submessage=>nil,
         :optional_string=>"foo",
         :optional_uint32=>0,
         :optional_uint64=>0,

ruby/tests/basic_test.proto

@@ -6,6 +6,7 @@ import "google/protobuf/wrappers.proto";
 import "google/protobuf/timestamp.proto";
 import "google/protobuf/duration.proto";
 import "google/protobuf/struct.proto";
+import "test_import_proto2.proto";
 
 message Foo {
   Bar bar = 1;
@@ -32,6 +33,7 @@ message TestMessage {
   optional bytes optional_bytes = 9;
   optional TestMessage2 optional_msg = 10;
   optional TestEnum optional_enum = 11;
+  optional foo_bar.proto2.TestImportedMessage optional_proto2_submessage = 24;
 
   repeated int32 repeated_int32 = 12;
   repeated int64 repeated_int64 = 13;

src/Makefile.am

@@ -546,6 +546,7 @@ EXTRA_DIST =                                                   \
   google/protobuf/compiler/package_info.h                      \
   google/protobuf/compiler/ruby/ruby_generated_code.proto      \
   google/protobuf/compiler/ruby/ruby_generated_code_pb.rb      \
+  google/protobuf/compiler/ruby/ruby_generated_code_proto2_import.proto         \
   google/protobuf/compiler/ruby/ruby_generated_code_proto2.proto         \
   google/protobuf/compiler/ruby/ruby_generated_code_proto2_pb.rb         \
   google/protobuf/compiler/ruby/ruby_generated_pkg_explicit.proto        \

src/google/protobuf/compiler/ruby/ruby_generated_code.proto

@@ -2,6 +2,8 @@ syntax = "proto3";
 
 package A.B.C;
 
+import "ruby_generated_code_proto2_import.proto";
+
 message TestMessage {
   int32 optional_int32 = 1;
   int64 optional_int64 = 2;
@@ -14,6 +16,7 @@ message TestMessage {
   bytes optional_bytes = 9;
   TestEnum optional_enum = 10;
   TestMessage optional_msg = 11;
+  TestImportedMessage optional_proto2_submessage = 12;
 
   repeated int32 repeated_int32 = 21;
   repeated int64 repeated_int64 = 22;

src/google/protobuf/compiler/ruby/ruby_generated_code_pb.rb

@@ -1,6 +1,7 @@
 # Generated by the protocol buffer compiler.  DO NOT EDIT!
 # source: ruby_generated_code.proto
 
+require 'ruby_generated_code_proto2_import_pb'
 require 'google/protobuf'
 
 Google::Protobuf::DescriptorPool.generated_pool.build do
@@ -17,6 +18,7 @@ Google::Protobuf::DescriptorPool.generated_pool.build do
       optional :optional_bytes, :bytes, 9
       optional :optional_enum, :enum, 10, "A.B.C.TestEnum"
       optional :optional_msg, :message, 11, "A.B.C.TestMessage"
+      optional :optional_proto2_submessage, :message, 12, "A.B.C.TestImportedMessage"
       repeated :repeated_int32, :int32, 21
       repeated :repeated_int64, :int64, 22
       repeated :repeated_uint32, :uint32, 23

src/google/protobuf/compiler/ruby/ruby_generated_code_proto2.proto

@@ -2,6 +2,8 @@ syntax = "proto2";
 
 package A.B.C;
 
+import "ruby_generated_code_proto2_import.proto";
+
 message TestMessage {
   optional int32 optional_int32 = 1 [default = 1];
   optional int64 optional_int64 = 2 [default = 2];
@@ -14,6 +16,7 @@ message TestMessage {
   optional bytes optional_bytes = 9 [default = "\0\1\2\100fubar"];
   optional TestEnum optional_enum = 10 [default = A];
   optional TestMessage optional_msg = 11;
+  optional TestImportedMessage optional_proto2_submessage = 12;
 
   repeated int32 repeated_int32 = 21;
   repeated int64 repeated_int64 = 22;

src/google/protobuf/compiler/ruby/ruby_generated_code_proto2_import.proto

@@ -0,0 +1,5 @@
+syntax = "proto2";
+
+package A.B.C;
+
+message TestImportedMessage {}

src/google/protobuf/compiler/ruby/ruby_generated_code_proto2_pb.rb

@@ -1,6 +1,7 @@
 # Generated by the protocol buffer compiler.  DO NOT EDIT!
 # source: ruby_generated_code_proto2.proto
 
+require 'ruby_generated_code_proto2_import_pb'
 require 'google/protobuf'
 
 Google::Protobuf::DescriptorPool.generated_pool.build do
@@ -17,6 +18,7 @@ Google::Protobuf::DescriptorPool.generated_pool.build do
       optional :optional_bytes, :bytes, 9, default: "\x00\x01\x02\x40\x66\x75\x62\x61\x72".force_encoding("ASCII-8BIT")
       optional :optional_enum, :enum, 10, "A.B.C.TestEnum", default: 1
       optional :optional_msg, :message, 11, "A.B.C.TestMessage"
+      optional :optional_proto2_submessage, :message, 12, "A.B.C.TestImportedMessage"
       repeated :repeated_int32, :int32, 21
       repeated :repeated_int64, :int64, 22
       repeated :repeated_uint32, :uint32, 23

src/google/protobuf/compiler/ruby/ruby_generator.cc

@@ -490,43 +490,6 @@ bool UsesTypeFromFile(const Descriptor* message, const FileDescriptor* file,
   return false;
 }
 
-// Ruby doesn't currently support proto2.  This causes a failure even for proto3
-// files that import proto2.  But in some cases, the proto2 file is only being
-// imported to extend another proto2 message.  The prime example is declaring
-// custom options by extending FileOptions/FieldOptions/etc.
-//
-// If the proto3 messages don't have any proto2 submessages, it is safe to omit
-// the dependency completely.  Users won't be able to use any proto2 extensions,
-// but they already couldn't because proto2 messages aren't supported.
-//
-// If/when we add proto2 support, we should remove this.
-bool MaybeEmitDependency(const FileDescriptor* import,
-                         const FileDescriptor* from,
-                         io::Printer* printer,
-                         std::string* error) {
-  if (from->syntax() == FileDescriptor::SYNTAX_PROTO3 &&
-      import->syntax() == FileDescriptor::SYNTAX_PROTO2) {
-    for (int i = 0; i < from->message_type_count(); i++) {
-      if (UsesTypeFromFile(from->message_type(i), import, error)) {
-        // Error text was already set by UsesTypeFromFile().
-        return false;
-      }
-    }
-
-    // Ok to omit this proto2 dependency -- so we won't print anything.
-    GOOGLE_LOG(WARNING) << "Omitting proto2 dependency '" << import->name()
-                        << "' from proto3 output file '"
-                        << GetOutputFilename(from->name())
-                        << "' because we don't support proto2 and no proto2 "
-                           "types from that file are being used.";
-    return true;
-  } else {
-    printer->Print(
-      "require '$name$'\n", "name", GetRequireName(import->name()));
-    return true;
-  }
-}
-
 bool GenerateDslDescriptor(const FileDescriptor* file, io::Printer* printer,
                            std::string* error) {
   printer->Print(
@@ -572,9 +535,7 @@ bool GenerateFile(const FileDescriptor* file, io::Printer* printer,
     "filename", file->name());
 
   for (int i = 0; i < file->dependency_count(); i++) {
-    if (!MaybeEmitDependency(file->dependency(i), file, printer, error)) {
-      return false;
-    }
+    printer->Print("require '$name$'\n", "name", GetRequireName(file->dependency(i)->name()));
   }
 
   // TODO: Remove this when ruby supports extensions for proto2 syntax.

src/google/protobuf/compiler/ruby/ruby_generator_unittest.cc

@@ -57,7 +57,7 @@ std::string FindRubyTestDir() {
 // Some day, we may integrate build systems between protoc and the language
 // extensions to the point where we can do this test in a more automated way.
 
-void RubyTest(string proto_file) {
+void RubyTest(string proto_file, string import_proto_file = "") {
   std::string ruby_tests = FindRubyTestDir();
 
   google::protobuf::compiler::CommandLineInterface cli;
@@ -77,9 +77,23 @@ void RubyTest(string proto_file) {
       test_input,
       true));
 
+  // Copy generated_code_import.proto to the temporary test directory.
+  std::string test_import;
+  if (!import_proto_file.empty()) {
+    GOOGLE_CHECK_OK(File::GetContents(
+        ruby_tests + import_proto_file + ".proto",
+        &test_import,
+        true));
+    GOOGLE_CHECK_OK(File::SetContents(
+        TestTempDir() + import_proto_file + ".proto",
+        test_import,
+        true));
+  }
+
   // Invoke the proto compiler (we will be inside TestTempDir() at this point).
   std::string ruby_out = "--ruby_out=" + TestTempDir();
   std::string proto_path = "--proto_path=" + TestTempDir();
+
   std::string proto_target = TestTempDir() + proto_file + ".proto";
   const char* argv[] = {
     "protoc",
@@ -105,11 +119,11 @@ void RubyTest(string proto_file) {
 }
 
 TEST(RubyGeneratorTest, Proto3GeneratorTest) {
-  RubyTest("/ruby_generated_code");
+  RubyTest("/ruby_generated_code", "/ruby_generated_code_proto2_import");
 }
 
 TEST(RubyGeneratorTest, Proto2GeneratorTest) {
-    RubyTest("/ruby_generated_code_proto2");
+    RubyTest("/ruby_generated_code_proto2", "/ruby_generated_code_proto2_import");
 }
 
 TEST(RubyGeneratorTest, Proto3ImplicitPackageTest) {