Revert overly broad Rice filtering heuristics

Author: cfis

lib/ruby-bindgen/generators/rice/rice.rb

@@ -180,6 +180,7 @@ def initialize(inputter, outputter, config)
         @incomplete_iterators = Hash.new
         # Iterator names per class (for aliasing each_const -> each)
         @class_iterator_names = Hash.new { |h, k| h[k] = Set.new }
+        @project_complete_types = Set.new
       end
 
       # Parse the configured inputs with libclang and stream the resulting
@@ -188,6 +189,7 @@ def generate
         clang_args = @config[:clang_args] || []
         parser = RubyBindgen::Parser.new(@inputter, clang_args, libclang: @config[:libclang])
         ::FFI::Clang::Cursor.namer = @namer
+        build_project_complete_type_index(parser)
         parser.generate(self)
       end
 
@@ -291,6 +293,7 @@ def unsupported_rice_opaque_namespace_type?(type)
         decl = type.canonical.declaration
         return false if decl.kind == :cursor_no_decl_found
         return false unless decl.opaque_declaration?
+        return false if project_complete_type?(decl)
         return false if decl.qualified_name&.start_with?("std::", "__gnu_cxx::")
         return false if [:cursor_class_decl, :cursor_struct].include?(decl.semantic_parent.kind)
 
@@ -449,7 +452,6 @@ def visit_translation_unit(translation_unit, path, relative_path)
         @non_member_operators.clear
         @incomplete_iterators.clear
         @class_iterator_names.clear
-        @declared_function_qualified_names = nil
         cursor = translation_unit.cursor
         @translation_unit_cursor = cursor
         @type_speller.printing_policy = cursor.printing_policy
@@ -516,6 +518,40 @@ def visit_translation_unit(translation_unit, path, relative_path)
         self.outputter.write(rice_header, content)
       end
 
+      def build_project_complete_type_index(parser)
+        @project_complete_types.clear
+
+        @inputter.each do |path, _relative_path|
+          begin
+            translation_unit = parser.send(:parse_translation_unit, path)
+          rescue RubyBindgen::Parser::ParseError
+            next
+          end
+
+          record_project_complete_types(translation_unit.cursor)
+        end
+      end
+
+      def record_project_complete_types(cursor)
+        cursor.find_by_kind(true, :cursor_class_decl, :cursor_struct) do |child|
+          next if child.spelling.empty?
+          next if child.opaque_declaration?
+          next unless translation_unit_file?(child)
+
+          qualified_name = child.qualified_name
+          next if qualified_name.nil? || qualified_name.empty?
+
+          @project_complete_types << qualified_name
+        end
+      end
+
+      def project_complete_type?(decl)
+        qualified_name = decl.qualified_name
+        return false if qualified_name.nil? || qualified_name.empty?
+
+        @project_complete_types.include?(qualified_name)
+      end
+
       # Render a public, callable constructor into the Rice chain for its class.
       def visit_constructor(cursor)
         # Do not process class constructors defined outside of the class definition
@@ -553,16 +589,7 @@ def visit_constructor(cursor)
       # constants, embedded types, and any auto-generated template bases needed
       # before the class itself can be registered.
       def visit_class_decl(cursor)
-        # Namespace-scope forward-declared C++ classes are often completed in a
-        # different header. Emitting a Rice class here creates a Ruby constant
-        # with no superclass, and a later complete definition then conflicts.
-        # Keep nested incomplete classes on the existing special path, and keep
-        # opaque structs available for handle-style APIs.
-        if cursor.kind == :cursor_class_decl &&
-           cursor.opaque_declaration? &&
-           ![:cursor_class_decl, :cursor_struct].include?(cursor.semantic_parent.kind)
-          return
-        end
+        return if skip_namespace_forward_declaration?(cursor)
 
         # Skip explicitly listed symbols
         return if skip_symbol?(cursor)
@@ -896,28 +923,12 @@ def skip_callable?(cursor)
           cursor.type.variadic?
       end
 
