csharp: Skip non-public members from interfaces

Summary:
Eolian allows non-public members in interfaces and mixins (usually
@protected). As both kinds are converted to C# interfaces, this
causes problem as non-public members are forbidden in C# interfaces.

This commit changes eolian_mono by removing those members from the C#
interfaces. If a generated class implements the interface, the method is
generated as if it were a protected member of the class directly.

For mixed properties like `Efl.Io.Reader.CanRead { get; set @protected; }`,
the interface has only the public getter and the the implementing class has both
the public getter and the protected setter.

With this, C# devs won't be able to directly implement protected Eo
methods from interfaces. (But this really does not make sense from the
C# point of view).

ref T7494

Reviewers: segfaultxavi, felipealmeida, YOhoho

Reviewed By: YOhoho

Subscribers: cedric, brunobelo, Jaehyun_Cho, #reviewers, woohyun, #committers

Tags: #efl

Maniphest Tasks: T7494

Differential Revision: https://phab.enlightenment.org/D9800

src/bin/eolian_mono/eolian/mono/async_function_definition.hh

@@ -52,6 +52,9 @@ struct async_function_declaration_generator
       return true;
     if (!f.return_type.original_type.visit(is_future{}))
       return true;
+    // C# interfaces can't have non-public members
+    if(f.scope != attributes::member_scope::scope_public)
+      return true;
 
     if (!as_generator(
             scope_tab << "/// <summary>Async wrapper for <see cref=\"" << name_helpers::managed_method_name(f) << "\" />.</summary>\n"

src/bin/eolian_mono/eolian/mono/blacklist.hh

@@ -64,6 +64,18 @@ inline bool is_function_blacklisted(attributes::function_def const& func, Contex
   return is_function_blacklisted(c_name);
 }
 
+inline bool is_non_public_interface_member(attributes::function_def const& func, attributes::klass_def const&current_klass)
+{
+  if (current_klass.type == attributes::class_type::interface_
+      || current_klass.type == attributes::class_type::mixin)
+    {
+       if (func.scope != attributes::member_scope::scope_public)
+         return true;
+    }
+
+  return false;
+}
+
 
 // Blacklist structs that require some kind of manual binding.
 inline bool is_struct_blacklisted(std::string const& full_name)

src/bin/eolian_mono/eolian/mono/documentation.hh

@@ -73,6 +73,25 @@ struct documentation_generator
       // Klass is needed to check the property naming rulles
       attributes::klass_def klass_d((const ::Eolian_Class *)klass, eolian_object_unit_get(klass));
 
+      // Comment the block below to enable @see reference conversion for non-public interface members.
+      // As they are not generated, this causes a doc warning that fails the build, but can be useful to track
+      // public methods referencing protected stuff.
+      if (ftype != EOLIAN_PROPERTY)
+        {
+           bool is_func_public = ::eolian_function_scope_get(function, ftype) == EOLIAN_SCOPE_PUBLIC;
+
+           if (helpers::is_managed_interface(klass_d) && !is_func_public)
+             return "";
+        }
+      else
+        {
+           bool is_get_public = ::eolian_function_scope_get(function, EOLIAN_PROP_GET) == EOLIAN_SCOPE_PUBLIC;
+           bool is_set_public = ::eolian_function_scope_get(function, EOLIAN_PROP_SET) == EOLIAN_SCOPE_PUBLIC;
+
+           if (helpers::is_managed_interface(klass_d) && !(is_get_public || is_set_public))
+             return "";
+        }
+
       switch(ftype)
       {
          case ::EOLIAN_METHOD:
@@ -112,6 +131,8 @@ struct documentation_generator
 
    static std::string function_conversion(attributes::function_def const& func)
    {
+      // This function is called only from the constructor reference conversion, so it does not
+      // need to check whether this function non-public in a interface returning an empty reference (yet).
       std::string name = name_helpers::klass_full_concrete_or_interface_name(func.klass);
       switch (func.type)
       {

src/bin/eolian_mono/eolian/mono/function_declaration.hh

@@ -23,6 +23,10 @@ struct function_declaration_generator
     if(blacklist::is_function_blacklisted(f, context) || f.is_static)
       return true;
 
+    // C# interfaces can't have non-public members
+    if(f.scope != attributes::member_scope::scope_public)
+      return true;
+
     if(!as_generator(documentation).generate(sink, f, context))
       return false;
 

src/bin/eolian_mono/eolian/mono/function_definition.hh

@@ -81,6 +81,10 @@ struct native_function_definition_generator
        .generate(sink, std::make_tuple(f.c_name, f.c_name, f.c_name, f.c_name), context))
       return false;
 
+    // We do not generate the wrapper to be called from C for non public interface member directly.
+    if (blacklist::is_non_public_interface_member(f, *klass))
+      return true;
+
     // Actual method implementation to be called from C.
     std::string return_type;
     if(!as_generator(eolian_mono::type(true)).generate(std::back_inserter(return_type), f.return_type, context))
@@ -303,12 +307,12 @@ struct property_wrapper_definition_generator
       if (blacklist::is_property_blacklisted(property, *implementing_klass, context))
         return true;
 
-      bool interface = context_find_tag<class_context>(context).current_wrapper_kind == class_context::interface;
+      bool is_interface = context_find_tag<class_context>(context).current_wrapper_kind == class_context::interface;
       bool is_static = (property.getter.is_engaged() && property.getter->is_static)
                        || (property.setter.is_engaged() && property.setter->is_static);
 
 
-      if (interface && is_static)
+      if (is_interface && is_static)
         return true;
 
       auto get_params = property.getter.is_engaged() ? property.getter->parameters.size() : 0;
@@ -389,19 +393,21 @@ struct property_wrapper_definition_generator
 
       std::string scope = "public ";
       std::string get_scope = property.getter.is_engaged() ? eolian_mono::function_scope_get(*property.getter) : "";
+      bool is_get_public = get_scope == "public ";
       std::string set_scope = property.setter.is_engaged() ? eolian_mono::function_scope_get(*property.setter) : "";
-      if (interface)
+      bool is_set_public = set_scope == "public ";
+
+      // No need to generate this wrapper as no accessor is public.
+      if (is_interface && (!is_get_public && !is_set_public))
+          return true;
+
+      // C# interface members are declared automatically as public
+      if (is_interface)
         {
            scope = "";
            get_scope = "";
            set_scope = "";
         }
-      else if ((property.klass.type == attributes::class_type::mixin) ||
-               (property.klass.type == attributes::class_type::interface_))
-        {
-           get_scope = "";
-           set_scope = "";
-        }
       else if ((get_scope != "") && (get_scope == set_scope))
         {
            scope = get_scope;
@@ -439,11 +445,12 @@ struct property_wrapper_definition_generator
           return false;
       }
 
-      if (property.getter.is_engaged() && interface)
+      if (property.getter.is_engaged() && is_interface)
       {
-        if (!as_generator(scope_tab << scope_tab << set_scope <<  "get;\n"
-                          ).generate(sink, attributes::unused, context))
-          return false;
+        if (is_get_public)
+          if (!as_generator(scope_tab << scope_tab << set_scope <<  "get;\n"
+                            ).generate(sink, attributes::unused, context))
+            return false;
       }
       else if (property.getter.is_engaged() && get_params == 0/*parameters.size() == 1 && property.getter.is_engaged()*/)
       {
@@ -483,11 +490,12 @@ struct property_wrapper_definition_generator
       //     return false;
       // }
 
-      if (property.setter.is_engaged() && interface)
+      if (property.setter.is_engaged() && is_interface)
       {
-        if (!as_generator(scope_tab << scope_tab << set_scope <<  "set;\n"
-                          ).generate(sink, attributes::unused, context))
-          return false;
+        if (is_set_public)
+          if (!as_generator(scope_tab << scope_tab << set_scope <<  "set;\n"
+                            ).generate(sink, attributes::unused, context))
+            return false;
       }
       else if (parameters.size() == 1 && property.setter.is_engaged())
       {

src/bin/eolian_mono/eolian/mono/function_helpers.hh

@@ -165,10 +165,6 @@ function_definition_epilogue_generator const as_generator(function_definition_ep
 
 inline std::string function_scope_get(attributes::function_def const& f)
 {
-  if ((f.klass.type == attributes::class_type::mixin) ||
-      (f.klass.type == attributes::class_type::interface_))
-    return "public ";
-
   switch (f.scope)
     {
      case attributes::member_scope::scope_public:
@@ -178,7 +174,8 @@ inline std::string function_scope_get(attributes::function_def const& f)
      case attributes::member_scope::scope_protected:
        return "protected ";
      case attributes::member_scope::scope_unknown:
-       return " ";
+       // This should trigger a compilation error
+       return "unkown_scope ";
     }
   return " ";
 }

src/bin/eolian_mono/eolian/mono/function_registration.hh

@@ -35,6 +35,11 @@ struct function_registration_generator
     if(blacklist::is_function_blacklisted(f, context) || f.is_static) // Static methods aren't overrideable
       return true;
 
+    // We do not generate registration wrappers for non public interface/mixin members in their concrete classes.
+    // They go in the first concrete/abstract implementation.
+    if(blacklist::is_non_public_interface_member(f, *klass))
+      return true;
+
     if(!as_generator(
                indent << "if (" << f.c_name << "_static_delegate == null)\n"
                << indent << "{\n"

src/bin/eolian_mono/eolian/mono/helpers.hh

@@ -233,6 +233,39 @@ std::vector<attributes::function_def> get_all_implementable_methods(attributes::
   return ret;
 }
 
+template<typename Klass>
+inline bool is_managed_interface(Klass const& klass)
+{
+    return klass.type == attributes::class_type::interface_
+           || klass.type == attributes::class_type::mixin;
+}
+
+
+/*
+ * Gets all methods that this class should register (i.e. that comes from it and non-public interface methods
+ * that this class is the first one implementing)
+ */
+template<typename Context>
+std::vector<attributes::function_def> get_all_registerable_methods(attributes::klass_def const& cls, Context const& context)
+{
+   std::vector<attributes::function_def> ret;
+
+   auto implementable_methods = get_all_implementable_methods(cls, context);
+
+   std::copy_if(implementable_methods.cbegin(), implementable_methods.cend(), std::back_inserter(ret)
+                , [&cls](attributes::function_def const & func) {
+
+                    if (cls == func.klass)
+                      return true;
+
+                    if (!is_managed_interface(func.klass) || func.scope != attributes::member_scope::scope_public)
+                      return true;
+                    return false;
+               });
+
+   return ret;
+}
+
 /*
  * Checks whether the given is unique going up the inheritance tree from leaf_klass
  */

src/bin/eolian_mono/eolian/mono/klass.hh

@@ -31,18 +31,6 @@
 
 namespace eolian_mono {
 
-/* Get the actual number of functions of a class, checking for blacklisted ones */
-template<typename Context>
-static std::size_t
-get_implementable_function_count(grammar::attributes::klass_def const& cls, Context context)
-{
-   auto methods = helpers::get_all_implementable_methods(cls, context);
-   return std::count_if(methods.cbegin(), methods.cend(), [&context](grammar::attributes::function_def const& func)
-     {
-        return !blacklist::is_function_blacklisted(func, context) && !func.is_static;
-     });
-}
-
 template<typename Context>
 static bool
 is_inherit_context(Context const& context)
@@ -239,6 +227,9 @@ struct klass
          if (!generate_events(sink, cls, concrete_cxt))
              return false;
 
+         if (!as_generator(lit("#pragma warning disable CS0628\n")).generate(sink, attributes::unused, concrete_cxt))
+            return false;
+
          // Parts
          if(!as_generator(*(part_definition))
             .generate(sink, cls.parts, concrete_cxt)) return false;
@@ -263,6 +254,9 @@ struct klass
                 return false;
            }
 
+         if (!as_generator(lit("#pragma warning restore CS0628\n")).generate(sink, attributes::unused, concrete_cxt))
+            return false;
+
          // Copied from nativeinherit class, used when setting up providers.
          if(!as_generator(
               scope_tab << "private static IntPtr GetEflClassStatic()\n"
@@ -398,7 +392,7 @@ struct klass
                                             context);
          auto native_inherit_name = name_helpers::klass_native_inherit_name(cls);
          auto inherit_name = name_helpers::klass_inherit_name(cls);
-         auto implementable_methods = cls.functions;
+         auto implementable_methods = helpers::get_all_registerable_methods(cls, context);
          bool root = !helpers::has_regular_ancestor(cls);
          auto const& indent = current_indentation(inative_cxt);
 

src/bindings/mono/efl_mono/Bind.cs

@@ -3,6 +3,7 @@
 using System;
 using System.Runtime.InteropServices;
 using System.Collections.Generic;
+using System.Reflection;
 using System.Linq;
 using System.ComponentModel;
 
@@ -47,8 +48,15 @@ public class BindableProperty<T>
                 throw new InvalidOperationException($"Failed to cast binder {binder} to IPart");
             }
 
-            var partBinder = partHolder.GetPart(this.partName) as Efl.Ui.IPropertyBind;
+            // We rely on reflection as GetPart is protected and not generated in IPart.
+            var partMethod = partHolder.GetType().GetMethod("GetPart", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
 
+            if (partMethod == null)
+            {
+                throw new InvalidOperationException($"Failed to get 'GetPart' method on property binder");
+            }
+
+            var partBinder = partMethod.Invoke(partHolder, new System.Object[] { this.partName }) as Efl.Ui.IPropertyBind;
             if (partBinder != null)
             {
                 return partBinder.PropertyBind(this.propertyName, modelProperty);

src/lib/eolian_cxx/grammar/klass_def.hpp

@@ -1277,6 +1277,26 @@ struct klass_def
        || lhs.parts < rhs.parts;
   }
 
+  friend inline bool operator==(klass_def const& lhs, klass_name const& rhs)
+  {
+     return lhs.namespaces == rhs.namespaces
+         && lhs.eolian_name == rhs.eolian_name
+         && lhs.type == rhs.type;
+  }
+  friend inline bool operator==(klass_name const& lhs, klass_def const& rhs)
+  {
+      return rhs == lhs;
+  }
+  friend inline bool operator!=(klass_def const& lhs, klass_name const& rhs)
+  {
+      return !(lhs == rhs);
+  }
+  friend inline bool operator!=(klass_name const& lhs, klass_def const& rhs)
+  {
+      return !(rhs == lhs);
+  }
+
+
   klass_def(std::string eolian_name, std::string cxx_name, std::string filename
             , documentation_def documentation
             , std::vector<std::string> namespaces

src/tests/efl_mono/Eo.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Linq;
 using System.Collections.Generic;
+using System.Reflection;
 
 namespace TestSuite
 {
@@ -533,4 +534,77 @@ class TestObjectDeletion
     }
 }
 
+class TestProtectedInterfaceMembers
+{
+
+    private class MyObject : Dummy.TestObject
+    {
+        public MyObject(Efl.Object parent = null) : base(parent)
+        {
+        }
+
+        protected override int MethodProtected(int x)
+        {
+            return x * x;
+        }
+    }
+
+    public static void test_protected_interface_in_generated_class_called_from_c()
+    {
+        var obj = new Dummy.TestObject();
+        Test.AssertEquals(obj.CallMethodProtected(42), -42);
+    }
+
+    public static void test_protected_interface_in_inherited_class_called_from_c()
+    {
+        var obj = new MyObject();
+        Test.AssertEquals(obj.CallMethodProtected(42), 42 * 42);
+    }
+
+    public static void test_interface_skipped_protected()
+    {
+        var type = typeof(Dummy.ITestIface);
+        var methods = type.GetMethods(BindingFlags.Public | BindingFlags.Instance);
+
+        // Fully protected property
+        Test.AssertNull(methods.SingleOrDefault(m => m.Name == "GetProtectedProp"));
+        Test.AssertNull(methods.SingleOrDefault(m => m.Name == "SetProtectedProp"));
+
+        // Partially protected property
+        Test.AssertNotNull(methods.SingleOrDefault(m => m.Name == "GetPublicGetterPrivateSetter"));
+        Test.AssertNull(methods.SingleOrDefault(m => m.Name == "SetPublicGetterPrivateSetter"));
+
+        var properties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance);
+        Test.AssertNull(properties.SingleOrDefault(m => m.Name == "ProtectedProp"));
+        Test.AssertNotNull(properties.SingleOrDefault(m => m.Name == "PublicGetterPrivateSetter"));
+    }
+
+    public static void test_interface_skipped_protected_in_implementation()
+    {
+        var type = typeof(Dummy.TestObject);
+
+        // Fully protected property
+        var protected_methods = type.GetMethods(BindingFlags.NonPublic | BindingFlags.Instance).Where(m => m.IsFamily);
+        Test.AssertNotNull(protected_methods.SingleOrDefault(m => m.Name == "GetProtectedProp"));
+        Test.AssertNotNull(protected_methods.SingleOrDefault(m => m.Name == "SetProtectedProp"));
+
+        // Partially protected property
+        var public_methods = type.GetMethods(BindingFlags.Public | BindingFlags.Instance);
+        Test.AssertNotNull(public_methods.SingleOrDefault(m => m.Name == "GetPublicGetterPrivateSetter"));
+        Test.AssertNotNull(protected_methods.SingleOrDefault(m => m.Name == "SetPublicGetterPrivateSetter"));
+
+        var protected_properties = type.GetProperties(BindingFlags.NonPublic | BindingFlags.Instance);
+        var prop = protected_properties.SingleOrDefault(m => m.Name == "ProtectedProp");
+        Test.AssertNotNull(prop);
+        Test.Assert(prop.GetMethod.IsFamily);
+        Test.Assert(prop.SetMethod.IsFamily);
+
+        var public_properties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance);
+        prop = public_properties.SingleOrDefault(m => m.Name == "PublicGetterPrivateSetter");
+        Test.AssertNotNull(prop);
+        Test.Assert(prop.GetMethod.IsPublic);
+        Test.Assert(prop.SetMethod.IsFamily);
+    }
+}
+
 }

src/tests/efl_mono/dummy_interfaces.c

@@ -1,4 +1,6 @@
 // Include file for interfaces .eo.c files
+#define DUMMY_TEST_IFACE_PROTECTED
+
 #include "libefl_mono_native_test.h"
 
 #include "dummy_test_iface.eo.c"

src/tests/efl_mono/dummy_test_iface.eo

@@ -10,6 +10,37 @@ interface Dummy.Test_Iface
             data: int;
          }
       }
+
+      method_protected @protected @const {
+         params {
+            @in x: int;
+         }
+         return: int;
+      }
+
+      call_method_protected @const {
+         params {
+            @in x: int;
+         }
+         return: int;
+      }
+
+      @property protected_prop @protected {
+         get {}
+         set {}
+         values {
+            data: int;
+         }
+      }
+
+      @property public_getter_private_setter {
+         get {}
+         set @protected {}
+         values {
+            data: int;
+         }
+      }
+
    }
    events {
       nonconflicted: void;

src/tests/efl_mono/dummy_test_object.c

@@ -1,4 +1,6 @@
 
+#define DUMMY_TEST_IFACE_PROTECTED
+
 #include "libefl_mono_native_test.h"
 
 typedef struct Dummy_Test_Object_Data
@@ -4704,6 +4706,16 @@ const Eina_Value_Type *_dummy_test_object_mirror_value_type(EINA_UNUSED const Eo
     return type;
 }
 
+int _dummy_test_object_dummy_test_iface_method_protected(EINA_UNUSED const Eo *obj, EINA_UNUSED Dummy_Test_Object_Data *pd, int x)
+{
+    return -x;
+}
+
+int _dummy_test_object_dummy_test_iface_call_method_protected(const Eo *obj, EINA_UNUSED Dummy_Test_Object_Data *pd, int x)
+{
+    return dummy_test_iface_method_protected(obj, x);
+}
+
 // Inherit
 int _dummy_inherit_helper_receive_dummy_and_call_int_out(Dummy_Test_Object *x)
 {

src/tests/efl_mono/dummy_test_object.eo

@@ -1653,6 +1653,8 @@ class Dummy.Test_Object extends Efl.Object implements Dummy.Test_Iface {
       Efl.Object.provider_find;
       Dummy.Test_Iface.emit_nonconflicted;
       Dummy.Test_Iface.iface_prop { get; set; }
+      Dummy.Test_Iface.method_protected;
+      Dummy.Test_Iface.call_method_protected;
    }
    events {
       evt,with,string @hot: const(string);