-      def unresolved_inline_calls?(cursor)
-        source = inline_definition_source(cursor)
-        return false unless source&.include?('{')
-
-        function_calls = source.scan(/([A-Za-z_]\w*(?:::[A-Za-z_]\w*)+)\s*\(/)
-                               .flatten
-                               .uniq
-                               .select { |name| name.split('::').last.match?(/\A[a-z_]\w*\z/) }
-        return false if function_calls.empty?
-
-        function_calls.any? do |qualified_name|
-          !declared_function_qualified_names.include?(qualified_name)
-        end
-      end
-
       # Render a class method, including special handling for iterator adapters
       # and mutable `operator[]` setter support.
       def visit_cxx_method(cursor)
         # Do not process method definitions outside of classes (because we already processed them)
         return if cursor.lexical_parent != cursor.semantic_parent
         return if skip_callable?(cursor)
-        return if unresolved_inline_calls?(cursor)
         return if has_skipped_param_type?(cursor)
         return if has_unsupported_rice_param_type?(cursor)
         return if has_skipped_return_type?(cursor)
@@ -965,43 +976,16 @@ def visit_cxx_method(cursor)
         result
       end
 
-      def declared_function_qualified_names
-        @declared_function_qualified_names ||= @translation_unit_cursor
-          .find_by_kind(true, :cursor_function, :cursor_function_template)
-          .map(&:qualified_name)
-          .reject(&:empty?)
-          .to_set
-      end
+      def skip_namespace_forward_declaration?(cursor)
+        return false unless cursor.kind == :cursor_class_decl
+        return false unless cursor.opaque_declaration?
+        return false if [:cursor_class_decl, :cursor_struct].include?(cursor.semantic_parent.kind)
 
-      def inline_definition_source(cursor)
-        parent = cursor.semantic_parent
-        source = parent&.extent&.text
-        return nil if source.nil? || source.empty?
-
-        line_index = cursor.extent.start.line - parent.extent.start.line
-        lines = source.lines
-        return nil if line_index.negative? || line_index >= lines.length
-
-        result = String.new
-        brace_depth = 0
-        saw_body = false
-
-        lines[line_index..].each do |line|
-          result << line
-          line.each_char do |char|
-            if char == '{'
-              saw_body = true
-              brace_depth += 1
-            elsif char == '}'
-              brace_depth -= 1 if brace_depth.positive?
-            end
-          end
-
-          break if saw_body && brace_depth.zero?
-          break if !saw_body && line.include?(';')
-        end
+        definition = cursor.definition
+        return false if [:cursor_invalid_file, :cursor_no_decl_found].include?(definition.kind)
+        return false if definition.opaque_declaration?
 
-        result
+        translation_unit_file?(definition)
       end
 
       # Check if an iterator type has proper std::iterator_traits.

test/bindings/cpp/unresolved_inline_calls-rb.cpp

@@ -12,4 +12,12 @@ void Init_UnresolvedInlineCalls()
   Module rb_mTests = define_module("Tests");
 
   Module rb_mTestsBroken = define_module_under(rb_mTests, "Broken");
+
+  Rice::Data_Type<Tests::Logger> rb_cTestsLogger = define_class_under<Tests::Logger>(rb_mTests, "Logger")
+    .define_constructor(Constructor<Tests::Logger>())
+    .define_singleton_function<int(*)()>("info", &Tests::Logger::info);
+
+  Module rb_mTestsUtils = define_module_under(rb_mTests, "Utils");
+
+  rb_mTestsUtils.define_module_function<int(*)()>("id", &Tests::Utils::id);
 }

test/bindings/cpp/unresolved_inline_calls-rb.ipp

@@ -4,5 +4,8 @@ template<typename T>
 inline Rice::Data_Type<Tests::Params<T>> Params_instantiate(Rice::Module parent, const char* name)
 {
   return Rice::define_class_under<Tests::Params<T>>(parent, name)
+    .template define_method<int(Tests::Params<T>::*)() const>("backend", &Tests::Params<T>::backend)
+    .template define_method<int(Tests::Params<T>::*)() const>("logger", &Tests::Params<T>::logger)
+    .template define_method<int(Tests::Params<T>::*)() const>("utility", &Tests::Params<T>::utility)
     .template define_method<int(Tests::Params<T>::*)() const>("ok", &Tests::Params<T>::ok);
 }

test/headers/cpp/opaque_namespace_api.hpp

@@ -0,0 +1,18 @@
+#include "opaque_namespace_handle.hpp"
+
+namespace Tests
+{
+  class OpaqueNamespaceConsumer
+  {
+  public:
+    OpaqueNamespaceConsumer();
+    OpaqueNamespaceConsumer(const Render::Handle& handle);
+
+    const Render::Handle& inHandle() const;
+    Render::Handle outHandle() const;
+    Render::Handle& outHandleRef();
+    void setHandle(const Render::Handle& handle);
+
+    int value() const;
+  };
+}

test/headers/cpp/opaque_namespace_full.hpp

@@ -0,0 +1,13 @@
+#include "opaque_namespace_handle.hpp"
+
+namespace Tests
+{
+  namespace Render
+  {
+    class Handle
+    {
+    public:
+      int id;
+    };
+  }
+}

test/headers/cpp/opaque_namespace_handle.hpp

@@ -0,0 +1,7 @@
+namespace Tests
+{
+  namespace Render
+  {
+    class Handle;
+  }
+}

test/headers/cpp/unresolved_inline_calls.hpp

@@ -4,10 +4,22 @@ namespace Tests {
 namespace Broken {
 }
 
+class Logger
+{
+public:
+  static int info();
+};
+
+namespace Utils {
+int id();
+}
+
 template<typename T>
 struct Params
 {
   int backend() const { return Tests::Broken::backend(); }
+  int logger() const { return Tests::Logger::info(); }
+  int utility() const { return Utils::id(); }
   int ok() const { return 1; }
 };
 }

test/rice_generator_test.rb

@@ -176,7 +176,7 @@ class FunctionTemplate
     assert_includes rendered, "FunctionTemplate_instantiate<&Tests::callback_ints>"
   end
 
-  def test_namespace_scope_forward_declared_classes_do_not_generate_bindings
+  def test_namespace_scope_forward_declared_classes_generate_placeholders_across_headers
     config_dir = File.join(__dir__, "headers", "cpp")
     config = load_config(config_dir)
     inputter = RubyBindgen::Inputter.new(config_dir, ["forward_declared_classes.hpp",
@@ -191,12 +191,61 @@ def test_namespace_scope_forward_declared_classes_do_not_generate_bindings
     classes_cpp = outputter.output_paths.fetch(outputter.output_path("forward_declared_classes-rb.cpp"))
     all_layers_cpp = outputter.output_paths.fetch(outputter.output_path("forward_declared_all_layers-rb.cpp"))
 
-    refute_includes classes_cpp,
+    assert_includes classes_cpp,
                     'define_class_under<ForwardDeclaredClasses::ActivationLayer>(rb_mForwardDeclaredClasses, "ActivationLayer")'
     assert_includes all_layers_cpp,
                     'define_class_under<ForwardDeclaredClasses::ActivationLayer, ForwardDeclaredClasses::Layer>(rb_mForwardDeclaredClasses, "ActivationLayer")'
   end
 
+  def test_same_file_forward_declarations_only_render_the_complete_class
+    parsed, = parse_cpp(<<~CPP)
+      namespace Tests {
+        class Forward;
+
+        class Base {
+        public:
+          virtual ~Base() = default;
+        };
+
+        class Forward : public Base {
+        public:
+          int value() const { return 1; }
+        };
+      }
+    CPP
+
+    inputter = RubyBindgen::Inputter.new(parsed.dir, ["fixture.hpp"])
+    outputter = create_outputter("cpp")
+    config = load_config(File.join(__dir__, "headers", "cpp"))
+    generator = RubyBindgen::Generators::Rice.new(inputter, outputter, config)
+
+    capture_io { generator.generate }
+
+    generated_cpp = outputter.output_paths.fetch(outputter.output_path("fixture-rb.cpp"))
+
+    assert_equal 1, generated_cpp.scan('define_class_under<Tests::Forward, Tests::Base>(rb_mTests, "Forward")').size
+    refute_includes generated_cpp, 'define_class_under<Tests::Forward>(rb_mTests, "Forward")'
+  end
+
+  def test_namespace_scope_opaque_types_with_project_definitions_are_generated
+    config_dir = File.join(__dir__, "headers", "cpp")
+    config = load_config(config_dir)
+    inputter = RubyBindgen::Inputter.new(config_dir, ["opaque_namespace_api.hpp",
+                                                      "opaque_namespace_full.hpp"])
+    outputter = create_outputter("cpp")
+    generator = RubyBindgen::Generators::Rice.new(inputter, outputter, config)
+
+    capture_io { generator.generate }
+
+    generated_cpp = outputter.output_paths.fetch(outputter.output_path("opaque_namespace_api-rb.cpp"))
+
+    assert_includes generated_cpp, "Constructor<Tests::OpaqueNamespaceConsumer, const Tests::Render::Handle &>()"
+    assert_includes generated_cpp, '"in_handle", &Tests::OpaqueNamespaceConsumer::inHandle'
+    assert_includes generated_cpp, '"out_handle", &Tests::OpaqueNamespaceConsumer::outHandle'
+    assert_includes generated_cpp, '"out_handle_ref", &Tests::OpaqueNamespaceConsumer::outHandleRef'
+    assert_includes generated_cpp, '"set_handle", &Tests::OpaqueNamespaceConsumer::setHandle'
+  end
+
   def test_out_of_class_nested_definitions_are_rendered_only_under_the_parent
     parsed, = parse_cpp(<<~CPP)
       namespace Tests {

test/rice_test.rb

@@ -552,7 +552,7 @@ def test_class_template_typedef_qualification
     end
   end
 
-  def test_unresolved_inline_calls_are_skipped
+  def test_inline_methods_with_qualified_calls_are_generated
     config_dir = File.join(__dir__, "headers", "cpp")
     config = load_config(config_dir)
     config[:match] = ["unresolved_inline_calls.hpp"]
@@ -565,7 +565,9 @@ def test_unresolved_inline_calls_are_skipped
 
     generated_ipp = outputter.output_paths.fetch(outputter.output_path("unresolved_inline_calls-rb.ipp"))
 
-    refute_includes generated_ipp, "\"backend\", &Tests::Params<T>::backend"
+    assert_includes generated_ipp, "\"backend\", &Tests::Params<T>::backend"
+    assert_includes generated_ipp, "\"logger\", &Tests::Params<T>::logger"
+    assert_includes generated_ipp, "\"utility\", &Tests::Params<T>::utility"
     assert_includes generated_ipp, "\"ok\", &Tests::Params<T>::ok"
 
     expected_cpp = outputter.output_path("unresolved_inline_calls-rb.cpp")